linux-kernel - Re: [BUG] usbip: vhci: Sleeping function called from invalid context in vhci_urb_enqueue on PREEMPT

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <1088432b-b433-4bab-a927-69e55d9eb433@rowland.harvard.edu>
Date: Sat, 16 Aug 2025 10:16:34 -0400
From: Alan Stern <stern@...land.harvard.edu>
To: Yunseong Kim <ysk@...lloc.com>
Cc: linux-usb@...r.kernel.org, gregkh@...uxfoundation.org,
	Andrey Konovalov <andreyknvl@...gle.com>,
	Shuah Khan <skhan@...uxfoundation.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
	Clark Williams <clrkwllms@...nel.org>,
	Steven Rostedt <rostedt@...dmis.org>,
	linux-rt-users@...r.kernel.org, linux-kernel@...r.kernel.org,
	syzkaller@...glegroups.com
Subject: Re: [BUG] usbip: vhci: Sleeping function called from invalid context
 in vhci_urb_enqueue on PREEMPT_RT

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