Date:   Wed, 7 Jun 2023 22:30:12 -0700
From:   Badhri Jagan Sridharan <badhri@...gle.com>
To:     Alan Stern <stern@...land.harvard.edu>
Cc:     gregkh@...uxfoundation.org, colin.i.king@...il.com,
        xuetao09@...wei.com, quic_eserrao@...cinc.com,
        water.zhangjiantao@...wei.com, peter.chen@...escale.com,
        balbi@...com, francesco@...cini.it, alistair@...stair23.me,
        stephan@...hold.net, bagasdotme@...il.com, luca@...tu.xyz,
        linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
        stable@...r.kernel.org
Subject: Re: [PATCH v6 2/2] usb: gadget: udc: core: Prevent
 soft_connect_store() race

On Wed, Jun 7, 2023 at 11:26 AM Alan Stern <stern@...land.harvard.edu> wrote:
>
> This patch looks basically okay.  Most of the comments below are
> concerned only with style or presentation.
>
> On Thu, Jun 01, 2023 at 03:10:28AM +0000, Badhri Jagan Sridharan wrote:
> > usb_udc_connect_control(), soft_connect_store() and
> > usb_gadget_deactivate() can potentialy race against each other to invoke
>
> "potentially" has two 'l's.
>
> > usb_gadget_connect()/usb_gadget_disconnect(). To prevent this, guarding
>
> s/guarding/guard/
>
> > udc->vbus, udc->started, gadget->allow_connect, gadget->deactivate with
>
> Insert "and" before "gadget->deactivate".
>
> Also, I don't think the patch guards udc->vbus.  After all, that flag
> can be changed in usb_udc_vbus_handler() without the protection of the
> mutex.
>
> > connect_lock so that ->pullup() is only invoked when gadget driver is
> > bound, not deactivated and started.
>
> "when the gadget", not "when gadget driver".  The driver isn't what gets
> deactivated or started; the gadget is.
>
> This would be easier to understand if the last two items were switched:
>
>         bound, started, and not deactivated.
>
> Also, it would help readers if there were some additional text to help
> separate the end of this sentence from the start of the next one.  For
> example, you might insert "The routines" at the beginning of the next
> sentence.
>
> >  usb_gadget_connect_locked(),
> > usb_gadget_disconnect_locked(), usb_udc_connect_control_locked(),
> > usb_gadget_udc_start_locked(), usb_gadget_udc_stop_locked() are called
> > with this lock held.
> >
> > This commit was reverted due to the crash reported in
>
> An earlier version of this commit...
>
> > https://lore.kernel.org/all/ZF4BvgsOyoKxdPFF@francesco-nb.int.toradex.com/.
> > commit 16737e78d190 ("usb: gadget: udc: core: Offload usb_udc_vbus_handler processing")
> > addresses the crash reported.
> >
> > Cc: stable@...r.kernel.org
> > Fixes: 628ef0d273a6 ("usb: udc: add usb_udc_vbus_handler")
> > Signed-off-by: Badhri Jagan Sridharan <badhri@...gle.com>
> > ---
> > v5 is the first version in this series.
> > Changes since v5:
> > ** Fixed up commit message
> > ** Wrapped comments at 76.
> > ---
> >  drivers/usb/gadget/udc/core.c | 151 ++++++++++++++++++++++++----------
> >  1 file changed, 109 insertions(+), 42 deletions(-)
> >
> > diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
> > index d2e4f78c53e3..b53f74fcad60 100644
> > --- a/drivers/usb/gadget/udc/core.c
> > +++ b/drivers/usb/gadget/udc/core.c
> > @@ -40,6 +40,11 @@ static const struct bus_type gadget_bus_type;
> >   * @allow_connect: Indicates whether UDC is allowed to be pulled up.
> >   * Set/cleared by gadget_(un)bind_driver() after gadget driver is bound or
> >   * unbound.
> > + * @connect_lock: protects udc->vbus, udc->started, gadget->connect,
> > + * gadget->deactivate. usb_gadget_connect_locked,
>
> Again, separate the two sentences with some extra text.  Otherwise the
> period looks too much like a comma for people to see it easily.
>
> > + * usb_gadget_disconnect_locked, usb_udc_connect_control_locked,
> > + * usb_gadget_udc_start_locked, usb_gadget_udc_stop_locked are called with
> > + * this lock held.
> >   *
> >   * This represents the internal data structure which is used by the UDC-class
> >   * to hold information about udc driver and gadget together.
> > @@ -53,6 +58,7 @@ struct usb_udc {
> >       bool                            started;
> >       bool                            allow_connect;
> >       struct work_struct              vbus_work;
> > +     struct mutex                    connect_lock;
> >  };
> >
> >  static struct class *udc_class;
> > @@ -692,17 +698,12 @@ int usb_gadget_vbus_disconnect(struct usb_gadget *gadget)
> >  }
> >  EXPORT_SYMBOL_GPL(usb_gadget_vbus_disconnect);
> >
> > -/**
> > - * usb_gadget_connect - software-controlled connect to USB host
> > - * @gadget:the peripheral being connected
> > - *
> > - * Enables the D+ (or potentially D-) pullup.  The host will start
> > - * enumerating this gadget when the pullup is active and a VBUS session
> > - * is active (the link is powered).
> > - *
> > - * Returns zero on success, else negative errno.
> > +/*
> > + * Internal version of usb_gadget_connect needs to be called with
> > + * connect_lock held.
>
> I'm not sure you need to say this; it's pretty obvious from the
> __must_hold() annotation a few lines later.  Consider removing this
> paragraph and the similar paragraphs in the other new internal routines.
>
> >   */
> > -int usb_gadget_connect(struct usb_gadget *gadget)
> > +static int usb_gadget_connect_locked(struct usb_gadget *gadget)
> > +     __must_hold(&gadget->udc->connect_lock)
> >  {
> >       int ret = 0;
> >
> > @@ -711,10 +712,12 @@ int usb_gadget_connect(struct usb_gadget *gadget)
> >               goto out;
> >       }
> >
> > -     if (gadget->deactivated || !gadget->udc->allow_connect) {
> > +     if (gadget->deactivated || !gadget->udc->allow_connect || !gadget->udc->started) {
> >               /*
> >                * If gadget is deactivated we only save new state.
> >                * Gadget will be connected automatically after activation.
>
> This comment is now inaccurate.  Change it to say:
>
>                  * If the gadget isn't usable (because it is deactivated,
>                  * unbound, or not yet started), we only save the new state.
>                  * The gadget will be connected automatically when it is
>                  * activated/bound/started.
>
> > +              *
> > +              * udc first needs to be started before gadget can be pulled up.
>
> And then this sentence won't be needed.
>
> >                */
> >               gadget->connected = true;
> >               goto out;
> > @@ -729,22 +732,35 @@ int usb_gadget_connect(struct usb_gadget *gadget)
> >
> >       return ret;
> >  }
> > -EXPORT_SYMBOL_GPL(usb_gadget_connect);
> >
> >  /**
> > - * usb_gadget_disconnect - software-controlled disconnect from USB host
> > - * @gadget:the peripheral being disconnected
> > - *
> > - * Disables the D+ (or potentially D-) pullup, which the host may see
> > - * as a disconnect (when a VBUS session is active).  Not all systems
> > - * support software pullup controls.
> > + * usb_gadget_connect - software-controlled connect to USB host
> > + * @gadget:the peripheral being connected
> >   *
> > - * Following a successful disconnect, invoke the ->disconnect() callback
> > - * for the current gadget driver so that UDC drivers don't need to.
> > + * Enables the D+ (or potentially D-) pullup.  The host will start
> > + * enumerating this gadget when the pullup is active and a VBUS session
> > + * is active (the link is powered).
> >   *
> >   * Returns zero on success, else negative errno.
> >   */
> > -int usb_gadget_disconnect(struct usb_gadget *gadget)
> > +int usb_gadget_connect(struct usb_gadget *gadget)
> > +{
> > +     int ret;
> > +
> > +     mutex_lock(&gadget->udc->connect_lock);
> > +     ret = usb_gadget_connect_locked(gadget);
> > +     mutex_unlock(&gadget->udc->connect_lock);
> > +
> > +     return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(usb_gadget_connect);
> > +
> > +/*
> > + * Internal version of usb_gadget_disconnect needs to be called with
> > + * connect_lock held.
> > + */
> > +static int usb_gadget_disconnect_locked(struct usb_gadget *gadget)
> > +     __must_hold(&gadget->udc->connect_lock)
> >  {
> >       int ret = 0;
> >
> > @@ -756,10 +772,12 @@ int usb_gadget_disconnect(struct usb_gadget *gadget)
> >       if (!gadget->connected)
> >               goto out;
> >
> > -     if (gadget->deactivated) {
> > +     if (gadget->deactivated || !gadget->udc->started) {
>
> Do you really need to add this extra test?  After all, if the gadget
> isn't started then how could the previous test of gadget->connected
> possibly succeed?
>
> In fact, I suspect this entire section of code was always useless, since
> the gadget couldn't be connected now if it was already deactivated.
>
Thanks Alan ! Will fix all other comments in v7 but not sure about this one.
Although the ->pullup() function will not be called,
-> connected flag could actually be set when the gadget is not started.

- if (gadget->deactivated || !gadget->udc->allow_connect) {
+ if (gadget->deactivated || !gadget->udc->allow_connect ||
!gadget->udc->started) {
  /*
  * If gadget is deactivated we only save new state.
  * Gadget will be connected automatically after activation.
+ *
+ * udc first needs to be started before gadget can be pulled up.
  */
  gadget->connected = true;

This could happen, for instance, when  usb_udc_vbus_handler()  is
invoked after soft_connect_store() disconnects the gadget.
Same applies to when usb_gadget_connect() is called after the gadget
has been deactivated through usb_gadget_deactivate().

This implies that the checks should be there, right ?

> >               /*
> >                * If gadget is deactivated we only save new state.
> >                * Gadget will stay disconnected after activation.
> > +              *
> > +              * udc should have been started before gadget being pulled down.
>
> No need to mention this.
>
> >                */
> >               gadget->connected = false;
> >               goto out;
> > @@ -779,6 +797,30 @@ int usb_gadget_disconnect(struct usb_gadget *gadget)
> >
> >       return ret;
> >  }
> > +
> > +/**
> > + * usb_gadget_disconnect - software-controlled disconnect from USB host
> > + * @gadget:the peripheral being disconnected
> > + *
> > + * Disables the D+ (or potentially D-) pullup, which the host may see
> > + * as a disconnect (when a VBUS session is active).  Not all systems
> > + * support software pullup controls.
> > + *
> > + * Following a successful disconnect, invoke the ->disconnect() callback
> > + * for the current gadget driver so that UDC drivers don't need to.
> > + *
> > + * Returns zero on success, else negative errno.
> > + */
> > +int usb_gadget_disconnect(struct usb_gadget *gadget)
> > +{
> > +     int ret;
> > +
> > +     mutex_lock(&gadget->udc->connect_lock);
> > +     ret = usb_gadget_disconnect_locked(gadget);
> > +     mutex_unlock(&gadget->udc->connect_lock);
> > +
> > +     return ret;
> > +}
> >  EXPORT_SYMBOL_GPL(usb_gadget_disconnect);
> >
> >  /**
> > @@ -799,10 +841,11 @@ int usb_gadget_deactivate(struct usb_gadget *gadget)
> >       if (gadget->deactivated)
> >               goto out;
> >
> > +     mutex_lock(&gadget->udc->connect_lock);
>
> The mutex should be acquired _before_ the test of gadget->deactivated.
> Otherwise the the state could change between the time of the test and
> the time of the mutex_lock().
>
> >       if (gadget->connected) {
> > -             ret = usb_gadget_disconnect(gadget);
> > +             ret = usb_gadget_disconnect_locked(gadget);
> >               if (ret)
> > -                     goto out;
> > +                     goto unlock;
> >
> >               /*
> >                * If gadget was being connected before deactivation, we want
> > @@ -812,6 +855,8 @@ int usb_gadget_deactivate(struct usb_gadget *gadget)
> >       }
> >       gadget->deactivated = true;
> >
> > +unlock:
> > +     mutex_unlock(&gadget->udc->connect_lock);
> >  out:
> >       trace_usb_gadget_deactivate(gadget, ret);
> >
> > @@ -835,6 +880,7 @@ int usb_gadget_activate(struct usb_gadget *gadget)
> >       if (!gadget->deactivated)
> >               goto out;
> >
> > +     mutex_lock(&gadget->udc->connect_lock);
> >       gadget->deactivated = false;
>
> Again, lock the mutex before testing the flag.
>
> >
> >       /*
> > @@ -842,7 +888,8 @@ int usb_gadget_activate(struct usb_gadget *gadget)
> >        * while it was being deactivated, we call usb_gadget_connect().
> >        */
> >       if (gadget->connected)
> > -             ret = usb_gadget_connect(gadget);
> > +             ret = usb_gadget_connect_locked(gadget);
> > +     mutex_unlock(&gadget->udc->connect_lock);
> >
> >  out:
> >       trace_usb_gadget_activate(gadget, ret);
> > @@ -1083,19 +1130,22 @@ EXPORT_SYMBOL_GPL(usb_gadget_set_state);
> >
> >  /* ------------------------------------------------------------------------- */
> >
> > -static void usb_udc_connect_control(struct usb_udc *udc)
> > +/* Acquire connect_lock before calling this function. */
> > +static void usb_udc_connect_control_locked(struct usb_udc *udc) __must_hold(&udc->connect_lock)
> >  {
> >       if (udc->vbus)
> > -             usb_gadget_connect(udc->gadget);
> > +             usb_gadget_connect_locked(udc->gadget);
> >       else
> > -             usb_gadget_disconnect(udc->gadget);
> > +             usb_gadget_disconnect_locked(udc->gadget);
> >  }
> >
> >  static void vbus_event_work(struct work_struct *work)
> >  {
> >       struct usb_udc *udc = container_of(work, struct usb_udc, vbus_work);
> >
> > -     usb_udc_connect_control(udc);
> > +     mutex_lock(&udc->connect_lock);
> > +     usb_udc_connect_control_locked(udc);
> > +     mutex_unlock(&udc->connect_lock);
> >  }
> >
> >  /**
> > @@ -1144,7 +1194,7 @@ void usb_gadget_udc_reset(struct usb_gadget *gadget,
> >  EXPORT_SYMBOL_GPL(usb_gadget_udc_reset);
> >
> >  /**
> > - * usb_gadget_udc_start - tells usb device controller to start up
> > + * usb_gadget_udc_start_locked - tells usb device controller to start up
> >   * @udc: The UDC to be started
> >   *
> >   * This call is issued by the UDC Class driver when it's about
> > @@ -1155,8 +1205,11 @@ EXPORT_SYMBOL_GPL(usb_gadget_udc_reset);
> >   * necessary to have it powered on.
> >   *
> >   * Returns zero on success, else negative errno.
> > + *
> > + * Caller should acquire connect_lock before invoking this function.
> >   */
> > -static inline int usb_gadget_udc_start(struct usb_udc *udc)
> > +static inline int usb_gadget_udc_start_locked(struct usb_udc *udc)
> > +     __must_hold(&udc->connect_lock)
> >  {
> >       int ret;
> >
> > @@ -1173,7 +1226,7 @@ static inline int usb_gadget_udc_start(struct usb_udc *udc)
> >  }
> >
> >  /**
> > - * usb_gadget_udc_stop - tells usb device controller we don't need it anymore
> > + * usb_gadget_udc_stop_locked - tells usb device controller we don't need it anymore
> >   * @udc: The UDC to be stopped
> >   *
> >   * This call is issued by the UDC Class driver after calling
> > @@ -1182,8 +1235,11 @@ static inline int usb_gadget_udc_start(struct usb_udc *udc)
> >   * The details are implementation specific, but it can go as
> >   * far as powering off UDC completely and disable its data
> >   * line pullups.
> > + *
> > + * Caller should acquire connect lock before invoking this function.
> >   */
> > -static inline void usb_gadget_udc_stop(struct usb_udc *udc)
> > +static inline void usb_gadget_udc_stop_locked(struct usb_udc *udc)
> > +     __must_hold(&udc->connect_lock)
> >  {
> >       if (!udc->started) {
> >               dev_err(&udc->dev, "UDC had already stopped\n");
> > @@ -1342,6 +1398,7 @@ int usb_add_gadget(struct usb_gadget *gadget)
> >
> >       udc->gadget = gadget;
> >       gadget->udc = udc;
> > +     mutex_init(&udc->connect_lock);
> >
> >       udc->started = false;
> >
> > @@ -1545,12 +1602,16 @@ static int gadget_bind_driver(struct device *dev)
> >       if (ret)
> >               goto err_bind;
> >
> > -     ret = usb_gadget_udc_start(udc);
> > -     if (ret)
> > +     mutex_lock(&udc->connect_lock);
> > +     ret = usb_gadget_udc_start_locked(udc);
> > +     if (ret) {
> > +             mutex_unlock(&udc->connect_lock);
> >               goto err_start;
> > +     }
> >       usb_gadget_enable_async_callbacks(udc);
> >       udc->allow_connect = true;
> > -     usb_udc_connect_control(udc);
> > +     usb_udc_connect_control_locked(udc);
> > +     mutex_unlock(&udc->connect_lock);
> >
> >       kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE);
> >       return 0;
> > @@ -1583,12 +1644,14 @@ static void gadget_unbind_driver(struct device *dev)
> >
> >       udc->allow_connect = false;
> >       cancel_work_sync(&udc->vbus_work);
> > -     usb_gadget_disconnect(gadget);
> > +     mutex_lock(&udc->connect_lock);
> > +     usb_gadget_disconnect_locked(gadget);
> >       usb_gadget_disable_async_callbacks(udc);
> >       if (gadget->irq)
> >               synchronize_irq(gadget->irq);
> >       udc->driver->unbind(gadget);
> > -     usb_gadget_udc_stop(udc);
> > +     usb_gadget_udc_stop_locked(udc);
> > +     mutex_unlock(&udc->connect_lock);
> >
> >       mutex_lock(&udc_lock);
> >       driver->is_bound = false;
>
> In both these routines, you are careful not to hold the
> udc->connect_lock and the udc_lock at the same time.  Good.
>
> > @@ -1674,11 +1737,15 @@ static ssize_t soft_connect_store(struct device *dev,
> >       }
> >
> >       if (sysfs_streq(buf, "connect")) {
> > -             usb_gadget_udc_start(udc);
> > -             usb_gadget_connect(udc->gadget);
> > +             mutex_lock(&udc->connect_lock);
> > +             usb_gadget_udc_start_locked(udc);
> > +             usb_gadget_connect_locked(udc->gadget);
>
> Interesting change of behavior here.  Before this patch the connect
> operation would succeed, even if no gadget driver was bound.  Now it
> won't, which is how it ought to behave.
>
> > +             mutex_unlock(&udc->connect_lock);
> >       } else if (sysfs_streq(buf, "disconnect")) {
> > -             usb_gadget_disconnect(udc->gadget);
> > -             usb_gadget_udc_stop(udc);
> > +             mutex_lock(&udc->connect_lock);
> > +             usb_gadget_disconnect_locked(udc->gadget);
> > +             usb_gadget_udc_stop_locked(udc);
> > +             mutex_unlock(&udc->connect_lock);
> >       } else {
> >               dev_err(dev, "unsupported command '%s'\n", buf);
> >               ret = -EINVAL;
>
> Alan Stern

Powered by blists - more mailing lists