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

[Oliver Neukum <oneukum@suse.com>]

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
if you want to do this cleanly, you need to revisit the locking
to use locks you can sleep under.

	HTH
		Oliver