[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <61e796d2-0139-4459-a4e3-f27892384de2@rowland.harvard.edu>
Date: Tue, 16 Sep 2025 11:47:56 -0400
From: Alan Stern <stern@...land.harvard.edu>
To: Lizhi Xu <lizhi.xu@...driver.com>,
Valentina Manea <valentina.manea.m@...il.com>,
Shuah Khan <shuah@...nel.org>
Cc: gregkh@...uxfoundation.org, linux-kernel@...r.kernel.org,
linux-usb@...r.kernel.org,
syzbot+205ef33a3b636b4181fb@...kaller.appspotmail.com,
syzkaller-bugs@...glegroups.com
Subject: Re: [PATCH V2] usbip: Fix locking bug in RT-enabled kernels
On Tue, Sep 16, 2025 at 09:41:43AM +0800, Lizhi Xu wrote:
> Interrupts are disabled before entering usb_hcd_giveback_urb().
> A spinlock_t becomes a sleeping lock on PREEMPT_RT, so it cannot be
> acquired with disabled interrupts.
>
> Save the interrupt status and restore it after usb_hcd_giveback_urb().
>
> syz reported:
> BUG: sleeping function called from invalid context at kernel/locking/spinlock_rt.c:48
> Call Trace:
> dump_stack_lvl+0x189/0x250 lib/dump_stack.c:120
> rt_spin_lock+0xc7/0x2c0 kernel/locking/spinlock_rt.c:57
> spin_lock include/linux/spinlock_rt.h:44 [inline]
> mon_bus_complete drivers/usb/mon/mon_main.c:134 [inline]
> mon_complete+0x5c/0x200 drivers/usb/mon/mon_main.c:147
> usbmon_urb_complete include/linux/usb/hcd.h:738 [inline]
> __usb_hcd_giveback_urb+0x254/0x5e0 drivers/usb/core/hcd.c:1647
> vhci_urb_enqueue+0xb4f/0xe70 drivers/usb/usbip/vhci_hcd.c:818
>
> Reported-by: syzbot+205ef33a3b636b4181fb@...kaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=205ef33a3b636b4181fb
> Signed-off-by: Lizhi Xu <lizhi.xu@...driver.com>
> ---
> V1 -> V2: fix it in usbip
>
> drivers/usb/usbip/vhci_hcd.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c
> index e70fba9f55d6..eb6de7e8ea7b 100644
> --- a/drivers/usb/usbip/vhci_hcd.c
> +++ b/drivers/usb/usbip/vhci_hcd.c
> @@ -809,15 +809,15 @@ static int vhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb, gfp_t mem_flag
> no_need_xmit:
> usb_hcd_unlink_urb_from_ep(hcd, urb);
> no_need_unlink:
> - spin_unlock_irqrestore(&vhci->lock, flags);
> if (!ret) {
> /* usb_hcd_giveback_urb() should be called with
> * irqs disabled
> */
> - local_irq_disable();
> + spin_unlock(&vhci->lock);
> usb_hcd_giveback_urb(hcd, urb, urb->status);
> - local_irq_enable();
> + spin_lock(&vhci->lock);
> }
> + spin_unlock_irqrestore(&vhci->lock, flags);
> return ret;
> }
This looks right to me; it's the same pattern that the other host
controller drivers use. However, the final decision is up to the usbip
maintainers.
Also, there are several other places in the usbip drivers where
usb_hcd_giveback_urb() gets called; shouldn't they all be changed to
follow this pattern?
Alan Stern
Powered by blists - more mailing lists