[PATCH v2 0/2] uvc: Fix race condition on uvc

[PATCH v2 0/2] uvc: Fix race condition on uvc

[Ricardo Ribalda]

Make sure that all the async work is finished when we stop the status urb.

To: Yunke Cao <yunkec@chromium.org>
To: Sergey Senozhatsky <senozhatsky@chromium.org>
To: Max Staudt <mstaudt@google.com>
To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Mauro Carvalho Chehab <mchehab@kernel.org>
Cc: linux-media@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>

---
Changes in v2:
- Add a patch for not kalloc dev->status
- Redo the logic mechanism, so it also works with suspend (Thanks Yunke!)
- Link to v1: https://lore.kernel.org/r/20221212-uvc-race-v1-0-c52e1783c31d@chromium.org

---
Ricardo Ribalda (2):
      media: uvcvideo: Fix race condition with usb_kill_urb
      media: uvcvideo: Do not alloc dev->status

 drivers/media/usb/uvc/uvc_ctrl.c   |  3 +++
 drivers/media/usb/uvc/uvc_status.c | 15 +++++++--------
 drivers/media/usb/uvc/uvcvideo.h   |  3 ++-
 3 files changed, 12 insertions(+), 9 deletions(-)
---
base-commit: 0ec5a38bf8499f403f81cb81a0e3a60887d1993c
change-id: 20221212-uvc-race-09276ea68bf8

Best regards,
-- 
Ricardo Ribalda <ribalda@chromium.org>

[PATCH v2 1/2] media: uvcvideo: Fix race condition with usb_kill_urb

[Ricardo Ribalda]

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

Re: [PATCH v2 1/2] media: uvcvideo: Fix race condition with usb_kill_urb

[Yunke Cao]

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>

[PATCH v2 2/2] media: uvcvideo: Do not alloc dev->status

[Ricardo Ribalda]

UVC_MAX_STATUS_SIZE is 16, simplify the code by inlining dev->status.

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_status.c | 9 +--------
 drivers/media/usb/uvc/uvcvideo.h   | 2 +-
 2 files changed, 2 insertions(+), 9 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_status.c b/drivers/media/usb/uvc/uvc_status.c
index 09a5802dc974..52999b3b7c48 100644
--- a/drivers/media/usb/uvc/uvc_status.c
+++ b/drivers/media/usb/uvc/uvc_status.c
@@ -259,15 +259,9 @@ int uvc_status_init(struct uvc_device *dev)
 
 	uvc_input_init(dev);
 
-	dev->status = kzalloc(UVC_MAX_STATUS_SIZE, GFP_KERNEL);
-	if (dev->status == NULL)
-		return -ENOMEM;
-
 	dev->int_urb = usb_alloc_urb(0, GFP_KERNEL);
-	if (dev->int_urb == NULL) {
-		kfree(dev->status);
+	if (!dev->int_urb)
 		return -ENOMEM;
-	}
 
 	pipe = usb_rcvintpipe(dev->udev, ep->desc.bEndpointAddress);
 
@@ -296,7 +290,6 @@ void uvc_status_unregister(struct uvc_device *dev)
 void uvc_status_cleanup(struct uvc_device *dev)
 {
 	usb_free_urb(dev->int_urb);
-	kfree(dev->status);
 }
 
 int uvc_status_start(struct uvc_device *dev, gfp_t flags)
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index 6a9b72d6789e..ccc7e3b60bf1 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -559,7 +559,7 @@ struct uvc_device {
 	/* Status Interrupt Endpoint */
 	struct usb_host_endpoint *int_ep;
 	struct urb *int_urb;
-	u8 *status;
+	u8 status[UVC_MAX_STATUS_SIZE];
 	bool flush_status;
 	struct input_dev *input;
 	char input_phys[64];

-- 
2.39.0.rc1.256.g54fd8350bd-goog-b4-0.11.0-dev-696ae

Re: [PATCH v2 2/2] media: uvcvideo: Do not alloc dev->status

[Sergey Senozhatsky]

On (22/12/13 15:35), Ricardo Ribalda wrote:
[..]
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -559,7 +559,7 @@ struct uvc_device {
>  	/* Status Interrupt Endpoint */
>  	struct usb_host_endpoint *int_ep;
>  	struct urb *int_urb;
> -	u8 *status;
> +	u8 status[UVC_MAX_STATUS_SIZE];

Can we use `struct uvc_control_status status;` instead of open-coding it?
Seems that this is what the code wants anyway:

	struct uvc_control_status *status =
				(struct uvc_control_status *)dev->status;

And then we can drop casts in uvc_status_complete().

Re: [PATCH v2 2/2] media: uvcvideo: Do not alloc dev->status

[Ricardo Ribalda]

Hi Sergey

Thanks for the review

On Wed, 14 Dec 2022 at 01:40, Sergey Senozhatsky
<senozhatsky@chromium.org> wrote:
>
> On (22/12/13 15:35), Ricardo Ribalda wrote:
> [..]
> > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > @@ -559,7 +559,7 @@ struct uvc_device {
> >       /* Status Interrupt Endpoint */
> >       struct usb_host_endpoint *int_ep;
> >       struct urb *int_urb;
> > -     u8 *status;
> > +     u8 status[UVC_MAX_STATUS_SIZE];
>
> Can we use `struct uvc_control_status status;` instead of open-coding it?
> Seems that this is what the code wants anyway:

It can also be a `struct uvc_streaming_status`

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/media/usb/uvc/uvc_status.c#n230

so we always need the casting :(

>
>         struct uvc_control_status *status =
>                                 (struct uvc_control_status *)dev->status;
>
> And then we can drop casts in uvc_status_complete().



-- 
Ricardo Ribalda

Re: [PATCH v2 2/2] media: uvcvideo: Do not alloc dev->status

[Sergey Senozhatsky]

On (22/12/14 06:57), Ricardo Ribalda wrote:
> > On (22/12/13 15:35), Ricardo Ribalda wrote:
> > [..]
> > > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > > @@ -559,7 +559,7 @@ struct uvc_device {
> > >       /* Status Interrupt Endpoint */
> > >       struct usb_host_endpoint *int_ep;
> > >       struct urb *int_urb;
> > > -     u8 *status;
> > > +     u8 status[UVC_MAX_STATUS_SIZE];
> >
> > Can we use `struct uvc_control_status status;` instead of open-coding it?
> > Seems that this is what the code wants anyway:
> 
> It can also be a `struct uvc_streaming_status`
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/media/usb/uvc/uvc_status.c#n230
> 
> so we always need the casting :(

Then perhaps we can put both of them into anon union in struct uvc_device
as stream_status and control_status?