The transfer() function in dummy_hcd iterates over all queued gadget
requests for a given endpoint via list_for_each_entry(). When the
per-frame bandwidth budget is exhausted mid-request, leaving a
partially-transferred gadget request, the loop continues to the next
queued request instead of stopping. This breaks data ordering in the
URB transfer buffer.

Two consequences:

  1. Data corruption: bytes from subsequent requests are written into
     the URB buffer ahead of the remaining bytes from the incomplete
     request. On the next timer tick the incomplete request resumes,
     appending its remaining data after the out-of-order bytes.

  2. Premature URB completion: if the next request is a ZLP or shorter
     than the remaining host buffer, it triggers the is_short path and
     completes the URB before all data has been transferred.

Fix this by breaking out of the loop when the current request has
remaining data (req->req.actual < req->req.length). The request
resumes on the next timer tick, preserving data ordering.

Signed-off-by: Sebastian Urban <surban@surban.net>
---
 drivers/usb/gadget/udc/dummy_hcd.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/usb/gadget/udc/dummy_hcd.c b/drivers/usb/gadget/udc/dummy_hcd.c
index 1cefca660..0eead4a85 100644
--- a/drivers/usb/gadget/udc/dummy_hcd.c
+++ b/drivers/usb/gadget/udc/dummy_hcd.c
@@ -1534,6 +1534,12 @@ static int transfer(struct dummy_hcd *dum_hcd, struct urb *urb,
 		/* rescan to continue with any other queued i/o */
 		if (rescan)
 			goto top;
+
+		/* request not fully transferred; stop iterating to
+		 * preserve data ordering across queued requests.
+		 */
+		if (req->req.actual < req->req.length)
+			break;
 	}
 	return sent;
 }
-- 
2.53.0

On Sun, Mar 15, 2026 at 10:03:07AM +0100, Sebastian Urban wrote:
> The transfer() function in dummy_hcd iterates over all queued gadget
> requests for a given endpoint via list_for_each_entry(). When the
> per-frame bandwidth budget is exhausted mid-request, leaving a
> partially-transferred gadget request, the loop continues to the next
> queued request instead of stopping.

Why do you say this?  Did you not see the code (at line 1445 
approximately):

		if (unlikely(len == 0))
			is_short = 1;
		else {
			/* not enough bandwidth left? */
			if (limit < ep->ep.maxpacket && limit < len)
				break;

It does break out of the loop when there is not enough space remaining 
in the bandwidth budget for another transaction.  But it does so at the 
start of the iteration following the last allowed transfer, rather than 
at the end of last transfer's iteration (as your patch does).

Furthermore, this test can't be removed, as there might not be enough 
space remaining for even a single transaction.

>  This breaks data ordering in the
> URB transfer buffer.
> 
> Two consequences:
> 
>   1. Data corruption: bytes from subsequent requests are written into
>      the URB buffer ahead of the remaining bytes from the incomplete
>      request. On the next timer tick the incomplete request resumes,
>      appending its remaining data after the out-of-order bytes.
> 
>   2. Premature URB completion: if the next request is a ZLP or shorter
>      than the remaining host buffer, it triggers the is_short path and
>      completes the URB before all data has been transferred.
> 
> Fix this by breaking out of the loop when the current request has
> remaining data (req->req.actual < req->req.length). The request
> resumes on the next timer tick, preserving data ordering.

As far as I can tell, this change is not necessary.  While it would 
avoid executing a few extra statements in the unlikely case where a 
request spans multiple URBs, in most situations it just adds a redundant 
test.

Alan Stern

> Signed-off-by: Sebastian Urban <surban@surban.net>
> ---
>  drivers/usb/gadget/udc/dummy_hcd.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/usb/gadget/udc/dummy_hcd.c b/drivers/usb/gadget/udc/dummy_hcd.c
> index 1cefca660..0eead4a85 100644
> --- a/drivers/usb/gadget/udc/dummy_hcd.c
> +++ b/drivers/usb/gadget/udc/dummy_hcd.c
> @@ -1534,6 +1534,12 @@ static int transfer(struct dummy_hcd *dum_hcd, struct urb *urb,
>  		/* rescan to continue with any other queued i/o */
>  		if (rescan)
>  			goto top;
> +
> +		/* request not fully transferred; stop iterating to
> +		 * preserve data ordering across queued requests.
> +		 */
> +		if (req->req.actual < req->req.length)
> +			break;
>  	}
>  	return sent;
>  }
> -- 
> 2.53.0
> 
>

