[PATCH v2] usb: udc: core: Use lock when write to soft_connect

[PATCH v2] usb: udc: core: Use lock when write to soft_connect

[Thinh Nguyen]

Use lock to guard against concurrent access for soft-connect/disconnect
operations when writing to soft_connect sysfs.

Cc: stable@vger.kernel.org
Fixes: 2ccea03a8f7e ("usb: gadget: introduce UDC Class")
Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
---
 Changes in v2:
 - Consolidate mutex_unlock to a single place using "goto out"

 drivers/usb/gadget/udc/core.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
index 6a62bbd01324..3363f5c282f1 100644
--- a/drivers/usb/gadget/udc/core.c
+++ b/drivers/usb/gadget/udc/core.c
@@ -1529,10 +1529,13 @@ static ssize_t soft_connect_store(struct device *dev,
 		struct device_attribute *attr, const char *buf, size_t n)
 {
 	struct usb_udc		*udc = container_of(dev, struct usb_udc, dev);
+	ssize_t			ret = n;
 
+	mutex_lock(&udc_lock);
 	if (!udc->driver) {
 		dev_err(dev, "soft-connect without a gadget driver\n");
-		return -EOPNOTSUPP;
+		ret = -EOPNOTSUPP;
+		goto out;
 	}
 
 	if (sysfs_streq(buf, "connect")) {
@@ -1543,10 +1546,12 @@ static ssize_t soft_connect_store(struct device *dev,
 		usb_gadget_udc_stop(udc);
 	} else {
 		dev_err(dev, "unsupported command '%s'\n", buf);
-		return -EINVAL;
+		ret = -EINVAL;
 	}
 
-	return n;
+out:
+	mutex_unlock(&udc_lock);
+	return ret;
 }
 static DEVICE_ATTR_WO(soft_connect);
 

base-commit: 4e0dcf62ab4cf917d0cbe751b8bf229a065248d4
-- 
2.28.0

Re: [PATCH v2] usb: udc: core: Use lock when write to soft_connect

[Ahmed S. Darwish]

On Wed, Jan 13, 2021 at 07:38:28PM -0800, Thinh Nguyen wrote:
...
> @@ -1543,10 +1546,12 @@ static ssize_t soft_connect_store(struct device *dev,
>  		usb_gadget_udc_stop(udc);
>  	} else {
>  		dev_err(dev, "unsupported command '%s'\n", buf);
> -		return -EINVAL;
> +		ret = -EINVAL;
>  	}
>
> -	return n;
> +out:
> +	mutex_unlock(&udc_lock);
> +	return ret;
>  }

This is *very* tricky: the return value will be easily broken if someone
adds any code later after the else body and before the "out" label.

Re: [PATCH v2] usb: udc: core: Use lock when write to soft_connect

[Thinh Nguyen]

Andy Shevchenko wrote:
>
>
> On Thursday, January 14, 2021, Ahmed S. Darwish
> <a.darwish@linutronix.de <mailto:a.darwish@linutronix.de>> wrote:
>
>     On Wed, Jan 13, 2021 at 07:38:28PM -0800, Thinh Nguyen wrote:
>     ...
>     > @@ -1543,10 +1546,12 @@ static ssize_t soft_connect_store(struct
>     device *dev,
>     >               usb_gadget_udc_stop(udc);
>     >       } else {
>     >               dev_err(dev, "unsupported command '%s'\n", buf);
>     > -             return -EINVAL;
>     > +             ret = -EINVAL;
>     >       }
>     >
>     > -     return n;
>
>
>
> Should be ret = n; here.
>  
>
>     > +out:
>     > +     mutex_unlock(&udc_lock);
>     > +     return ret;
>     >  }
>
>     This is *very* tricky: the return value will be easily broken if
>     someone
>     adds any code later after the else body and before the "out" label.
>
>
>
> +1 and it’s not good pattern to assign return value at the definition
> time (see comment above ). 
>
>
> -- 
> With Best Regards,
> Andy Shevchenko
>
>

Good point. I didn't think about that. Thanks guys.

Thinh

Re: [PATCH v2] usb: udc: core: Use lock when write to soft_connect

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 388 bytes --]

Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:

> Use lock to guard against concurrent access for soft-connect/disconnect
> operations when writing to soft_connect sysfs.
>
> Cc: stable@vger.kernel.org
> Fixes: 2ccea03a8f7e ("usb: gadget: introduce UDC Class")
> Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>

Acked-by: Felipe Balbi <balbi@kernel.org>

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 857 bytes --]

Re: [PATCH v2] usb: udc: core: Use lock when write to soft_connect

[Greg Kroah-Hartman]

On Thu, Jan 14, 2021 at 09:03:53AM +0200, Andy Shevchenko wrote:
> On Thursday, January 14, 2021, Ahmed S. Darwish <a.darwish@linutronix.de>
> wrote:
> 
> > On Wed, Jan 13, 2021 at 07:38:28PM -0800, Thinh Nguyen wrote:
> > ...
> > > @@ -1543,10 +1546,12 @@ static ssize_t soft_connect_store(struct device
> > *dev,
> > >               usb_gadget_udc_stop(udc);
> > >       } else {
> > >               dev_err(dev, "unsupported command '%s'\n", buf);
> > > -             return -EINVAL;
> > > +             ret = -EINVAL;
> > >       }
> > >
> > > -     return n;
> 
> 
> 
> Should be ret = n; here.

Why?  That happened higher up in the patch.

thanks,

greg k-h