usbip: Fix locking bug in RT-enabled kernels

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

Posted by Lizhi Xu 2 weeks, 2 days ago

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;
 }
 
-- 
2.43.0

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

Posted by Alan Stern 2 weeks, 1 day ago

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?

Alan Stern

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

Posted by Shuah Khan 2 weeks, 1 day ago

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?

thanks,
-- Shuah

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

Posted by Lizhi Xu 2 weeks, 1 day ago

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.

BR,
Lizhi

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

Posted by Shuah Khan 2 weeks ago

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