[PATCH] usb: core: hcd: cancel giveback work synchronously on stop

[PATCH] usb: core: hcd: cancel giveback work synchronously on stop

[Igor Putko]

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() finishes.

Since the caller may immediately free the struct usb_hcd via usb_put_hcd()
after usb_stop_hcd() or usb_remove_hcd(), the running worker accesses
the embedded giveback_urb_bh structure after it has been freed,
triggering a slab-use-after-free write detected by KASAN.

Ensure all giveback work is fully flushed and cancelled in usb_stop_hcd()
by calling cancel_work_sync() on both high and low priority BHs.

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
Signed-off-by: Igor Putko <igorpetindev@gmail.com>
---
 drivers/usb/core/hcd.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 8e8a5f59b319..0b7d30900f7a 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -2787,6 +2787,9 @@ 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);
+
+	cancel_work_sync(&hcd->high_prio_bh.bh);
+	cancel_work_sync(&hcd->low_prio_bh.bh);
 }
 
 /**
-- 
2.47.3

Re: [PATCH] usb: core: hcd: cancel giveback work synchronously on stop

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] usb: core: hcd: cancel_work_sync() in usb_stop_hcd() strands pending BH URBs
--

commit 6e7c5c14d78d88d1d1fec5ad2d48ff9cf59b253d
Author: Igor Putko <igorpetindev@gmail.com>

usb: core: hcd: cancel giveback work synchronously on stop

This commit adds cancel_work_sync() calls for both high and low priority
giveback BH work items in usb_stop_hcd() to prevent use-after-free accesses
to struct usb_hcd when usb_giveback_urb_bh() is still running during HCD
teardown.

> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index a865a15ea3863..e0f00d68598a1 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -2788,6 +2788,9 @@ 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);
> +
> +	cancel_work_sync(&hcd->high_prio_bh.bh);
> +	cancel_work_sync(&hcd->low_prio_bh.bh);

[Severity: High]
Could calling cancel_work_sync() instead of draining the BH work items leave
URBs stranded on hcd->high_prio_bh.head and hcd->low_prio_bh.head, leaking
struct urb references and DMA mappings or hanging callers in usb_kill_urb()?

Unlike flush_work(), cancel_work_sync() steals the pending bit if &bh->bh has
not started executing yet, so usb_giveback_urb_bh() never runs for URBs
already queued on bh->head. In addition, if usb_giveback_urb_bh() is already
running a snapshot when new URBs are added to bh->head, it relies on
re-queueing &bh->bh before returning:

drivers/usb/core/hcd.c:usb_giveback_urb_bh() {
	...
	spin_lock_irq(&bh->lock);
	if (!list_empty(&bh->head)) {
		if (bh->high_prio)
			queue_work(system_bh_highpri_wq, &bh->bh);
		else
			queue_work(system_bh_wq, &bh->bh);
	}
	bh->running = false;
	spin_unlock_irq(&bh->lock);
}

Because cancel_work_sync() sets WORK_CANCEL_DISABLE while waiting for the
current execution to finish, that queue_work() call is silently dropped.

This can be reached in two ways during HCD teardown:

1. In usb_stop_hcd(), hcd->driver->stop(hcd) runs right before
cancel_work_sync(). For HCD drivers with HCD_BH set that complete remaining
URBs in their stop callback, such as in drivers/usb/dwc2/hcd.c:

_dwc2_hcd_stop()
  -> dwc2_hcd_disconnect()
    -> dwc2_kill_all_urbs()
      -> dwc2_host_complete()
        -> usb_hcd_giveback_urb()

URBs are added to bh->head and &bh->bh is queued immediately before
cancel_work_sync() cancels the pending work item.

2. During usb_remove_hcd() -> usb_disconnect() -> usb_disable_device() ->
usb_hcd_flush_endpoint(), usb_hcd_unlink_urb_from_ep() removes the URB from
ep->urb_list before usb_hcd_giveback_urb() appends it to bh->head. In
drivers/usb/core/hcd.c:usb_hcd_flush_endpoint():

	/* Wait until the endpoint queue is completely empty */
	while (!list_empty (&ep->urb_list)) {

this loop can observe an empty ep->urb_list and return without calling
usb_kill_urb() while the URB is still sitting on bh->head, allowing
usb_stop_hcd() to cancel &bh->bh before usb_giveback_urb_bh() processes it.

Any URB stranded on bh->head never reaches __usb_hcd_giveback_urb(), so it
is never unmapped via unmap_urb_for_dma(), its struct urb reference from
usb_get_urb() is leaked, urb->complete() is skipped, and urb->use_count
remains non-zero (which hangs any caller waiting in usb_kill_urb() or
usb_poison_urb()).

Also, should the comment in drivers/usb/core/hcd.c:usb_remove_hcd() right
above usb_stop_hcd(hcd):

	/*
	 * 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
	 */

be updated now that usb_stop_hcd() synchronously waits on the BH work items?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261023174015.54793-1-igorpetindev@gmail.com?part=1

Re: [PATCH] usb: core: hcd: cancel giveback work synchronously on stop

[Alan Stern]

On Fri, Oct 23, 2026 at 08:40:15PM +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() finishes.
> 
> Since the caller may immediately free the struct usb_hcd via usb_put_hcd()
> after usb_stop_hcd() or usb_remove_hcd(), the running worker accesses
> the embedded giveback_urb_bh structure after it has been freed,
> triggering a slab-use-after-free write detected by KASAN.
> 
> Ensure all giveback work is fully flushed and cancelled in usb_stop_hcd()
> by calling cancel_work_sync() on both high and low priority BHs.

Sashiko's criticism has a point.  If there are any URBs still pending on 
either of the BH lists when usb_stop_hcd() returns, something is already 
badly wrong -- that's never supposed to happen.  Whatever it is, it 
needs to be fixed in the driver that submitted the pending URB, not here 
in usb_stop_hcd().

Alan Stern

Re: [PATCH] usb: core: hcd: cancel giveback work synchronously on stop

[IgorpetinDev]

Hi Alan,

Thanks for looking at the patch.

I think the bot caused some confusion by talking about stranded URBs.
There are actually no pending URBs left on the list at all. The driver
properly killed all of them.
The issue is a race inside usb_giveback_urb_bh itself. The function
usb_kill_urb() unblocks as soon as __usb_hcd_giveback_urb() drops
use_count and wakes the waitqueue. The caller immediately wakes up,
finishes removal, and frees struct usb_hcd via usb_put_hcd().

Meanwhile, the worker thread on the other CPU is still running the
tail of usb_giveback_urb_bh(). Right after __usb_hcd_giveback_urb()
returns, it tries to access bh->completing_ep, bh->lock, and
bh->running. Because bh is embedded directly inside struct usb_hcd,
this access hits already freed memory.
So all URBs are already completed and bh->head is empty, but the
teardown path still needs to wait for the worker thread to fully exit
before the HCD structure gets freed.
Would calling flush_work() on both BH work items be the better way to
synchronize this without the concern raised by the bot?

Thanks,
Igor

On Fri, Oct 23, 2026 at 10:15 PM Alan Stern <stern@rowland.harvard.edu> wrote:
>
> On Fri, Oct 23, 2026 at 08:40:15PM +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() finishes.
> >
> > Since the caller may immediately free the struct usb_hcd via usb_put_hcd()
> > after usb_stop_hcd() or usb_remove_hcd(), the running worker accesses
> > the embedded giveback_urb_bh structure after it has been freed,
> > triggering a slab-use-after-free write detected by KASAN.
> >
> > Ensure all giveback work is fully flushed and cancelled in usb_stop_hcd()
> > by calling cancel_work_sync() on both high and low priority BHs.
>
> Sashiko's criticism has a point.  If there are any URBs still pending on
> either of the BH lists when usb_stop_hcd() returns, something is already
> badly wrong -- that's never supposed to happen.  Whatever it is, it
> needs to be fixed in the driver that submitted the pending URB, not here
> in usb_stop_hcd().
>
> Alan Stern

Re: [PATCH] usb: core: hcd: cancel giveback work synchronously on stop

[Alan Stern]

On Fri, Oct 23, 2026 at 10:31:27PM +0300, IgorpetinDev wrote:
> Hi Alan,
> 
> Thanks for looking at the patch.
> 
> I think the bot caused some confusion by talking about stranded URBs.
> There are actually no pending URBs left on the list at all. The driver
> properly killed all of them.
> The issue is a race inside usb_giveback_urb_bh itself. The function
> usb_kill_urb() unblocks as soon as __usb_hcd_giveback_urb() drops
> use_count and wakes the waitqueue. The caller immediately wakes up,
> finishes removal, and frees struct usb_hcd via usb_put_hcd().
> 
> Meanwhile, the worker thread on the other CPU is still running the
> tail of usb_giveback_urb_bh(). Right after __usb_hcd_giveback_urb()
> returns, it tries to access bh->completing_ep, bh->lock, and
> bh->running. Because bh is embedded directly inside struct usb_hcd,
> this access hits already freed memory.

Okay, now I understand your concern.

> So all URBs are already completed and bh->head is empty, but the
> teardown path still needs to wait for the worker thread to fully exit
> before the HCD structure gets freed.
> Would calling flush_work() on both BH work items be the better way to
> synchronize this without the concern raised by the bot?

Yes, I think it would.  After all, it's just good form to flush or 
cancel a work item before deallocating it, if you can't prove that it 
must already be idle.

Alan Stern

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

[Igor Putko]

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(), 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.

Fixes: 8fea0c8fda30 ("usb: core: hcd: Convert from tasklet to BH workqueue")
Cc: stable@vger.kernel.org
Reported-by: syzbot+a5b220cacb0618384d5a@syzkaller.appspotmail.com
Closes: https://syzkaller.appspotmail.com/bug?extid=a5b220cacb0618384d5a
Suggested-by: Alan Stern <stern@rowland.harvard.edu>
Signed-off-by: Igor Putko <igorpetindev@gmail.com>
---
v1 -> v2:
- Use flush_work() instead of cancel_work_sync() to ensure no pending
  work items are dropped, as suggested by Alan Stern.
---
 drivers/usb/core/hcd.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 8e8a5f59b319..e6854d67ed8c 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -2787,6 +2787,9 @@ 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);
