* FAILED: patch "[PATCH] media: uvcvideo: Fix race condition with usb_kill_urb" failed to apply to 6.2-stable tree
@ 2023-03-06 10:56 gregkh
2023-03-08 13:30 ` [PATCH 6.2.y] media: uvcvideo: Fix race condition with usb_kill_urb Ricardo Ribalda
2023-03-10 12:05 ` Ricardo Ribalda
0 siblings, 2 replies; 6+ messages in thread
From: gregkh @ 2023-03-06 10:56 UTC (permalink / raw)
To: ribalda, laurent.pinchart, yunkec; +Cc: stable
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@vger.kernel.org>.
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@vger.kernel.org>' --in-reply-to '16781002113959@kroah.com' --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@chromium.org>
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@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>
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@ideasonboard.com)
*/
+#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@ideasonboard.com)
*/
+#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];
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 6.2.y] media: uvcvideo: Fix race condition with usb_kill_urb
2023-03-06 10:56 FAILED: patch "[PATCH] media: uvcvideo: Fix race condition with usb_kill_urb" failed to apply to 6.2-stable tree gregkh
@ 2023-03-08 13:30 ` Ricardo Ribalda
2023-03-10 11:54 ` Greg KH
2023-03-10 12:05 ` Ricardo Ribalda
1 sibling, 1 reply; 6+ messages in thread
From: Ricardo Ribalda @ 2023-03-08 13:30 UTC (permalink / raw)
To: stable; +Cc: Ricardo Ribalda, Yunke Cao, Laurent Pinchart
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>
(cherry picked from commit 619d9b710cf06f7a00a17120ca92333684ac45a8)
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
drivers/media/usb/uvc/uvc_ctrl.c | 5 ++++
drivers/media/usb/uvc/uvc_status.c | 37 ++++++++++++++++++++++++++++++
drivers/media/usb/uvc/uvcvideo.h | 1 +
3 files changed, 43 insertions(+)
diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index c95a2229f4fa..f3a98ef8e01f 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@ideasonboard.com)
*/
+#include <asm/barrier.h>
#include <linux/kernel.h>
#include <linux/list.h>
#include <linux/module.h>
@@ -1442,6 +1443,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 7518ffce22ed..4a92c989cf33 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@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 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.40.0.rc0.216.gc4246ad0f0-goog
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 6.2.y] media: uvcvideo: Fix race condition with usb_kill_urb
2023-03-08 13:30 ` [PATCH 6.2.y] media: uvcvideo: Fix race condition with usb_kill_urb Ricardo Ribalda
@ 2023-03-10 11:54 ` Greg KH
2023-03-10 12:17 ` Greg KH
0 siblings, 1 reply; 6+ messages in thread
From: Greg KH @ 2023-03-10 11:54 UTC (permalink / raw)
To: Ricardo Ribalda; +Cc: stable, Yunke Cao, Laurent Pinchart
On Wed, Mar 08, 2023 at 02:30:25PM +0100, Ricardo Ribalda 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")
> 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>
> (cherry picked from commit 619d9b710cf06f7a00a17120ca92333684ac45a8)
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
> drivers/media/usb/uvc/uvc_ctrl.c | 5 ++++
> drivers/media/usb/uvc/uvc_status.c | 37 ++++++++++++++++++++++++++++++
> drivers/media/usb/uvc/uvcvideo.h | 1 +
> 3 files changed, 43 insertions(+)
This fails to apply to the 6.2.y queue right now:
checking file drivers/media/usb/uvc/uvc_ctrl.c
Hunk #1 FAILED at 6.
Hunk #2 succeeded at 1509 (offset 67 lines).
1 out of 2 hunks FAILED
checking file drivers/media/usb/uvc/uvc_status.c
checking file drivers/media/usb/uvc/uvcvideo.h
Hunk #1 succeeded at 559 (offset -1 lines).
Can you redo this?
thanks,
greg k-h
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 6.2.y] media: uvcvideo: Fix race condition with usb_kill_urb
2023-03-06 10:56 FAILED: patch "[PATCH] media: uvcvideo: Fix race condition with usb_kill_urb" failed to apply to 6.2-stable tree gregkh
2023-03-08 13:30 ` [PATCH 6.2.y] media: uvcvideo: Fix race condition with usb_kill_urb Ricardo Ribalda
@ 2023-03-10 12:05 ` Ricardo Ribalda
1 sibling, 0 replies; 6+ messages in thread
From: Ricardo Ribalda @ 2023-03-10 12:05 UTC (permalink / raw)
To: stable; +Cc: Ricardo Ribalda, Yunke Cao, Laurent Pinchart
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>
(cherry picked from commit 619d9b710cf06f7a00a17120ca92333684ac45a8)
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
drivers/media/usb/uvc/uvc_ctrl.c | 5 ++++
drivers/media/usb/uvc/uvc_status.c | 37 ++++++++++++++++++++++++++++++
drivers/media/usb/uvc/uvcvideo.h | 1 +
3 files changed, 43 insertions(+)
diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index 44b0cfb8ee1c..067b43a1cb3e 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@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 7518ffce22ed..4a92c989cf33 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@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 1227ae63f85b..864eef81174a 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.40.0.rc1.284.g88254d51c5-goog
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 6.2.y] media: uvcvideo: Fix race condition with usb_kill_urb
2023-03-10 11:54 ` Greg KH
@ 2023-03-10 12:17 ` Greg KH
2023-03-10 12:18 ` Ricardo Ribalda
0 siblings, 1 reply; 6+ messages in thread
From: Greg KH @ 2023-03-10 12:17 UTC (permalink / raw)
To: Ricardo Ribalda; +Cc: stable, Yunke Cao, Laurent Pinchart
On Fri, Mar 10, 2023 at 12:54:48PM +0100, Greg KH wrote:
> On Wed, Mar 08, 2023 at 02:30:25PM +0100, Ricardo Ribalda 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")
> > 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>
> > (cherry picked from commit 619d9b710cf06f7a00a17120ca92333684ac45a8)
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > ---
> > drivers/media/usb/uvc/uvc_ctrl.c | 5 ++++
> > drivers/media/usb/uvc/uvc_status.c | 37 ++++++++++++++++++++++++++++++
> > drivers/media/usb/uvc/uvcvideo.h | 1 +
> > 3 files changed, 43 insertions(+)
>
> This fails to apply to the 6.2.y queue right now:
>
> checking file drivers/media/usb/uvc/uvc_ctrl.c
> Hunk #1 FAILED at 6.
> Hunk #2 succeeded at 1509 (offset 67 lines).
> 1 out of 2 hunks FAILED
> checking file drivers/media/usb/uvc/uvc_status.c
> checking file drivers/media/usb/uvc/uvcvideo.h
> Hunk #1 succeeded at 559 (offset -1 lines).
>
> Can you redo this?
I got 5.15.y and newer to work here on my own, can you redo the older
ones as well?
thanks,
greg k-h
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 6.2.y] media: uvcvideo: Fix race condition with usb_kill_urb
2023-03-10 12:17 ` Greg KH
@ 2023-03-10 12:18 ` Ricardo Ribalda
0 siblings, 0 replies; 6+ messages in thread
From: Ricardo Ribalda @ 2023-03-10 12:18 UTC (permalink / raw)
To: Greg KH; +Cc: stable, Yunke Cao, Laurent Pinchart
Hi Greg
On Fri, 10 Mar 2023 at 13:17, Greg KH <greg@kroah.com> wrote:
>
> On Fri, Mar 10, 2023 at 12:54:48PM +0100, Greg KH wrote:
> > On Wed, Mar 08, 2023 at 02:30:25PM +0100, Ricardo Ribalda 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")
> > > 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>
> > > (cherry picked from commit 619d9b710cf06f7a00a17120ca92333684ac45a8)
> > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > ---
> > > drivers/media/usb/uvc/uvc_ctrl.c | 5 ++++
> > > drivers/media/usb/uvc/uvc_status.c | 37 ++++++++++++++++++++++++++++++
> > > drivers/media/usb/uvc/uvcvideo.h | 1 +
> > > 3 files changed, 43 insertions(+)
> >
> > This fails to apply to the 6.2.y queue right now:
> >
> > checking file drivers/media/usb/uvc/uvc_ctrl.c
> > Hunk #1 FAILED at 6.
> > Hunk #2 succeeded at 1509 (offset 67 lines).
> > 1 out of 2 hunks FAILED
> > checking file drivers/media/usb/uvc/uvc_status.c
> > checking file drivers/media/usb/uvc/uvcvideo.h
> > Hunk #1 succeeded at 559 (offset -1 lines).
> >
> > Can you redo this?
>
> I got 5.15.y and newer to work here on my own, can you redo the older
> ones as well?
Sure thanks!
>
> thanks,
>
> greg k-h
--
Ricardo Ribalda
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-03-10 12:19 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-03-06 10:56 FAILED: patch "[PATCH] media: uvcvideo: Fix race condition with usb_kill_urb" failed to apply to 6.2-stable tree gregkh
2023-03-08 13:30 ` [PATCH 6.2.y] media: uvcvideo: Fix race condition with usb_kill_urb Ricardo Ribalda
2023-03-10 11:54 ` Greg KH
2023-03-10 12:17 ` Greg KH
2023-03-10 12:18 ` Ricardo Ribalda
2023-03-10 12:05 ` Ricardo Ribalda
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox