| Message ID | 20221212-uvc-race-v2-1-54496cc3b8ab@chromium.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | uvc: Fix race condition on uvc | expand |
On Tue, Dec 13, 2022 at 11:36 PM Ricardo Ribalda <ribalda@chromium.org> wrote: > > usb_kill_urb warranties that all the handlers are finished when it > returns, but does not protect against threads that might be handling > asynchronously the urb. > > For UVC, the function uvc_ctrl_status_event_async() takes care of > control changes asynchronously. > > If the code is executed in the following order: > > CPU 0 CPU 1 > ===== ===== > uvc_status_complete() > uvc_status_stop() > uvc_ctrl_status_event_work() > uvc_status_start() -> FAIL > > Then uvc_status_start will keep failing and this error will be shown: > > <4>[ 5.540139] URB 0000000000000000 submitted while active > drivers/usb/core/urb.c:378 usb_submit_urb+0x4c3/0x528 > > Let's improve the current situation, by not re-submiting the urb if > we are stopping the status event. Also process the queued work > (if any) during stop. > > CPU 0 CPU 1 > ===== ===== > uvc_status_complete() > uvc_status_stop() > uvc_status_start() > uvc_ctrl_status_event_work() -> FAIL > > Hopefully, with the usb layer protection this should be enough to cover > all the cases. > > Cc: stable@vger.kernel.org > Fixes: e5225c820c05 ("media: uvcvideo: Send a control event when a Control Change interrupt arrives") > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > --- > drivers/media/usb/uvc/uvc_ctrl.c | 3 +++ > drivers/media/usb/uvc/uvc_status.c | 6 ++++++ > drivers/media/usb/uvc/uvcvideo.h | 1 + > 3 files changed, 10 insertions(+) > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c > index c95a2229f4fa..5160facc8e20 100644 > --- a/drivers/media/usb/uvc/uvc_ctrl.c > +++ b/drivers/media/usb/uvc/uvc_ctrl.c > @@ -1442,6 +1442,9 @@ static void uvc_ctrl_status_event_work(struct work_struct *work) > > uvc_ctrl_status_event(w->chain, w->ctrl, w->data); > > + if (dev->flush_status) > + return; > + > /* Resubmit the URB. */ > w->urb->interval = dev->int_ep->desc.bInterval; > ret = usb_submit_urb(w->urb, GFP_KERNEL); > diff --git a/drivers/media/usb/uvc/uvc_status.c b/drivers/media/usb/uvc/uvc_status.c > index 7518ffce22ed..09a5802dc974 100644 > --- a/drivers/media/usb/uvc/uvc_status.c > +++ b/drivers/media/usb/uvc/uvc_status.c > @@ -304,10 +304,16 @@ int uvc_status_start(struct uvc_device *dev, gfp_t flags) > if (dev->int_urb == NULL) > return 0; > > + dev->flush_status = false; > return usb_submit_urb(dev->int_urb, flags); > } > > void uvc_status_stop(struct uvc_device *dev) > { > + struct uvc_ctrl_work *w = &dev->async_ctrl; > + > + dev->flush_status = true; > usb_kill_urb(dev->int_urb); > + if (cancel_work_sync(&w->work)) > + uvc_ctrl_status_event(w->chain, w->ctrl, w->data); > } > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h > index df93db259312..6a9b72d6789e 100644 > --- a/drivers/media/usb/uvc/uvcvideo.h > +++ b/drivers/media/usb/uvc/uvcvideo.h > @@ -560,6 +560,7 @@ struct uvc_device { > struct usb_host_endpoint *int_ep; > struct urb *int_urb; > u8 *status; > + bool flush_status; > struct input_dev *input; > char input_phys[64]; > > > -- > 2.39.0.rc1.256.g54fd8350bd-goog-b4-0.11.0-dev-696ae Reviewed-by: Yunke Cao <yunkec@chromium.org>
diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c index c95a2229f4fa..5160facc8e20 100644 --- a/drivers/media/usb/uvc/uvc_ctrl.c +++ b/drivers/media/usb/uvc/uvc_ctrl.c @@ -1442,6 +1442,9 @@ static void uvc_ctrl_status_event_work(struct work_struct *work) uvc_ctrl_status_event(w->chain, w->ctrl, w->data); + if (dev->flush_status) + return; + /* Resubmit the URB. */ w->urb->interval = dev->int_ep->desc.bInterval; ret = usb_submit_urb(w->urb, GFP_KERNEL); diff --git a/drivers/media/usb/uvc/uvc_status.c b/drivers/media/usb/uvc/uvc_status.c index 7518ffce22ed..09a5802dc974 100644 --- a/drivers/media/usb/uvc/uvc_status.c +++ b/drivers/media/usb/uvc/uvc_status.c @@ -304,10 +304,16 @@ int uvc_status_start(struct uvc_device *dev, gfp_t flags) if (dev->int_urb == NULL) return 0; + dev->flush_status = false; return usb_submit_urb(dev->int_urb, flags); } void uvc_status_stop(struct uvc_device *dev) { + struct uvc_ctrl_work *w = &dev->async_ctrl; + + dev->flush_status = true; usb_kill_urb(dev->int_urb); + if (cancel_work_sync(&w->work)) + uvc_ctrl_status_event(w->chain, w->ctrl, w->data); } diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h index df93db259312..6a9b72d6789e 100644 --- a/drivers/media/usb/uvc/uvcvideo.h +++ b/drivers/media/usb/uvc/uvcvideo.h @@ -560,6 +560,7 @@ struct uvc_device { struct usb_host_endpoint *int_ep; struct urb *int_urb; u8 *status; + bool flush_status; struct input_dev *input; char input_phys[64];
usb_kill_urb warranties that all the handlers are finished when it returns, but does not protect against threads that might be handling asynchronously the urb. For UVC, the function uvc_ctrl_status_event_async() takes care of control changes asynchronously. If the code is executed in the following order: CPU 0 CPU 1 ===== ===== uvc_status_complete() uvc_status_stop() uvc_ctrl_status_event_work() uvc_status_start() -> FAIL Then uvc_status_start will keep failing and this error will be shown: <4>[ 5.540139] URB 0000000000000000 submitted while active drivers/usb/core/urb.c:378 usb_submit_urb+0x4c3/0x528 Let's improve the current situation, by not re-submiting the urb if we are stopping the status event. Also process the queued work (if any) during stop. CPU 0 CPU 1 ===== ===== uvc_status_complete() uvc_status_stop() uvc_status_start() uvc_ctrl_status_event_work() -> FAIL Hopefully, with the usb layer protection this should be enough to cover all the cases. Cc: stable@vger.kernel.org Fixes: e5225c820c05 ("media: uvcvideo: Send a control event when a Control Change interrupt arrives") Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> --- drivers/media/usb/uvc/uvc_ctrl.c | 3 +++ drivers/media/usb/uvc/uvc_status.c | 6 ++++++ drivers/media/usb/uvc/uvcvideo.h | 1 + 3 files changed, 10 insertions(+)