Re: [PATCH] USB:bugfix a controller halt error

[Alan Stern]

On Thu, Jul 27, 2023 at 05:31:41PM +0200, Oliver Neukum wrote:
> On 27.07.23 16:42, Alan Stern wrote:
> > On Thu, Jul 27, 2023 at 03:03:57PM +0800, liulongfang wrote:
> > > On 2023/7/26 22:20, Alan Stern wrote:
> 
> > > > It seems to me that something along these lines must be necessary in
> > > > any case.  Unless the bad memory is cleared somehow, it would never be
> > > > usable again.  The kernel might deallocate it, then reallocate for
> > > > another purpose, and then crash when the new user tries to access it.
> > > > 
> > > > In fact, this scenario could still happen even with your patch, which
> > > > means the patch doesn't really fix the problem.
> 
> I suppose in theory you could have something like a bad blocks list
> just for RAM, but that would really hurt. You'd have to do something
> about every DMA operation in every driver in theory.
> 
> Error handling would basically be an intentional memory leak.

I started out thinking this way, but maybe that's not how it works.  
Perhaps simply overwriting the part of memory that got the ECC error 
would clear the error state.  (This may depend on the kind of error, 
one-time vs. permanent.)

If that's the case, and if the memory buffer was deallocated without 
being accessed and then later reallocated, things would be okay.  The 
routine that reallocated the buffer wouldn't try to read from it before 
initializing it somehow.

> > > This patch is only used to prevent data in the buffer from being accessed.
> > > As long as the data is not accessed, the kernel does not crash.
> > 
> > I still don't understand.  You haven't provided nearly enough
> > information.  You should start by answering the questions that Oliver
> > asked.  Then answer this question:
> > 
> > The code you are concerned about is this:
> > 
> > 		r = usb_control_msg(udev, usb_rcvaddr0pipe(),
> > 				USB_REQ_GET_DESCRIPTOR, USB_DIR_IN,
> > 				USB_DT_DEVICE << 8, 0,
> > 				buf, GET_DESCRIPTOR_BUFSIZE,
> > 				initial_descriptor_timeout);
> > 		switch (buf->bMaxPacketSize0) {
> > 
> > You're worried that if an ECC memory error occurs during the
> > usb_control_msg transfer, the kernel will crash when the "switch"
> > statement tries to read the value of buf->bMaxPacketSize0.  That's a
> > reasonable thing to worry about.
> 
> Albeit unlikely. If the hardware and implementation are reasonable
> you'd return a specific error code from the HCD and clean up the
> RAM in your ecc driver.
> 
> The fix for USB would then conceptually be something like
> 
> retryio:
> 	r = usb_control_msg()
> 	if (r == -EMEMORYCORRUPTION)
> 		goto retryio;

Yes, we could do this, but it's not necessary.  Let's say that the HCD 
returns -EMEMORYCORRUPTION and the ecc driver cleans up the RAM 
(probably by resetting its contents to 0, but possibly leaving garbage 
there instead).  Then when the following code in hub_port_init() tests 
buf->bMaxPacketSize0, it will see an invalid value and will retry the 
transfer.

Or, with low probability, it will see a valid but incorrect value.  If 
that happens then later transfers using ep0 will fail, causing the hub 
driver to reiterate the outer loop in hub_port_connect().  Eventually 
the device will be correctly initialized and enumerated.

Alan Stern