Re: [PATCH V2] usbip: Fix locking bug in RT-enabled kernels

[Shuah Khan <skhan@linuxfoundation.org>]

On 9/16/25 18:51, Lizhi Xu wrote:
> On Tue, 16 Sep 2025 17:33:44 -0600, Shuah Khan wrote:
>> On 9/16/25 09:47, Alan Stern wrote:
>>> 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@syzkaller.appspotmail.com
>>>> Closes: https://syzkaller.appspot.com/bug?extid=205ef33a3b636b4181fb
>>>> Signed-off-by: Lizhi Xu <lizhi.xu@windriver.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?
>>>
>>
>> Looks good to me.
>> +1 on changing all other instances - can we do that?
> I'm replying to both of you. Personally, I suggest this isn't necessary
> right now; it's safer to wait until a problem is reported before fixing it.
> 
> Also, the context of several other calls to usb_hcd_giveback_urb() in usbip
> differs from the current issue. Enabling RT_PREEMPT shouldn't cause similar
> issues.

Fair enough reasoning to wait. Here is my Acked by.

Acked-by: Shuah Khan <skhan@linuxfoundation.org>

Greg, please pick this up.

thanks,
-- Shuah