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

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

[Chien Kun Niu]

usb_hcd_giveback_urb() can be called with local interrupts enabled,
but it acquires bh->lock with plain spin_lock(). bh->lock is also
taken in hardirq context by HCD interrupt handlers and in BH context
by usb_giveback_urb_bh().

If an interrupt arrives on the same CPU while usb_hcd_giveback_urb()
holds bh->lock with IRQs enabled, it can deadlock on bh->lock in
usb_hcd_irq(), or in usb_giveback_urb_bh() on IRQ exit if the BH work
was already queued. We hit the latter during reboot when usbnet
unlinked RX URBs on a halted xHCI controller:

  Call trace:
   queued_spin_lock_slowpath
   _raw_spin_lock_irq
   usb_giveback_urb_bh
   process_scheduled_works
   bh_worker
   workqueue_softirq_action
   __do_softirq
   irq_exit_rcu
   el1_interrupt
   _raw_spin_lock
   usb_hcd_giveback_urb
   xhci_urb_dequeue
   unlink1
   usb_hcd_unlink_urb
   usb_unlink_urb
   unlink_urbs [usbnet]

Use spin_lock_irqsave() and spin_unlock_irqrestore() when taking
bh->lock in usb_hcd_giveback_urb() to prevent same-CPU hardirq and
softirq re-entrancy.

Fixes: 94dfd7edfd5c ("USB: HCD: support giveback of URB in tasklet context")
Cc: stable@vger.kernel.org
Signed-off-by: Chien Kun Niu <rickyniu@google.com>
---
 drivers/usb/core/hcd.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 8e8a5f59b..7bb25e7c4 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);
 
 	if (running)
 		;
-- 
2.56.0.611.g1030f0b940-goog

[PATCH 2/2] xhci: Give back URB under lock in xhci_urb_dequeue()

[Chien Kun Niu]

Before commit 36dc01657b49 ("usb: host: xhci: Support running urb
giveback in tasklet context") set HCD_BH, usb_hcd_giveback_urb() ran
urb->complete() synchronously, so xHCI had to drop xhci->lock before
giving back an URB. That commit updated xhci_giveback_urb_in_irq() to
keep xhci->lock held across usb_hcd_giveback_urb(), covering normal
completions, xhci_process_cancelled_tds(), and xhci_hc_died(), while
the open-coded err_giveback path in xhci_urb_dequeue() was missed.

Dropping xhci->lock between usb_hcd_unlink_urb_from_ep() and
usb_hcd_giveback_urb() breaks per-endpoint giveback serialization: a
concurrent xhci_urb_dequeue() or xhci_hc_died() can acquire xhci->lock
and queue a later URB onto bh->head ahead of this one, completing URBs
out of order. When spinlocks disable interrupts, unlocking first also
causes a redundant IRQ restore/save bounce into bh->lock, and without
an IRQ-safe bh->lock it exposed task-context unlinks to a same-CPU
deadlock.

Move usb_hcd_giveback_urb() before spin_unlock_irqrestore(&xhci->lock,
flags) in err_giveback to match xhci_giveback_urb_in_irq().

Fixes: 36dc01657b49 ("usb: host: xhci: Support running urb giveback in tasklet context")
Cc: stable@vger.kernel.org
Signed-off-by: Chien Kun Niu <rickyniu@google.com>
---
 drivers/usb/host/xhci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 30ac1bc45..3bca3a91f 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -1879,8 +1879,8 @@ static int xhci_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, int status)
 	if (urb_priv)
 		xhci_urb_free_priv(urb_priv);
 	usb_hcd_unlink_urb_from_ep(hcd, urb);
-	spin_unlock_irqrestore(&xhci->lock, flags);
 	usb_hcd_giveback_urb(hcd, urb, -ESHUTDOWN);
+	spin_unlock_irqrestore(&xhci->lock, flags);
 	return ret;
 }
 
-- 
2.56.0.611.g1030f0b940-goog

[PATCH 0/2] usb: Fix giveback_urb_bh deadlock on URB dequeue

[Chien Kun Niu]

We hit a CPU hardlockup during reboot while usbnet was unlinking
queued RX URBs on a halted xHCI controller.

In xhci_urb_dequeue(), the err_giveback path releases xhci->lock and
restores interrupts before calling usb_hcd_giveback_urb(). Because
usb_hcd_giveback_urb() takes bh->lock with plain spin_lock(), an
interrupt arriving on the same CPU can deadlock on bh->lock either in
usb_hcd_irq() or when softirq runs usb_giveback_urb_bh().

Patch 1 switches usb_hcd_giveback_urb() to spin_lock_irqsave() since
bh->lock is also acquired in hardirq and BH contexts.

Patch 2 keeps xhci->lock held across usb_hcd_giveback_urb() in
xhci_urb_dequeue(), matching xhci_giveback_urb_in_irq() and keeping
per-endpoint URB giveback ordering serialized.

The two patches are independent and can be applied in either order.

Chien Kun Niu (2):
  usb: core: hcd: Disable IRQs when taking bh->lock
  xhci: Give back URB under lock in xhci_urb_dequeue()

 drivers/usb/core/hcd.c  | 5 +++--
 drivers/usb/host/xhci.c | 2 +-
 2 files changed, 4 insertions(+), 3 deletions(-)


base-commit: b3dfbde62cf1981c4638939c159dfd087b7a72e9
-- 
2.56.0.567.gdb80ae8cd5-goog

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

[sashiko-bot]

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

Re: [PATCH 2/2] xhci: Give back URB under lock in xhci_urb_dequeue()

[sashiko-bot]

> Before commit 36dc01657b49 ("usb: host: xhci: Support running urb
> giveback in tasklet context") set HCD_BH, usb_hcd_giveback_urb() ran
> urb->complete() synchronously, so xHCI had to drop xhci->lock before
> giving back an URB. That commit updated xhci_giveback_urb_in_irq() to
> keep xhci->lock held across usb_hcd_giveback_urb(), covering normal
> completions, xhci_process_cancelled_tds(), and xhci_hc_died(), while
> the open-coded err_giveback path in xhci_urb_dequeue() was missed.
> 
> Dropping xhci->lock between usb_hcd_unlink_urb_from_ep() and
> usb_hcd_giveback_urb() breaks per-endpoint giveback serialization: a
> concurrent xhci_urb_dequeue() or xhci_hc_died() can acquire xhci->lock
> and queue a later URB onto bh->head ahead of this one, completing URBs
> out of order. When spinlocks disable interrupts, unlocking first also
> causes a redundant IRQ restore/save bounce into bh->lock, and without
> an IRQ-safe bh->lock it exposed task-context unlinks to a same-CPU
> deadlock.
> 
> Move usb_hcd_giveback_urb() before spin_unlock_irqrestore(&xhci->lock,
> flags) in err_giveback to match xhci_giveback_urb_in_irq().
> 
> Fixes: 36dc01657b49 ("usb: host: xhci: Support running urb giveback in tasklet context")
> Cc: stable@vger.kernel.org
> Signed-off-by: Chien Kun Niu <rickyniu@google.com>

Sashiko has reviewed this patch and found no issues.

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