Re: [PATCH] usb: gadget: aspeed_udc: avoid past-the-end iterator in dequeue

On Mon, May 18, 2026 at 03:34:03PM +0800, Maoyi Xie wrote:
> From: Maoyi Xie <[email protected]>
> 
> ast_udc_ep_dequeue() declares the loop cursor `req` outside the
> list_for_each_entry(). After the loop it tests `&req->req != _req`
> to decide whether the request was found. If the queue holds no
> match, `req` is past-the-end. It then aliases
> container_of(&ep->queue, struct ast_udc_request, queue) via offset
> cancellation. Whether that synthetic address equals `_req` depends
> on heap layout. The function can return 0 without dequeueing
> anything.
> 
> Use the cursor-vs-result idiom from the sibling aspeed-vhub driver,
> ast_vhub_epn_dequeue() in drivers/usb/gadget/udc/aspeed-vhub/epn.c.
> A separate `iter` walks the list. `req` is set only when a request
> matches. After the loop, `req` is NULL if nothing matched.
> 
> The same idiom is used by the other UDC drivers in
> drivers/usb/gadget/udc/ (at91_udc, atmel_usba_udc, dummy_hcd,
> fsl_qe_udc, fsl_udc_core, goku_udc, gr_udc, lpc32xx_udc,
> max3420_udc, net2280, omap_udc, pxa25x_udc, pxa27x_udc, udc-xilinx,
> bcm63xx_udc).
> 
> Signed-off-by: Maoyi Xie <[email protected]>
> ---
>  drivers/usb/gadget/udc/aspeed_udc.c | 24 ++++++++++++++----------
>  1 file changed, 14 insertions(+), 10 deletions(-)
> 
> --- a/drivers/usb/gadget/udc/aspeed_udc.c
> +++ b/drivers/usb/gadget/udc/aspeed_udc.c
> @@ -692,26 +692,30 @@
>  {
>  	struct ast_udc_ep *ep = to_ast_ep(_ep);
>  	struct ast_udc_dev *udc = ep->udc;
> -	struct ast_udc_request *req;
> +	struct ast_udc_request *req = NULL, *iter;
>  	unsigned long flags;
>  	int rc = 0;
>  
>  	spin_lock_irqsave(&udc->lock, flags);
>  
>  	/* make sure it's actually queued on this endpoint */
> -	list_for_each_entry(req, &ep->queue, queue) {
> -		if (&req->req == _req) {
> -			list_del_init(&req->queue);
> -			ast_udc_done(ep, req, -ESHUTDOWN);
> -			_req->status = -ECONNRESET;
> -			break;
> -		}
> +	list_for_each_entry(iter, &ep->queue, queue) {
> +		if (&iter->req != _req)
> +			continue;
> +		req = iter;
> +		break;
>  	}

There's nothing wrong with doing it this way, and this is how the other 
drivers do it.  Still, organizing the loop in this way does look a 
little strange.  Consider this instead:

	list_for_each_entry(iter, &ep->queue, queue) {
		if (&iter->req == _req) {
			req = iter;
			break;
		}
	}

Alan Stern