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

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

[Yunseong Kim]

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:
Link: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git/commit/?h=usb-linus&id=9528d32873b38281ae105f2f5799e79ae9d086c2

  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()  <--- 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)

Stack trace excerpt:

 BUG: sleeping function called from invalid context at kernel/locking/spinlock_rt.c:48
 in_atomic(): 0, irqs_disabled(): 1, non_block: 0, pid: 11063, name: kworker/0:5
 preempt_count: 0, expected: 0
 RCU nest depth: 0, expected: 0
 CPU: 0 UID: 0 PID: 11063 Comm: kworker/0:5 Not tainted 6.17.0-rc1-00001-g1149a5db27c8-dirty #55 PREEMPT_RT 
 Hardware name: QEMU KVM Virtual Machine, BIOS 2025.02-8ubuntu1 06/11/2025
 Workqueue: usb_hub_wq hub_event
 Call trace:
  show_stack+0x2c/0x3c arch/arm64/kernel/stacktrace.c:499 (C)
  __dump_stack+0x30/0x40 lib/dump_stack.c:94
  dump_stack_lvl+0x148/0x1d8 lib/dump_stack.c:120
  dump_stack+0x1c/0x3c lib/dump_stack.c:129
  __might_resched+0x2e4/0x52c kernel/sched/core.c:8957
  __rt_spin_lock kernel/locking/spinlock_rt.c:48 [inline]
  rt_spin_lock+0xa8/0x1bc 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/0x1fc drivers/usb/mon/mon_main.c:147
  usbmon_urb_complete include/linux/usb/hcd.h:738 [inline]
  __usb_hcd_giveback_urb+0x1e4/0x59c drivers/usb/core/hcd.c:1647
  usb_hcd_giveback_urb+0x100/0x364 drivers/usb/core/hcd.c:1745
  vhci_urb_enqueue+0x86c/0xc08 drivers/usb/usbip/vhci_hcd.c:818
  usb_hcd_submit_urb+0x2ec/0x1790 drivers/usb/core/hcd.c:1546
  usb_submit_urb+0xd3c/0x13ec drivers/usb/core/urb.c:581
  usb_start_wait_urb+0xf0/0x3c8 drivers/usb/core/message.c:59
  usb_internal_control_msg drivers/usb/core/message.c:103 [inline]
  usb_control_msg+0x1d0/0x350 drivers/usb/core/message.c:154
  hub_set_address drivers/usb/core/hub.c:4769 [inline]
  hub_port_init+0xbac/0x2094 drivers/usb/core/hub.c:5074
  hub_port_connect drivers/usb/core/hub.c:5495 [inline]
  hub_port_connect_change drivers/usb/core/hub.c:5706 [inline]
  port_event drivers/usb/core/hub.c:5870 [inline]
  hub_event+0x1de4/0x3c44 drivers/usb/core/hub.c:5952
  process_one_work kernel/workqueue.c:3236 [inline]
  process_scheduled_works+0x68c/0x1118 kernel/workqueue.c:3319
  worker_thread+0x834/0xc1c kernel/workqueue.c:3400
  kthread+0x5f4/0x754 kernel/kthread.c:463
  ret_from_fork+0x10/0x20 arch/arm64/kernel/entry.S:844

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.

Because vhci_urb_enqueue has already disabled interrupts, calling the
potentially sleepable rt_spin_lock() within this atomic context is invalid
and triggers the kernel BUG.

I request a review and correction of this locking mechanism to ensure
stability on PREEMPT_RT configurations.  Kernel config, full logs, and
reproduction steps can be provided on request.


Thanks,

Best Regards,
Yunseong Kim

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

[Yunseong Kim]

P.S.

Just to clarify,

On 8/16/25 10:29 AM, 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:
> Link: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git/commit/?h=usb-linus&id=9528d32873b38281ae105f2f5799e79ae9d086c2

To be clear and avoid any misunderstanding, reporting was done after
this work "kcov, usb: Don't disable interrupts in kcov_remote_start_usb_softirq()"
because the fuzzing could proceed smoothly on PREEMPT_RT following this work,
which allowed the issue to be discovered. It does not mean that this step
introduced the error at all.

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

[Alan Stern]

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?

Alan Stern

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

[Yunseong Kim]

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

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

[Alan Stern]

On Sat, Aug 16, 2025 at 11:18:02AM +0900, Yunseong Kim wrote:
> 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)

