linux-kernel - Re: [BUG] usb: gadget: dummy_hcd: Sleeping function called from invalid context in dummy_dequeue 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: <20250816065933.EPwBJ0Sd@linutronix.de>
Date: Sat, 16 Aug 2025 08:59:33 +0200
From: Nam Cao <namcao@...utronix.de>
To: Yunseong Kim <ysk@...lloc.com>
Cc: gregkh@...uxfoundation.org, stern@...land.harvard.edu,
	linux-usb@...r.kernel.org, Thomas Gleixner <tglx@...utronix.de>,
	Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
	Clark Williams <clrkwllms@...nel.org>,
	Steven Rostedt <rostedt@...dmis.org>,
	Marcello Sylvester Bauer <sylv@...v.io>,
	Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
	Uwe Kleine-König <u.kleine-koenig@...libre.com>,
	Al Viro <viro@...iv.linux.org.uk>, andreyknvl@...il.com,
	Austin Kim <austindh.kim@...il.com>, linux-rt-users@...r.kernel.org,
	linux-kernel@...r.kernel.org, syzkaller@...glegroups.com
Subject: Re: [BUG] usb: gadget: dummy_hcd: Sleeping function called from
 invalid context in dummy_dequeue on PREEMPT_RT

On Sat, Aug 16, 2025 at 11:38:14AM +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 dummy_dequeue function in the dummy USB driver.
...
> The pattern of manually disabling IRQs and then taking a spinlock
> local_irq_save() + spin_lock() is unsafe on PREEMPT_RT, the current code
> structure keeps IRQs disabled even after spin_unlock(&dum->lock) while
> calling usb_gadget_giveback_request(). This extended atomic context can
> also be problematic if the completion handler attempts to acquire another
> sleepable lock.

I don't know the USB subsystem well, but the comments above struct
usb_request says:

 * @complete: Function called when request completes, so this request and
 *	its buffer may be re-used.  The function will always be called with
 *	interrupts disabled, and it must not sleep.

Therefore it shouldn't be a concern that "completion handler attempts to
acquire another sleepable lock".

> 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.

This was introduced by b4dbda1a22d2 ("USB: dummy-hcd: disable interrupts
during req->complete") which split the spin_lock_irqsave() into
local_irq_save() and spin_lock().

The untested patch below should help?

Enabling interrupt (spin_unlock_irqrestore) and then immediately disabling
interrupt (local_irq_save) is not the nicest thing. But then I don't see
how to avoid that while being non-hacky and human-readable.

Nam

diff --git a/drivers/usb/gadget/udc/dummy_hcd.c b/drivers/usb/gadget/udc/dummy_hcd.c
index 21dbfb0b3bac..a4653c919664 100644
--- a/drivers/usb/gadget/udc/dummy_hcd.c
+++ b/drivers/usb/gadget/udc/dummy_hcd.c
@@ -765,8 +765,7 @@ static int dummy_dequeue(struct usb_ep *_ep, struct usb_request *_req)
 	if (!dum->driver)
 		return -ESHUTDOWN;
 
-	local_irq_save(flags);
-	spin_lock(&dum->lock);
+	spin_lock_irqsave(&dum->lock, flags);
 	list_for_each_entry(iter, &ep->queue, queue) {
 		if (&iter->req != _req)
 			continue;
@@ -776,15 +775,16 @@ static int dummy_dequeue(struct usb_ep *_ep, struct usb_request *_req)
 		retval = 0;
 		break;
 	}
-	spin_unlock(&dum->lock);
+	spin_unlock_irqrestore(&dum->lock, flags);
 
 	if (retval == 0) {
 		dev_dbg(udc_dev(dum),
 				"dequeued req %p from %s, len %d buf %p\n",
 				req, _ep->name, _req->length, _req->buf);
+		local_irq_save(flags);
 		usb_gadget_giveback_request(_ep, _req);
+		local_irq_restore(flags);
 	}
-	local_irq_restore(flags);
 	return retval;
 }