Re: [PATCH 1/2] usb: core: hcd: Disable IRQs when taking bh->lock

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [Critical] usb: core: hcd: missing BH work cancellation on error path
  in usb_add_hcd() [1]
--

commit 4dc28efe0885658abc7f649ad1d1ba25793691d1
Author: Chien Kun Niu <rickyniu@google.com>

usb: core: hcd: Disable IRQs when taking bh->lock

This commit switches usb_hcd_giveback_urb() from spin_lock() to
spin_lock_irqsave() when acquiring bh->lock to prevent same-CPU hardirq and
softirq deadlocks.

> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index 8e8a5f59b319b..7bb25e7c447d4 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -1731,6 +1731,7 @@ static void usb_giveback_urb_bh(struct work_struct *work)
>  void usb_hcd_giveback_urb(struct usb_hcd *hcd, struct urb *urb, int status)
>  {
>  	struct giveback_urb_bh *bh;
> +	unsigned long flags;
>  	bool running;
>  
>  	/* pass status to BH via unlinked */
> @@ -1747,10 +1748,10 @@ void usb_hcd_giveback_urb(struct usb_hcd *hcd, struct urb *urb, int status)
>  	else
>  		bh = &hcd->low_prio_bh;
>  
> -	spin_lock(&bh->lock);
> +	spin_lock_irqsave(&bh->lock, flags);
>  	list_add_tail(&urb->urb_list, &bh->head);
>  	running = bh->running;
> -	spin_unlock(&bh->lock);
> +	spin_unlock_irqrestore(&bh->lock, flags);

[Severity: Critical]
This problem wasn't introduced by this patch, but can usb_giveback_urb_bh()
access a freed struct usb_hcd [1] if register_root_hub() fails in
usb_add_hcd()?

During register_root_hub(), root-hub URBs have is_root_hub(urb->dev) set, so
usb_hcd_giveback_urb() queues them on hcd->low_prio_bh.bh or
hcd->high_prio_bh.bh.  In usb_giveback_urb_bh(), __usb_hcd_giveback_urb()
invokes urb->complete() and wakes the waiting task before finishing its own
bookkeeping on struct giveback_urb_bh:

CPU0                                CPU1
usb_add_hcd()
  register_root_hub()
    ...
      usb_hcd_giveback_urb()
        queue_work(..., &bh->bh)
                                    usb_giveback_urb_bh()
                                      __usb_hcd_giveback_urb()
                                        urb->complete()
    // fails and returns error
  goto err_register_root_hub;
usb_put_hcd(hcd)                    // frees hcd
                                      bh->completing_ep = NULL;
                                      spin_lock_irq(&bh->lock);  <-- use-after-free

Neither the err_register_root_hub error path in usb_add_hcd() nor
usb_stop_hcd() cancels hcd->high_prio_bh.bh and hcd->low_prio_bh.bh before
the caller drops the last reference to hcd.

[1] https://sashiko.dev/#/bug/linux-6f3b1df1-c32a-430a-a126-7f20495af396

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113041948.1280304-1-rickyniu@google.com?part=1