+
+	flush_work(&hcd->high_prio_bh.bh);
+	flush_work(&hcd->low_prio_bh.bh);
 }
 
 /**
-- 
2.47.3

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] usb: core: hcd: incomplete bh work flush in usb_stop_hcd() on self-requeue
--

commit 9c9e1488a3b8b0e4cd42d6243a942c8d42aeb233
Author: Igor Putko <igorpetindev@gmail.com>

usb: core: hcd: flush giveback work on stop

This commit adds flush_work() calls on the high and low priority BH work
items in usb_stop_hcd() to prevent a use-after-free on struct usb_hcd when
usb_giveback_urb_bh() is still executing as the HCD is stopped and freed.

> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index a865a15ea3863..9db0f39896065 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -2788,6 +2788,9 @@ 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);
> +
> +	flush_work(&hcd->high_prio_bh.bh);
> +	flush_work(&hcd->low_prio_bh.bh);

[Severity: Critical]
Is a single flush_work() call on each BH work item sufficient to drain all
pending givebacks if usb_giveback_urb_bh() requeues itself?

When an instance of usb_giveback_urb_bh() starts running, process_one_work()
clears WORK_STRUCT_PENDING_BIT and WORK_STRUCT_PWQ on &bh->bh, and
usb_giveback_urb_bh() sets bh->running = true while moving bh->head to
local_list.

If another URB is passed to usb_hcd_giveback_urb() while bh->running is true,
it is appended to bh->head without calling queue_work():

drivers/usb/core/hcd.c:usb_hcd_giveback_urb() {
	...
	spin_lock(&bh->lock);
	list_add_tail(&urb->urb_list, &bh->head);
	running = bh->running;
	spin_unlock(&bh->lock);

	if (running)
		;
	else if (bh->high_prio)
		queue_work(system_bh_highpri_wq, &bh->bh);
	else
		queue_work(system_bh_wq, &bh->bh);
}

This can happen without a caller waiting in usb_kill_urb(). For example,
during usb_remove_hcd() -> usb_disconnect(), usb_hcd_flush_endpoint() calls
unlink1(), where usb_hcd_unlink_urb_from_ep() removes the URB from
ep->urb_list before usb_hcd_giveback_urb() adds it to bh->head. The second
loop in usb_hcd_flush_endpoint():

	/* Wait until the endpoint queue is completely empty */
	while (!list_empty (&ep->urb_list)) {

then sees ep->urb_list as empty and returns without calling usb_kill_urb().
Similarly, HCD ->stop(hcd) callbacks invoked in usb_stop_hcd() (such as
_dwc2_hcd_stop() -> dwc2_hcd_disconnect() -> dwc2_kill_all_urbs()) can give
back remaining URBs via usb_hcd_giveback_urb() while an earlier execution of
usb_giveback_urb_bh() is still in progress.

If usb_stop_hcd() calls flush_work(&bh->bh) before the running worker reaches
the end of usb_giveback_urb_bh(), &bh->bh is not marked pending, so
start_flush_work() -> insert_wq_barrier() places the flush barrier on
worker->scheduled:

kernel/workqueue.c:insert_wq_barrier() {
	...
	if (worker) {
		head = worker->scheduled.next;
		work_color = worker->current_color;
	}
	...
}

When the running usb_giveback_urb_bh() then finishes its loop, it sees the
newly added URB on bh->head and requeues &bh->bh onto pool->worklist:

drivers/usb/core/hcd.c:usb_giveback_urb_bh() {
	...
	spin_lock_irq(&bh->lock);
	if (!list_empty(&bh->head)) {
		if (bh->high_prio)
			queue_work(system_bh_highpri_wq, &bh->bh);
		else
			queue_work(system_bh_wq, &bh->bh);
	}
	bh->running = false;
	spin_unlock_irq(&bh->lock);
}

When that first execution returns, process_scheduled_works() immediately runs
the barrier on worker->scheduled and wakes flush_work() before bh_worker()
picks up the requeued &bh->bh from pool->worklist.

Could usb_stop_hcd() then return and allow usb_put_hcd() to free struct
usb_hcd while the requeued usb_giveback_urb_bh() is still pending or running?
Also, does the comment above usb_stop_hcd() in usb_remove_hcd() stating that
flush_work() isn't needed there need to be updated?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261024083647.2599-1-igorpetindev@gmail.com?part=1

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

[Alan Stern]

On Sat, Oct 24, 2026 at 09:11:11AM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Critical] usb: core: hcd: incomplete bh work flush in usb_stop_hcd() on self-requeue
> --
> 
> commit 9c9e1488a3b8b0e4cd42d6243a942c8d42aeb233
> Author: Igor Putko <igorpetindev@gmail.com>
> 
> usb: core: hcd: flush giveback work on stop
> 
> This commit adds flush_work() calls on the high and low priority BH work
> items in usb_stop_hcd() to prevent a use-after-free on struct usb_hcd when
> usb_giveback_urb_bh() is still executing as the HCD is stopped and freed.
> 
> > diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> > index a865a15ea3863..9db0f39896065 100644
> > --- a/drivers/usb/core/hcd.c
> > +++ b/drivers/usb/core/hcd.c
> > @@ -2788,6 +2788,9 @@ 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);
> > +
> > +	flush_work(&hcd->high_prio_bh.bh);
> > +	flush_work(&hcd->low_prio_bh.bh);
> 
> [Severity: Critical]
> Is a single flush_work() call on each BH work item sufficient to drain all
> pending givebacks if usb_giveback_urb_bh() requeues itself?

