Re: [PATCH] usb: dwc2: extend treatment for incomplete transfer

[Boris ARZUR <boris@konbu.org>]

[-- Attachment #1: Type: text/plain, Size: 12945 bytes --]

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.

Guenter Roeck wrote:
>On Sat, Feb 15, 2020 at 02:36:47PM +0900, Boris ARZUR wrote:
>> Hi Guenter,
>> 
>> >> 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

>From a1c0551b62b038b495177737828f986961184110 Mon Sep 17 00:00:00 2001
>From: Guenter Roeck <linux@roeck-us.net>
>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 <linux@roeck-us.net>
>---
> drivers/usb/dwc2/hcd.c | 50 +++++++++++++++++++-----------------------
> 1 file changed, 22 insertions(+), 28 deletions(-)
>
>diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
>index b90f858af960..b5841197165a 100644
>--- a/drivers/usb/dwc2/hcd.c
>+++ b/drivers/usb/dwc2/hcd.c
>@@ -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;
> 
>-- 
>2.17.1
>

>From 9df13854b3717f8c16a2012dec614f737bb8c15d Mon Sep 17 00:00:00 2001
>From: Guenter Roeck <linux@roeck-us.net>
>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 <boris@konbu.org>
>Cc: Boris ARZUR <boris@konbu.org>
>Cc: Jonathan Bell <jonathan@raspberrypi.org>
>Cc: Antti Seppälä <a.seppala@gmail.com>
>Fixes: 56406e017a88 ("usb: dwc2: Fix DMA alignment to start at allocated boundary")
>Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>---
> drivers/usb/dwc2/hcd.c | 37 +++++++++++++++++++++++++++----------
> 1 file changed, 27 insertions(+), 10 deletions(-)
>
>diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
>index b5841197165a..f27dc11e409c 100644
>--- a/drivers/usb/dwc2/hcd.c
>+++ b/drivers/usb/dwc2/hcd.c
>@@ -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)
>-- 
>2.17.1
>


[-- Attachment #2: break_original.patch --]
[-- Type: text/plain, Size: 1197 bytes --]

diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
index 2192a28..4c45642 100644
--- a/drivers/usb/dwc2/hcd.c
+++ b/drivers/usb/dwc2/hcd.c
@@ -2503,12 +2503,27 @@ static int dwc2_alloc_dma_aligned_buffer(struct urb *urb, gfp_t mem_flags)
 {
 	void *kmalloc_ptr;
 	size_t kmalloc_size;
+	struct usb_host_endpoint *ep = urb->ep;
+	int maxp = usb_endpoint_maxp(&ep->desc);
 
 	if (urb->num_sgs || urb->sg ||
-	    urb->transfer_buffer_length == 0 ||
-	    !((uintptr_t)urb->transfer_buffer & (DWC2_USB_DMA_ALIGN - 1)))
+	    urb->transfer_buffer_length == 0)
 		return 0;
 
+	if (!((uintptr_t)urb->transfer_buffer & (DWC2_USB_DMA_ALIGN - 1))) {
+		//[ 8906.761517] EARLY RET len:13 out:0 in:1 type:3 mx:512
+		//[ 8908.776755] EARLY RET len:13 out:0 in:1 type:3 mx:512
+		if (urb->transfer_buffer_length == 13) {
+			printk("EARLY RET len:%u out:%d in:%d type:%d mx:%d ",
+				urb->transfer_buffer_length,
+				usb_urb_dir_out(urb) ? 1 : 0,
+				usb_urb_dir_in(urb) ? 1 : 0,
+				usb_pipetype(urb->pipe),
+				maxp);
+			return 0;
+		}
+	}
+
 	/*
 	 * Allocate a buffer with enough padding for original transfer_buffer
 	 * pointer. This allocation is guaranteed to be aligned properly for