From: Oliver Neukum oneukum@suse.com Sent: Thursday, March 14, 2024 3:54 PM To: yuanlinyu yuanlinyu@hihonor.com; Alan Stern stern@rowland.harvard.edu; Greg Kroah-Hartman gregkh@linuxfoundation.org Cc: linux-usb@vger.kernel.org; stable@vger.kernel.org Subject: Re: [PATCH v1] usb: f_mass_storage: reduce chance to queue disable ep
Hi,
I am sorry, but this contains a major issue.
On 14.03.24 07:59, yuan linyu wrote:
It is possible trigger below warning message from mass storage function,
------------[ cut here ]------------ WARNING: CPU: 6 PID: 3839 at drivers/usb/gadget/udc/core.c:294
usb_ep_queue+0x7c/0x104
CPU: 6 PID: 3839 Comm: file-storage Tainted: G S WC O
6.1.25-android14-11-g354e2a7e7cd9 #1
pstate: 22400005 (nzCv daif +PAN -UAO +TCO -DIT -SSBS BTYPE=--) pc : usb_ep_queue+0x7c/0x104 lr : fsg_main_thread+0x494/0x1b3c
Root cause is mass storage function try to queue request from main thread, but other thread may already disable ep when function disable.
As mass storage function have record of ep enable/disable state, let's add the state check before queue request to UDC, it maybe avoid warning.
Also use common lock to protect ep state which avoid race between main thread and function disable.
Cc: stable@vger.kernel.org # 6.1 Signed-off-by: yuan linyu yuanlinyu@hihonor.com
Nacked-by: Oliver Neukum oneukum@suse.com
drivers/usb/gadget/function/f_mass_storage.c | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/gadget/function/f_mass_storage.c
b/drivers/usb/gadget/function/f_mass_storage.c
index c265a1f62fc1..056083cb68cb 100644 --- a/drivers/usb/gadget/function/f_mass_storage.c +++ b/drivers/usb/gadget/function/f_mass_storage.c @@ -520,12 +520,25 @@ static int fsg_setup(struct usb_function *f, static int start_transfer(struct fsg_dev *fsg, struct usb_ep *ep, struct usb_request *req) {
- unsigned long flags; int rc;
- if (ep == fsg->bulk_in)
- spin_lock_irqsave(&fsg->common->lock, flags);
Taking a spinlock.
if (ep == fsg->bulk_in) {
if (!fsg->bulk_in_enabled) {spin_unlock_irqrestore(&fsg->common->lock, flags);return -ESHUTDOWN;}dump_msg(fsg, "bulk-in", req->buf, req->length);
} else {
if (!fsg->bulk_out_enabled) {spin_unlock_irqrestore(&fsg->common->lock, flags);return -ESHUTDOWN;}}
rc = usb_ep_queue(ep, req, GFP_KERNEL);
This can sleep.
- spin_unlock_irqrestore(&fsg->common->lock, flags);
Giving up the lock.
Sorry, now for the longer explanation. You'd introduce a deadlock. You just cannot sleep with a spinlock held. It seems to me that
I didn't review usb_ep_queue() clearly, in my test, I didn't hit sleep. But the concern is good, will find better way to avoid it.
if you want to do this cleanly, you need to revisit the locking to use locks you can sleep under.
HTH Oliver