[PATCH] USB:bugfix a controller halt error

[PATCH] USB:bugfix a controller halt error

[liulongfang]

On systems that use ECC memory. The ECC error of the memory will
cause the USB controller to halt. It causes the usb_control_msg()
operation to fail.
At this point, the returned buffer data is an abnormal value, and
continuing to use it will lead to incorrect results.

Therefore, it is necessary to judge the return value and exit.

Signed-off-by: liulongfang <liulongfang@huawei.com>
---
 drivers/usb/core/hub.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index a739403a9e45..6a43198be263 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -4891,6 +4891,16 @@ hub_port_init(struct usb_hub *hub, struct usb_device *udev, int port1,
 					USB_DT_DEVICE << 8, 0,
 					buf, GET_DESCRIPTOR_BUFSIZE,
 					initial_descriptor_timeout);
+				/* On systems that use ECC memory, ECC errors can
+				 * cause the USB controller to halt.
+				 * It causes this operation to fail. At this time,
+				 * the buf data is an abnormal value and needs to be exited.
+				 */
+				if (r < 0) {
+					kfree(buf);
+					goto fail;
+				}
+
 				switch (buf->bMaxPacketSize0) {
 				case 8: case 16: case 32: case 64: case 255:
 					if (buf->bDescriptorType ==
-- 
2.24.0

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

[Greg KH]

On Fri, Jul 21, 2023 at 06:00:15PM +0800, liulongfang wrote:
> On systems that use ECC memory. The ECC error of the memory will
> cause the USB controller to halt. It causes the usb_control_msg()
> operation to fail.

Why does ECC memory matter here?

> At this point, the returned buffer data is an abnormal value, and
> continuing to use it will lead to incorrect results.
> 
> Therefore, it is necessary to judge the return value and exit.
> 
> Signed-off-by: liulongfang <liulongfang@huawei.com>
> ---
>  drivers/usb/core/hub.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index a739403a9e45..6a43198be263 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -4891,6 +4891,16 @@ hub_port_init(struct usb_hub *hub, struct usb_device *udev, int port1,
>  					USB_DT_DEVICE << 8, 0,
>  					buf, GET_DESCRIPTOR_BUFSIZE,
>  					initial_descriptor_timeout);
> +				/* On systems that use ECC memory, ECC errors can
> +				 * cause the USB controller to halt.
> +				 * It causes this operation to fail. At this time,
> +				 * the buf data is an abnormal value and needs to be exited.
> +				 */
> +				if (r < 0) {
> +					kfree(buf);
> +					goto fail;
> +				}

Are you sure this is correct?  How was this tested?  Seems to me that
this will still return "success" if this code path ever happens, what am
I missing?

thanks,

greg k-h

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

[liulongfang]

On 2023/7/21 19:08, Greg KH Wrote:
> On Fri, Jul 21, 2023 at 06:00:15PM +0800, liulongfang wrote:
>> On systems that use ECC memory. The ECC error of the memory will
>> cause the USB controller to halt. It causes the usb_control_msg()
>> operation to fail.
> 
> Why does ECC memory matter here?
>

This is a test conducted under a special test scenario.
ECC memory errors are caused by some test tools.

>> At this point, the returned buffer data is an abnormal value, and
>> continuing to use it will lead to incorrect results.
>>
>> Therefore, it is necessary to judge the return value and exit.
>>
>> Signed-off-by: liulongfang <liulongfang@huawei.com>
>> ---
>>  drivers/usb/core/hub.c | 10 ++++++++++
>>  1 file changed, 10 insertions(+)
>>
>> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
>> index a739403a9e45..6a43198be263 100644
>> --- a/drivers/usb/core/hub.c
>> +++ b/drivers/usb/core/hub.c
>> @@ -4891,6 +4891,16 @@ hub_port_init(struct usb_hub *hub, struct usb_device *udev, int port1,
>>  					USB_DT_DEVICE << 8, 0,
>>  					buf, GET_DESCRIPTOR_BUFSIZE,
>>  					initial_descriptor_timeout);
>> +				/* On systems that use ECC memory, ECC errors can
>> +				 * cause the USB controller to halt.
>> +				 * It causes this operation to fail. At this time,
>> +				 * the buf data is an abnormal value and needs to be exited.
>> +				 */
>> +				if (r < 0) {
>> +					kfree(buf);
>> +					goto fail;
>> +				}
> 
> Are you sure this is correct?  How was this tested?  Seems to me that
> this will still return "success" if this code path ever happens, what am

You are right. I made a patch error here. The code modification should be like this:
if (r < 0) {
	retval = r;
	kfree(buf);
	goto fail;
}

thanks,

Longfang.
> I missing?
> 
> thanks,
> 
> greg k-h
> .
> 

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

[Greg KH]

On Wed, Jul 26, 2023 at 02:44:01PM +0800, liulongfang wrote:
> On 2023/7/21 19:08, Greg KH Wrote:
> > On Fri, Jul 21, 2023 at 06:00:15PM +0800, liulongfang wrote:
> >> On systems that use ECC memory. The ECC error of the memory will
> >> cause the USB controller to halt. It causes the usb_control_msg()
> >> operation to fail.
> > 
> > Why does ECC memory matter here?
> >
> 
> This is a test conducted under a special test scenario.
> ECC memory errors are caused by some test tools.

What memory is failing, and why does just this single check matter in
the whole kernel?

If hardware is broken, and failing, it's not the job of the kernel to
protect against that, is it?  Shouldn't the ECC memory controller have
properly notified the kernel of the fault and reset the machine because
it is now in an undetermined state?

> > Are you sure this is correct?  How was this tested?  Seems to me that
> > this will still return "success" if this code path ever happens, what am
> 
> You are right. I made a patch error here. The code modification should be like this:
> if (r < 0) {
> 	retval = r;
> 	kfree(buf);
> 	goto fail;
> }

This means that you didn't test this change at all, so I don't really
think it is needed :(

thanks,

greg k-h

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

[Oliver Neukum]

On 26.07.23 09:18, Greg KH wrote:
> On Wed, Jul 26, 2023 at 02:44:01PM +0800, liulongfang wrote:

>> This is a test conducted under a special test scenario.
>> ECC memory errors are caused by some test tools.

So we are looking at a corner case here.

I think we need to step back, get an overview. And I would
like to apologize for not being entirely helpful.

I see a theoretical possibility here of what is going wrong
and an extremely theoretical bug, but it would be very good
if you could describe your test setup.

So. You are inducing simulated memory errors.
For this scenario to make any sense your failure must be

1. temporary - that is you have detected memory corruption but the RAM cell is not broken
2. unrecoverable - that is we have lost data
3. locateable - that is you know it hit the buffer of this operation and only it

Am I correct so far?

Furthermore your system reports the error to the HC, so that in last
consequence the transfer fails. Right?

> What memory is failing, and why does just this single check matter in
> the whole kernel?

The difference here is that we are deliberately ignoring errors.
  
> If hardware is broken, and failing, it's not the job of the kernel to
> protect against that, is it?  Shouldn't the ECC memory controller have
> properly notified the kernel of the fault

Definitely it should. But this whole discussion makes only sense
if exactly that happens.

> and reset the machine because
> it is now in an undetermined state?

No. It is not in an undetermined state if your detection logic is good enough.

	Regards
		Oliver

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

[liulongfang]

On 2023/7/26 19:16, Oliver Neukum wrote:
> On 26.07.23 09:18, Greg KH wrote:
>> On Wed, Jul 26, 2023 at 02:44:01PM +0800, liulongfang wrote:
> 
>>> This is a test conducted under a special test scenario.
>>> ECC memory errors are caused by some test tools.
> 
> So we are looking at a corner case here.
> 
> I think we need to step back, get an overview. And I would
> like to apologize for not being entirely helpful.
> 
> I see a theoretical possibility here of what is going wrong
> and an extremely theoretical bug, but it would be very good
> if you could describe your test setup.
> 
> So. You are inducing simulated memory errors.
> For this scenario to make any sense your failure must be
> 
> 1. temporary - that is you have detected memory corruption but the RAM cell is not broken
> 2. unrecoverable - that is we have lost data
> 3. locateable - that is you know it hit the buffer of this operation and only it
> 
> Am I correct so far?
>
You are right about the testing process.
But this problem can exist in the real environment, just the probability of
occurrence is very low.
Use a test tool just to make it easier to trigger it.

> Furthermore your system reports the error to the HC, so that in last
> consequence the transfer fails. Right?
> 
>> What memory is failing, and why does just this single check matter in
>> the whole kernel?
> 
> The difference here is that we are deliberately ignoring errors.
>  
>> If hardware is broken, and failing, it's not the job of the kernel to
>> protect against that, is it?  Shouldn't the ECC memory controller have
>> properly notified the kernel of the fault
> 
> Definitely it should. But this whole discussion makes only sense
> if exactly that happens.
>
Our test tool only simulates that external interference destroys this part
of the data in the buffer on the ECC memory. Even without this testing tool.
This problem may also occur on real business hardware devices.

>> and reset the machine because
>> it is now in an undetermined state?
> 
> No. It is not in an undetermined state if your detection logic is good enough.
> 
>     Regards
>         Oliver
> .
> 
Thanks,
Longfang.

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

[Oliver Neukum]

On 27.07.23 09:00, liulongfang wrote:
> On 2023/7/26 19:16, Oliver Neukum wrote:

>> 1. temporary - that is you have detected memory corruption but the RAM cell is not broken
>> 2. unrecoverable - that is we have lost data
>> 3. locateable - that is you know it hit the buffer of this operation and only it
>>
>> Am I correct so far?
>>
> You are right about the testing process.
> But this problem can exist in the real environment, just the probability of
> occurrence is very low.

Understood. Bit flips are random.

But this leaves two open questions.

1. How is the error reported

2. How are we supposed to handle it

Firstly, if we already know that there is an ECC failure
on the host we can use a specific error code and can check
for that.

Secondly, does this mean that the affected memory location
must not be touched until the machine is power cycled
or does it simply mean that the buffer is invalid?

> Our test tool only simulates that external interference destroys this part
> of the data in the buffer on the ECC memory. Even without this testing tool.
> This problem may also occur on real business hardware devices.

Understood. But what is the correct remedy if teh problem strikes
for real?

	Regards
		Oliver

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

[liulongfang]

On 2023/7/26 15:18, Greg KH wrote:
> On Wed, Jul 26, 2023 at 02:44:01PM +0800, liulongfang wrote:
>> On 2023/7/21 19:08, Greg KH Wrote:
>>> On Fri, Jul 21, 2023 at 06:00:15PM +0800, liulongfang wrote:
>>>> On systems that use ECC memory. The ECC error of the memory will
>>>> cause the USB controller to halt. It causes the usb_control_msg()
>>>> operation to fail.
>>>
>>> Why does ECC memory matter here?
>>>
>>
>> This is a test conducted under a special test scenario.
>> ECC memory errors are caused by some test tools.
> 
> What memory is failing, and why does just this single check matter in
> the whole kernel?
> 
> If hardware is broken, and failing, it's not the job of the kernel to
> protect against that, is it?  Shouldn't the ECC memory controller have
> properly notified the kernel of the fault and reset the machine because
> it is now in an undetermined state?
> 
>>> Are you sure this is correct?  How was this tested?  Seems to me that
>>> this will still return "success" if this code path ever happens, what am
>>
>> You are right. I made a patch error here. The code modification should be like this:
>> if (r < 0) {
>> 	retval = r;
>> 	kfree(buf);
>> 	goto fail;
>> }
> 
> This means that you didn't test this change at all, so I don't really
> think it is needed :(
> 
In fact, currently we have not added this retval assignment operation.
Due to the circumvention of buf access during patch testing.
This problem causes calltrace not to trigger.

Thanks.
Longfang.
> thanks,
> 
> greg k-h
> .
> 

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

[Alan Stern]

On Fri, Jul 21, 2023 at 06:00:15PM +0800, liulongfang wrote:
> On systems that use ECC memory. The ECC error of the memory will
> cause the USB controller to halt. It causes the usb_control_msg()
> operation to fail.

How often does this happen in real life?  (Besides, it seems to me that 
if your system is getting a bunch of ECC memory errors then you've got 
much worse problems than a simple USB failure!)

And why do you worry about ECC memory failures in particular?  Can't 
_any_ kind of failure cause the usb_control_msg() operation to fail?

> At this point, the returned buffer data is an abnormal value, and
> continuing to use it will lead to incorrect results.

The driver already contains code to check for abnormal values.  The 
check is not perfect, but it should prevent things from going too 
badly wrong.

> Therefore, it is necessary to judge the return value and exit.
> 
> Signed-off-by: liulongfang <liulongfang@huawei.com>

There is a flaw in your reasoning.

The operation carried out here is deliberately unsafe (for full-speed 
devices).  It is made before we know the actual maxpacket size for ep0, 
and as a result it might return an error code even when it works okay.  
This shouldn't happen, but a lot of USB hardware is unreliable.

Therefore we must not ignore the result merely because r < 0.  If we do 
that, the kernel might stop working with some devices.

Alan Stern

> ---
>  drivers/usb/core/hub.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index a739403a9e45..6a43198be263 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -4891,6 +4891,16 @@ hub_port_init(struct usb_hub *hub, struct usb_device *udev, int port1,
>  					USB_DT_DEVICE << 8, 0,
>  					buf, GET_DESCRIPTOR_BUFSIZE,
>  					initial_descriptor_timeout);
> +				/* On systems that use ECC memory, ECC errors can
> +				 * cause the USB controller to halt.
> +				 * It causes this operation to fail. At this time,
> +				 * the buf data is an abnormal value and needs to be exited.
> +				 */
> +				if (r < 0) {
> +					kfree(buf);
> +					goto fail;
> +				}
> +
>  				switch (buf->bMaxPacketSize0) {
>  				case 8: case 16: case 32: case 64: case 255:
>  					if (buf->bDescriptorType ==
> -- 
> 2.24.0
> 

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

[Oliver Neukum]

On 21.07.23 16:57, Alan Stern wrote:
  
> There is a flaw in your reasoning.
> 
> The operation carried out here is deliberately unsafe (for full-speed
> devices).  It is made before we know the actual maxpacket size for ep0,
> and as a result it might return an error code even when it works okay.
> This shouldn't happen, but a lot of USB hardware is unreliable.
> 
> Therefore we must not ignore the result merely because r < 0.  If we do
> that, the kernel might stop working with some devices.

Right. However, we must make sure we are operating on controlled results.
As is we are operating on a random buffer without checking an IO operation
has been performed on it.

	Regards
		Oliver

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

[liulongfang]

On 2023/7/21 22:57, Alan Stern Wrote:
> On Fri, Jul 21, 2023 at 06:00:15PM +0800, liulongfang wrote:
>> On systems that use ECC memory. The ECC error of the memory will
>> cause the USB controller to halt. It causes the usb_control_msg()
>> operation to fail.
> 
> How often does this happen in real life?  (Besides, it seems to me that 
> if your system is getting a bunch of ECC memory errors then you've got 
> much worse problems than a simple USB failure!)
>

This problem is on ECC memory platform.
In the test scenario, the problem is 100% reproducible.

> And why do you worry about ECC memory failures in particular?  Can't 
> _any_ kind of failure cause the usb_control_msg() operation to fail?
> 
>> At this point, the returned buffer data is an abnormal value, and
>> continuing to use it will lead to incorrect results.
> 
> The driver already contains code to check for abnormal values.  The 
> check is not perfect, but it should prevent things from going too 
> badly wrong.
>

If it is ECC memory error. These parameter checks would also
actually be invalid.

>> Therefore, it is necessary to judge the return value and exit.
>>
>> Signed-off-by: liulongfang <liulongfang@huawei.com>
> 
> There is a flaw in your reasoning.
> 
> The operation carried out here is deliberately unsafe (for full-speed 
> devices).  It is made before we know the actual maxpacket size for ep0, 
> and as a result it might return an error code even when it works okay.  
> This shouldn't happen, but a lot of USB hardware is unreliable.
> 
> Therefore we must not ignore the result merely because r < 0.  If we do 
> that, the kernel might stop working with some devices.
> 
It may be that the handling solution for ECC errors is different from that
of the OS platform. On the test platform, after usb_control_msg() fails,
reading the memory data of buf will directly lead to kernel crash:

[ T14] Call trace:
[ T14] hub_port_init+0x280/0x9f0
[ T14] hub_port_connect+0x1d4/0xa40
[ T14] hub_port_connect_change+0xb8/0x2b0
[ T14] port_event+0x430/0x5d0
[ T14] hub_event+0x138/0x4a0
[ T14] process_one_work+0x1c8/0x39c
[ T14] worker_thread+0x150/0x3d0
[ T14] kthread+0xfc/0x130
[ T14] ret_from_fork+0x10/0x18
[ T14] Code: 528000c2 b9007fea 94002c9a b9407fea (39401f41)

thanks,
Longfang.
> Alan Stern
> 
>> ---
>>  drivers/usb/core/hub.c | 10 ++++++++++
>>  1 file changed, 10 insertions(+)
>>
>> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
>> index a739403a9e45..6a43198be263 100644
>> --- a/drivers/usb/core/hub.c
>> +++ b/drivers/usb/core/hub.c
>> @@ -4891,6 +4891,16 @@ hub_port_init(struct usb_hub *hub, struct usb_device *udev, int port1,
>>  					USB_DT_DEVICE << 8, 0,
>>  					buf, GET_DESCRIPTOR_BUFSIZE,
>>  					initial_descriptor_timeout);
>> +				/* On systems that use ECC memory, ECC errors can
>> +				 * cause the USB controller to halt.
>> +				 * It causes this operation to fail. At this time,
>> +				 * the buf data is an abnormal value and needs to be exited.
>> +				 */
>> +				if (r < 0) {
>> +					kfree(buf);
>> +					goto fail;
>> +				}
>> +
>>  				switch (buf->bMaxPacketSize0) {
>>  				case 8: case 16: case 32: case 64: case 255:
>>  					if (buf->bDescriptorType ==
>> -- 
>> 2.24.0
>>
> 
> .
> 

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

[Alan Stern]

On Wed, Jul 26, 2023 at 02:58:18PM +0800, liulongfang wrote:
> On 2023/7/21 22:57, Alan Stern Wrote:
> > On Fri, Jul 21, 2023 at 06:00:15PM +0800, liulongfang wrote:
> >> On systems that use ECC memory. The ECC error of the memory will
> >> cause the USB controller to halt. It causes the usb_control_msg()
> >> operation to fail.
> > 
> > How often does this happen in real life?  (Besides, it seems to me that 
> > if your system is getting a bunch of ECC memory errors then you've got 
> > much worse problems than a simple USB failure!)
> >
> 
> This problem is on ECC memory platform.
> In the test scenario, the problem is 100% reproducible.
> 
> > And why do you worry about ECC memory failures in particular?  Can't 
> > _any_ kind of failure cause the usb_control_msg() operation to fail?
> > 
> >> At this point, the returned buffer data is an abnormal value, and
> >> continuing to use it will lead to incorrect results.
> > 
> > The driver already contains code to check for abnormal values.  The 
> > check is not perfect, but it should prevent things from going too 
> > badly wrong.
> >
> 
> If it is ECC memory error. These parameter checks would also
> actually be invalid.
> 
> >> Therefore, it is necessary to judge the return value and exit.
> >>
> >> Signed-off-by: liulongfang <liulongfang@huawei.com>
> > 
> > There is a flaw in your reasoning.
> > 
> > The operation carried out here is deliberately unsafe (for full-speed 
> > devices).  It is made before we know the actual maxpacket size for ep0, 
> > and as a result it might return an error code even when it works okay.  
> > This shouldn't happen, but a lot of USB hardware is unreliable.
> > 
> > Therefore we must not ignore the result merely because r < 0.  If we do 
> > that, the kernel might stop working with some devices.
> > 
> It may be that the handling solution for ECC errors is different from that
> of the OS platform. On the test platform, after usb_control_msg() fails,
> reading the memory data of buf will directly lead to kernel crash:

All right, then here's a proposal for a different way to solve the 
problem: Change the kernel's handler for the ECC error notification.  
Have it clear the affected parts of memory, so that the kernel can go 
ahead and use them without crashing.

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.

Alan Stern

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

[liulongfang]

On 2023/7/26 22:20, Alan Stern wrote:
> On Wed, Jul 26, 2023 at 02:58:18PM +0800, liulongfang wrote:
>> On 2023/7/21 22:57, Alan Stern Wrote:
>>> On Fri, Jul 21, 2023 at 06:00:15PM +0800, liulongfang wrote:
>>>> On systems that use ECC memory. The ECC error of the memory will
>>>> cause the USB controller to halt. It causes the usb_control_msg()
>>>> operation to fail.
>>>
>>> How often does this happen in real life?  (Besides, it seems to me that 
>>> if your system is getting a bunch of ECC memory errors then you've got 
>>> much worse problems than a simple USB failure!)
>>>
>>
>> This problem is on ECC memory platform.
>> In the test scenario, the problem is 100% reproducible.
>>
>>> And why do you worry about ECC memory failures in particular?  Can't 
>>> _any_ kind of failure cause the usb_control_msg() operation to fail?
>>>
>>>> At this point, the returned buffer data is an abnormal value, and
>>>> continuing to use it will lead to incorrect results.
>>>
>>> The driver already contains code to check for abnormal values.  The 
>>> check is not perfect, but it should prevent things from going too 
>>> badly wrong.
>>>
>>
>> If it is ECC memory error. These parameter checks would also
>> actually be invalid.
>>
>>>> Therefore, it is necessary to judge the return value and exit.
>>>>
>>>> Signed-off-by: liulongfang <liulongfang@huawei.com>
>>>
>>> There is a flaw in your reasoning.
>>>
>>> The operation carried out here is deliberately unsafe (for full-speed 
>>> devices).  It is made before we know the actual maxpacket size for ep0, 
>>> and as a result it might return an error code even when it works okay.  
>>> This shouldn't happen, but a lot of USB hardware is unreliable.
>>>
>>> Therefore we must not ignore the result merely because r < 0.  If we do 
>>> that, the kernel might stop working with some devices.
>>>
>> It may be that the handling solution for ECC errors is different from that
>> of the OS platform. On the test platform, after usb_control_msg() fails,
>> reading the memory data of buf will directly lead to kernel crash:
> 
> All right, then here's a proposal for a different way to solve the 
> problem: Change the kernel's handler for the ECC error notification.  
> Have it clear the affected parts of memory, so that the kernel can go 
> ahead and use them without crashing.
> 
> 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.
> 

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.

thanks,
Longfang.

> Alan Stern
> 
> .
> 

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

[Oliver Neukum]

On 27.07.23 09:03, liulongfang wrote:

> 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.

For how long? That information is cruical.

	Regards
		Oliver

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

[Alan Stern]

On Thu, Jul 27, 2023 at 03:03:57PM +0800, liulongfang wrote:
> On 2023/7/26 22:20, Alan Stern wrote:
> >> It may be that the handling solution for ECC errors is different from that
> >> of the OS platform. On the test platform, after usb_control_msg() fails,
> >> reading the memory data of buf will directly lead to kernel crash:
> > 
> > All right, then here's a proposal for a different way to solve the 
> > problem: Change the kernel's handler for the ECC error notification.  
> > Have it clear the affected parts of memory, so that the kernel can go 
> > ahead and use them without crashing.
> > 
> > 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.
> > 
> 
> 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.

Now think about what will happen if usb_control_msg works successfully 
but an ECC memory error occurs when the return code from the function 
call is stored in r?  Won't the kernel crash then?  Or if not then, when 
it reads the value of r a few lines later?

So why bother to handle the first kind of ECC error but not the second?  
What makes one ECC error more important than another?

Alan Stern

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

[Oliver Neukum]

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.

>> 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;

	Regards
		Oliver

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

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

[liulongfang]

On 2023/7/27 23:57, Alan Stern wrote:
> 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
>

OK, thanks.
Longfang.
> .
>