The patch below does not apply to the 6.2-stable tree. If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <stable@xxxxxxxxxxxxxxx>. To reproduce the conflict and resubmit, you may use the following commands: git fetch https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/ linux-6.2.y git checkout FETCH_HEAD git cherry-pick -x 619d9b710cf06f7a00a17120ca92333684ac45a8 # <resolve conflicts, build, test, etc.> git commit -s git send-email --to '<stable@xxxxxxxxxxxxxxx>' --in-reply-to '16781002113959@xxxxxxxxx' --subject-prefix 'PATCH 6.2.y' HEAD^.. Possible dependencies: 619d9b710cf0 ("media: uvcvideo: Fix race condition with usb_kill_urb") 40140eda661e ("media: uvcvideo: Implement mask for V4L2_CTRL_TYPE_MENU") adfd3910c27f ("media: uvcvideo: Remove void casting for the status endpoint") thanks, greg k-h ------------------ original commit in Linus's tree ------------------ >From 619d9b710cf06f7a00a17120ca92333684ac45a8 Mon Sep 17 00:00:00 2001 From: Ricardo Ribalda <ribalda@xxxxxxxxxxxx> Date: Thu, 5 Jan 2023 15:31:29 +0100 Subject: [PATCH] media: uvcvideo: Fix race condition with usb_kill_urb 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@xxxxxxxxxxxxxxx Fixes: e5225c820c05 ("media: uvcvideo: Send a control event when a Control Change interrupt arrives") Reviewed-by: Yunke Cao <yunkec@xxxxxxxxxxxx> Signed-off-by: Ricardo Ribalda <ribalda@xxxxxxxxxxxx> Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> Signed-off-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c index ee58f0db2763..c3aeba3fe31b 100644 --- a/drivers/media/usb/uvc/uvc_ctrl.c +++ b/drivers/media/usb/uvc/uvc_ctrl.c @@ -6,6 +6,7 @@ * Laurent Pinchart (laurent.pinchart@xxxxxxxxxxxxxxxx) */ +#include <asm/barrier.h> #include <linux/bitops.h> #include <linux/kernel.h> #include <linux/list.h> @@ -1571,6 +1572,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 602830a8023e..a78a88c710e2 100644 --- a/drivers/media/usb/uvc/uvc_status.c +++ b/drivers/media/usb/uvc/uvc_status.c @@ -6,6 +6,7 @@ * Laurent Pinchart (laurent.pinchart@xxxxxxxxxxxxxxxx) */ +#include <asm/barrier.h> #include <linux/kernel.h> #include <linux/input.h> #include <linux/slab.h> @@ -311,5 +312,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 e85df8deb965..c0e706fcd2cb 100644 --- a/drivers/media/usb/uvc/uvcvideo.h +++ b/drivers/media/usb/uvc/uvcvideo.h @@ -577,6 +577,7 @@ struct uvc_device { struct usb_host_endpoint *int_ep; struct urb *int_urb; struct uvc_status *status; + bool flush_status; struct input_dev *input; char input_phys[64];
