Re: [PATCH v3] usb: core: hcd: flush giveback work on stop

[Michal Pecio <michal.pecio@gmail.com>]

Dupe?

https://lore.kernel.org/linux-usb/20260823125831.6ea35650.michal.pecio@gmail.com/

On Sat, 24 Oct 2026 18:56:14 +0300, Igor Putko wrote:
> When an HCD is stopped/removed (either during module unload, driver
> unbind, or an error path in probe), pending URBs are unlinked and
> completed. However, for root hubs or HCDs with giveback done in BH
> context, the work function usb_giveback_urb_bh() may still be
> executing on another CPU after __usb_hcd_giveback_urb() drops the URB
> use_count and wakes up waiters in usb_kill_urb().
>
> Since the caller may immediately free the struct usb_hcd via
> usb_put_hcd() after usb_stop_hcd() or usb_remove_hcd()

Is usb_stop_hcd() relevant here?

I placed this in usb_remove_hcd() where other works are cancelled.
This seems good enough, because no URBs (or wakeups or dead) should
exist until usb_add_hcd() completes successfully, at which point
I suppose usb_remove_hcd() must be used, not merely usb_stop_hcd().

> the running worker accesses the embedded giveback_urb_bh structure
> in its epilogue (e.g. clearing completing_ep or clearing the running
> flag) after the memory has been freed, triggering a
> slab-use-after-free write detected by KASAN.
> 
> Ensure all giveback work has fully completed in usb_stop_hcd() by
> calling flush_work() on both high and low priority BHs before
> deallocating. Also remove the misleading comment in usb_remove_hcd()
> claiming flush_work() is unneeded, and document the queue emptiness
> in the usb_stop_hcd() kerneldoc.
> 
> Fixes: 8fea0c8fda30 ("usb: core: hcd: Convert from tasklet to BH
> workqueue") Reported-by:
> syzbot+a5b220cacb0618384d5a@syzkaller.appspotmail.com Closes:
> https://syzkaller.appspotmail.com/bug?extid=a5b220cacb0618384d5a

No such domain?

Looks like you trusted an LLM ;)

> Suggested-by: Alan Stern <stern@rowland.harvard.edu>
> Signed-off-by: Igor Putko <igorpetindev@gmail.com>
> ---
> v2 -> v3:
> - Remove obsolete comment in usb_remove_hcd() regarding flush_work().
> - Add comment in usb_stop_hcd() explaining why flush_work() is needed.
> - Update kerneldoc for usb_stop_hcd() to state giveback queues are empty.
> v1 -> v2:
> - Use flush_work() instead of cancel_work_sync() as suggested by Alan Stern.

But why?

Cancel costs nothing and turns potential UAF into a leak, sounds like
a win to me. I did the same thing deliberately for this exact reason.

This review bot is completely hopeless in out-of-spec and bug-vs-bug
tradeoff scenarios.

> 
>  drivers/usb/core/hcd.c | 19 +++++++++----------
>  1 file changed, 9 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index 8e8a5f59b319..f403795b17ba 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -2773,6 +2773,8 @@ static void usb_put_invalidate_rhdev(struct usb_hcd *hcd)
>   * usb_stop_hcd - Halt the HCD
>   * @hcd: the usb_hcd that has to be halted
>   *
> + * The two giveback queues should be entirely empty when this runs.

No disagreement,

> + *
>   * Stop the root-hub polling timer and invoke the HCD's ->stop callback.
>   */
>  static void usb_stop_hcd(struct usb_hcd *hcd)
> @@ -2787,6 +2789,13 @@ static void usb_stop_hcd(struct usb_hcd *hcd)
>  	/* In case the HCD restarted the timer, stop it again. */
>  	clear_bit(HCD_FLAG_POLL_RH, &hcd->flags);
>  	timer_delete_sync(&hcd->rh_timer);
> +
> +	/*
> +	 * Even though all URBs have been given back,

but does anyone actually believe this here?

Hundreds of drivers written over three decades, often
without linux-usb review. I don't trust them at all.

Regards,
Michal

> +	 * may still be finishing up and accessing the usb_hcd structure.
> +	 */
> +	flush_work(&hcd->high_prio_bh.bh);
> +	flush_work(&hcd->low_prio_bh.bh);
>  }
>  
>  /**
> @@ -3059,16 +3068,6 @@ void usb_remove_hcd(struct usb_hcd *hcd)
>  		usb_disconnect(&rhdev);		/* Sets rhdev to NULL */
>  	mutex_unlock(&usb_bus_idr_lock);
>  
> -	/*
> -	 * flush_work() isn't needed here because:
> -	 * - driver's disconnect() called from usb_disconnect() should
> -	 *   make sure its URBs are completed during the disconnect()
> -	 *   callback
> -	 *
> -	 * - it is too late to run complete() here since driver may have
> -	 *   been removed already now
> -	 */
> -
>  	/* Prevent any more root-hub status calls from the timer.
>  	 * The HCD might still restart the timer (if a port status change
>  	 * interrupt occurs), but usb_hcd_poll_rh_status() won't invoke
> -- 
> 2.47.3
>