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 for Android: free password hash cracker in your pocket

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

Message-ID: <20250819102124.O6E7YfEJ@linutronix.de>
Date: Tue, 19 Aug 2025 12:21:24 +0200
From: Sebastian Andrzej Siewior <bigeasy@...utronix.de>
To: Nam Cao <namcao@...utronix.de>
Cc: Yunseong Kim <ysk@...lloc.com>, gregkh@...uxfoundation.org,
	stern@...land.harvard.edu, linux-usb@...r.kernel.org,
	Thomas Gleixner <tglx@...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 2025-08-16 08:59:33 [+0200], Nam Cao wrote:
> 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);

The two above are fine.

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

This is not. I don't see the need for it. The queue part does
	spin_lock_irqsave()
	spin_unlock();
	usb_gadget_giveback_request()
	spin_lock();
	spin_unlock_irqrestore();

and keeps the interrupts disabled during callback invocation. This seems
to be just to unify the code vs the else path.

>  	}
> -	local_irq_restore(flags);
>  	return retval;
>  }

Sebastian