So it looks like we should be using a different function instead of 
local_irq_disable().  We need something which in a non-RT build will 
disable interrupts on the local CPU, but in an RT build will merely 
disable preemption.  (In fact, every occurrence of local_irq_disable() 
in the USB subsystem probably should be changed in this way.)

Is there such a function?

Alan Stern

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

[Alan Stern]

On Sat, Aug 16, 2025 at 10:16:34AM -0400, Alan Stern wrote:
> So it looks like we should be using a different function instead of 
> local_irq_disable().  We need something which in a non-RT build will 
> disable interrupts on the local CPU, but in an RT build will merely 
> disable preemption.  (In fact, every occurrence of local_irq_disable() 
> in the USB subsystem probably should be changed in this way.)

Or maybe what we need is something that in a non-RT build will disable 
local interrupts and in an RT build will do nothing.  (I suspect that RT 
kernels won't like it if we call spin_lock() while preemption is 
disabled.)

> Is there such a function?

Alan Stern

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

[Sebastian Andrzej Siewior]

On 2025-08-17 10:27:11 [-0400], Alan Stern wrote:
> On Sat, Aug 16, 2025 at 10:16:34AM -0400, Alan Stern wrote:
> > So it looks like we should be using a different function instead of 
> > local_irq_disable().  We need something which in a non-RT build will 
> > disable interrupts on the local CPU, but in an RT build will merely 
> > disable preemption.  (In fact, every occurrence of local_irq_disable() 
> > in the USB subsystem probably should be changed in this way.)
> 
> Or maybe what we need is something that in a non-RT build will disable 
> local interrupts and in an RT build will do nothing.  (I suspect that RT 
> kernels won't like it if we call spin_lock() while preemption is 
> disabled.)

