Hi Alan,
On Sat, Jul 29, 2023 at 7:59 AM Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
>
> 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@xxxxxxxxxx>
> Signed-off-by: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>
> Fixes: 286d9975a838 ("usb: gadget: udc: core: Prevent soft_connect_store() race")
> Link: https://lore.kernel.org/linux-usb/4d7aa3f4-22d9-9f5a-3d70-1bd7148ff4ba@xxxxxxxxxx/
> Cc: Badhri Jagan Sridharan <badhri@xxxxxxxxxx>
> Cc: <stable@xxxxxxxxxxxxxxx>
>
> ---
>
> drivers/usb/gadget/udc/core.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> Index: usb-devel/drivers/usb/gadget/udc/core.c
> ===================================================================
> --- usb-devel.orig/drivers/usb/gadget/udc/core.c
> +++ usb-devel/drivers/usb/gadget/udc/core.c
> @@ -822,6 +822,9 @@ EXPORT_SYMBOL_GPL(usb_gadget_disconnect)
> * usb_gadget_activate() is called. For example, user mode components may
> * need to be activated before the system can talk to hosts.
> *
> + * This routine may sleep; it must not be called in interrupt context
> + * (such as from within a gadget driver's disconnect() callback).
> + *
> * Returns zero on success, else negative errno.
> */
> int usb_gadget_deactivate(struct usb_gadget *gadget)
> @@ -860,6 +863,8 @@ EXPORT_SYMBOL_GPL(usb_gadget_deactivate)
> * This routine activates gadget which was previously deactivated with
> * usb_gadget_deactivate() call. It calls usb_gadget_connect() if needed.
> *
> + * This routine may sleep; it must not be called in interrupt context.
> + *
> * Returns zero on success, else negative errno.
> */
> int usb_gadget_activate(struct usb_gadget *gadget)
> @@ -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.
Thanks,
Badhri
> udc->driver->unbind(gadget);
> +
> + mutex_lock(&udc->connect_lock);
> usb_gadget_udc_stop_locked(udc);
> mutex_unlock(&udc->connect_lock);
>
