From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id C1497EB64DA for ; Thu, 20 Jul 2023 14:56:03 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232331AbjGTO4C (ORCPT ); Thu, 20 Jul 2023 10:56:02 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:42642 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230317AbjGTO4B (ORCPT ); Thu, 20 Jul 2023 10:56:01 -0400 Received: from netrider.rowland.org (netrider.rowland.org [192.131.102.5]) by lindbergh.monkeyblade.net (Postfix) with SMTP id 20DF9196 for ; Thu, 20 Jul 2023 07:55:59 -0700 (PDT) Received: (qmail 1683964 invoked by uid 1000); 20 Jul 2023 10:55:59 -0400 Date: Thu, 20 Jul 2023 10:55:59 -0400 From: Alan Stern To: Aman Deep Cc: gregkh@linuxfoundation.org, laurent.pinchart@ideasonboard.com, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org, Anuj Gupta Subject: Re: [PATCH] USB: Fix race condition during UVC webcam disconnect Message-ID: References: <20230720113142.3070583-1-aman.deep@samsung.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20230720113142.3070583-1-aman.deep@samsung.com> Precedence: bulk List-ID: X-Mailing-List: linux-usb@vger.kernel.org On Thu, Jul 20, 2023 at 05:01:42PM +0530, Aman Deep wrote: > In the bug happened during uvc webcam disconect,there is race > between stopping a video transfer and usb disconnect.This issue is > reproduced in my system running Linux kernel when UVC webcam play is > stopped and UVC webcam is disconnected at the same time. This causes the > below backtrace: > > [2-3496.7275] PC is at 0xbf418000+0x2d8 [usbcore] > [2-3496.7275] LR is at 0x00000005 > [2-3496.7275] pc : []((usb_ifnum_to_if > [usbcore.ko]>)) lr : [<00000005>]() psr: 20000013 > [2-3496.7275] Function entered at []((usb_ifnum_to_if > [usbcore.ko]>)) (0xbf418000+0x2a4 [usbcore]) from > []((usb_hcd_alloc_bandwidth > [usbcore.ko]>)) (0xbf418000+0xb974 [usbcore]) > [2-3496.7275] Function entered at []((usb_hcd_alloc_bandwidth > [usbcore.ko]>)) (0xbf418000+0xb738 [usbcore]) from > []((usb_set_interface > [usbcore.ko]>)) (0xbf418000+0xeca0 [usbcore]) > [2-3496.7275] Function entered at []((usb_set_interface > [usbcore.ko]>)) (0xbf418000+0xeb9c [usbcore]) from > []((uvc_video_clock_cleanup > uvc_video_stop_streaming > [uvcvideo.ko]>)) (0xbf9bd000+0x7dd4 [uvcvideo]) > [2-3496.7275] Function entered at []((uvc_video_stop_streaming > [uvcvideo.ko]>)) (0xbf9bd000+0x7d98 [uvcvideo]) from > []((spin_lock_irq > uvc_stop_streaming > [uvcvideo.ko]>)) (0xbf9bd000+0x2ab8 [uvcvideo]) > [2-3496.7276] Function entered at []((uvc_stop_streaming > [uvcvideo.ko]>)) (0xbf9bd000+0x2a94 [uvcvideo]) from > []((__read_once_size > (discriminator 1) __vb2_queue_cancel > (discriminator 1) [videobuf2_common.ko]>)) (0xbe306000+0x1150 > [videobuf2_common]) > [2-3496.7276] Function entered at []((__vb2_queue_cancel > [videobuf2_common.ko]>)) (0xbe306000+0x1120 [videobuf2_common]) from > []((vb2_core_streamoff > This below solution patch fixes this race condition at USB core level > occurring during UVC webcam device disconnect. How can a race in the UVC driver be fixed by changing the USB core? It seems obvious that the only way to fix such a race is by changing the UVC driver. > Signed-off-by: Anuj Gupta > Signed-off-by: Aman Deep > --- > drivers/usb/core/hcd.c | 7 ++++++- > drivers/usb/core/message.c | 4 ++++ > drivers/usb/core/usb.c | 9 ++++++--- > 3 files changed, 16 insertions(+), 4 deletions(-) > > diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c > index 8300baedafd2..a06452cbbaa4 100644 > --- a/drivers/usb/core/hcd.c > +++ b/drivers/usb/core/hcd.c > @@ -1931,7 +1931,12 @@ int usb_hcd_alloc_bandwidth(struct usb_device *udev, > } > } > if (cur_alt && new_alt) { > - struct usb_interface *iface = usb_ifnum_to_if(udev, > + struct usb_interface *iface; > + > + if (udev->state == USB_STATE_NOTATTACHED) > + return -ENODEV; What will happen if the state changes to USB_STATE_NOTATTACHED at this point, after that test was made? > + > + iface = usb_ifnum_to_if(udev, > cur_alt->desc.bInterfaceNumber); > > if (!iface) > diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c > index b5811620f1de..f31c7287dc01 100644 > --- a/drivers/usb/core/message.c > +++ b/drivers/usb/core/message.c > @@ -1575,7 +1575,11 @@ int usb_set_interface(struct usb_device *dev, int interface, int alternate) > for (i = 0; i < iface->cur_altsetting->desc.bNumEndpoints; i++) > iface->cur_altsetting->endpoint[i].streams = 0; > > + if (dev->state == USB_STATE_NOTATTACHED) > + return -ENODEV; Same question: What happens if the state changes right here? > + > ret = usb_hcd_alloc_bandwidth(dev, NULL, iface->cur_altsetting, alt); > + > if (ret < 0) { > dev_info(&dev->dev, "Not enough bandwidth for altsetting %d\n", > alternate); > diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c > index 901ec732321c..6fb8b14469ae 100644 > --- a/drivers/usb/core/usb.c > +++ b/drivers/usb/core/usb.c > @@ -352,10 +352,13 @@ struct usb_interface *usb_ifnum_to_if(const struct usb_device *dev, > > if (!config) > return NULL; > - for (i = 0; i < config->desc.bNumInterfaces; i++) > - if (config->interface[i]->altsetting[0] > - .desc.bInterfaceNumber == ifnum) > + for (i = 0; i < config->desc.bNumInterfaces; i++) { > + if (config->interface[i] && This new test can fail only if the routine is called after (or while) the device is unconfigured or removed. But if a driver makes such a call then the driver is buggy. The proper solution is to fix the driver, not add this test here. Besides, this test has the same problem as the others you added above. What happens if config->interface[i] gets set to NULL right here? Alan Stern > + config->interface[i]->altsetting[0] > + .desc.bInterfaceNumber == ifnum) { > return config->interface[i]; > + } > + } > > return NULL; > } > -- > 2.34.1 >