RE: [PATCH v1] usb: f_mass_storage: reduce chance to queue disable ep

[yuanlinyu <yuanlinyu@hihonor.com>]

> 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