usb: dwc2: extend treatment for incomplete transfer

<[email protected]>, Greg Kroah-Hartman <[email protected]>, Minas Harutyunyan <[email protected]>, William Wu <[email protected]>, Dmitry Torokhov <[email protected]>, Douglas Anderson <[email protected]>

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

In the meantime, can you by any chance test the attached patch ? It _might_ fix the problem, but it is a bit of a wild shot.

Unfortunately, I have been unable to reproduce the problem. It is seen only with certain phones and with certain Ethernet adapters, and I was unable to get any of those. I'll keep trying.

Hi Boris,

Quoted 11 lines · Boris ARZUR

<[email protected]>, Greg Kroah-Hartman <[email protected]>, Minas Harutyunyan <[email protected]>, William Wu <[email protected]>, Dmitry Torokhov <[email protected]>, Douglas Anderson <[email protected]>

Hello Guenter,

Quoted 2 lines

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

Quoted 8 lines

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

Unfortunately, I have been unable to reproduce the problem. It is seen only with certain phones and with certain Ethernet adapters, and I was unable to get any of those. I'll keep trying.

In the meantime, can you by any chance test the attached patch ? It _might_ fix the problem, but it is a bit of a wild shot.

Thanks,
Guenter

---
From 29e0949531a27f14a5b46d70e34aa43546e6a3d1 Mon Sep 17 00:00:00 2001
From: Guenter Roeck <[email protected]>
Date: Mon, 10 Feb 2020 13:11:00 -0800
Subject: [PATCH] usb: dwc2: constrain endpoint transfer size on split IN

The following messages are seen on Veyron Chromebooks running v4.19 or later kernels.

dwc2 ff580000.usb: dwc2_update_urb_state(): trimming xfer length
dwc2 ff580000.usb: dwc2_hc_chhltd_intr_dma: Channel 7 - ChHltd set, but reason is unknown dwc2 ff580000.usb: hcint 0x00000002, intsts 0x04600021

This is typically followed by a crash.

