USB: Gadget: core: Help prevent panic during UVC unconfigure

Avichal Rakesh reported a kernel panic that occurred when the UVC
gadget driver was removed from a gadget's configuration.  The panic
involves a somewhat complicated interaction between the kernel driver
and a userspace component (as described in the Link tag below), but
the analysis did make one thing clear: The Gadget core should
accomodate gadget drivers calling usb_gadget_deactivate() as part of
their unbind procedure.

Currently this doesn't work.  gadget_unbind_driver() calls
driver->unbind() while holding the udc->connect_lock mutex, and
usb_gadget_deactivate() attempts to acquire that mutex, which will
result in a deadlock.

The simple fix is for gadget_unbind_driver() to release the mutex when
invoking the ->unbind() callback.  There is no particular reason for
it to be holding the mutex at that time, and the mutex isn't held
while the ->bind() callback is invoked.  So we'll drop the mutex
before performing the unbind callback and reacquire it afterward.

We'll also add a couple of comments to usb_gadget_activate() and
usb_gadget_deactivate().  Because they run in process context they
must not be called from a gadget driver's ->disconnect() callback,
which (according to the kerneldoc for struct usb_gadget_driver in
include/linux/usb/gadget.h) may run in interrupt context.  This may
help prevent similar bugs from arising in the future.

Reported-and-tested-by: Avichal Rakesh <arakesh@google.com>
Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
Fixes: 286d9975a838 ("usb: gadget: udc: core: Prevent soft_connect_store() race")
Link: https://lore.kernel.org/linux-usb/4d7aa3f4-22d9-9f5a-3d70-1bd7148ff4ba@google.com/
Cc: Badhri Jagan Sridharan <badhri@google.com>
Cc: <stable@vger.kernel.org>

---

 drivers/usb/gadget/udc/core.c |    9 +++++++++
 1 file changed, 9 insertions(+)

On Tue, Aug 01, 2023 at 06:36:53AM -0700, Badhri Jagan Sridharan wrote:
> Hi Alan,

> > @@ -1639,7 +1644,11 @@ static void gadget_unbind_driver(struct
> >         usb_gadget_disable_async_callbacks(udc);
> >         if (gadget->irq)
> >                 synchronize_irq(gadget->irq);
> > +       mutex_unlock(&udc->connect_lock);
> > +
> 
> In my humble opinion, this should be OK.
> I was wondering what would happen if soft_connect_store() is invoked
> right after the udc->connect_lock is dropped. One of your previous
> patches(1016fc0c096c USB: gadget: Fix obscure lockdep violation for
> udc_mutex) already prevents this race by making soft_connect_store()
> acquire device_lock(&udc->gadget->dev); and hence avoids the race.

I wouldn't go so far as to say that all the problems have been fixed.  
There's nothing to prevent the user from writing to the soft_connect 
attribute immediately after gadget_unbind_driver() finishes.

Doing so would cause usb_gadget_udc_start_locked() and 
usb_gadget_connect_locked() to run.  The first would tell the UDC 
driver to turn the controller on, and the second would do essentially 
nothing (because the allow_connect flag is clear).

But this would still leave the controller on when it should be off.  
Maybe we can chalk this outcome up to user error.

Alan Stern