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

[sashiko-bot@kernel.org]

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