Unable to handle kernel paging request at virtual address 29f9d9fc
pgd = 4797dac9
[29f9d9fc] *pgd=80000000004003, *pmd=00000000
Internal error: Oops: a06 [#1] PREEMPT SMP ARM
Modules linked in: ip6t_REJECT rfcomm i2c_dev uinput hci_uart btbcm ...
CPU: 0 PID: 0 Comm: swapper/0 Tainted: G        W         4.19.87-07825-g4ab3515f6e4d #1
Hardware name: Rockchip (Device Tree)
PC is at memcpy+0x50/0x330

LR is at 0xdd9ac94
...
[<c0a89f50>] (memcpy) from [<c0783b94>] (dwc2_free_dma_aligned_buffer+0x5c/0x7c) [<c0783b94>] (dwc2_free_dma_aligned_buffer) from [<c0765dcc>] (__usb_hcd_giveback_urb+0x78/0x130) [<c0765dcc>] (__usb_hcd_giveback_urb) from [<c07678fc>] (usb_giveback_urb_bh+0xa0/0xe4) [<c07678fc>] (usb_giveback_urb_bh) from [<c023a164>] (tasklet_action_common+0xc0/0xdc) [<c023a164>] (tasklet_action_common) from [<c02021f0>] (__do_softirq+0x1b8/0x434) [<c02021f0>] (__do_softirq) from [<c0239a14>] (irq_exit+0xdc/0xe0)
[<c0239a14>] (irq_exit) from [<c029f260>] (__handle_domain_irq+0x94/0xd0) [<c029f260>] (__handle_domain_irq) from [<c05da780>] (gic_handle_irq+0x74/0xb0) [<c05da780>] (gic_handle_irq) from [<c02019f8>] (__irq_svc+0x58/0x8c)

The crash suggests that the memory after the end of a temporary DMA-aligned buffer is overwritten.

The Raspberry Pi Linux kernel includes a patch suggesting that a similar problem was observed with the dwg2 otc driver used there. The patch description is as follows.

The hcd would unconditionally set the transfer length to the endpoint packet size for non-isoc IN transfers. If the remaining buffer length was less than the length of returned data, random memory would get scribbled over, with bad effects if it crossed a page boundary.

Force a babble error if this happens by limiting the max transfer size to the available buffer space. DMA will stop writing to memory on a babble condition.

Apply the same fix to this driver.

Reported-by Boris ARZUR <[email protected]>

Cc Boris ARZUR <[email protected]>

Cc Jonathan Bell <[email protected]>

Signed-off-by Guenter Roeck <[email protected]>

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

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

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

index b90f858af960..2c81b346b464 100644
@@ -1264,7 +1264,8 @@ static void dwc2_hc_start_transfer(struct dwc2_hsotg *hsotg,
 			 */
 			chan->xfer_len = 0;
 		else if (chan->ep_is_in || chan->xfer_len > chan->max_packet)
-			chan->xfer_len = chan->max_packet;
+			chan->xfer_len = min_t(uint32_t, chan->xfer_len,
+					       chan->max_packet);
 		else if (!chan->ep_is_in && chan->xfer_len > 188)
 			chan->xfer_len = 188;
 

Signature · 1 line

2.17.1

Hello Guenter,

Quoted 2 lines

In the meantime, can you by any chance test the attached patch ? It _might_ fix the problem, but it is a bit of a wild shot.

I tried your patch, but the machine does not finish booting.

I would like to give you a dump, but the screen scrolls fast, and what's left when paused is not interesting. How do I get it to dump on disk?

My journalctl doesn't show anything. I have no kmesg.log anywhere. The first time around I was 0/ changing fonts 1/ trimming the dump message in the kernel 2/ filming my screen. That's not practical at all...

I have been looking a bit at things. I believe that part of the issue is the need to re-align the buffer we get in the URB. I'm wondering if asking for a specific alignment when creating the URB could be doable.

As a stop-gap, maybe doing things like in tegra ehci could fix our bug:
https://github.com/torvalds/linux/blob/master/drivers/usb/host/ehci-tegra.c#L288
i.e. having the old pointer before the new buffer instead of at the end of it.

Now if something is overwriting the buffer end, that would also be hiding the issue... but if the bug is related to lengths that don't match between allocation and free, that could work. In this case, that would also be hiding the issue :)

Hello Guenter,

Quoted 2 lines

In the meantime, can you by any chance test the attached patch ? It _might_ fix the problem, but it is a bit of a wild shot.

I tried your patch, but the machine does not finish booting.

I would like to give you a dump, but the screen scrolls fast, and what's left when paused is not interesting. How do I get it to dump on disk?

My journalctl doesn't show anything. I have no kmesg.log anywhere. The first time around I was 0/ changing fonts 1/ trimming the dump message in the kernel 2/ filming my screen. That's not practical at all...

I have been looking a bit at things. I believe that part of the issue is the need to re-align the buffer we get in the URB. I'm wondering if asking for a specific alignment when creating the URB could be doable.

As a stop-gap, maybe doing things like in tegra ehci could fix our bug:
https://github.com/torvalds/linux/blob/master/drivers/usb/host/ehci-tegra.c#L288
i.e. having the old pointer before the new buffer instead of at the end of it.

Now if something is overwriting the buffer end, that would also be hiding the issue... but if the bug is related to lengths that don't match between allocation and free, that could work. In this case, that would also be hiding the issue :)

Quoted 4 lines

Unfortunately, I have been unable to reproduce the problem. It is seen only with certain phones and with certain Ethernet adapters, and I was unable to get any of those. I'll keep trying.

If you want, I can run a kernel with some printk instrumentation or run experiments. I'll research a bit on how to get that kernel oops data, that

Quoted 2 lines

The first time around I was 0/ changing fonts 1/ trimming the dump message in the kernel 2/ filming my screen. That's not practical at all...

Hi Boris,

Quoted 25 lines · Boris ARZUR

Hello Guenter,

Quoted 2 lines

In the meantime, can you by any chance test the attached patch ? It _might_ fix the problem, but it is a bit of a wild shot.

I tried your patch, but the machine does not finish booting.

I would like to give you a dump, but the screen scrolls fast, and what's left when paused is not interesting. How do I get it to dump on disk?

My journalctl doesn't show anything. I have no kmesg.log anywhere. The first time around I was 0/ changing fonts 1/ trimming the dump message in the kernel 2/ filming my screen. That's not practical at all...

I have been looking a bit at things. I believe that part of the issue is the need to re-align the buffer we get in the URB. I'm wondering if asking for a specific alignment when creating the URB could be doable.

As a stop-gap, maybe doing things like in tegra ehci could fix our bug:
https://github.com/torvalds/linux/blob/master/drivers/usb/host/ehci-tegra.c#L288
i.e. having the old pointer before the new buffer instead of at the end of it.

Quoted 5 lines

Now if something is overwriting the buffer end, that would also be hiding the issue... but if the bug is related to lengths that don't match between allocation and free, that could work. In this case, that would also be hiding the issue :)

See below for a patch (untested) doing just that. It may solve your immediate problem, though it would still suffer from the buffer end overwrite.

Quoted 6 lines

Quoted 4 lines

Unfortunately, I have been unable to reproduce the problem. It is seen only with certain phones and with certain Ethernet adapters, and I was unable to get any of those. I'll keep trying.

If you want, I can run a kernel with some printk instrumentation or run experiments. I'll research a bit on how to get that kernel oops data, that

Unfortunately I have no real idea what to look out for. The problem may be related to the phone sending more than one Ethernet packet in a single USB transfer. See rndis_rx_fixup() for how that is handled in the driver. I don't know how that would result in the observed problem, though.

Thanks,
Guenter

---
From 8efa9c598f2390dca2e97cbbe41e981caba41ca1 Mon Sep 17 00:00:00 2001
From: Guenter Roeck <[email protected]>
Date: Mon, 10 Feb 2020 14:04:06 -0800
Subject: [PATCH] usb: dwc2: Simplify DMA alignment code

The code to align buffers for DMA was first introduced with commit 3bc04e28a030 ("usb: dwc2: host: Get aligned DMA in a more supported way"). It was updated with commit 56406e017a88 ("usb: dwc2: Fix DMA alignment to start at allocated boundary") because it did not really align buffers to DMA boundaries but to DWC2_USB_DMA_ALIGN. This was then optimized in commit 1e111e885238 ("usb: dwc2: Fix inefficient copy of unaligned buffers") to only copy actual data rather than the whole buffer. Commit 4a4863bf2e79 ("usb: dwc2: Fix DMA cache alignment issues") changed this further to add a padding at the end of the buffer to ensure that the old data pointer is not in the same cache line as the buffer.

This last commit states "Otherwise, the stored_xfer_buffer gets corrupted for IN URBs on non-cache-coherent systems". However, such corruptions are still observed. Either case, DMA is not expected to overwrite more memory than it is supposed to do, suggesting that the commit may have been hiding a problem rather than fixing it.

On top of that, DMA alignment is still not guaranteed since it only happens if the original buffer is not aligned to DWC2_USB_DMA_ALIGN, which is still a constant of 4 and not associated with DMA alignment.

Move the old data pointer back to the beginning of the new buffer, restoring most of the original commit and to simplify the code. Define DWC2_USB_DMA_ALIGN to dma_get_cache_alignment() to fix the DMA alignment for real this time.

Signed-off-by Guenter Roeck <[email protected]>

---
drivers/usb/dwc2/hcd.c | 50 +++++++++++++++++++-----------------------

1 file changed, 22 insertions(+), 28 deletions(-)

drivers/usb/dwc2/hcd.c +22 -28

index 2c81b346b464..9e04b3a314eb 100644
@@ -2470,21 +2470,24 @@ static int dwc2_alloc_split_dma_aligned_buf(struct dwc2_hsotg *hsotg,
 	return 0;
 }
 
-#define DWC2_USB_DMA_ALIGN 4
+#define DWC2_USB_DMA_ALIGN	dma_get_cache_alignment()
+
+struct dma_aligned_buffer {
+	void *kmalloc_ptr;
+	void *old_xfer_buffer;
+	u8 data[0];
+};
 
 static void dwc2_free_dma_aligned_buffer(struct urb *urb)
 {
-	void *stored_xfer_buffer;
+	struct dma_aligned_buffer *temp;
 	size_t length;
 
 	if (!(urb->transfer_flags & URB_ALIGNED_TEMP_BUFFER))
 		return;
 
-	/* Restore urb->transfer_buffer from the end of the allocated area */
-	memcpy(&stored_xfer_buffer,
-	       PTR_ALIGN(urb->transfer_buffer + urb->transfer_buffer_length,
-			 dma_get_cache_alignment()),
-	       sizeof(urb->transfer_buffer));
+	temp = container_of(urb->transfer_buffer,
+			    struct dma_aligned_buffer, data);
 
 	if (usb_urb_dir_in(urb)) {
 		if (usb_pipeisoc(urb->pipe))

@@ -2492,17 +2495,17 @@ static void dwc2_free_dma_aligned_buffer(struct urb *urb)
 		else
 			length = urb->actual_length;
 
-		memcpy(stored_xfer_buffer, urb->transfer_buffer, length);
+		memcpy(temp->old_xfer_buffer, temp->data, length);
 	}
-	kfree(urb->transfer_buffer);
-	urb->transfer_buffer = stored_xfer_buffer;
+	urb->transfer_buffer = temp->old_xfer_buffer;
+	kfree(temp->kmalloc_ptr);
 
 	urb->transfer_flags &= ~URB_ALIGNED_TEMP_BUFFER;
 }
 
 static int dwc2_alloc_dma_aligned_buffer(struct urb *urb, gfp_t mem_flags)
 {
-	void *kmalloc_ptr;
+	struct dma_aligned_buffer *temp, *kmalloc_ptr;
 	size_t kmalloc_size;
 
 	if (urb->num_sgs || urb->sg ||

@@ -2510,31 +2513,22 @@ static int dwc2_alloc_dma_aligned_buffer(struct urb *urb, gfp_t mem_flags)
 	    !((uintptr_t)urb->transfer_buffer & (DWC2_USB_DMA_ALIGN - 1)))
 		return 0;
 
-	/*
-	 * Allocate a buffer with enough padding for original transfer_buffer
-	 * pointer. This allocation is guaranteed to be aligned properly for
-	 * DMA
-	 */
+	/* Allocate a buffer with enough padding for alignment */
 	kmalloc_size = urb->transfer_buffer_length +
-		(dma_get_cache_alignment() - 1) +
-		sizeof(urb->transfer_buffer);
+		sizeof(struct dma_aligned_buffer) + DWC2_USB_DMA_ALIGN - 1;
 
 	kmalloc_ptr = kmalloc(kmalloc_size, mem_flags);
 	if (!kmalloc_ptr)
 		return -ENOMEM;
 
-	/*
-	 * Position value of original urb->transfer_buffer pointer to the end
-	 * of allocation for later referencing
-	 */
-	memcpy(PTR_ALIGN(kmalloc_ptr + urb->transfer_buffer_length,
-			 dma_get_cache_alignment()),
-	       &urb->transfer_buffer, sizeof(urb->transfer_buffer));
-
+	/* Position our struct dma_aligned_buffer such that data is aligned */
+	temp = PTR_ALIGN(kmalloc_ptr + 1, DWC2_USB_DMA_ALIGN) - 1;
+	temp->kmalloc_ptr = kmalloc_ptr;
+	temp->old_xfer_buffer = urb->transfer_buffer;
 	if (usb_urb_dir_out(urb))
-		memcpy(kmalloc_ptr, urb->transfer_buffer,
+		memcpy(temp->data, urb->transfer_buffer,
 		       urb->transfer_buffer_length);
-	urb->transfer_buffer = kmalloc_ptr;
+	urb->transfer_buffer = temp->data;
 
 	urb->transfer_flags |= URB_ALIGNED_TEMP_BUFFER;
 

Signature · 1 line

2.17.1

Hi Guenter,

Quoted 2 lines

Quoted 2 lines

The first time around I was 0/ changing fonts 1/ trimming the dump message in the kernel 2/ filming my screen. That's not practical at all...

Mmm, pstore does seem to work on my machine. CHROME_PSTORE is not available to me because I'm not on x86, I just enabled the rest and nothing pops up in /sys/fs/pstore...

So I took pictures (OCR did not help):
- https://funyu.konbu.org/pQUF2P08etcpVxuKZ0A720%2b0/IMG_20200215_134343.jpg.webp
is a stack trace for your earlier patch "min_t", in
https://www.spinics.net/lists/linux-usb/msg191019.html ;
- https://funyu.konbu.org/pQUF2P08etcpVxuKZ0A720%2b0/IMG_20200215_135816.jpg.webp
is a stack trace for your later patch "container_of", in
https://www.spinics.net/lists/linux-usb/msg191074.html .

Both patches crash (without even loading the machine), but "container_of" is a bit more resilient.

Yes, those patches didn't address the core problem. Can you test with the attached two patches ?

Thanks,
Guenter

we should be aligning the data[0] pointer inside the struct instead.

with the alignment hacks altogether and hit the issue with a heavier

pointer are separate and no corruptions should occur.

Hi Guenter,

Quoted 6 lines · Guenter Roeck

Yes, those patches didn't address the core problem. Can you test with the attached two patches ?

Thanks,
Guenter

I took a look at your patch (usb: dwc2: Simplify DMA alignment code) and I don't believe it is correct.

The patch re-introduces the dma_aligned_buffer struct and takes some care to align the beginning of the struct to dma cache lines. However we should be aligning the data[0] pointer inside the struct instead. With the code in the patch data[0] gets pushed to be at an offset from the alignment by kmalloc_ptr and old_xfer_buffer pointers. In other words data[0] is now not aligned to dma cache boundaries.

Reviewing the code got me thinking that what if we stopped playing with the alignment hacks altogether and hit the issue with a heavier hammer instead? Attached you can find a new patch that introduces a list to keep track of the allocations. The code then looks up the entry from the list when it is time to restore the original pointer. This way the allocations for the aligned dma area and the original pointer are separate and no corruptions should occur.

Thoughts, comments? I should note that the patch has received only light testing and not very thorough thinking. I can prepare a proper patch to be sent inline if the idea seems worth exploring further.

Signature · 1 line

Antti

Hi Guenter,

Quoted 6 lines · Guenter Roeck

Yes, those patches didn't address the core problem. Can you test with the attached two patches ?

Thanks,
Guenter

I took a look at your patch (usb: dwc2: Simplify DMA alignment code) and I don't believe it is correct.

The patch re-introduces the dma_aligned_buffer struct and takes some care to align the beginning of the struct to dma cache lines. However we should be aligning the data[0] pointer inside the struct instead. With the code in the patch data[0] gets pushed to be at an offset from the alignment by kmalloc_ptr and old_xfer_buffer pointers. In other words data[0] is now not aligned to dma cache boundaries.

Reviewing the code got me thinking that what if we stopped playing with the alignment hacks altogether and hit the issue with a heavier hammer instead? Attached you can find a new patch that introduces a list to keep track of the allocations. The code then looks up the entry from the list when it is time to restore the original pointer. This way the allocations for the aligned dma area and the original pointer are separate and no corruptions should occur.

Thoughts, comments? I should note that the patch has received only light testing and not very thorough thinking. I can prepare a proper patch to be sent inline if the idea seems worth exploring further.

Quoted 13 lines · Antti Seppälä

Hi Guenter,

Quoted 6 lines · Guenter Roeck

Yes, those patches didn't address the core problem. Can you test with the attached two patches ?

Thanks,
Guenter

I took a look at your patch (usb: dwc2: Simplify DMA alignment code) and I don't believe it is correct.

Quoted 7 lines

The patch re-introduces the dma_aligned_buffer struct and takes some care to align the beginning of the struct to dma cache lines. However we should be aligning the data[0] pointer inside the struct instead. With the code in the patch data[0] gets pushed to be at an offset from the alignment by kmalloc_ptr and old_xfer_buffer pointers. In other words data[0] is now not aligned to dma cache boundaries.

I thought so too initially. However,

temp = PTR_ALIGN(kmalloc_ptr + 1, TEGRA_USB_DMA_ALIGN) - 1;

aligns the structure pointer such that its _end_ is DMA-aligned, which is ->data[0].

Quoted 25 lines · Guenter Roeck

Quoted 13 lines · Antti Seppälä

Hi Guenter,

Quoted 6 lines · Guenter Roeck

Yes, those patches didn't address the core problem. Can you test with the attached two patches ?

Thanks,
Guenter

I took a look at your patch (usb: dwc2: Simplify DMA alignment code) and I don't believe it is correct.

Quoted 7 lines

The patch re-introduces the dma_aligned_buffer struct and takes some care to align the beginning of the struct to dma cache lines. However we should be aligning the data[0] pointer inside the struct instead. With the code in the patch data[0] gets pushed to be at an offset from the alignment by kmalloc_ptr and old_xfer_buffer pointers. In other words data[0] is now not aligned to dma cache boundaries.

I thought so too initially. However,

Quoted 5 lines

temp = PTR_ALIGN(kmalloc_ptr + 1, TEGRA_USB_DMA_ALIGN) - 1;

aligns the structure pointer such that its _end_ is DMA-aligned, which is ->data[0].

Hmm, looks like you're right. I somehow missed the - 1 at the end. Sorry for the noise I guess.

Would be nice to know what makes the previous code prone to pointer corruption issues though. With the added padding that pointer should also be on another dma cache line.

Quoted 16 lines · Boris ARZUR

Hi Guenter,

Quoted 2 lines

Quoted 2 lines

The first time around I was 0/ changing fonts 1/ trimming the dump message in the kernel 2/ filming my screen. That's not practical at all...

Mmm, pstore does seem to work on my machine. CHROME_PSTORE is not available to me because I'm not on x86, I just enabled the rest and nothing pops up in /sys/fs/pstore...

So I took pictures (OCR did not help):
- https://funyu.konbu.org/pQUF2P08etcpVxuKZ0A720%2b0/IMG_20200215_134343.jpg.webp
is a stack trace for your earlier patch "min_t", in
https://www.spinics.net/lists/linux-usb/msg191019.html ;
- https://funyu.konbu.org/pQUF2P08etcpVxuKZ0A720%2b0/IMG_20200215_135816.jpg.webp
is a stack trace for your later patch "container_of", in
https://www.spinics.net/lists/linux-usb/msg191074.html .

Quoted 3 lines

Both patches crash (without even loading the machine), but "container_of" is a bit more resilient.

Yes, those patches didn't address the core problem. Can you test with the attached two patches ?

Thanks,
Guenter

From a1c0551b62b038b495177737828f986961184110 Mon Sep 17 00:00:00 2001
From: Guenter Roeck <[email protected]>
Date: Mon, 10 Feb 2020 14:04:06 -0800
Subject: [PATCH 1/2] usb: dwc2: Simplify DMA alignment code

The code to align buffers for DMA was first introduced with commit 3bc04e28a030 ("usb: dwc2: host: Get aligned DMA in a more supported way"). It was updated with commit 56406e017a88 ("usb: dwc2: Fix DMA alignment to start at allocated boundary") because it did not really align buffers to DMA boundaries but to DWC2_USB_DMA_ALIGN. This was then optimized in commit 1e111e885238 ("usb: dwc2: Fix inefficient copy of unaligned buffers") to only copy actual data rather than the whole buffer. Commit 4a4863bf2e79 ("usb: dwc2: Fix DMA cache alignment issues") changed this further to add a padding at the end of the buffer to ensure that the old data pointer is not in the same cache line as the buffer.

This last commit states "Otherwise, the stored_xfer_buffer gets corrupted for IN URBs on non-cache-coherent systems". However, such corruptions are still observed. Either case, DMA is not expected to overwrite more memory than it is supposed to do, suggesting that the commit may have been hiding a problem rather than fixing it.

On top of that, DMA alignment is still not guaranteed since it only happens if the original buffer is not aligned to DWC2_USB_DMA_ALIGN, which is still a constant of 4 and not really associated with DMA alignment.

Move the old data pointer back to the beginning of the new buffer, restoring most of the original commit and to simplify the code. Define DWC2_USB_DMA_ALIGN to dma_get_cache_alignment() to fix the DMA alignment for real this time.

Signed-off-by Guenter Roeck <[email protected]>

---
drivers/usb/dwc2/hcd.c | 50 +++++++++++++++++++-----------------------

1 file changed, 22 insertions(+), 28 deletions(-)

drivers/usb/dwc2/hcd.c +22 -28

index b90f858af960..b5841197165a 100644
@@ -2469,21 +2469,24 @@ static int dwc2_alloc_split_dma_aligned_buf(struct dwc2_hsotg *hsotg,
 	return 0;
 }
 
-#define DWC2_USB_DMA_ALIGN 4
+#define DWC2_USB_DMA_ALIGN	dma_get_cache_alignment()
+
+struct dma_aligned_buffer {
+	void *kmalloc_ptr;
+	void *old_xfer_buffer;
+	u8 data[0];
+};
 
 static void dwc2_free_dma_aligned_buffer(struct urb *urb)
 {
-	void *stored_xfer_buffer;
+	struct dma_aligned_buffer *temp;
 	size_t length;
 
 	if (!(urb->transfer_flags & URB_ALIGNED_TEMP_BUFFER))
 		return;
 
-	/* Restore urb->transfer_buffer from the end of the allocated area */
-	memcpy(&stored_xfer_buffer,
-	       PTR_ALIGN(urb->transfer_buffer + urb->transfer_buffer_length,
-			 dma_get_cache_alignment()),
-	       sizeof(urb->transfer_buffer));
+	temp = container_of(urb->transfer_buffer,
+			    struct dma_aligned_buffer, data);
 
 	if (usb_urb_dir_in(urb)) {
 		if (usb_pipeisoc(urb->pipe))

@@ -2491,17 +2494,17 @@ static void dwc2_free_dma_aligned_buffer(struct urb *urb)
 		else
 			length = urb->actual_length;
 
-		memcpy(stored_xfer_buffer, urb->transfer_buffer, length);
+		memcpy(temp->old_xfer_buffer, temp->data, length);
 	}
-	kfree(urb->transfer_buffer);
-	urb->transfer_buffer = stored_xfer_buffer;
+	urb->transfer_buffer = temp->old_xfer_buffer;
+	kfree(temp->kmalloc_ptr);
 
 	urb->transfer_flags &= ~URB_ALIGNED_TEMP_BUFFER;
 }
 
 static int dwc2_alloc_dma_aligned_buffer(struct urb *urb, gfp_t mem_flags)
 {
-	void *kmalloc_ptr;
+	struct dma_aligned_buffer *temp, *kmalloc_ptr;
 	size_t kmalloc_size;
 
 	if (urb->num_sgs || urb->sg ||

@@ -2509,31 +2512,22 @@ static int dwc2_alloc_dma_aligned_buffer(struct urb *urb, gfp_t mem_flags)
 	    !((uintptr_t)urb->transfer_buffer & (DWC2_USB_DMA_ALIGN - 1)))
 		return 0;
 
-	/*
-	 * Allocate a buffer with enough padding for original transfer_buffer
-	 * pointer. This allocation is guaranteed to be aligned properly for
-	 * DMA
-	 */
+	/* Allocate a buffer with enough padding for alignment */
 	kmalloc_size = urb->transfer_buffer_length +
-		(dma_get_cache_alignment() - 1) +
-		sizeof(urb->transfer_buffer);
+		sizeof(struct dma_aligned_buffer) + DWC2_USB_DMA_ALIGN - 1;
 
 	kmalloc_ptr = kmalloc(kmalloc_size, mem_flags);
 	if (!kmalloc_ptr)
 		return -ENOMEM;
 
-	/*
-	 * Position value of original urb->transfer_buffer pointer to the end
-	 * of allocation for later referencing
-	 */
-	memcpy(PTR_ALIGN(kmalloc_ptr + urb->transfer_buffer_length,
-			 dma_get_cache_alignment()),
-	       &urb->transfer_buffer, sizeof(urb->transfer_buffer));
-
+	/* Position our struct dma_aligned_buffer such that data is aligned */
+	temp = PTR_ALIGN(kmalloc_ptr + 1, DWC2_USB_DMA_ALIGN) - 1;
+	temp->kmalloc_ptr = kmalloc_ptr;
+	temp->old_xfer_buffer = urb->transfer_buffer;
 	if (usb_urb_dir_out(urb))
-		memcpy(kmalloc_ptr, urb->transfer_buffer,
+		memcpy(temp->data, urb->transfer_buffer,
 		       urb->transfer_buffer_length);
-	urb->transfer_buffer = kmalloc_ptr;
+	urb->transfer_buffer = temp->data;
 
 	urb->transfer_flags |= URB_ALIGNED_TEMP_BUFFER;
 

Signature · 71 lines

2.17.1


From 9df13854b3717f8c16a2012dec614f737bb8c15d Mon Sep 17 00:00:00 2001
From: Guenter Roeck <[email protected]>
Date: Mon, 10 Feb 2020 13:11:00 -0800
Subject: [PATCH 2/2] usb: dwc2: Allocate input buffers as multiples of
wMaxPacketSize
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The following messages are seen on Veyron Chromebooks running v4.19 or
later kernels.

dwc2 ff580000.usb: dwc2_update_urb_state(): trimming xfer length
dwc2 ff580000.usb: dwc2_hc_chhltd_intr_dma: Channel 7 - ChHltd set, but reason is unknown
dwc2 ff580000.usb: hcint 0x00000002, intsts 0x04600021

This is typically followed by a crash.

Unable to handle kernel paging request at virtual address 29f9d9fc
pgd = 4797dac9
[29f9d9fc] *pgd=80000000004003, *pmd=00000000
Internal error: Oops: a06 [#1] PREEMPT SMP ARM
Modules linked in: ip6t_REJECT rfcomm i2c_dev uinput hci_uart btbcm ...
CPU: 0 PID: 0 Comm: swapper/0 Tainted: G        W         4.19.87-07825-g4ab3515f6e4d #1
Hardware name: Rockchip (Device Tree)
PC is at memcpy+0x50/0x330
LR is at 0xdd9ac94
...
[<c0a89f50>] (memcpy) from [<c0783b94>] (dwc2_free_dma_aligned_buffer+0x5c/0x7c)
[<c0783b94>] (dwc2_free_dma_aligned_buffer) from [<c0765dcc>] (__usb_hcd_giveback_urb+0x78/0x130)
[<c0765dcc>] (__usb_hcd_giveback_urb) from [<c07678fc>] (usb_giveback_urb_bh+0xa0/0xe4)
[<c07678fc>] (usb_giveback_urb_bh) from [<c023a164>] (tasklet_action_common+0xc0/0xdc)
[<c023a164>] (tasklet_action_common) from [<c02021f0>] (__do_softirq+0x1b8/0x434)
[<c02021f0>] (__do_softirq) from [<c0239a14>] (irq_exit+0xdc/0xe0)
[<c0239a14>] (irq_exit) from [<c029f260>] (__handle_domain_irq+0x94/0xd0)
[<c029f260>] (__handle_domain_irq) from [<c05da780>] (gic_handle_irq+0x74/0xb0)
[<c05da780>] (gic_handle_irq) from [<c02019f8>] (__irq_svc+0x58/0x8c)

The crash suggests that the memory after the end of a temporary DMA-aligned
buffer is overwritten.

The problem is typically only seen in kernels which include commit
56406e017a88 ("usb: dwc2: Fix DMA alignment to start at allocated
boundary"), presumably because that commit moves the pointer to the old
buffer to the end of the newly allocated buffer, where it is more likely
to be overwritten.

Code analysis shows that the transfer size programmed into the chip for
input transfers is a multiple of an endpoint's maximum packet size
(wMaxPacketSize). In the observed situation, the transfer size and thus
the size of the input buffer is 1522 bytes. With a maximum packet size
of 64 bytes, the chip is programmed to receive up to 1536 bytes of data.
This overwrites the end of the buffer.

To work around the problem, always allocate a multiple of wMaxPacketSize
bytes for receive buffers. Do this even for DMA-aligned buffers if the
receive buffer size is not a multiple of wMaxPacketSize. At the same time,
do not update chan->xfer_len if the transfer size is 0.

Reported-by: Boris ARZUR <[email protected]>
Cc: Boris ARZUR <[email protected]>
Cc: Jonathan Bell <[email protected]>
Cc: Antti Seppälä <[email protected]>
Fixes: 56406e017a88 ("usb: dwc2: Fix DMA alignment to start at allocated boundary")
Signed-off-by: Guenter Roeck <[email protected]>
---
drivers/usb/dwc2/hcd.c | 37 +++++++++++++++++++++++++++----------
1 file changed, 27 insertions(+), 10 deletions(-)

drivers/usb/dwc2/hcd.c +27 -10

index b5841197165a..f27dc11e409c 100644
@@ -1313,18 +1313,20 @@ static void dwc2_hc_start_transfer(struct dwc2_hsotg *hsotg,
 			if (num_packets > max_hc_pkt_count) {
 				num_packets = max_hc_pkt_count;
 				chan->xfer_len = num_packets * chan->max_packet;
+			} else if (chan->ep_is_in) {
+				/*
+				 * Always program an integral # of max packets for IN
+				 * transfers.
+				 * Note: This assumes that the input buffer is
+				 * aligned accordingly.
+				 */
+				chan->xfer_len = num_packets * chan->max_packet;
 			}
 		} else {
 			/* Need 1 packet for transfer length of 0 */
 			num_packets = 1;
 		}
 
-		if (chan->ep_is_in)
-			/*
-			 * Always program an integral # of max packets for IN
-			 * transfers
-			 */
-			chan->xfer_len = num_packets * chan->max_packet;
 
 		if (chan->ep_type == USB_ENDPOINT_XFER_INT ||
 		    chan->ep_type == USB_ENDPOINT_XFER_ISOC)

@@ -2505,16 +2507,31 @@ static void dwc2_free_dma_aligned_buffer(struct urb *urb)
 static int dwc2_alloc_dma_aligned_buffer(struct urb *urb, gfp_t mem_flags)
 {
 	struct dma_aligned_buffer *temp, *kmalloc_ptr;
+	struct usb_host_endpoint *ep = urb->ep;
+	int maxp = usb_endpoint_maxp(&ep->desc);
 	size_t kmalloc_size;
 
-	if (urb->num_sgs || urb->sg ||
-	    urb->transfer_buffer_length == 0 ||
+	if (urb->num_sgs || urb->sg || urb->transfer_buffer_length == 0)
+		return 0;
+
+	/*
+	 * Input transfer buffer size must be a multiple of the endpoint's
+	 * maximum packet size to match the transfer limit programmed into
+	 * the chip.
+	 * See calculation of chan->xfer_len in dwc2_hc_start_transfer().
+	 */
+	if (usb_urb_dir_out(urb))
+		kmalloc_size = urb->transfer_buffer_length;
+	else
+		kmalloc_size = roundup(urb->transfer_buffer_length, maxp);
+
+	if (kmalloc_size == urb->transfer_buffer_length &&
 	    !((uintptr_t)urb->transfer_buffer & (DWC2_USB_DMA_ALIGN - 1)))
 		return 0;
 
 	/* Allocate a buffer with enough padding for alignment */
-	kmalloc_size = urb->transfer_buffer_length +
-		sizeof(struct dma_aligned_buffer) + DWC2_USB_DMA_ALIGN - 1;
+	kmalloc_size += sizeof(struct dma_aligned_buffer) +
+		DWC2_USB_DMA_ALIGN - 1;
 
 	kmalloc_ptr = kmalloc(kmalloc_size, mem_flags);
 	if (!kmalloc_ptr)

Signature · 1 line

2.17.1

Hi Guenter,

I tried your series of patch. rndis_host tethering & loading the machine seems to work fine. No more crashing.

That being said, I now have an issue with regular USB keys (I tried a few):
usb 3-1: reset high-speed USB device number 2 using dwc2

I was able to reproduce this issue with the unpatched kernel, by disabling the early return in dwc2_alloc_dma_aligned_buffer(), see attached. There are times were re-allocation fails, either with your patch or with the (almost-)original code.

In particular it seems that there is a packet of lenght 13, usb_urb_dir_in() == true, usb_pipetype(urb->pipe) == PIPE_BULK, that comes in every 2s or so, that does not reallocate properly.

In the original code, it's not a problem thanks to the early return, but your code wants 512B (maxp) and forces reallocation...

Thanks, Boris.

Quoted 7 lines · Guenter Roeck

Quoted 6 lines · Boris ARZUR

Hi Guenter,

Quoted 2 lines

Quoted 2 lines

The first time around I was 0/ changing fonts 1/ trimming the dump message in the kernel 2/ filming my screen. That's not practical at all...

Mmm, pstore does seem to work on my machine. CHROME_PSTORE is not available to me because I'm not on x86, I just enabled the rest and nothing pops up

Quoted 297 lines

Quoted 10 lines

in /sys/fs/pstore...

So I took pictures (OCR did not help):
- https://funyu.konbu.org/pQUF2P08etcpVxuKZ0A720%2b0/IMG_20200215_134343.jpg.webp
is a stack trace for your earlier patch "min_t", in
https://www.spinics.net/lists/linux-usb/msg191019.html ;
- https://funyu.konbu.org/pQUF2P08etcpVxuKZ0A720%2b0/IMG_20200215_135816.jpg.webp
is a stack trace for your later patch "container_of", in
https://www.spinics.net/lists/linux-usb/msg191074.html .

Quoted 3 lines

Both patches crash (without even loading the machine), but "container_of" is a bit more resilient.

Yes, those patches didn't address the core problem. Can you test with the attached two patches ?

Thanks,
Guenter

From a1c0551b62b038b495177737828f986961184110 Mon Sep 17 00:00:00 2001
From: Guenter Roeck <[email protected]>
Date: Mon, 10 Feb 2020 14:04:06 -0800
Subject: [PATCH 1/2] usb: dwc2: Simplify DMA alignment code

The code to align buffers for DMA was first introduced with commit 3bc04e28a030 ("usb: dwc2: host: Get aligned DMA in a more supported way"). It was updated with commit 56406e017a88 ("usb: dwc2: Fix DMA alignment to start at allocated boundary") because it did not really align buffers to DMA boundaries but to DWC2_USB_DMA_ALIGN. This was then optimized in commit 1e111e885238 ("usb: dwc2: Fix inefficient copy of unaligned buffers") to only copy actual data rather than the whole buffer. Commit 4a4863bf2e79 ("usb: dwc2: Fix DMA cache alignment issues") changed this further to add a padding at the end of the buffer to ensure that the old data pointer is not in the same cache line as the buffer.

This last commit states "Otherwise, the stored_xfer_buffer gets corrupted for IN URBs on non-cache-coherent systems". However, such corruptions are still observed. Either case, DMA is not expected to overwrite more memory than it is supposed to do, suggesting that the commit may have been hiding a problem rather than fixing it.

On top of that, DMA alignment is still not guaranteed since it only happens if the original buffer is not aligned to DWC2_USB_DMA_ALIGN, which is still a constant of 4 and not really associated with DMA alignment.

Move the old data pointer back to the beginning of the new buffer, restoring most of the original commit and to simplify the code. Define DWC2_USB_DMA_ALIGN to dma_get_cache_alignment() to fix the DMA alignment for real this time.

Signed-off-by Guenter Roeck <[email protected]>

---
drivers/usb/dwc2/hcd.c | 50 +++++++++++++++++++-----------------------

1 file changed, 22 insertions(+), 28 deletions(-)

drivers/usb/dwc2/hcd.c +22 -28

index b90f858af960..b5841197165a 100644
@@ -2469,21 +2469,24 @@ static int dwc2_alloc_split_dma_aligned_buf(struct dwc2_hsotg *hsotg,
 	return 0;
 }
 
-#define DWC2_USB_DMA_ALIGN 4
+#define DWC2_USB_DMA_ALIGN	dma_get_cache_alignment()
+
+struct dma_aligned_buffer {
+	void *kmalloc_ptr;
+	void *old_xfer_buffer;
+	u8 data[0];
+};
 
 static void dwc2_free_dma_aligned_buffer(struct urb *urb)
 {
-	void *stored_xfer_buffer;
+	struct dma_aligned_buffer *temp;
 	size_t length;
 
 	if (!(urb->transfer_flags & URB_ALIGNED_TEMP_BUFFER))
 		return;
 
-	/* Restore urb->transfer_buffer from the end of the allocated area */
-	memcpy(&stored_xfer_buffer,
-	       PTR_ALIGN(urb->transfer_buffer + urb->transfer_buffer_length,
-			 dma_get_cache_alignment()),
-	       sizeof(urb->transfer_buffer));
+	temp = container_of(urb->transfer_buffer,
+			    struct dma_aligned_buffer, data);
 
 	if (usb_urb_dir_in(urb)) {
 		if (usb_pipeisoc(urb->pipe))

@@ -2491,17 +2494,17 @@ static void dwc2_free_dma_aligned_buffer(struct urb *urb)
 		else
 			length = urb->actual_length;
 
-		memcpy(stored_xfer_buffer, urb->transfer_buffer, length);
+		memcpy(temp->old_xfer_buffer, temp->data, length);
 	}
-	kfree(urb->transfer_buffer);
-	urb->transfer_buffer = stored_xfer_buffer;
+	urb->transfer_buffer = temp->old_xfer_buffer;
+	kfree(temp->kmalloc_ptr);
 
 	urb->transfer_flags &= ~URB_ALIGNED_TEMP_BUFFER;
 }
 
 static int dwc2_alloc_dma_aligned_buffer(struct urb *urb, gfp_t mem_flags)
 {
-	void *kmalloc_ptr;
+	struct dma_aligned_buffer *temp, *kmalloc_ptr;
 	size_t kmalloc_size;
 
 	if (urb->num_sgs || urb->sg ||

@@ -2509,31 +2512,22 @@ static int dwc2_alloc_dma_aligned_buffer(struct urb *urb, gfp_t mem_flags)
 	    !((uintptr_t)urb->transfer_buffer & (DWC2_USB_DMA_ALIGN - 1)))
 		return 0;
 
-	/*
-	 * Allocate a buffer with enough padding for original transfer_buffer
-	 * pointer. This allocation is guaranteed to be aligned properly for
-	 * DMA
-	 */
+	/* Allocate a buffer with enough padding for alignment */
 	kmalloc_size = urb->transfer_buffer_length +
-		(dma_get_cache_alignment() - 1) +
-		sizeof(urb->transfer_buffer);
+		sizeof(struct dma_aligned_buffer) + DWC2_USB_DMA_ALIGN - 1;
 
 	kmalloc_ptr = kmalloc(kmalloc_size, mem_flags);
 	if (!kmalloc_ptr)
 		return -ENOMEM;
 
-	/*
-	 * Position value of original urb->transfer_buffer pointer to the end
-	 * of allocation for later referencing
-	 */
-	memcpy(PTR_ALIGN(kmalloc_ptr + urb->transfer_buffer_length,
-			 dma_get_cache_alignment()),
-	       &urb->transfer_buffer, sizeof(urb->transfer_buffer));
-
+	/* Position our struct dma_aligned_buffer such that data is aligned */
+	temp = PTR_ALIGN(kmalloc_ptr + 1, DWC2_USB_DMA_ALIGN) - 1;
+	temp->kmalloc_ptr = kmalloc_ptr;
+	temp->old_xfer_buffer = urb->transfer_buffer;
 	if (usb_urb_dir_out(urb))
-		memcpy(kmalloc_ptr, urb->transfer_buffer,
+		memcpy(temp->data, urb->transfer_buffer,
 		       urb->transfer_buffer_length);
-	urb->transfer_buffer = kmalloc_ptr;
+	urb->transfer_buffer = temp->data;
 
 	urb->transfer_flags |= URB_ALIGNED_TEMP_BUFFER;
 

Signature · 71 lines

2.17.1


From 9df13854b3717f8c16a2012dec614f737bb8c15d Mon Sep 17 00:00:00 2001
From: Guenter Roeck <[email protected]>
Date: Mon, 10 Feb 2020 13:11:00 -0800
Subject: [PATCH 2/2] usb: dwc2: Allocate input buffers as multiples of
wMaxPacketSize
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The following messages are seen on Veyron Chromebooks running v4.19 or
later kernels.

dwc2 ff580000.usb: dwc2_update_urb_state(): trimming xfer length
dwc2 ff580000.usb: dwc2_hc_chhltd_intr_dma: Channel 7 - ChHltd set, but reason is unknown
dwc2 ff580000.usb: hcint 0x00000002, intsts 0x04600021

This is typically followed by a crash.

Unable to handle kernel paging request at virtual address 29f9d9fc
pgd = 4797dac9
[29f9d9fc] *pgd=80000000004003, *pmd=00000000
Internal error: Oops: a06 [#1] PREEMPT SMP ARM
Modules linked in: ip6t_REJECT rfcomm i2c_dev uinput hci_uart btbcm ...
CPU: 0 PID: 0 Comm: swapper/0 Tainted: G        W         4.19.87-07825-g4ab3515f6e4d #1
Hardware name: Rockchip (Device Tree)
PC is at memcpy+0x50/0x330
LR is at 0xdd9ac94
...
[<c0a89f50>] (memcpy) from [<c0783b94>] (dwc2_free_dma_aligned_buffer+0x5c/0x7c)
[<c0783b94>] (dwc2_free_dma_aligned_buffer) from [<c0765dcc>] (__usb_hcd_giveback_urb+0x78/0x130)
[<c0765dcc>] (__usb_hcd_giveback_urb) from [<c07678fc>] (usb_giveback_urb_bh+0xa0/0xe4)
[<c07678fc>] (usb_giveback_urb_bh) from [<c023a164>] (tasklet_action_common+0xc0/0xdc)
[<c023a164>] (tasklet_action_common) from [<c02021f0>] (__do_softirq+0x1b8/0x434)
[<c02021f0>] (__do_softirq) from [<c0239a14>] (irq_exit+0xdc/0xe0)
[<c0239a14>] (irq_exit) from [<c029f260>] (__handle_domain_irq+0x94/0xd0)
[<c029f260>] (__handle_domain_irq) from [<c05da780>] (gic_handle_irq+0x74/0xb0)
[<c05da780>] (gic_handle_irq) from [<c02019f8>] (__irq_svc+0x58/0x8c)

The crash suggests that the memory after the end of a temporary DMA-aligned
buffer is overwritten.

The problem is typically only seen in kernels which include commit
56406e017a88 ("usb: dwc2: Fix DMA alignment to start at allocated
boundary"), presumably because that commit moves the pointer to the old
buffer to the end of the newly allocated buffer, where it is more likely
to be overwritten.

Code analysis shows that the transfer size programmed into the chip for
input transfers is a multiple of an endpoint's maximum packet size
(wMaxPacketSize). In the observed situation, the transfer size and thus
the size of the input buffer is 1522 bytes. With a maximum packet size
of 64 bytes, the chip is programmed to receive up to 1536 bytes of data.
This overwrites the end of the buffer.

To work around the problem, always allocate a multiple of wMaxPacketSize
bytes for receive buffers. Do this even for DMA-aligned buffers if the
receive buffer size is not a multiple of wMaxPacketSize. At the same time,
do not update chan->xfer_len if the transfer size is 0.

Reported-by: Boris ARZUR <[email protected]>
Cc: Boris ARZUR <[email protected]>
Cc: Jonathan Bell <[email protected]>
Cc: Antti Seppälä <[email protected]>
Fixes: 56406e017a88 ("usb: dwc2: Fix DMA alignment to start at allocated boundary")
Signed-off-by: Guenter Roeck <[email protected]>
---
drivers/usb/dwc2/hcd.c | 37 +++++++++++++++++++++++++++----------
1 file changed, 27 insertions(+), 10 deletions(-)

drivers/usb/dwc2/hcd.c +27 -10

index b5841197165a..f27dc11e409c 100644
@@ -1313,18 +1313,20 @@ static void dwc2_hc_start_transfer(struct dwc2_hsotg *hsotg,
 			if (num_packets > max_hc_pkt_count) {
 				num_packets = max_hc_pkt_count;
 				chan->xfer_len = num_packets * chan->max_packet;
+			} else if (chan->ep_is_in) {
+				/*
+				 * Always program an integral # of max packets for IN
+				 * transfers.
+				 * Note: This assumes that the input buffer is
+				 * aligned accordingly.
+				 */
+				chan->xfer_len = num_packets * chan->max_packet;
 			}
 		} else {
 			/* Need 1 packet for transfer length of 0 */
 			num_packets = 1;
 		}
 
-		if (chan->ep_is_in)
-			/*
-			 * Always program an integral # of max packets for IN
-			 * transfers
-			 */
-			chan->xfer_len = num_packets * chan->max_packet;
 
 		if (chan->ep_type == USB_ENDPOINT_XFER_INT ||
 		    chan->ep_type == USB_ENDPOINT_XFER_ISOC)

@@ -2505,16 +2507,31 @@ static void dwc2_free_dma_aligned_buffer(struct urb *urb)
 static int dwc2_alloc_dma_aligned_buffer(struct urb *urb, gfp_t mem_flags)
 {
 	struct dma_aligned_buffer *temp, *kmalloc_ptr;
+	struct usb_host_endpoint *ep = urb->ep;
+	int maxp = usb_endpoint_maxp(&ep->desc);
 	size_t kmalloc_size;
 
-	if (urb->num_sgs || urb->sg ||
-	    urb->transfer_buffer_length == 0 ||
+	if (urb->num_sgs || urb->sg || urb->transfer_buffer_length == 0)
+		return 0;
+
+	/*
+	 * Input transfer buffer size must be a multiple of the endpoint's
+	 * maximum packet size to match the transfer limit programmed into
+	 * the chip.
+	 * See calculation of chan->xfer_len in dwc2_hc_start_transfer().
+	 */
+	if (usb_urb_dir_out(urb))
+		kmalloc_size = urb->transfer_buffer_length;
+	else
+		kmalloc_size = roundup(urb->transfer_buffer_length, maxp);
+
+	if (kmalloc_size == urb->transfer_buffer_length &&
 	    !((uintptr_t)urb->transfer_buffer & (DWC2_USB_DMA_ALIGN - 1)))
 		return 0;
 
 	/* Allocate a buffer with enough padding for alignment */
-	kmalloc_size = urb->transfer_buffer_length +
-		sizeof(struct dma_aligned_buffer) + DWC2_USB_DMA_ALIGN - 1;
+	kmalloc_size += sizeof(struct dma_aligned_buffer) +
+		DWC2_USB_DMA_ALIGN - 1;
 
 	kmalloc_ptr = kmalloc(kmalloc_size, mem_flags);
 	if (!kmalloc_ptr)

Signature · 1 line

2.17.1

Hi Guenter,

I tried your series of patch. rndis_host tethering & loading the machine seems to work fine. No more crashing.

That being said, I now have an issue with regular USB keys (I tried a few):
usb 3-1: reset high-speed USB device number 2 using dwc2

I was able to reproduce this issue with the unpatched kernel, by disabling the early return in dwc2_alloc_dma_aligned_buffer(), see attached. There are times were re-allocation fails, either with your patch or with the (almost-)original code.

In particular it seems that there is a packet of lenght 13, usb_urb_dir_in() == true, usb_pipetype(urb->pipe) == PIPE_BULK, that comes in every 2s or so, that does not reallocate properly.

Hi Guenter,

Quoted 2 lines

Quoted 2 lines

The first time around I was 0/ changing fonts 1/ trimming the dump message in the kernel 2/ filming my screen. That's not practical at all...

Mmm, pstore does seem to work on my machine. CHROME_PSTORE is not available to me because I'm not on x86, I just enabled the rest and nothing pops up in /sys/fs/pstore...

So I took pictures (OCR did not help):
- https://funyu.konbu.org/pQUF2P08etcpVxuKZ0A720%2b0/IMG_20200215_134343.jpg.webp
is a stack trace for your earlier patch "min_t", in
https://www.spinics.net/lists/linux-usb/msg191019.html ;
- https://funyu.konbu.org/pQUF2P08etcpVxuKZ0A720%2b0/IMG_20200215_135816.jpg.webp
is a stack trace for your later patch "container_of", in
https://www.spinics.net/lists/linux-usb/msg191074.html .

Both patches crash (without even loading the machine), but "container_of" is a bit more resilient.

= urb->length). However, the urb is still left in the queue. When it is