That's not an issue in this case.  By the time usb_stop_hcd() runs, the 
two giveback queues should be entirely empty and there should be no more 
devices remaining on the USB bus creating new entries to add to the 
queues.

However, it would be worthwhile to add a sentence to the kerneldoc for 
usb_stop_hcd() explaining this fact.

> Also, does the comment above usb_stop_hcd() in usb_remove_hcd() stating that
> flush_work() isn't needed there need to be updated?

Yes, that comment should be removed.  In its place, add a comment to 
usb_stop_hcd() explaining that even though all URBs have been given 
back, the work routine itself may still be finishing up and accessing 
the usb_hcd structure; that's why the work items need to be flushed.

Alan Stern

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

[Igor Putko]

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(), 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
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.

 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.
+ *
  * 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, the work routine itself
+	 * 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

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

[sashiko-bot]

> 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(), 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
> Suggested-by: Alan Stern <stern@rowland.harvard.edu>
> Signed-off-by: Igor Putko <igorpetindev@gmail.com>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261024155614.2732-1-igorpetindev@gmail.com?part=1

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

[Alan Stern]

On Sat, Oct 24, 2026 at 06:56:14PM +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(), 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
> 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.

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

>  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.
> + *
>   * 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, the work routine itself
> +	 * 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
> 

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

