> From: Oliver Neukum <oneukum@xxxxxxxx>
> Sent: Thursday, March 14, 2024 3:54 PM
> To: yuanlinyu <yuanlinyu@xxxxxxxxxxx>; Alan Stern
> <stern@xxxxxxxxxxxxxxxxxxx>; Greg Kroah-Hartman
> <gregkh@xxxxxxxxxxxxxxxxxxx>
> Cc: linux-usb@xxxxxxxxxxxxxxx; stable@xxxxxxxxxxxxxxx
> 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@xxxxxxxxxxxxxxx> # 6.1
> > Signed-off-by: yuan linyu <yuanlinyu@xxxxxxxxxxx>
> Nacked-by: Oliver Neukum <oneukum@xxxxxxxx>
>
> > ---
> > 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
