Was this supposed to be an actual patch submission? It doesn't look
like it, because each line of the email begins with "> ".
On Wed, Nov 09, 2022 at 10:11:51PM +0800, jiantao zhang wrote:
> > In the process of switching USB config from rndis to other config,
> > if function gadget->ops->pullup() return an error,it will inevitably
> > cause a system panic(use after free).
> >
> > Analysis as follows:
> > =====================================================================
> > (1) write /config/usb_gadget/g1/UDC "none" (init.usb.configfs.rc:2)
> >
> > gether_disconnect+0x2c/0x1f8 (dev->port_usb = NULL)
> > rndis_disable+0x4c/0x74
> > composite_disconnect+0x74/0xb0
> > configfs_composite_disconnect+0x60/0x7c
> > usb_gadget_disconnect+0x70/0x124
> > usb_gadget_unregister_driver+0xc8/0x1d8
> > gadget_dev_desc_UDC_store+0xec/0x1e4
> >
> > In general, pointer dev->port will be set to null.
> > In function usb_gadget_disconnect(),gadget->udc->driver->disconnect()
> > will not be called when gadget->ops->pullup() return an error, therefore,
> > pointer dev->port will not be set to NULL.
> > If pointer dev->port_usb is not null, it will cause an exception of using
> > the freed memory in step3.
Good point.
...
> > Through above analysis, i think gadget->udc->driver->disconnect() need
> > to be called even if gadget->udc->driver->disconnect() return an error.
You mean "even if gadget->ops->pullup() returns an error". That seems
reasonable. The only reason for the ->pullup callback to fail is if the
hardware doesn't support it, but gadget drivers sometimes need to be
unloaded regardless of the hardware's capabilities.
> > This reverts commit 0a55187a1ec8c03d0619e7ce41d10fdc39cff036
But this is not reasonable. You should change the code so that it does
what you want: Make it call driver->disconnect() even if ops->pullup()
returns an error. Don't revert an entire commit just because of one
side effect.
> > I think this change is a code optimization, not a solution to specific
> > problem. And i think this problem is caused by this change,revert it can
> > solve this problem.
That commit was not just a code optimization. It did fix a problem:
Some UDCs would not call driver->disconnect() because they expected the
core to make the call for them.
> > Of course, my understanding may be inaccurate.There may be a better
> > solution to this problem. If possible, please give some suggestions,
> > thanks.
Simply move the lines which grab the mutex and do the callback after
the "if" statement, so that they will run regardless of what
ops->pullup() returns.
Alan Stern
> > Fixes:<0a55187a1ec8c> (USB: gadget core: Issue ->disconnect()
> > callback from usb_gadget_disconnect())
> >
> > Signed-off-by: Jiantao Zhang <water.zhangjiantao@xxxxxxxxxx>
> > Signed-off-by: TaoXue <xuetao09@xxxxxxxxxx>
> > ---
> > drivers/usb/gadget/udc/core.c | 20 +++++++++++---------
> > 1 file changed, 11 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
> > index c63c0c2cf649..b502b2ac4824 100644
> > --- a/drivers/usb/gadget/udc/core.c
> > +++ b/drivers/usb/gadget/udc/core.c
> > @@ -707,9 +707,6 @@ EXPORT_SYMBOL_GPL(usb_gadget_connect);
> > * 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)
> > @@ -734,13 +731,8 @@ int usb_gadget_disconnect(struct usb_gadget *gadget)
> > }
> > ret = gadget->ops->pullup(gadget, 0);
> > - if (!ret) {
> > + if (!ret)
> > gadget->connected = 0;
> > - mutex_lock(&udc_lock);
> > - if (gadget->udc->driver)
> > - gadget->udc->driver->disconnect(gadget);
> > - mutex_unlock(&udc_lock);
> > - }
> > out:
> > trace_usb_gadget_disconnect(gadget, ret);
> > @@ -1532,6 +1524,11 @@ static void gadget_unbind_driver(struct device *dev)
> > kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE);
> > usb_gadget_disconnect(gadget);
> > +
> > + mutex_lock(&udc_lock);
> > + udc->driver->disconnect(udc->gadget);
> > + mutex_unlock(&udc_lock);
> > +
> > usb_gadget_disable_async_callbacks(udc);
> > if (gadget->irq)
> > synchronize_irq(gadget->irq);
> > @@ -1626,6 +1623,11 @@ static ssize_t soft_connect_store(struct device *dev,
> > usb_gadget_connect(udc->gadget);
> > } else if (sysfs_streq(buf, "disconnect")) {
> > usb_gadget_disconnect(udc->gadget);
> > +
> > + mutex_lock(&udc_lock);
> > + udc->driver->disconnect(udc->gadget);
> > + mutex_unlock(&udc_lock);
> > +
> > usb_gadget_udc_stop(udc);
> > } else {
> > dev_err(dev, "unsupported command '%s'\n", buf);