[Michal Pecio]

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
> 

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@gmail.com> wrote:
>
> 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
> >

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

[Alan Stern]

On Sun, Oct 25, 2026 at 10:54:57AM +0300, IgorpetinDev wrote:
> 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.

I don't much care whether the new calls go in usb_remove_hcd() or 
usb_stop_hcd().  I suppose usb_remove_hcd() would make more sense, since 
no URBs can be sent to the HCD until it is added.

As for cancel_work_sync() vs. flush_work(), it doesn't really matter.  
The situation we're in is that there are no more entries remaining in 
the workqueue and no more will be added, but the final one may still be 
executing.  In this setting, cancel_work_sync() and flush_work() do the 
same thing.  However, the name "flush_work" does a better job of 
expressing what we want to accomplish: wait for the final work item to 
finish, not try to cancel it.

> > > @@ -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.

If it's not true, there's likely to be trouble when the completion 
handler is called.  We could forcibly remove any remaining URBs from the 
lists before flushing the work item and just leak them (with a warning 
of some sort -- ideally, one that prints out the names of the completion 
handler routines for the leaked URBs, if at all possible).

Alan Stern

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

[Michal Pecio]

On Sun, 25 Oct 2026 11:16:59 -0400, Alan Stern wrote:
> > > > +      * 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.  
> 
> If it's not true, there's likely to be trouble when the completion 
> handler is called.  We could forcibly remove any remaining URBs from
> the lists before flushing the work item and just leak them (with a
> warning of some sort -- ideally, one that prints out the names of the
> completion handler routines for the leaked URBs, if at all possible).

