usb: dwc2: extend treatment for incomplete transfer

PATCH In review Boris ARZUR 2 participants 5 messages Nov 5, 2019 – Feb 2, 2020

Subsystems (2)

USB DesignWare USB2 DRD IP driver

lore

Share & tools

lore

Boris ARZUR

author

2019-11-05 03:56 UTC #1 lore raw

Hi,

First post in this list, please be lenient.

Replying to self to give some context: I'm on a Asus c201 (rk3288) and I see some crashes with cdc_ether.

Here is how to repro:
- create heavy usb network load: I tether my phone and
netcat some file from it;
- create heavy CPU load (pushd linux; make -j 6)
- observe kernel messages:
dwc2 ff580000.usb: dwc2_hc_chhltd_intr_dma: Channel 7 - ChHltd set, but reason is unknown dwc2 ff580000.usb: hcint 0x00000002, intsts 0x04200021
dwc2 ff580000.usb: ep_type 0x00000002 bulk /* ba: ADDED LOG */

The kernel will write to 0 at line 2494 below in file drivers/usb/dwc2/hcd.c 2474 static void dwc2_free_dma_aligned_buffer(struct urb *urb) 2475 {
/* ... */
2482 /* Restore urb->transfer_buffer from the end of the allocated area */
2483 memcpy(&stored_xfer_buffer,
2484 PTR_ALIGN(urb->transfer_buffer + urb->transfer_buffer_length,
2485 dma_get_cache_alignment()),
2486 sizeof(urb->transfer_buffer));
/* ... */
2494 memcpy(stored_xfer_buffer, urb->transfer_buffer, length);
/* ... */
2500 }

The fix I propose has been working fine on my machine, but I confess I am less than familiar with this area...

My guess is that the kernel misses some deadlines due to contention and we see channel halts. I tried treating these as we do the other (with other end point types) and it solved the crashes. I verified on next-20191030 that the data is correctly transfered over the network (no corruption).

Thank you & regards,
Boris.

Quoted 26 lines

Channel halt can happen with BULK endpoints when the cpu is under high load. Treating it as an error leads to a null-pointer dereference in dwc2_free_dma_aligned_buffer().

Signed-off-by Boris Arzur <[email protected]>

---
drivers/usb/dwc2/hcd_intr.c | 3 ++-

1 file changed, 2 insertions(+), 1 deletion(-)

drivers/usb/dwc2/hcd_intr.c +2 -1

