| Message ID | 48b2f1f1-0639-46bf-bbfc-98cb05a24914@rowland.harvard.edu (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 65dadb2beeb7360232b09ebc4585b54475dfee06 |
| Headers | show |
| Series | USB: Gadget: core: Help prevent panic during UVC unconfigure | expand |
Hi Alan, On Sat, Jul 29, 2023 at 7:59 AM Alan Stern <stern@rowland.harvard.edu> 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@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(+) > > 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); >
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
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); + udc->driver->unbind(gadget); + + mutex_lock(&udc->connect_lock); usb_gadget_udc_stop_locked(udc); mutex_unlock(&udc->connect_lock);
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(+)