But you are now talking about developing a whole new feature of
unclear usefulness (will anyone pay attention to those logs?),
while my suggestion is merely a small tweak which costs nothing
and makes no functional difference under normal circumstances,
but reduces severity of hypothetical failures.

Regards,
Michal

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

[Alan Stern]

On Sun, Oct 25, 2026 at 05:10:19PM +0100, Michal Pecio wrote:
> On Sun, 25 Oct 2026 11:16:59 -0400, Alan Stern wrote:
> > > > > +      * 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.  
> > 
> > If it's not true, there's likely to be trouble when the completion 
> > handler is called.  We could forcibly remove any remaining URBs from
> > the lists before flushing the work item and just leak them (with a
> > warning of some sort -- ideally, one that prints out the names of the
> > completion handler routines for the leaked URBs, if at all possible).
> 
> But you are now talking about developing a whole new feature of
> unclear usefulness (will anyone pay attention to those logs?),

Actually I mentioned two new features: forcibly removing any remaining 
URBs, and printing a warning about them.  I guess you're saying that the 
warning would be of unclear usefulness.  That may be true; I was just 
thinking that if anybody was interesting in chasing down the cause of 
such errors, knowing which driver was involved would be useful.

> while my suggestion is merely a small tweak which costs nothing
> and makes no functional difference under normal circumstances,
> but reduces severity of hypothetical failures.