index a052d39b4375..697fed530aeb 100644
@@ -1944,7 +1944,8 @@ static void dwc2_hc_chhltd_intr_dma(struct dwc2_hsotg
 *hsotg,
                        */
                       dwc2_hc_ack_intr(hsotg, chan, chnum, qtd);
               } else {
-                       if (chan->ep_type == USB_ENDPOINT_XFER_INT ||
+                       if (chan->ep_type == USB_ENDPOINT_XFER_BULK ||
+                           chan->ep_type == USB_ENDPOINT_XFER_INT ||
                           chan->ep_type == USB_ENDPOINT_XFER_ISOC) {
                               /*

* A periodic transfer halted with no other
--
2.23.0

Guenter Roeck

2020-01-31 22:09 UTC #2 lore raw

In reply to Boris ARZUR

Hi Boris,

Quoted 4 lines · Boris ARZUR

Channel halt can happen with BULK endpoints when the cpu is under high load. Treating it as an error leads to a null-pointer dereference in dwc2_free_dma_aligned_buffer().

good find, and good analysis. We stated to see this problem as well in the latest ChromeOS kernel.

I am still trying understand what exactly happens. To do that, I'll need to be able to reproduce the problem. Maybe you can help me. How do you tether your phone through USB ?

Thanks,
Guenter

Quoted 23 lines

Signed-off-by Boris Arzur <[email protected]>

---
 drivers/usb/dwc2/hcd_intr.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

* A periodic transfer halted with no other
--
2.23.0

drivers/usb/dwc2/hcd_intr.c +2 -1

index a052d39b4375..697fed530aeb 100644
@@ -1944,7 +1944,8 @@ static void dwc2_hc_chhltd_intr_dma(struct dwc2_hsotg
 *hsotg,
                         */
                        dwc2_hc_ack_intr(hsotg, chan, chnum, qtd);
                } else {
-                       if (chan->ep_type == USB_ENDPOINT_XFER_INT ||
+                       if (chan->ep_type == USB_ENDPOINT_XFER_BULK ||
+                           chan->ep_type == USB_ENDPOINT_XFER_INT ||
                            chan->ep_type == USB_ENDPOINT_XFER_ISOC) {
                                /*

Boris ARZUR

author

2020-02-02 05:22 UTC #3 lore raw

Hello Guenter,

Quoted 2 lines

good find, and good analysis. We stated to see this problem as well in the latest ChromeOS kernel.

I'm glad you find my report helpful.

Quoted 2 lines

be able to reproduce the problem. Maybe you can help me. How do you tether your phone through USB ?

You mention thethering, so I think you have read my follow-up:
https://www.spinics.net/lists/linux-usb/msg187497.html

My setup is as follows:
- 'kenzo' phone (https://wiki.lineageos.org/devices/kenzo) on AICP 12.1 (android 7.1.2 linux 3.10.105);
- 'veyron speedy' chromebook (https://wiki.gentoo.org/wiki/Asus_Chromebook_C201)
on Arch Linux ARM, vanilla linux 5.2.14;

Here are my repro steps, sorry if tedious, I'm not sure of the level of details you want, so I will go verbose squared :) :

0. plug in phone to chromebook, with a USB2 micro b cable;

1. activate usb tethering in phone settings:
settings> more> tethering & portable hotspot> USB tethering click and confirm "tethered";

2. chromebook sees phone as:

[ 2128.080551] rndis_host 2-1:1.0 usb0: register 'rndis_host' at usb-ff580000.usb-1, RNDIS device, 4a:5e:0c:89:ec:09

3. chromebook$ sudo dhcpcd --noarp usb0
usb0: adding default route via 192.168.42.129

4. on phone, start termux (https://f-droid.org/en/packages/com.termux/)

5. phone$ dd if=/dev/urandom of=blob count=50 bs=1M

6. phone$ sha512sum blob
b9e...14d blob

7. phone$ pkg install netcat

8. phone$ while true; do <blob netcat -l -p 9999; done

9. chromebook$ sudo pacman -Syu extra/gnu-netcat community/pigz

10. chromebook$ dd if=/dev/urandom of=job count=10 bs=1M

11. chromebook terminal 0$ while true; do <job pigz -11 -i -p 1024 >/dev/null; done

12. chromebook terminal 1$ cat /proc/loadavg
28.18 8.76 3.74 54/521 8826

13. chromebook terminal 1$ while true; do netcat 192.168.42.129 9999 | sha512sum; done b9e...14d -

13. chromebook will panic soon (I see repros in tens of seconds);

I managed to track the issue to:

Quoted 3 lines

The kernel will write to 0 at line 2494 below in file drivers/usb/dwc2/hcd.c 2474 static void dwc2_free_dma_aligned_buffer(struct urb *urb)
2494 memcpy(stored_xfer_buffer, urb->transfer_buffer, length);

I discussed the below patch with [email protected], who expressed doubts about its correctness.

I tested it a while back and it seemed solid (no crash & correct hashes), but while writing this mail I see that sometimes the output of sha512sum on the chromebook is wrong... also, I'm thinking that the fix below may be a memory leak.

In conclusion, do not commit, the fix needs more work :)

I hope to restart experimenting in a short while, when I get a bit more free time.

I am waiting for any question you may have, thank you for your time. Boris.

Quoted 41 lines · Guenter Roeck

Hi Boris,
Quoted 4 lines · Boris ARZUR
Channel halt can happen with BULK endpoints when the cpu is under high load. Treating it as an error leads to a null-pointer dereference in dwc2_free_dma_aligned_buffer().
good find, and good analysis. We stated to see this problem as well in the latest ChromeOS kernel.
I am still trying understand what exactly happens. To do that, I'll need to be able to reproduce the problem. Maybe you can help me. How do you tether your phone through USB ?
Thanks,
Guenter
Quoted 23 lines
Signed-off-by Boris Arzur <[email protected]>
---
 drivers/usb/dwc2/hcd_intr.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
* A periodic transfer halted with no other
--
2.23.0
drivers/usb/dwc2/hcd_intr.c +2 -1
index a052d39b4375..697fed530aeb 100644
@@ -1944,7 +1944,8 @@ static void dwc2_hc_chhltd_intr_dma(struct dwc2_hsotg
 *hsotg,
                         */
                        dwc2_hc_ack_intr(hsotg, chan, chnum, qtd);
                } else {
-                       if (chan->ep_type == USB_ENDPOINT_XFER_INT ||
+                       if (chan->ep_type == USB_ENDPOINT_XFER_BULK ||
+                           chan->ep_type == USB_ENDPOINT_XFER_INT ||
                            chan->ep_type == USB_ENDPOINT_XFER_ISOC) {
                                /*

Guenter Roeck

2020-02-02 18:53 UTC #4 lore raw

Hi Boris,

Quoted 53 lines · Boris ARZUR

Hello Guenter,
Quoted 2 lines
good find, and good analysis. We stated to see this problem as well in the latest ChromeOS kernel.
I'm glad you find my report helpful.
Quoted 2 lines
be able to reproduce the problem. Maybe you can help me. How do you tether your phone through USB ?
You mention thethering, so I think you have read my follow-up:
https://www.spinics.net/lists/linux-usb/msg187497.html
My setup is as follows:
- 'kenzo' phone (https://wiki.lineageos.org/devices/kenzo) on AICP 12.1 (android 7.1.2 linux 3.10.105);
- 'veyron speedy' chromebook (https://wiki.gentoo.org/wiki/Asus_Chromebook_C201)
on Arch Linux ARM, vanilla linux 5.2.14;
Here are my repro steps, sorry if tedious, I'm not sure of the level of details you want, so I will go verbose squared :) :
0. plug in phone to chromebook, with a USB2 micro b cable;
1. activate usb tethering in phone settings:
settings> more> tethering & portable hotspot> USB tethering click and confirm "tethered";
2. chromebook sees phone as:
[ 2128.080551] rndis_host 2-1:1.0 usb0: register 'rndis_host' at usb-ff580000.usb-1, RNDIS device, 4a:5e:0c:89:ec:09
3. chromebook$ sudo dhcpcd --noarp usb0
usb0: adding default route via 192.168.42.129
4. on phone, start termux (https://f-droid.org/en/packages/com.termux/)
5. phone$ dd if=/dev/urandom of=blob count=50 bs=1M
6. phone$ sha512sum blob
b9e...14d blob
7. phone$ pkg install netcat
8. phone$ while true; do <blob netcat -l -p 9999; done
9. chromebook$ sudo pacman -Syu extra/gnu-netcat community/pigz
10. chromebook$ dd if=/dev/urandom of=job count=10 bs=1M
11. chromebook terminal 0$ while true; do <job pigz -11 -i -p 1024 >/dev/null; done

Quoted 8 lines

12. chromebook terminal 1$ cat /proc/loadavg
28.18 8.76 3.74 54/521 8826
13. chromebook terminal 1$ while true; do netcat 192.168.42.129 9999 | sha512sum; done b9e...14d -
13. chromebook will panic soon (I see repros in tens of seconds);

Thanks a lot for the information. I'll see if I can reproduce the problem using this (or a similar) approach. Tethering an Android phone isn't really difficult, but the traffic pattern seems to play a role as well.

Quoted 9 lines

I managed to track the issue to:
Quoted 3 lines
The kernel will write to 0 at line 2494 below in file drivers/usb/dwc2/hcd.c 2474 static void dwc2_free_dma_aligned_buffer(struct urb *urb)
2494 memcpy(stored_xfer_buffer, urb->transfer_buffer, length);
I discussed the below patch with [email protected], who expressed doubts about its correctness.

Quoted 8 lines

I tested it a while back and it seemed solid (no crash & correct hashes), but while writing this mail I see that sometimes the output of sha512sum on the chromebook is wrong... also, I'm thinking that the fix below may be a memory leak.
In conclusion, do not commit, the fix needs more work :)

Yes, I suspect that your patch is not a real fix but rather a bandage; that is why I want to reproduce the problem and spend some time trying to figure out what is going on. In a nutshell, even if the current code doesn't handle the situation well, it should not result in the observed problem (which looks like a memory overwrite).

Quoted 7 lines

I hope to restart experimenting in a short while, when I get a bit more free time.
I am waiting for any question you may have, thank you for your time. Boris.

Thanks!

Guenter

Quoted 42 lines

Quoted 41 lines · Guenter Roeck
Hi Boris,
Quoted 4 lines · Boris ARZUR
Channel halt can happen with BULK endpoints when the cpu is under high load. Treating it as an error leads to a null-pointer dereference in dwc2_free_dma_aligned_buffer().
good find, and good analysis. We stated to see this problem as well in the latest ChromeOS kernel.
I am still trying understand what exactly happens. To do that, I'll need to be able to reproduce the problem. Maybe you can help me. How do you tether your phone through USB ?
Thanks,
Guenter
Quoted 23 lines
Signed-off-by Boris Arzur <[email protected]>
---
  drivers/usb/dwc2/hcd_intr.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)
* A periodic transfer halted with no other
--
2.23.0
drivers/usb/dwc2/hcd_intr.c +2 -1
index a052d39b4375..697fed530aeb 100644
@@ -1944,7 +1944,8 @@ static void dwc2_hc_chhltd_intr_dma(struct dwc2_hsotg
 *hsotg,
                          */
                         dwc2_hc_ack_intr(hsotg, chan, chnum, qtd);
                 } else {
-                       if (chan->ep_type == USB_ENDPOINT_XFER_INT ||
+                       if (chan->ep_type == USB_ENDPOINT_XFER_BULK ||
+                           chan->ep_type == USB_ENDPOINT_XFER_INT ||
                             chan->ep_type == USB_ENDPOINT_XFER_ISOC) {
                                 /*