On 3/15/26 15:44, Alan Stern wrote:
> On Sun, Mar 15, 2026 at 10:03:07AM +0100, Sebastian Urban wrote:
>> The transfer() function in dummy_hcd iterates over all queued gadget
>> requests for a given endpoint via list_for_each_entry(). When the
>> per-frame bandwidth budget is exhausted mid-request, leaving a
>> partially-transferred gadget request, the loop continues to the next
>> queued request instead of stopping.
> 
> Why do you say this?  Did you not see the code (at line 1445
> approximately):
> 
> 		if (unlikely(len == 0))
> 			is_short = 1;
> 		else {
> 			/* not enough bandwidth left? */
> 			if (limit < ep->ep.maxpacket && limit < len)
> 				break;
> 
> It does break out of the loop when there is not enough space remaining
> in the bandwidth budget for another transaction.  But it does so at the
> start of the iteration following the last allowed transfer, rather than
> at the end of last transfer's iteration (as your patch does).
> 
You're right that the existing bandwidth check at line 1444 handles the 
general case of a non-zero request following a partial transfer.

However, if a ZLP follows a partially-transferred request, the check is 
skipped because of the unlikely(len == 0) branch, leading to a false 
complete of the transfer with a shortened length.

Thus the bug can only be triggered by a ZLP. I will resubmit the patch 
with an updated commit message.

On Sun, Mar 15, 2026 at 03:06:21PM +0000, Sebastian Urban wrote:
> On 3/15/26 15:44, Alan Stern wrote:
> > 		if (unlikely(len == 0))
> > 			is_short = 1;
> > 		else {
> > 			/* not enough bandwidth left? */
> > 			if (limit < ep->ep.maxpacket && limit < len)
> > 				break;
> > 
> > It does break out of the loop when there is not enough space remaining
> > in the bandwidth budget for another transaction.  But it does so at the
> > start of the iteration following the last allowed transfer, rather than
> > at the end of last transfer's iteration (as your patch does).
> > 
> You're right that the existing bandwidth check at line 1444 handles the 
> general case of a non-zero request following a partial transfer.
> 
> However, if a ZLP follows a partially-transferred request, the check is 
> skipped because of the unlikely(len == 0) branch, leading to a false 
> complete of the transfer with a shortened length.

How can a ZLP follow a partially transferred request?  What follows a 
partially transferred request is always the remainder of that request, 
which by definition must have length > 0.

Alan Stern

> Thus the bug can only be triggered by a ZLP. I will resubmit the patch 
> with an updated commit message.
>

On Sun, Mar 15, 2026 at 11:21:26AM -0400, Alan Stern wrote:
> On Sun, Mar 15, 2026 at 03:06:21PM +0000, Sebastian Urban wrote:
> > On 3/15/26 15:44, Alan Stern wrote:
> > > 		if (unlikely(len == 0))
> > > 			is_short = 1;
> > > 		else {
> > > 			/* not enough bandwidth left? */
> > > 			if (limit < ep->ep.maxpacket && limit < len)
> > > 				break;
> > > 
> > > It does break out of the loop when there is not enough space remaining
> > > in the bandwidth budget for another transaction.  But it does so at the
> > > start of the iteration following the last allowed transfer, rather than
> > > at the end of last transfer's iteration (as your patch does).
> > > 
> > You're right that the existing bandwidth check at line 1444 handles the 
> > general case of a non-zero request following a partial transfer.
> > 
> > However, if a ZLP follows a partially-transferred request, the check is 
> > skipped because of the unlikely(len == 0) branch, leading to a false 
> > complete of the transfer with a shortened length.
> 
> How can a ZLP follow a partially transferred request?  What follows a 
> partially transferred request is always the remainder of that request, 
> which by definition must have length > 0.

Oh, I see now what you mean.  The whole thing uses 
loop_for_each_entry(), so it always proceeds to the next request in the 
queue even if the current request isn't finished.  Instead of doing 
that, it should always handle the first request in the queue.

The loop should be rewritten; it should be more like

	while (!list_empty(&ep->queue)) {
		req = list_first_entry(&ep->queue);
		...

Then it would behave as expected.

Alan Stern

On 3/15/26 16:28, Alan Stern wrote:
> On Sun, Mar 15, 2026 at 11:21:26AM -0400, Alan Stern wrote:
>> On Sun, Mar 15, 2026 at 03:06:21PM +0000, Sebastian Urban wrote:
>>> On 3/15/26 15:44, Alan Stern wrote:
>>>> 		if (unlikely(len == 0))
>>>> 			is_short = 1;
>>>> 		else {
>>>> 			/* not enough bandwidth left? */
>>>> 			if (limit < ep->ep.maxpacket && limit < len)
>>>> 				break;
>>>>
>>>> It does break out of the loop when there is not enough space remaining
>>>> in the bandwidth budget for another transaction.  But it does so at the
>>>> start of the iteration following the last allowed transfer, rather than
>>>> at the end of last transfer's iteration (as your patch does).
>>>>
>>> You're right that the existing bandwidth check at line 1444 handles the
>>> general case of a non-zero request following a partial transfer.
>>>
>>> However, if a ZLP follows a partially-transferred request, the check is
>>> skipped because of the unlikely(len == 0) branch, leading to a false
>>> complete of the transfer with a shortened length.
>>
>> How can a ZLP follow a partially transferred request?  What follows a
>> partially transferred request is always the remainder of that request,
>> which by definition must have length > 0.
> 
> Oh, I see now what you mean.  The whole thing uses
> loop_for_each_entry(), so it always proceeds to the next request in the
> queue even if the current request isn't finished.  Instead of doing
> that, it should always handle the first request in the queue.
> 
> The loop should be rewritten; it should be more like
> 
> 	while (!list_empty(&ep->queue)) {
> 		req = list_first_entry(&ep->queue);
> 		...
> 
> Then it would behave as expected.
> 
I agree that the loop should only ever process the first matching entry. 
The break I added at the end of the loop body achieves exactly that. 
  
  


I kept list_for_each_entry rather than rewriting to while + 
list_first_entry because the stream ID filtering at line 1421 uses 
continue to skip non-matching requests:

       if (dummy_ep_stream_en(dum_hcd, urb)) {
           if ((urb->stream_id != req->req.stream_id))
               continue;
       }

This wouldn't work with list_first_entry. A structural rewrite would 
need a separate scan to find the first matching request, which seems 
risky for a bug fix. If you'd prefer that approach I'm happy to do it as 
a follow-up patch.

Sebastian

On Sun, Mar 15, 2026 at 03:55:03PM +0000, Sebastian Urban wrote:
> On 3/15/26 16:28, Alan Stern wrote:
> > Oh, I see now what you mean.  The whole thing uses
> > loop_for_each_entry(), so it always proceeds to the next request in the
> > queue even if the current request isn't finished.  Instead of doing
> > that, it should always handle the first request in the queue.
> > 
> > The loop should be rewritten; it should be more like
> > 
> > 	while (!list_empty(&ep->queue)) {
> > 		req = list_first_entry(&ep->queue);
> > 		...
> > 
> > Then it would behave as expected.
> > 
> I agree that the loop should only ever process the first matching entry. 
> The break I added at the end of the loop body achieves exactly that. 
>   
>   
> 
> 
> I kept list_for_each_entry rather than rewriting to while + 
> list_first_entry because the stream ID filtering at line 1421 uses 
> continue to skip non-matching requests:
> 
>        if (dummy_ep_stream_en(dum_hcd, urb)) {
>            if ((urb->stream_id != req->req.stream_id))
>                continue;
>        }

Ah, that's right.  I had forgotten about that case.

> This wouldn't work with list_first_entry. A structural rewrite would 
> need a separate scan to find the first matching request, which seems 
> risky for a bug fix. If you'd prefer that approach I'm happy to do it as 
> a follow-up patch.

All right, you've convinced me.

Alan Stern

When a gadget request is only partially transferred in transfer()
because the per-frame bandwidth budget is exhausted, the loop advances
to the next queued request. If that next request is a zero-length
packet (ZLP), len evaluates to zero and the code takes the
unlikely(len == 0) path, which sets is_short = 1. This bypasses the
bandwidth guard ("limit < ep->ep.maxpacket && limit < len") that
lives in the else branch and would otherwise break out of the loop for
non-zero requests. The is_short path then completes the URB before all
data from the first request has been transferred.

Reproducer (bulk IN, high speed):

  Device side (FunctionFS with Linux AIO):
    1. Queue a 65024-byte write via io_submit (127 * 512, i.e. a
       multiple of the HS bulk max packet size).
    2. Immediately queue a zero-length write (ZLP) via io_submit.

  Host side:
    3. Submit a 65536-byte bulk IN URB.

  Expected: URB completes with actual_length = 65024.
  Actual:   URB completes with actual_length = 53248, losing 11776
            bytes that leak into subsequent URBs.

At high speed the per-frame budget is 53248 bytes (512 * 13 * 8).
The 65024-byte request exhausts this budget after 53248 bytes, leaving
the request incomplete (req->req.actual < req->req.length). Neither
the request nor the URB is finished, and rescan is 0, so the loop
advances to the ZLP. For the ZLP, dev_len = 0, so len = min(12288, 0)
= 0, taking the unlikely(len == 0) path and setting is_short = 1.
The is_short handler then sets *status = 0, completing the URB with
only 53248 of the expected 65024 bytes.

Fix this by breaking out of the loop when the current request has
remaining data (req->req.actual < req->req.length). The request
resumes on the next timer tick, preserving correct data ordering.

Signed-off-by: Sebastian Urban <surban@surban.net>
---
Changes in v2:
  - Rewrote commit message to describe the specific ZLP-after-partial-transfer
    scenario rather than overstating the general case (Alan Stern).
  - Added reproducer and step-by-step code path walkthrough.

 drivers/usb/gadget/udc/dummy_hcd.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/usb/gadget/udc/dummy_hcd.c b/drivers/usb/gadget/udc/dummy_hcd.c
index 1cefca660..0eead4a85 100644
--- a/drivers/usb/gadget/udc/dummy_hcd.c
+++ b/drivers/usb/gadget/udc/dummy_hcd.c
@@ -1534,6 +1534,12 @@ static int transfer(struct dummy_hcd *dum_hcd, struct urb *urb,
 		/* rescan to continue with any other queued i/o */
 		if (rescan)
 			goto top;
+
+		/* request not fully transferred; stop iterating to
+		 * preserve data ordering across queued requests.
+		 */
+		if (req->req.actual < req->req.length)
+			break;
 	}
 	return sent;
 }
--
2.53.0

On Sun, Mar 15, 2026 at 04:10:45PM +0100, Sebastian Urban wrote:
> When a gadget request is only partially transferred in transfer()
> because the per-frame bandwidth budget is exhausted, the loop advances
> to the next queued request. If that next request is a zero-length
> packet (ZLP), len evaluates to zero and the code takes the
> unlikely(len == 0) path, which sets is_short = 1. This bypasses the
> bandwidth guard ("limit < ep->ep.maxpacket && limit < len") that
> lives in the else branch and would otherwise break out of the loop for
> non-zero requests. The is_short path then completes the URB before all
> data from the first request has been transferred.
> 
> Reproducer (bulk IN, high speed):
> 
>   Device side (FunctionFS with Linux AIO):
>     1. Queue a 65024-byte write via io_submit (127 * 512, i.e. a
>        multiple of the HS bulk max packet size).
>     2. Immediately queue a zero-length write (ZLP) via io_submit.
> 
>   Host side:
>     3. Submit a 65536-byte bulk IN URB.
> 
>   Expected: URB completes with actual_length = 65024.
>   Actual:   URB completes with actual_length = 53248, losing 11776
>             bytes that leak into subsequent URBs.
> 
> At high speed the per-frame budget is 53248 bytes (512 * 13 * 8).
> The 65024-byte request exhausts this budget after 53248 bytes, leaving
> the request incomplete (req->req.actual < req->req.length). Neither
> the request nor the URB is finished, and rescan is 0, so the loop
> advances to the ZLP. For the ZLP, dev_len = 0, so len = min(12288, 0)
> = 0, taking the unlikely(len == 0) path and setting is_short = 1.
> The is_short handler then sets *status = 0, completing the URB with
> only 53248 of the expected 65024 bytes.
> 
> Fix this by breaking out of the loop when the current request has
> remaining data (req->req.actual < req->req.length). The request
> resumes on the next timer tick, preserving correct data ordering.
> 
> Signed-off-by: Sebastian Urban <surban@surban.net>
> ---

Reviewed-by: Alan Stern <stern@rowland.harvard.edu>

> Changes in v2:
>   - Rewrote commit message to describe the specific ZLP-after-partial-transfer
>     scenario rather than overstating the general case (Alan Stern).
>   - Added reproducer and step-by-step code path walkthrough.
> 
>  drivers/usb/gadget/udc/dummy_hcd.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/usb/gadget/udc/dummy_hcd.c b/drivers/usb/gadget/udc/dummy_hcd.c
> index 1cefca660..0eead4a85 100644
> --- a/drivers/usb/gadget/udc/dummy_hcd.c
> +++ b/drivers/usb/gadget/udc/dummy_hcd.c
> @@ -1534,6 +1534,12 @@ static int transfer(struct dummy_hcd *dum_hcd, struct urb *urb,
>  		/* rescan to continue with any other queued i/o */
>  		if (rescan)
>  			goto top;
> +
> +		/* request not fully transferred; stop iterating to
> +		 * preserve data ordering across queued requests.
> +		 */
> +		if (req->req.actual < req->req.length)
> +			break;
>  	}
>  	return sent;
>  }
> --
> 2.53.0