This is the local_irq_disable() in vhci_urb_enqueue() before
usb_hcd_giveback_urb() is invoked. It was added in 9e8586827a706
("usbip: vhci_hcd: fix calling usb_hcd_giveback_urb() with irqs
enabled").
The warning that fixed back then was 
|         if (WARN_ON(in_task() && kcov_mode_enabled(mode))) {
which was kernel/kcov.c:834 as of v5.9-rc8 (as of report the mentioned
in the commit).
local_irq_disable() does not change the preemption counter so I am a bit
puzzled why this did shut the warning.

> > Is there such a function?

We could use some API that accidentally does what you ask for. There
would be local_lock_t where local_lock_irq() does that.
What about moving the completion callback to softirq by setting HCD_BH?

> Alan Stern

Sebastian

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

[Alan Stern]

On Tue, Aug 19, 2025 at 01:04:57PM +0200, Sebastian Andrzej Siewior wrote:
> On 2025-08-17 10:27:11 [-0400], Alan Stern wrote:
> > On Sat, Aug 16, 2025 at 10:16:34AM -0400, Alan Stern wrote:
> > > So it looks like we should be using a different function instead of 
> > > local_irq_disable().  We need something which in a non-RT build will 
> > > disable interrupts on the local CPU, but in an RT build will merely 
> > > disable preemption.  (In fact, every occurrence of local_irq_disable() 
> > > in the USB subsystem probably should be changed in this way.)
> > 
> > Or maybe what we need is something that in a non-RT build will disable 
> > local interrupts and in an RT build will do nothing.  (I suspect that RT 
> > kernels won't like it if we call spin_lock() while preemption is 
> > disabled.)
> 
> This is the local_irq_disable() in vhci_urb_enqueue() before
> usb_hcd_giveback_urb() is invoked. It was added in 9e8586827a706
> ("usbip: vhci_hcd: fix calling usb_hcd_giveback_urb() with irqs
> enabled").
> The warning that fixed back then was 
> |         if (WARN_ON(in_task() && kcov_mode_enabled(mode))) {
> which was kernel/kcov.c:834 as of v5.9-rc8 (as of report the mentioned
> in the commit).
> local_irq_disable() does not change the preemption counter so I am a bit
> puzzled why this did shut the warning.
> 
> > > Is there such a function?
> 
> We could use some API that accidentally does what you ask for. There
> would be local_lock_t where local_lock_irq() does that.
> What about moving the completion callback to softirq by setting HCD_BH?

You're missing the point.

There are several places in the USB stack that disable local interrupts.  
The idea was that -- on a non-RT system, which was all we had at the 
time -- spin_lock_irqsave() is logically equivalent to a combination of 
local_irq_save() and spin_lock().  Similarly, spin_lock_irq() is 
logically equivalent to local_irq_disable() plus spin_lock().

So code was written which, for various reasons, used local_irq_save() 
(or local_irq_disable()) and spin_lock() instead of spin_lock_irqsave() 
(or spin_lock_irq()).  But now we see that in RT builds, this 
equivalency is not valid.  Instead, spin_lock_irqsave(flags) is 
logically equivalent to "flags = 0" plus spin_lock() (and 
spin_lock_irq() is logically equivalent to a nop plus spin_lock()).  At 
least, that's how the material quoted earlier by Yunseong defines it.

Therefore, throughout the USB stack, we should replace calls to 
local_irq_save() and local_irq_disable() with functions that behave like 
the original in non-RT builds but do nothing in RT builds.  We shouldn't 
just worry about this one spot.

I would expect that RT already defines functions which do this, but I 
don't know their names.

Alan Stern

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

[Sebastian Andrzej Siewior]

On 2025-08-19 10:12:31 [-0400], Alan Stern wrote:
> > We could use some API that accidentally does what you ask for. There
> > would be local_lock_t where local_lock_irq() does that.
> > What about moving the completion callback to softirq by setting HCD_BH?
> 
> You're missing the point.
> 
> There are several places in the USB stack that disable local interrupts.  

But *why*? You need locking due to SMP. So it should be simply to avoid
irqrestore()/ irqsave() during unlock/lock or to avoid deadlocks if a
callback is invoked from IRQ and process context and the callback
handler does simply spin_lock() (without the _irq suffix).
The latter shouldn't be problem due to commit
	ed194d1367698 ("usb: core: remove local_irq_save() around ->complete() handler")

So if completing the URB tasklet/ softirq context works for ehci/ xhci
without creating any warning, it should also work for vhci, dummy_hcd.
Only RH code completes directly, everything else is shifted to softirq
context (for ehci/HCD_BH).

> The idea was that -- on a non-RT system, which was all we had at the 
> time -- spin_lock_irqsave() is logically equivalent to a combination of 
> local_irq_save() and spin_lock().  Similarly, spin_lock_irq() is 
> logically equivalent to local_irq_disable() plus spin_lock().
> 
> So code was written which, for various reasons, used local_irq_save() 
> (or local_irq_disable()) and spin_lock() instead of spin_lock_irqsave() 
> (or spin_lock_irq()).  But now we see that in RT builds, this 
> equivalency is not valid.  Instead, spin_lock_irqsave(flags) is 
> logically equivalent to "flags = 0" plus spin_lock() (and 
> spin_lock_irq() is logically equivalent to a nop plus spin_lock()).  At 
> least, that's how the material quoted earlier by Yunseong defines it.
> 
> Therefore, throughout the USB stack, we should replace calls to 
> local_irq_save() and local_irq_disable() with functions that behave like 
> the original in non-RT builds but do nothing in RT builds.  We shouldn't 
> just worry about this one spot.

| git grep -E 'local_irq_save|local_irq_disable' drivers/usb/ | wc -l
| 21
of which 10 are in pxa udc. The only one I am a bit concerned about is
the one in usb_hcd_pci_remove() and I think we had reports and patches
but somehow nothing did happen and I obviously forgot.

> I would expect that RT already defines functions which do this, but I 
> don't know their names.

We don't have anything where
	local_irq_disable()
	spin_lock()

can be mapped to something equivalent other than
	spin_lock_irq()

I was running around and kept changing code so that we don't end up in
this scenario where we need to disable interrupts for some reason but on
RT we don't.

The closest thing we have is local_lock_irq() which maps to
local_irq_disable() on !PREEMPT_RT systems. But I would prefer to avoid
it because it serves a different purpose.
What works is something like
	spin_lock_irqsave();
	spin_unlock();
	$SOMETHING
	spin_lock();
	spin_unlock_irqestore().

The question is why should $SOMETHING be invoked with disabled
interrupts if the function was called from process context.

If your concern is a missing _irqsave() in the callback then this
shouldn't be an issue. If it is the wrong context from kcov's point of
view then making the driver complete in tasklet should fix it since it
would match what ehci/ xhci does. 

> Alan Stern

Sebastian

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

[Alan Stern]

On Tue, Aug 19, 2025 at 04:57:00PM +0200, Sebastian Andrzej Siewior wrote:
> > There are several places in the USB stack that disable local interrupts.  
> 
> But *why*? You need locking due to SMP. So it should be simply to avoid
> irqrestore()/ irqsave() during unlock/lock or to avoid deadlocks if a
> callback is invoked from IRQ and process context and the callback
> handler does simply spin_lock() (without the _irq suffix).
> The latter shouldn't be problem due to commit
> 	ed194d1367698 ("usb: core: remove local_irq_save() around ->complete() handler")
> 
> So if completing the URB tasklet/ softirq context works for ehci/ xhci
> without creating any warning, it should also work for vhci, dummy_hcd.

dummy-hcd is different from the others; its use of local_irq_save() is 
in the gadget giveback path, not the host giveback path.  We would need 
another audit similar to the one you did for ed194d136769, but this 
time checking gadget completion handlers.

> Only RH code completes directly, everything else is shifted to softirq
> context (for ehci/HCD_BH).

Correct (except that RH code always uses softirq context; it never 
completes directly -- the kerneldoc is wrong and Greg just accepted a 
patch to change it).

There are other places that disable local interrupts.  ehci-brcm.c does 
it in order to meet a timing constraint.  ohci-omap.c and ohci-s3c2410.c 
do it for reasons I'm not aware of (no comment about it in the source).  
gadget/legacy/inode.c does it in ep_aio_cancel() -- I can only guess 
that this is somehow related to aio and not to anything in USB.

> | git grep -E 'local_irq_save|local_irq_disable' drivers/usb/ | wc -l
> | 21
> of which 10 are in pxa udc. The only one I am a bit concerned about is
> the one in usb_hcd_pci_remove() and I think we had reports and patches
> but somehow nothing did happen and I obviously forgot.
> 
> > I would expect that RT already defines functions which do this, but I 
> > don't know their names.
> 
> We don't have anything where
> 	local_irq_disable()
> 	spin_lock()
> 
> can be mapped to something equivalent other than
> 	spin_lock_irq()
> 
> I was running around and kept changing code so that we don't end up in
> this scenario where we need to disable interrupts for some reason but on
> RT we don't.
> 
> The closest thing we have is local_lock_irq() which maps to
> local_irq_disable() on !PREEMPT_RT systems. But I would prefer to avoid
> it because it serves a different purpose.
> What works is something like
> 	spin_lock_irqsave();
> 	spin_unlock();
> 	$SOMETHING
> 	spin_lock();
> 	spin_unlock_irqestore().

That's just silly.  We should have something like this:

#ifdef CONFIG_PREEMPT_RT
static inline void local_irqsave_nonrt(unsigned long flags) {}
static inline void local_irqrestore_nonrt(unsigned long flags) {}
static inline void local_irq_disable_nonrt() {}
static inline void local_irq_enable_nonrt() {}
#else
#define local_irqsave_nonrt	local_irqsave
#define local_irqrestore_nonrt	local_irqrestore
#define local_irq_disable_nonrt	local_irq_disable
#define local_irq_enable_nonrt	local_irq_enable
#endif

> The question is why should $SOMETHING be invoked with disabled
> interrupts if the function was called from process context.

More to the point, out of all the possible reasons why $SOMETHING might 
be invoked with disabled interrupts, which of these reasons remain valid 
in RT builds and which don't?

> If your concern is a missing _irqsave() in the callback then this
> shouldn't be an issue. If it is the wrong context from kcov's point of
> view then making the driver complete in tasklet should fix it since it
> would match what ehci/ xhci does. 

No, I'm not concerned about that.

Alan Stern

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

[Sebastian Andrzej Siewior]

On 2025-08-19 11:44:52 [-0400], Alan Stern wrote:
> dummy-hcd is different from the others; its use of local_irq_save() is 
> in the gadget giveback path, not the host giveback path.  We would need 
> another audit similar to the one you did for ed194d136769, but this 
> time checking gadget completion handlers.

ach right. 

> > Only RH code completes directly, everything else is shifted to softirq
> > context (for ehci/HCD_BH).
> 
> Correct (except that RH code always uses softirq context; it never 
> completes directly -- the kerneldoc is wrong and Greg just accepted a 
> patch to change it).

Ach okay. I assumed it because the completion handler skips it. But that
might be a shortcut because it already is in softirq context.

> There are other places that disable local interrupts.  ehci-brcm.c does 
> it in order to meet a timing constraint.  ohci-omap.c and ohci-s3c2410.c 
> do it for reasons I'm not aware of (no comment about it in the source).  
> gadget/legacy/inode.c does it in ep_aio_cancel() -- I can only guess 
> that this is somehow related to aio and not to anything in USB.

the inode.c looks interesting.c. It got there in 75787d943ab37 ("[PATCH]
USB: gadgetfs AIO support") which is 2004. So it might assume !SMP in
terms of locking. Anyway…

> > The closest thing we have is local_lock_irq() which maps to
> > local_irq_disable() on !PREEMPT_RT systems. But I would prefer to avoid
> > it because it serves a different purpose.
> > What works is something like
> > 	spin_lock_irqsave();
> > 	spin_unlock();
> > 	$SOMETHING
> > 	spin_lock();
> > 	spin_unlock_irqestore().
> 
> That's just silly.  We should have something like this:
> 
> #ifdef CONFIG_PREEMPT_RT
> static inline void local_irqsave_nonrt(unsigned long flags) {}
> static inline void local_irqrestore_nonrt(unsigned long flags) {}
> static inline void local_irq_disable_nonrt() {}
> static inline void local_irq_enable_nonrt() {}
> #else
> #define local_irqsave_nonrt	local_irqsave
> #define local_irqrestore_nonrt	local_irqrestore
> #define local_irq_disable_nonrt	local_irq_disable
> #define local_irq_enable_nonrt	local_irq_enable
> #endif

We managed over the years to get rid of each one of this instances/
requirements. The RT tree used to have
|#ifdef CONFIG_PREEMPT_RT_FULL
|# define local_irq_disable_nort()     do { } while (0)
|# define local_irq_disable_rt()               local_irq_disable()
|#else
|# define local_irq_disable_nort()     local_irq_disable()
|# define local_irq_disable_rt()               do { } while (0)
|#endif

which was removed as of v4.16.7-rt1.
The problem is usually that nobody knows why exactly interrupts are
disabled an what purpose it serves. Often the reasons is no longer there
but the code still does it.

> > The question is why should $SOMETHING be invoked with disabled
> > interrupts if the function was called from process context.
> 
> More to the point, out of all the possible reasons why $SOMETHING might 
> be invoked with disabled interrupts, which of these reasons remain valid 
> in RT builds and which don't?

None (in most cases) because interrupt handler are threaded. So
interrupts are never truly disabled.
Adding the macros as you suggest would gain probably three users:
- inode
- dummy_hcd
- vhci-hcd

Instead I would:
- vhci I would suggest to remove the disabling and move its completion
   to BH.
- dummy_hcd I would suggest to either do the thing you called silly or
  audit the gadgets and remove it.
- inode I would suggest to keep it as-is and audit it properly later
  once someone intends to use it. It would also give the opportunity to
  clean up the commented locking statement.

> Alan Stern

Sebastian

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

[Alan Stern]

On Wed, Aug 20, 2025 at 06:26:21PM +0200, Sebastian Andrzej Siewior wrote:
> The problem is usually that nobody knows why exactly interrupts are
> disabled an what purpose it serves. Often the reasons is no longer there
> but the code still does it.

Indeed, that seems to be the problem in several places here.

> > More to the point, out of all the possible reasons why $SOMETHING might 
> > be invoked with disabled interrupts, which of these reasons remain valid 
> > in RT builds and which don't?
> 
> None (in most cases) because interrupt handler are threaded. So
> interrupts are never truly disabled.
> Adding the macros as you suggest would gain probably three users:
> - inode
> - dummy_hcd
> - vhci-hcd
> 
> Instead I would:
> - vhci I would suggest to remove the disabling and move its completion
>    to BH.

Agreed.  But it's up to the maintainer.

> - dummy_hcd I would suggest to either do the thing you called silly or
>   audit the gadgets and remove it.

Auditing is the best approach.  It would be a lot of work, though.
Also, it's worth noting that other UDC drivers do use the 

	spin_lock_irqsave();
	...
	spin_unlock();
	usb_gadget_giveback_request();
	spin_lock();
	... 
	spin_unlock_irqrestore();

scheme, so using it in dummy_hcd would be reasonable.

> - inode I would suggest to keep it as-is and audit it properly later
>   once someone intends to use it. It would also give the opportunity to
>   clean up the commented locking statement.

I don't know if anyone is using AIO with gadgetfs any more.  I've never 
seen any applications for it other than to test the implementation.

Alan Stern