Re: [BUG] usbip: vhci: Sleeping function called from invalid context in vhci_urb_enqueue on PREEMPT_RT

[Yunseong Kim <ysk@kzalloc.com>]

Hi Alan,

On 8/16/25 10:51 AM, Alan Stern wrote:
> On Sat, Aug 16, 2025 at 10:29:34AM +0900, Yunseong Kim wrote:
>> While testing a PREEMPT_RT enabled kernel (based on v6.17.0-rc1),
>> I encountered a "BUG: sleeping function called from invalid context"
>> error originating from the USB/IP VHCI driver.
>>
>> On PREEMPT_RT configurations, standard spin_lock() calls are replaced by
>> rt_spin_lock(). Since rt_spin_lock() may sleep when contended, it must not
>> be called from an atomic context (e.g., with interrupts disabled).
>>
>> The issue occurs within the vhci_urb_enqueue function This function
>> explicitly disables local interrupts using local_irq_disable() immediately
>> before calling usb_hcd_giveback_urb(), adhering to HCD requirements.
> 
> ...
> 
>> This error reported after this work:
>> It occurs after going through the code below:
>>
>>  static int vhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb, gfp_t mem_flags)
>>  {
>>  
>>  	...
>>  
>>  no_need_unlink:
>>  	spin_unlock_irqrestore(&vhci->lock, flags);
>>  	if (!ret) {
>>  		/* usb_hcd_giveback_urb() should be called with
>>  		 * irqs disabled
>>  		 */
>>  		local_irq_disable(); // <--- Entering atomic context (IRQs disabled)
>>  		usb_hcd_giveback_urb(hcd, urb, urb->status);
>>  		local_irq_enable();
>>  	}
>>  	return ret;
>>  }
>>
>>  static void mon_bus_complete(struct mon_bus *mbus, struct urb *urb, int status)
>>  {
>>  	...
>>  	spin_lock_irqsave(&mbus->lock, flags);
>                   ^
> ------------------^
> 
>>  	...
>>  }
>>
>> When called with interrupts disabled, usb_hcd_giveback_urb() eventually
>> leads to mon_complete() in the USB monitoring, if usbmon is enabled,
>> via __usb_hcd_giveback_urb().
>>
>> mon_complete() attempts to acquire a lock via spin_lock(), observed in the
>> trace within the inlined mon_bus_complete.
> 
> Look again.  mon_bus_complete() calls spin_lock_irqsave(), not 
> spin_lock().
> 
> Is the kernel tree that you are using different from Linus's tree?
I think this part is a macro, so it appears this way.

Link: https://github.com/torvalds/linux/blob/v6.17-rc1/include/linux/spinlock_rt.h#L96

#define spin_lock_irqsave(lock, flags)			 \
	do {						 \
		typecheck(unsigned long, flags);	 \
		flags = 0;				 \
		spin_lock(lock);			 \
	} while (0)

My tree is indeed 6.17-rc1. I made a mistake in the diagram,
which caused the misunderstanding. I’ve redrawn the diagram:

  kworker (hub_event)
      |
      v
  vhci_urb_enqueue() [drivers/usb/usbip/vhci_hcd.c]
      |
      |---> spin_unlock_irqrestore(&vhci->lock, flags);
      |     (Context: IRQs Enabled, Process Context)
      |---> local_irq_disable();
      |
      |     *** STATE CHANGE: IRQs Disabled (Atomic Context) ***
      |
      +-----> usb_hcd_giveback_urb() [drivers/usb/core/hcd.c]
              |
              v
              __usb_hcd_giveback_urb()
              |
              v
              mon_complete() [drivers/usb/mon/mon_main.c]
              |
              |---> spin_lock_irqsave() [include/linux/spinlock_rt.h]
                    |
                    v https://github.com/torvalds/linux/blob/v6.17-rc1/include/linux/spinlock_rt.h#L96
                    spin_lock() [kernel/locking/spinlock_rt.c] <--- Attempts to acquire lock
                    |
                    | [On PREEMPT_RT]
                    v
                    rt_spin_lock() [kernel/locking/spinlock_rt.c]
                    |
                    v
                    [May Sleep if contended]
                    |
      X <----------- BUG: Sleeping in atomic context (IRQs are disabled!)

      |
      |---> local_irq_enable();
            (Context: IRQs Enabled)


> Alan Stern

Thank you!

Yunseong Kim