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

[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

[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