What suggestion was that?  Using cancel_work_sync() instead of 
flush_work()?  Doesn't that amount to the same thing as forcibly 
removing remaining URBs from the giveback lists?

I still think if you're going to deliberately leak some memory because 
of a misbehaving driver, there ought to be some mention of it in the 
kernel log.  Otherwise, how would anybody ever know?

Alan Stern

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

[Greg Kroah-Hartman]

On Sun, Oct 25, 2026 at 10:54:57AM +0300, IgorpetinDev wrote:
> 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.

Please fix up and send a new version.

thanks,

greg k-h

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

[Michal Pecio]

On Thu, 29 Oct 2026 10:42:35 +0100, Greg Kroah-Hartman wrote:
> On Sun, Oct 25, 2026 at 10:54:57AM +0300, IgorpetinDev wrote:
> > 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.  
> 
> Please fix up and send a new version.
> 
> thanks,
> 
> greg k-h

Hi Greg,

This race had been reported by syzbot previously and a patch
has been hanging on linux-usb since August.

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

There is only a bit of controversy over flushing vs cancelling the
work. I think that cancelling (and leaking stray completions as opposed
to letting them run, likely UAF things and maybe corrupt memory) is the
right choice, as a courtesy to anyone who might create such bugs and
need to deal with them.

Regards,
Michal

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

[Alan Stern]

On Thu, Oct 29, 2026 at 11:44:22AM +0100, Michal Pecio wrote:
> On Thu, 29 Oct 2026 10:42:35 +0100, Greg Kroah-Hartman wrote:
> > On Sun, Oct 25, 2026 at 10:54:57AM +0300, IgorpetinDev wrote:
> > > 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.  
> > 
> > Please fix up and send a new version.
> > 
> > thanks,
> > 
> > greg k-h
> 
> Hi Greg,
> 
> This race had been reported by syzbot previously and a patch
> has been hanging on linux-usb since August.
> 
> https://lore.kernel.org/linux-usb/20260823125831.6ea35650.michal.pecio@gmail.com/
> 
> There is only a bit of controversy over flushing vs cancelling the
> work. I think that cancelling (and leaking stray completions as opposed
> to letting them run, likely UAF things and maybe corrupt memory) is the
> right choice, as a courtesy to anyone who might create such bugs and
> need to deal with them.

I have to agree that Michal's patch is more elegant than Igor's.

(Although, as a small nit, I would change the last line of the new 
comment from:

	 * However, we still must kill these works so they don't UAF the HCD.

to something more precise, like this:

	 * However, we still must wait for the BH work items to finish
	 * because they access the hcd structure, which is about to be
	 * deallocated.  Any URBs not yet handled will just have to leak.

It's a minor thing.)

Alan Stern

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

[Igor Putko]

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(), 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.appspot.com/bug?extid=a5b220cacb0618384d5a
Suggested-by: Alan Stern <stern@rowland.harvard.edu>
Reviewed-by: Alan Stern <stern@rowland.harvard.edu>
Signed-off-by: Igor Putko <igorpetindev@gmail.com>
---
v3 -> v4:
- Fix domain typo in Closes: tag URL (appspotmail.com -> appspot.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.
---
 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.
+ *
  * 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, the work routine itself
+	 * 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

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

[sashiko-bot]

> 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(), 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.appspot.com/bug?extid=a5b220cacb0618384d5a
> Suggested-by: Alan Stern <stern@rowland.harvard.edu>
> Reviewed-by: Alan Stern <stern@rowland.harvard.edu>
> Signed-off-by: Igor Putko <igorpetindev@gmail.com>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261029114733.2688-1-igorpetindev@gmail.com?part=1