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

[IgorpetinDev]

Hi Michal,

> No such domain?
> Looks like you trusted an LLM ;)

good catch, that was just a clumsy typo on my part: I accidentally typed
appspotmail.com instead of appspot.com in the closes tag, anyway - the syzbot ID
itself is valid. If Greg or Alan prefer a v4 to fix the URL typo, I'll
gladly resend it, or it can be fixed up when applying.

> Dupe?

I wasn't aware of your patch from august; I simply saw an open and
unresolved syzbot report and decided to address it.

Regarding cancel_work_sync() vs. flush_work() and usb_remove_hcd() vs.
usb_stop_hcd(): these changes were specifically suggested by Alan Stern
during the review of v1 and v2, which is why v3 was shaped this way and
subsequently received his reviewed-by tag.

I'll defer to Alan and Greg on how they wish to proceed and which
approach they prefer to take.

Thanks,
Igor

On Sun, Oct 25, 2026 at 1:06 AM Michal Pecio <michal.pecio@xxxxxxxxx> wrote:
>
> Dupe?
>
> https://lore.kernel.org/linux-usb/20260823125831.6ea35650.michal.pecio@xxxxxxxxx/
>
> 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@xxxxxxxxxxxxxxxxxxxxxxxxx Closes:
> > https://syzkaller.appspotmail.com/bug?extid=a5b220cacb0618384d5a
>
> No such domain?
>
> Looks like you trusted an LLM ;)
>
> > Suggested-by: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>
> > Signed-off-by: Igor Putko <igorpetindev@xxxxxxxxx>
> > ---
> > 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
> >