media: uvcvideo: Fix race condition with usb_kill_urb

commit 619d9b710cf06f7a00a17120ca92333684ac45a8 upstream.

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")
Reviewed-by: Yunke Cao <yunkec@chromium.org>
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index 44b0cfb8ee1c7cd8c2e6efa0e40ea79a4aafbeb0..067b43a1cb3eb5a132069a87d51caaf0be5ffa2e 100644 (file)
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -6,6 +6,7 @@
  *          Laurent Pinchart (laurent.pinchart@ideasonboard.com)
  */
 
+#include <asm/barrier.h>
 #include <linux/bitops.h>
 #include <linux/kernel.h>
 #include <linux/list.h>
@@ -1509,6 +1510,10 @@ static void uvc_ctrl_status_event_work(struct work_struct *work)
 
        uvc_ctrl_status_event(w->chain, w->ctrl, w->data);
 
+       /* The barrier is needed to synchronize with uvc_status_stop(). */
+       if (smp_load_acquire(&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 7518ffce22edb2495db5eb3b86bca45c2f327e92..4a92c989cf33572335f0d569fc0972f10aa0d2d8 100644 (file)
--- a/drivers/media/usb/uvc/uvc_status.c
+++ b/drivers/media/usb/uvc/uvc_status.c
@@ -6,6 +6,7 @@
  *          Laurent Pinchart (laurent.pinchart@ideasonboard.com)
  */
 
+#include <asm/barrier.h>
 #include <linux/kernel.h>
 #include <linux/input.h>
 #include <linux/slab.h>
@@ -309,5 +310,41 @@ int uvc_status_start(struct uvc_device *dev, gfp_t flags)
 
 void uvc_status_stop(struct uvc_device *dev)
 {
+       struct uvc_ctrl_work *w = &dev->async_ctrl;
+
+       /*
+        * Prevent the asynchronous control handler from requeing the URB. The
+        * barrier is needed so the flush_status change is visible to other
+        * CPUs running the asynchronous handler before usb_kill_urb() is
+        * called below.
+        */
+       smp_store_release(&dev->flush_status, true);
+
+       /*
+        * Cancel any pending asynchronous work. If any status event was queued,
+        * process it synchronously.
+        */
+       if (cancel_work_sync(&w->work))
+               uvc_ctrl_status_event(w->chain, w->ctrl, w->data);
+
+       /* Kill the urb. */
        usb_kill_urb(dev->int_urb);
+
+       /*
+        * The URB completion handler may have queued asynchronous work. This
+        * won't resubmit the URB as flush_status is set, but it needs to be
+        * cancelled before returning or it could then race with a future
+        * uvc_status_start() call.
+        */
+       if (cancel_work_sync(&w->work))
+               uvc_ctrl_status_event(w->chain, w->ctrl, w->data);
+
+       /*
+        * From this point, there are no events on the queue and the status URB
+        * is dead. No events will be queued until uvc_status_start() is called.
+        * The barrier is needed to make sure that flush_status is visible to
+        * uvc_ctrl_status_event_work() when uvc_status_start() will be called
+        * again.
+        */
+       smp_store_release(&dev->flush_status, false);
 }

diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index b0937703c72548413b4dd28c5e35fd5698ef04ee..33e7475d4e64ac79e26c1af3d479d2043690fb8b 100644 (file)
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -558,6 +558,7 @@ struct uvc_device {
        /* Status Interrupt Endpoint */
        struct usb_host_endpoint *int_ep;
        struct urb *int_urb;
+       bool flush_status;
        u8 *status;
        struct input_dev *input;
        char input_phys[64];