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] [thread-next>] [day] [month] [year] [list]
Message-ID: <49ee1b8a-d47a-42df-aa64-d0d62798c45b@rowland.harvard.edu>
Date: Tue, 19 Aug 2025 10:12:31 -0400
From: Alan Stern <stern@...land.harvard.edu>
To: Sebastian Andrzej Siewior <bigeasy@...utronix.de>
Cc: Yunseong Kim <ysk@...lloc.com>, linux-usb@...r.kernel.org,
	gregkh@...uxfoundation.org,
	Andrey Konovalov <andreyknvl@...gle.com>,
	Shuah Khan <skhan@...uxfoundation.org>,
	Thomas Gleixner <tglx@...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 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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