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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250819145700.sIWRW7Oe@linutronix.de>
Date: Tue, 19 Aug 2025 16:57:00 +0200
From: Sebastian Andrzej Siewior <bigeasy@...utronix.de>
To: Alan Stern <stern@...land.harvard.edu>
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 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