[PATCH v3 0/3] uvcvideo: Attempt N to land UVC race conditions fixes

[PATCH v3 0/3] uvcvideo: Attempt N to land UVC race conditions fixes

[Ricardo Ribalda]

Back in 2020 Guenter published a set of patches to fix some race
conditions on UVC.
https://lore.kernel.org/all/20200917022547.198090-5-linux@roeck-us.net/

That kind of race conditions are not only seen on UVC, but are a common
sin on almost all the kernel, so this is what it was decided back then
that we should try to fix them at higher levels.

After that. A lot of video_is_registered() were added to the core:

```
ribalda@alco:~/work/linux$ git grep is_registered drivers/media/v4l2-core/
drivers/media/v4l2-core/v4l2-compat-ioctl32.c:  if (!video_is_registered(vdev))
drivers/media/v4l2-core/v4l2-dev.c:     if (video_is_registered(vdev))
drivers/media/v4l2-core/v4l2-dev.c:     if (video_is_registered(vdev))
drivers/media/v4l2-core/v4l2-dev.c:     if (video_is_registered(vdev)) {
drivers/media/v4l2-core/v4l2-dev.c:             if (video_is_registered(vdev))
drivers/media/v4l2-core/v4l2-dev.c:     if (!video_is_registered(vdev))
drivers/media/v4l2-core/v4l2-dev.c:     if (video_is_registered(vdev))
drivers/media/v4l2-core/v4l2-dev.c:     if (vdev == NULL || !video_is_registered(vdev)) {
drivers/media/v4l2-core/v4l2-dev.c:             if (video_is_registered(vdev))
drivers/media/v4l2-core/v4l2-dev.c:     if (!vdev || !video_is_registered(vdev))
drivers/media/v4l2-core/v4l2-ioctl.c:   if (!video_is_registered(vfd)) {
drivers/media/v4l2-core/v4l2-subdev.c:  if (video_is_registered(vdev)) {
```

And recently Sakari is trying to land:
https://lore.kernel.org/linux-media/20230201214535.347075-1-sakari.ailus@linux.intel.com/

Which will make obsolete a lot of (all?) of the video_is_registered() checks on
Guenter's patches.

Besides those checks, there were some other valid races fixed on his
patches.

This series is just a rebase of what I think is missing on UVC even
if we fixed v4l2/core with all the video_is_register() checks removed.

Thanks!

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
Changes in v3:
- Introduce media: uvcvideo: Do not halt the device after disconnect
- Introduce media: uvcvideo: Always use uvc_status_stop()
- Link to v2: https://lore.kernel.org/r/20230309-guenter-mini-v2-0-e6410d590d43@chromium.org

Changes in v2:
- Actually send the series to the ML an not only to individuals.
- Link to v1: https://lore.kernel.org/r/20230309-guenter-mini-v1-0-627d10cf6e96@chromium.org

---
Guenter Roeck (1):
      media: uvcvideo: Lock video streams and queues while unregistering

Ricardo Ribalda (2):
      media: uvcvideo: Always use uvc_status_stop()
      media: uvcvideo: Do not halt the device after disconnect

 drivers/media/usb/uvc/uvc_ctrl.c   |  4 ----
 drivers/media/usb/uvc/uvc_driver.c | 11 ++++++++++
 drivers/media/usb/uvc/uvc_status.c |  2 +-
 drivers/media/usb/uvc/uvc_video.c  | 45 ++++++++++++++++++++++++--------------
 drivers/media/usb/uvc/uvcvideo.h   |  2 ++
 5 files changed, 42 insertions(+), 22 deletions(-)
---
base-commit: 98b1cc82c4affc16f5598d4fa14b1858671b2263
change-id: 20230309-guenter-mini-89861b084ef1

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

[PATCH v3 1/3] media: uvcvideo: Always use uvc_status_stop()

[Ricardo Ribalda]

uvc_status_stop() handles properly the race conditions with the
asynchronous worker.
Let's use uvc_status_stop() for all the code paths that require stopping
it.

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_ctrl.c   | 4 ----
 drivers/media/usb/uvc/uvc_status.c | 2 +-
 2 files changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index e59a463c2761..8e22a07e3e7b 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -2765,10 +2765,6 @@ void uvc_ctrl_cleanup_device(struct uvc_device *dev)
 	struct uvc_entity *entity;
 	unsigned int i;
 
-	/* Can be uninitialized if we are aborting on probe error. */
-	if (dev->async_ctrl.work.func)
-		cancel_work_sync(&dev->async_ctrl.work);
-
 	/* Free controls and control mappings for all entities. */
 	list_for_each_entry(entity, &dev->entities, list) {
 		for (i = 0; i < entity->ncontrols; ++i) {
diff --git a/drivers/media/usb/uvc/uvc_status.c b/drivers/media/usb/uvc/uvc_status.c
index a78a88c710e2..0208612a9f12 100644
--- a/drivers/media/usb/uvc/uvc_status.c
+++ b/drivers/media/usb/uvc/uvc_status.c
@@ -292,7 +292,7 @@ int uvc_status_init(struct uvc_device *dev)
 
 void uvc_status_unregister(struct uvc_device *dev)
 {
-	usb_kill_urb(dev->int_urb);
+	uvc_status_stop(dev);
 	uvc_input_unregister(dev);
 }
 

-- 
2.43.0.rc1.413.gea7ed67945-goog

[PATCH v3 2/3] media: uvcvideo: Do not halt the device after disconnect

[Ricardo Ribalda]

usb drivers should not call to any usb_() function after the
.disconnect() callback has been triggered.

If the camera is streaming, the uvc driver will call usb_set_interface or
usb_clear_halt once the device is being released. Let's fix this issue.

This is probably not the only driver affected with this kind of bug, but
until there is a better way to do it in the core this is the way to
solve this issue.

When/if a different mechanism is implemented in the core to solve the
lifetime of devices we will adopt it in uvc.

Trace:
[ 1065.389723] drivers/media/usb/uvc/uvc_driver.c:2248 uvc_disconnect enter
[ 1065.390160] drivers/media/usb/uvc/uvc_driver.c:2264 uvc_disconnect exit
[ 1065.433956] drivers/media/usb/uvc/uvc_v4l2.c:659 uvc_v4l2_release enter
[ 1065.433973] drivers/media/usb/uvc/uvc_video.c:2274 uvc_video_stop_streaming enter
[ 1065.434560] drivers/media/usb/uvc/uvc_video.c:2285 uvc_video_stop_streaming exit
[ 1065.435154] drivers/media/usb/uvc/uvc_v4l2.c:680 uvc_v4l2_release exit
[ 1065.435188] drivers/media/usb/uvc/uvc_driver.c:2248 uvc_disconnect enter

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

diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index 08fcd2ffa727..413c32867617 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -2257,6 +2257,8 @@ static void uvc_disconnect(struct usb_interface *intf)
 		return;
 
 	uvc_unregister_video(dev);
+	/* Barrier needed to synchronize with uvc_video_stop_streaming(). */
+	smp_store_release(&dev->disconnected, true);
 	kref_put(&dev->ref, uvc_delete);
 }
 
diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
index 28dde08ec6c5..032b44e45b22 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -2243,28 +2243,39 @@ int uvc_video_start_streaming(struct uvc_streaming *stream)
 	return ret;
 }
 
-void uvc_video_stop_streaming(struct uvc_streaming *stream)
+static void uvc_video_halt(struct uvc_streaming *stream)
 {
-	uvc_video_stop_transfer(stream, 1);
+	unsigned int epnum;
+	unsigned int pipe;
+	unsigned int dir;
 
 	if (stream->intf->num_altsetting > 1) {
 		usb_set_interface(stream->dev->udev, stream->intfnum, 0);
-	} else {
-		/*
-		 * UVC doesn't specify how to inform a bulk-based device
-		 * when the video stream is stopped. Windows sends a
-		 * CLEAR_FEATURE(HALT) request to the video streaming
-		 * bulk endpoint, mimic the same behaviour.
-		 */
-		unsigned int epnum = stream->header.bEndpointAddress
-				   & USB_ENDPOINT_NUMBER_MASK;
-		unsigned int dir = stream->header.bEndpointAddress
-				 & USB_ENDPOINT_DIR_MASK;
-		unsigned int pipe;
-
-		pipe = usb_sndbulkpipe(stream->dev->udev, epnum) | dir;
-		usb_clear_halt(stream->dev->udev, pipe);
+		return;
 	}
 
+	/*
+	 * UVC doesn't specify how to inform a bulk-based device
+	 * when the video stream is stopped. Windows sends a
+	 * CLEAR_FEATURE(HALT) request to the video streaming
+	 * bulk endpoint, mimic the same behaviour.
+	 */
+	epnum = stream->header.bEndpointAddress & USB_ENDPOINT_NUMBER_MASK;
+	dir = stream->header.bEndpointAddress & USB_ENDPOINT_DIR_MASK;
+	pipe = usb_sndbulkpipe(stream->dev->udev, epnum) | dir;
+	usb_clear_halt(stream->dev->udev, pipe);
+}
+
+void uvc_video_stop_streaming(struct uvc_streaming *stream)
+{
+	uvc_video_stop_transfer(stream, 1);
+
+	/*
+	 * Barrier needed to synchronize with uvc_disconnect().
+	 * We cannot call usb_* functions on a disconnected USB device.
+	 */
+	if (!smp_load_acquire(&stream->dev->disconnected))
+		uvc_video_halt(stream);
+
 	uvc_video_clock_cleanup(stream);
 }
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index 6fb0a78b1b00..4318ce8e31db 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -559,6 +559,8 @@ struct uvc_device {
 	unsigned int users;
 	atomic_t nmappings;
 
+	bool disconnected;
+
 	/* Video control interface */
 #ifdef CONFIG_MEDIA_CONTROLLER
 	struct media_device mdev;

-- 
2.43.0.rc1.413.gea7ed67945-goog

[PATCH v3 3/3] media: uvcvideo: Lock video streams and queues while unregistering

[Ricardo Ribalda]

From: Guenter Roeck <linux@roeck-us.net>

The call to uvc_disconnect() is not protected by any mutex.
This means it can and will be called while other accesses to the video
device are in progress. This can cause all kinds of race conditions,
including crashes such as the following.

usb 1-4: USB disconnect, device number 3
BUG: unable to handle kernel NULL pointer dereference at 0000000000000000
PGD 0 P4D 0
Oops: 0000 [#1] PREEMPT SMP PTI
CPU: 0 PID: 5633 Comm: V4L2CaptureThre Not tainted 4.19.113-08536-g5d29ca36db06 #1
Hardware name: GOOGLE Edgar, BIOS Google_Edgar.7287.167.156 03/25/2019
RIP: 0010:usb_ifnum_to_if+0x29/0x40
Code: <...>
RSP: 0018:ffffa46f42a47a80 EFLAGS: 00010246
RAX: 0000000000000000 RBX: 0000000000000000 RCX: ffff904a396c9000
RDX: ffff904a39641320 RSI: 0000000000000001 RDI: 0000000000000000
RBP: ffffa46f42a47a80 R08: 0000000000000002 R09: 0000000000000000
R10: 0000000000009975 R11: 0000000000000009 R12: 0000000000000000
R13: ffff904a396b3800 R14: ffff904a39e88000 R15: 0000000000000000
FS: 00007f396448e700(0000) GS:ffff904a3ba00000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000000000 CR3: 000000016cb46000 CR4: 00000000001006f0
Call Trace:
 usb_hcd_alloc_bandwidth+0x1ee/0x30f
 usb_set_interface+0x1a3/0x2b7
 uvc_video_start_transfer+0x29b/0x4b8 [uvcvideo]
 uvc_video_start_streaming+0x91/0xdd [uvcvideo]
 uvc_start_streaming+0x28/0x5d [uvcvideo]
 vb2_start_streaming+0x61/0x143 [videobuf2_common]
 vb2_core_streamon+0xf7/0x10f [videobuf2_common]
 uvc_queue_streamon+0x2e/0x41 [uvcvideo]
 uvc_ioctl_streamon+0x42/0x5c [uvcvideo]
 __video_do_ioctl+0x33d/0x42a
 video_usercopy+0x34e/0x5ff
 ? video_ioctl2+0x16/0x16
 v4l2_ioctl+0x46/0x53
 do_vfs_ioctl+0x50a/0x76f
 ksys_ioctl+0x58/0x83
 __x64_sys_ioctl+0x1a/0x1e
 do_syscall_64+0x54/0xde

usb_set_interface() should not be called after the USB device has been
unregistered. However, in the above case the disconnect happened after
v4l2_ioctl() was called, but before the call to usb_ifnum_to_if().

Acquire various mutexes in uvc_unregister_video() to fix the majority
(maybe all) of the observed race conditions.

The uvc_device lock prevents races against suspend and resume calls
and the poll function.

The uvc_streaming lock prevents races against stream related functions;
for the most part, those are ioctls. This lock also requires other
functions using this lock to check if a video device is still registered
after acquiring it. For example, it was observed that the video device
was already unregistered by the time the stream lock was acquired in
uvc_ioctl_streamon().

The uvc_queue lock prevents races against queue functions, Most of
those are already protected by the uvc_streaming lock, but some
are called directly. This is done as added protection; an actual race
was not (yet) observed.

Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Alan Stern <stern@rowland.harvard.edu>
Cc: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Reviewed-by: Tomasz Figa <tfiga@chromium.org>
Reviewed-by: Sean Paul <seanpaul@chromium.org>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_driver.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index 413c32867617..3408b865d346 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -1907,14 +1907,22 @@ static void uvc_unregister_video(struct uvc_device *dev)
 {
 	struct uvc_streaming *stream;
 
+	mutex_lock(&dev->lock);
+
 	list_for_each_entry(stream, &dev->streams, list) {
 		if (!video_is_registered(&stream->vdev))
 			continue;
 
+		mutex_lock(&stream->mutex);
+		mutex_lock(&stream->queue.mutex);
+
 		video_unregister_device(&stream->vdev);
 		video_unregister_device(&stream->meta.vdev);
 
 		uvc_debugfs_cleanup_stream(stream);
+
+		mutex_unlock(&stream->queue.mutex);
+		mutex_unlock(&stream->mutex);
 	}
 
 	uvc_status_unregister(dev);
@@ -1925,6 +1933,7 @@ static void uvc_unregister_video(struct uvc_device *dev)
 	if (media_devnode_is_registered(dev->mdev.devnode))
 		media_device_unregister(&dev->mdev);
 #endif
+	mutex_unlock(&dev->lock);
 }
 
 int uvc_register_video_device(struct uvc_device *dev,

-- 
2.43.0.rc1.413.gea7ed67945-goog

Re: [PATCH v3 1/3] media: uvcvideo: Always use uvc_status_stop()

[Sergey Senozhatsky]

On (23/11/21 19:53), Ricardo Ribalda wrote:
> uvc_status_stop() handles properly the race conditions with the
> asynchronous worker.
> Let's use uvc_status_stop() for all the code paths that require stopping
> it.
> 
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_ctrl.c   | 4 ----
>  drivers/media/usb/uvc/uvc_status.c | 2 +-
>  2 files changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index e59a463c2761..8e22a07e3e7b 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -2765,10 +2765,6 @@ void uvc_ctrl_cleanup_device(struct uvc_device *dev)
>  	struct uvc_entity *entity;
>  	unsigned int i;
>  
> -	/* Can be uninitialized if we are aborting on probe error. */
> -	if (dev->async_ctrl.work.func)
> -		cancel_work_sync(&dev->async_ctrl.work);
> -
>  	/* Free controls and control mappings for all entities. */
>  	list_for_each_entry(entity, &dev->entities, list) {
>  		for (i = 0; i < entity->ncontrols; ++i) {
> diff --git a/drivers/media/usb/uvc/uvc_status.c b/drivers/media/usb/uvc/uvc_status.c
> index a78a88c710e2..0208612a9f12 100644
> --- a/drivers/media/usb/uvc/uvc_status.c
> +++ b/drivers/media/usb/uvc/uvc_status.c
> @@ -292,7 +292,7 @@ int uvc_status_init(struct uvc_device *dev)
>  
>  void uvc_status_unregister(struct uvc_device *dev)
>  {
> -	usb_kill_urb(dev->int_urb);
> +	uvc_status_stop(dev);

Sort of feels like this needs dev->lock somewhere here. Should we move 3/3
to the head of the series?

The question is, can this be called in parallel with uvc_v4l2_release(),
for instance?

>  	uvc_input_unregister(dev);
>  }

Re: [PATCH v3 1/3] media: uvcvideo: Always use uvc_status_stop()

[Ricardo Ribalda]

Hi Sergey

On Wed, 22 Nov 2023 at 08:21, Sergey Senozhatsky
<senozhatsky@chromium.org> wrote:
>
> On (23/11/21 19:53), Ricardo Ribalda wrote:
> > uvc_status_stop() handles properly the race conditions with the
> > asynchronous worker.
> > Let's use uvc_status_stop() for all the code paths that require stopping
> > it.
> >
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > ---
> >  drivers/media/usb/uvc/uvc_ctrl.c   | 4 ----
> >  drivers/media/usb/uvc/uvc_status.c | 2 +-
> >  2 files changed, 1 insertion(+), 5 deletions(-)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> > index e59a463c2761..8e22a07e3e7b 100644
> > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > @@ -2765,10 +2765,6 @@ void uvc_ctrl_cleanup_device(struct uvc_device *dev)
> >       struct uvc_entity *entity;
> >       unsigned int i;
> >
> > -     /* Can be uninitialized if we are aborting on probe error. */
> > -     if (dev->async_ctrl.work.func)
> > -             cancel_work_sync(&dev->async_ctrl.work);
> > -
> >       /* Free controls and control mappings for all entities. */
> >       list_for_each_entry(entity, &dev->entities, list) {
> >               for (i = 0; i < entity->ncontrols; ++i) {
> > diff --git a/drivers/media/usb/uvc/uvc_status.c b/drivers/media/usb/uvc/uvc_status.c
> > index a78a88c710e2..0208612a9f12 100644
> > --- a/drivers/media/usb/uvc/uvc_status.c
> > +++ b/drivers/media/usb/uvc/uvc_status.c
> > @@ -292,7 +292,7 @@ int uvc_status_init(struct uvc_device *dev)
> >
> >  void uvc_status_unregister(struct uvc_device *dev)
> >  {
> > -     usb_kill_urb(dev->int_urb);
> > +     uvc_status_stop(dev);
>
> Sort of feels like this needs dev->lock somewhere here. Should we move 3/3
> to the head of the series?
>
> The question is, can this be called in parallel with uvc_v4l2_release(),
> for instance?

I can be called in parallel with uvc_v4l2_release(), but
uvc_status_stop() is thread-safe and does not take any locks after:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=619d9b710cf06f7a00a17120ca92333684ac45a8

So this "should" be good. key-word here is should :P


>
> >       uvc_input_unregister(dev);
> >  }



-- 
Ricardo Ribalda

Re: [PATCH v3 2/3] media: uvcvideo: Do not halt the device after disconnect

[Sergey Senozhatsky]

On (23/11/21 19:53), Ricardo Ribalda wrote:
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index 08fcd2ffa727..413c32867617 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -2257,6 +2257,8 @@ static void uvc_disconnect(struct usb_interface *intf)
>  		return;
>  
>  	uvc_unregister_video(dev);
> +	/* Barrier needed to synchronize with uvc_video_stop_streaming(). */
> +	smp_store_release(&dev->disconnected, true);
>  	kref_put(&dev->ref, uvc_delete);
>  }

[..]

> +void uvc_video_stop_streaming(struct uvc_streaming *stream)
> +{
> +	uvc_video_stop_transfer(stream, 1);
> +
> +	/*
> +	 * Barrier needed to synchronize with uvc_disconnect().
> +	 * We cannot call usb_* functions on a disconnected USB device.
> +	 */
> +	if (!smp_load_acquire(&stream->dev->disconnected))
> +		uvc_video_halt(stream);
> +
>  	uvc_video_clock_cleanup(stream);
>  }

Can the following happen?

CPU0                                            CPU1
 uvc_disconnect()
						uvc_video_stop_streaming()
 usb_set_intfdata()
 uvc_unregister_video()

						if (!smp_load(&dev->disconnected))
							uvc_video_halt()

 smp_store_release(&dev->disconnected, true);

 kref_put(&dev->ref, uvc_delete);

Re: [PATCH v3 2/3] media: uvcvideo: Do not halt the device after disconnect

[Sergey Senozhatsky]

On (23/11/22 16:47), Sergey Senozhatsky wrote:
> Can the following happen?

Consider the following case (when CPU1 experienced a delay, a preemption
or anything):

> CPU0                                            CPU1
>  uvc_disconnect()
> 						uvc_video_stop_streaming()
>  usb_set_intfdata()
>  uvc_unregister_video()
> 
> 						if (!smp_load(&dev->disconnected))
> 
>  smp_store_release(&dev->disconnected, true);
> 
>  kref_put(&dev->ref, uvc_delete);

> 							uvc_video_halt()

That uvc_video_halt() cannot be legal, right?

Re: [PATCH v3 3/3] media: uvcvideo: Lock video streams and queues while unregistering

[Sergey Senozhatsky]

On (23/11/21 19:53), Ricardo Ribalda wrote:
> The call to uvc_disconnect() is not protected by any mutex.
> This means it can and will be called while other accesses to the video
> device are in progress. This can cause all kinds of race conditions,
> including crashes such as the following.
> 
[..]
> 
> Call Trace:
>  usb_hcd_alloc_bandwidth+0x1ee/0x30f
>  usb_set_interface+0x1a3/0x2b7
>  uvc_video_start_transfer+0x29b/0x4b8 [uvcvideo]
>  uvc_video_start_streaming+0x91/0xdd [uvcvideo]
>  uvc_start_streaming+0x28/0x5d [uvcvideo]
>  vb2_start_streaming+0x61/0x143 [videobuf2_common]
>  vb2_core_streamon+0xf7/0x10f [videobuf2_common]
>  uvc_queue_streamon+0x2e/0x41 [uvcvideo]
>  uvc_ioctl_streamon+0x42/0x5c [uvcvideo]
>  __video_do_ioctl+0x33d/0x42a
>  video_usercopy+0x34e/0x5ff
>  ? video_ioctl2+0x16/0x16
>  v4l2_ioctl+0x46/0x53
>  do_vfs_ioctl+0x50a/0x76f
>  ksys_ioctl+0x58/0x83
>  __x64_sys_ioctl+0x1a/0x1e
>  do_syscall_64+0x54/0xde
> 
> usb_set_interface() should not be called after the USB device has been
> unregistered. However, in the above case the disconnect happened after
> v4l2_ioctl() was called, but before the call to usb_ifnum_to_if().
> 
> Acquire various mutexes in uvc_unregister_video() to fix the majority
> (maybe all) of the observed race conditions.
> 
> The uvc_device lock prevents races against suspend and resume calls
> and the poll function.
> 
> The uvc_streaming lock prevents races against stream related functions;
> for the most part, those are ioctls. This lock also requires other
> functions using this lock to check if a video device is still registered
> after acquiring it. For example, it was observed that the video device
> was already unregistered by the time the stream lock was acquired in
> uvc_ioctl_streamon().
> 
> The uvc_queue lock prevents races against queue functions, Most of
> those are already protected by the uvc_streaming lock, but some
> are called directly. This is done as added protection; an actual race
> was not (yet) observed.
> 
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Alan Stern <stern@rowland.harvard.edu>
> Cc: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> Reviewed-by: Tomasz Figa <tfiga@chromium.org>
> Reviewed-by: Sean Paul <seanpaul@chromium.org>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>

Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org>

Re: [PATCH v3 1/3] media: uvcvideo: Always use uvc_status_stop()

[Ricardo Ribalda]

Hi Sergey

On Wed, 22 Nov 2023 at 08:35, Ricardo Ribalda <ribalda@chromium.org> wrote:
>
> Hi Sergey
>
> On Wed, 22 Nov 2023 at 08:21, Sergey Senozhatsky
> <senozhatsky@chromium.org> wrote:
> >
> > On (23/11/21 19:53), Ricardo Ribalda wrote:
> > > uvc_status_stop() handles properly the race conditions with the
> > > asynchronous worker.
> > > Let's use uvc_status_stop() for all the code paths that require stopping
> > > it.
> > >
> > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > ---
> > >  drivers/media/usb/uvc/uvc_ctrl.c   | 4 ----
> > >  drivers/media/usb/uvc/uvc_status.c | 2 +-
> > >  2 files changed, 1 insertion(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> > > index e59a463c2761..8e22a07e3e7b 100644
> > > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > > @@ -2765,10 +2765,6 @@ void uvc_ctrl_cleanup_device(struct uvc_device *dev)
> > >       struct uvc_entity *entity;
> > >       unsigned int i;
> > >
> > > -     /* Can be uninitialized if we are aborting on probe error. */
> > > -     if (dev->async_ctrl.work.func)
> > > -             cancel_work_sync(&dev->async_ctrl.work);
> > > -
> > >       /* Free controls and control mappings for all entities. */
> > >       list_for_each_entry(entity, &dev->entities, list) {
> > >               for (i = 0; i < entity->ncontrols; ++i) {
> > > diff --git a/drivers/media/usb/uvc/uvc_status.c b/drivers/media/usb/uvc/uvc_status.c
> > > index a78a88c710e2..0208612a9f12 100644
> > > --- a/drivers/media/usb/uvc/uvc_status.c
> > > +++ b/drivers/media/usb/uvc/uvc_status.c
> > > @@ -292,7 +292,7 @@ int uvc_status_init(struct uvc_device *dev)
> > >
> > >  void uvc_status_unregister(struct uvc_device *dev)
> > >  {
> > > -     usb_kill_urb(dev->int_urb);
> > > +     uvc_status_stop(dev);
> >
> > Sort of feels like this needs dev->lock somewhere here. Should we move 3/3
> > to the head of the series?
> >
> > The question is, can this be called in parallel with uvc_v4l2_release(),
> > for instance?
>
> I can be called in parallel with uvc_v4l2_release(), but
> uvc_status_stop() is thread-safe and does not take any locks after:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=619d9b710cf06f7a00a17120ca92333684ac45a8
>
> So this "should" be good. key-word here is should :P

To be on the safe side I am not going to run the async work on the
release path. will send a new revision

>
>
> >
> > >       uvc_input_unregister(dev);
> > >  }
>
>
>
> --
> Ricardo Ribalda



-- 
Ricardo Ribalda

Re: [PATCH v3 2/3] media: uvcvideo: Do not halt the device after disconnect

[Ricardo Ribalda]

Hi Sergey

On Wed, 22 Nov 2023 at 09:01, Sergey Senozhatsky
<senozhatsky@chromium.org> wrote:
>
> On (23/11/22 16:47), Sergey Senozhatsky wrote:
> > Can the following happen?
>
> Consider the following case (when CPU1 experienced a delay, a preemption
> or anything):
>
> > CPU0                                            CPU1
> >  uvc_disconnect()
> >                                               uvc_video_stop_streaming()
> >  usb_set_intfdata()
> >  uvc_unregister_video()
> >
> >                                               if (!smp_load(&dev->disconnected))
> >
> >  smp_store_release(&dev->disconnected, true);
> >
> >  kref_put(&dev->ref, uvc_delete);
>
> >                                                       uvc_video_halt()
>
> That uvc_video_halt() cannot be legal, right?

This patch only takes care of calls to uvc_video_stop_streaming()
after .disconnect.

Guenter's patch from this series should take care of the concurrent
calls. I will resend making it explicit.

Thanks!


-- 
Ricardo Ribalda

Re: [PATCH v3 1/3] media: uvcvideo: Always use uvc_status_stop()

[Sakari Ailus]

Hi Ricardo,

On Tue, Nov 21, 2023 at 07:53:48PM +0000, Ricardo Ribalda wrote:
> uvc_status_stop() handles properly the race conditions with the
> asynchronous worker.
> Let's use uvc_status_stop() for all the code paths that require stopping
> it.
> 
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>

Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>

-- 
Sakari Ailus

Re: [PATCH v3 3/3] media: uvcvideo: Lock video streams and queues while unregistering

[Sakari Ailus]

Hi Ricardo,

On Tue, Nov 21, 2023 at 07:53:50PM +0000, Ricardo Ribalda wrote:
> From: Guenter Roeck <linux@roeck-us.net>
> 
> The call to uvc_disconnect() is not protected by any mutex.
> This means it can and will be called while other accesses to the video
> device are in progress. This can cause all kinds of race conditions,
> including crashes such as the following.
> 
> usb 1-4: USB disconnect, device number 3
> BUG: unable to handle kernel NULL pointer dereference at 0000000000000000
> PGD 0 P4D 0
> Oops: 0000 [#1] PREEMPT SMP PTI
> CPU: 0 PID: 5633 Comm: V4L2CaptureThre Not tainted 4.19.113-08536-g5d29ca36db06 #1
> Hardware name: GOOGLE Edgar, BIOS Google_Edgar.7287.167.156 03/25/2019
> RIP: 0010:usb_ifnum_to_if+0x29/0x40
> Code: <...>
> RSP: 0018:ffffa46f42a47a80 EFLAGS: 00010246
> RAX: 0000000000000000 RBX: 0000000000000000 RCX: ffff904a396c9000
> RDX: ffff904a39641320 RSI: 0000000000000001 RDI: 0000000000000000
> RBP: ffffa46f42a47a80 R08: 0000000000000002 R09: 0000000000000000
> R10: 0000000000009975 R11: 0000000000000009 R12: 0000000000000000
> R13: ffff904a396b3800 R14: ffff904a39e88000 R15: 0000000000000000
> FS: 00007f396448e700(0000) GS:ffff904a3ba00000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 0000000000000000 CR3: 000000016cb46000 CR4: 00000000001006f0
> Call Trace:
>  usb_hcd_alloc_bandwidth+0x1ee/0x30f
>  usb_set_interface+0x1a3/0x2b7
>  uvc_video_start_transfer+0x29b/0x4b8 [uvcvideo]
>  uvc_video_start_streaming+0x91/0xdd [uvcvideo]
>  uvc_start_streaming+0x28/0x5d [uvcvideo]
>  vb2_start_streaming+0x61/0x143 [videobuf2_common]
>  vb2_core_streamon+0xf7/0x10f [videobuf2_common]
>  uvc_queue_streamon+0x2e/0x41 [uvcvideo]
>  uvc_ioctl_streamon+0x42/0x5c [uvcvideo]
>  __video_do_ioctl+0x33d/0x42a
>  video_usercopy+0x34e/0x5ff
>  ? video_ioctl2+0x16/0x16
>  v4l2_ioctl+0x46/0x53
>  do_vfs_ioctl+0x50a/0x76f
>  ksys_ioctl+0x58/0x83
>  __x64_sys_ioctl+0x1a/0x1e
>  do_syscall_64+0x54/0xde
> 
> usb_set_interface() should not be called after the USB device has been
> unregistered. However, in the above case the disconnect happened after
> v4l2_ioctl() was called, but before the call to usb_ifnum_to_if().
> 
> Acquire various mutexes in uvc_unregister_video() to fix the majority
> (maybe all) of the observed race conditions.
> 
> The uvc_device lock prevents races against suspend and resume calls
> and the poll function.
> 
> The uvc_streaming lock prevents races against stream related functions;
> for the most part, those are ioctls. This lock also requires other
> functions using this lock to check if a video device is still registered
> after acquiring it. For example, it was observed that the video device
> was already unregistered by the time the stream lock was acquired in
> uvc_ioctl_streamon().
> 
> The uvc_queue lock prevents races against queue functions, Most of
> those are already protected by the uvc_streaming lock, but some
> are called directly. This is done as added protection; an actual race
> was not (yet) observed.
> 
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Alan Stern <stern@rowland.harvard.edu>
> Cc: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> Reviewed-by: Tomasz Figa <tfiga@chromium.org>
> Reviewed-by: Sean Paul <seanpaul@chromium.org>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_driver.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index 413c32867617..3408b865d346 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -1907,14 +1907,22 @@ static void uvc_unregister_video(struct uvc_device *dev)
>  {
>  	struct uvc_streaming *stream;
>  
> +	mutex_lock(&dev->lock);
> +
>  	list_for_each_entry(stream, &dev->streams, list) {
>  		if (!video_is_registered(&stream->vdev))
>  			continue;
>  
> +		mutex_lock(&stream->mutex);
> +		mutex_lock(&stream->queue.mutex);
> +
>  		video_unregister_device(&stream->vdev);
>  		video_unregister_device(&stream->meta.vdev);
>  
>  		uvc_debugfs_cleanup_stream(stream);
> +
> +		mutex_unlock(&stream->queue.mutex);
> +		mutex_unlock(&stream->mutex);
>  	}
>  
>  	uvc_status_unregister(dev);
> @@ -1925,6 +1933,7 @@ static void uvc_unregister_video(struct uvc_device *dev)
>  	if (media_devnode_is_registered(dev->mdev.devnode))
>  		media_device_unregister(&dev->mdev);
>  #endif
> +	mutex_unlock(&dev->lock);
>  }
>  
>  int uvc_register_video_device(struct uvc_device *dev,
> 

Instead of acquiring all the possible locks, could you instead take the
same approach as you do with ISP? It should use refcount_t btw.
<URL:https://chromium.googlesource.com/chromiumos/third_party/kernel/+/refs/heads/kcam-6.1/drivers/isp/isp-device.c:

-- 
Regards,

Sakari Ailus

Re: [PATCH v3 2/3] media: uvcvideo: Do not halt the device after disconnect

[Sakari Ailus]

Hi Ricardo,

Thank you for the patch.

On Tue, Nov 21, 2023 at 07:53:49PM +0000, Ricardo Ribalda wrote:
> usb drivers should not call to any usb_() function after the
> .disconnect() callback has been triggered.
> 
> If the camera is streaming, the uvc driver will call usb_set_interface or
> usb_clear_halt once the device is being released. Let's fix this issue.
> 
> This is probably not the only driver affected with this kind of bug, but
> until there is a better way to do it in the core this is the way to
> solve this issue.
> 
> When/if a different mechanism is implemented in the core to solve the
> lifetime of devices we will adopt it in uvc.
> 
> Trace:
> [ 1065.389723] drivers/media/usb/uvc/uvc_driver.c:2248 uvc_disconnect enter
> [ 1065.390160] drivers/media/usb/uvc/uvc_driver.c:2264 uvc_disconnect exit
> [ 1065.433956] drivers/media/usb/uvc/uvc_v4l2.c:659 uvc_v4l2_release enter
> [ 1065.433973] drivers/media/usb/uvc/uvc_video.c:2274 uvc_video_stop_streaming enter
> [ 1065.434560] drivers/media/usb/uvc/uvc_video.c:2285 uvc_video_stop_streaming exit
> [ 1065.435154] drivers/media/usb/uvc/uvc_v4l2.c:680 uvc_v4l2_release exit
> [ 1065.435188] drivers/media/usb/uvc/uvc_driver.c:2248 uvc_disconnect enter
> 
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_driver.c |  2 ++
>  drivers/media/usb/uvc/uvc_video.c  | 45 ++++++++++++++++++++++++--------------
>  drivers/media/usb/uvc/uvcvideo.h   |  2 ++
>  3 files changed, 32 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index 08fcd2ffa727..413c32867617 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -2257,6 +2257,8 @@ static void uvc_disconnect(struct usb_interface *intf)
>  		return;
>  
>  	uvc_unregister_video(dev);
> +	/* Barrier needed to synchronize with uvc_video_stop_streaming(). */
> +	smp_store_release(&dev->disconnected, true);
>  	kref_put(&dev->ref, uvc_delete);
>  }
>  
> diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> index 28dde08ec6c5..032b44e45b22 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -2243,28 +2243,39 @@ int uvc_video_start_streaming(struct uvc_streaming *stream)
>  	return ret;
>  }
>  
> -void uvc_video_stop_streaming(struct uvc_streaming *stream)
> +static void uvc_video_halt(struct uvc_streaming *stream)
>  {
> -	uvc_video_stop_transfer(stream, 1);
> +	unsigned int epnum;
> +	unsigned int pipe;
> +	unsigned int dir;
>  
>  	if (stream->intf->num_altsetting > 1) {

Doesn't this imply the device is using isochronous mode?

>  		usb_set_interface(stream->dev->udev, stream->intfnum, 0);
> -	} else {
> -		/*
> -		 * UVC doesn't specify how to inform a bulk-based device
> -		 * when the video stream is stopped. Windows sends a
> -		 * CLEAR_FEATURE(HALT) request to the video streaming
> -		 * bulk endpoint, mimic the same behaviour.
> -		 */
> -		unsigned int epnum = stream->header.bEndpointAddress
> -				   & USB_ENDPOINT_NUMBER_MASK;
> -		unsigned int dir = stream->header.bEndpointAddress
> -				 & USB_ENDPOINT_DIR_MASK;
> -		unsigned int pipe;
> -
> -		pipe = usb_sndbulkpipe(stream->dev->udev, epnum) | dir;
> -		usb_clear_halt(stream->dev->udev, pipe);
> +		return;
>  	}
>  
> +	/*
> +	 * UVC doesn't specify how to inform a bulk-based device

Then this comment doesn't look right. What about the code? This isn't
mentioned in the commit message either.

> +	 * when the video stream is stopped. Windows sends a
> +	 * CLEAR_FEATURE(HALT) request to the video streaming
> +	 * bulk endpoint, mimic the same behaviour.
> +	 */
> +	epnum = stream->header.bEndpointAddress & USB_ENDPOINT_NUMBER_MASK;
> +	dir = stream->header.bEndpointAddress & USB_ENDPOINT_DIR_MASK;
> +	pipe = usb_sndbulkpipe(stream->dev->udev, epnum) | dir;
> +	usb_clear_halt(stream->dev->udev, pipe);
> +}
> +
> +void uvc_video_stop_streaming(struct uvc_streaming *stream)
> +{
> +	uvc_video_stop_transfer(stream, 1);
> +
> +	/*
> +	 * Barrier needed to synchronize with uvc_disconnect().
> +	 * We cannot call usb_* functions on a disconnected USB device.
> +	 */
> +	if (!smp_load_acquire(&stream->dev->disconnected))
> +		uvc_video_halt(stream);
> +
>  	uvc_video_clock_cleanup(stream);
>  }
> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> index 6fb0a78b1b00..4318ce8e31db 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -559,6 +559,8 @@ struct uvc_device {
>  	unsigned int users;
>  	atomic_t nmappings;
>  
> +	bool disconnected;
> +
>  	/* Video control interface */
>  #ifdef CONFIG_MEDIA_CONTROLLER
>  	struct media_device mdev;
> 

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v3 2/3] media: uvcvideo: Do not halt the device after disconnect

[Ricardo Ribalda]

Hi Sakari

On Wed, 22 Nov 2023 at 11:25, Sakari Ailus <sakari.ailus@iki.fi> wrote:
>
> Hi Ricardo,
>
> Thank you for the patch.
>
> On Tue, Nov 21, 2023 at 07:53:49PM +0000, Ricardo Ribalda wrote:
> > usb drivers should not call to any usb_() function after the
> > .disconnect() callback has been triggered.
> >
> > If the camera is streaming, the uvc driver will call usb_set_interface or
> > usb_clear_halt once the device is being released. Let's fix this issue.
> >
> > This is probably not the only driver affected with this kind of bug, but
> > until there is a better way to do it in the core this is the way to
> > solve this issue.
> >
> > When/if a different mechanism is implemented in the core to solve the
> > lifetime of devices we will adopt it in uvc.
> >
> > Trace:
> > [ 1065.389723] drivers/media/usb/uvc/uvc_driver.c:2248 uvc_disconnect enter
> > [ 1065.390160] drivers/media/usb/uvc/uvc_driver.c:2264 uvc_disconnect exit
> > [ 1065.433956] drivers/media/usb/uvc/uvc_v4l2.c:659 uvc_v4l2_release enter
> > [ 1065.433973] drivers/media/usb/uvc/uvc_video.c:2274 uvc_video_stop_streaming enter
> > [ 1065.434560] drivers/media/usb/uvc/uvc_video.c:2285 uvc_video_stop_streaming exit
> > [ 1065.435154] drivers/media/usb/uvc/uvc_v4l2.c:680 uvc_v4l2_release exit
> > [ 1065.435188] drivers/media/usb/uvc/uvc_driver.c:2248 uvc_disconnect enter
> >
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > ---
> >  drivers/media/usb/uvc/uvc_driver.c |  2 ++
> >  drivers/media/usb/uvc/uvc_video.c  | 45 ++++++++++++++++++++++++--------------
> >  drivers/media/usb/uvc/uvcvideo.h   |  2 ++
> >  3 files changed, 32 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > index 08fcd2ffa727..413c32867617 100644
> > --- a/drivers/media/usb/uvc/uvc_driver.c
> > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > @@ -2257,6 +2257,8 @@ static void uvc_disconnect(struct usb_interface *intf)
> >               return;
> >
> >       uvc_unregister_video(dev);
> > +     /* Barrier needed to synchronize with uvc_video_stop_streaming(). */
> > +     smp_store_release(&dev->disconnected, true);
> >       kref_put(&dev->ref, uvc_delete);
> >  }
> >
> > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> > index 28dde08ec6c5..032b44e45b22 100644
> > --- a/drivers/media/usb/uvc/uvc_video.c
> > +++ b/drivers/media/usb/uvc/uvc_video.c
> > @@ -2243,28 +2243,39 @@ int uvc_video_start_streaming(struct uvc_streaming *stream)
> >       return ret;
> >  }
> >
> > -void uvc_video_stop_streaming(struct uvc_streaming *stream)
> > +static void uvc_video_halt(struct uvc_streaming *stream)
> >  {
> > -     uvc_video_stop_transfer(stream, 1);
> > +     unsigned int epnum;
> > +     unsigned int pipe;
> > +     unsigned int dir;
> >
> >       if (stream->intf->num_altsetting > 1) {
>
> Doesn't this imply the device is using isochronous mode?

I haven't changed the behaviour for halt, it is just that git diff is
being a bit too creative here:

Basically it is doing:

void video_halt() {
   if (is_isoc()) {
     usb_set_interface(stream->dev->udev, stream->intfnum, 0);
     return;
   }
   usb_clear_halt();
}

instead of the old:

void video_halt() {
   if (is_isoc()) {
     usb_set_interface(stream->dev->udev, stream->intfnum, 0);
   }  else {
      usb_clear_halt();
   }
}

Thanks!
>
> >               usb_set_interface(stream->dev->udev, stream->intfnum, 0);
> > -     } else {
> > -             /*
> > -              * UVC doesn't specify how to inform a bulk-based device
> > -              * when the video stream is stopped. Windows sends a
> > -              * CLEAR_FEATURE(HALT) request to the video streaming
> > -              * bulk endpoint, mimic the same behaviour.
> > -              */
> > -             unsigned int epnum = stream->header.bEndpointAddress
> > -                                & USB_ENDPOINT_NUMBER_MASK;
> > -             unsigned int dir = stream->header.bEndpointAddress
> > -                              & USB_ENDPOINT_DIR_MASK;
> > -             unsigned int pipe;
> > -
> > -             pipe = usb_sndbulkpipe(stream->dev->udev, epnum) | dir;
> > -             usb_clear_halt(stream->dev->udev, pipe);
> > +             return;
> >       }
> >
> > +     /*
> > +      * UVC doesn't specify how to inform a bulk-based device
>
> Then this comment doesn't look right. What about the code? This isn't
> mentioned in the commit message either.
>
> > +      * when the video stream is stopped. Windows sends a
> > +      * CLEAR_FEATURE(HALT) request to the video streaming
> > +      * bulk endpoint, mimic the same behaviour.
> > +      */
> > +     epnum = stream->header.bEndpointAddress & USB_ENDPOINT_NUMBER_MASK;
> > +     dir = stream->header.bEndpointAddress & USB_ENDPOINT_DIR_MASK;
> > +     pipe = usb_sndbulkpipe(stream->dev->udev, epnum) | dir;
> > +     usb_clear_halt(stream->dev->udev, pipe);
> > +}
> > +
> > +void uvc_video_stop_streaming(struct uvc_streaming *stream)
> > +{
> > +     uvc_video_stop_transfer(stream, 1);
> > +
> > +     /*
> > +      * Barrier needed to synchronize with uvc_disconnect().
> > +      * We cannot call usb_* functions on a disconnected USB device.
> > +      */
> > +     if (!smp_load_acquire(&stream->dev->disconnected))
> > +             uvc_video_halt(stream);
> > +
> >       uvc_video_clock_cleanup(stream);
> >  }
> > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> > index 6fb0a78b1b00..4318ce8e31db 100644
> > --- a/drivers/media/usb/uvc/uvcvideo.h
> > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > @@ -559,6 +559,8 @@ struct uvc_device {
> >       unsigned int users;
> >       atomic_t nmappings;
> >
> > +     bool disconnected;
> > +
> >       /* Video control interface */
> >  #ifdef CONFIG_MEDIA_CONTROLLER
> >       struct media_device mdev;
> >
>
> --
> Kind regards,
>
> Sakari Ailus



-- 
Ricardo Ribalda

Re: [PATCH v3 1/3] media: uvcvideo: Always use uvc_status_stop()

[Ricardo Ribalda]

Hi Sakari

On Wed, 22 Nov 2023 at 10:58, Sakari Ailus <sakari.ailus@iki.fi> wrote:
>
> Hi Ricardo,
>
> On Tue, Nov 21, 2023 at 07:53:48PM +0000, Ricardo Ribalda wrote:
> > uvc_status_stop() handles properly the race conditions with the
> > asynchronous worker.
> > Let's use uvc_status_stop() for all the code paths that require stopping
> > it.
> >
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
>
> Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Thanks! I have slightly changed the code in v3 and kept your tag, hope
that it is fine.

Regards!

>
> --
> Sakari Ailus



-- 
Ricardo Ribalda

Re: [PATCH v3 2/3] media: uvcvideo: Do not halt the device after disconnect

[Sakari Ailus]

Hi Ricardo,

On Wed, Nov 22, 2023 at 11:32:16AM +0100, Ricardo Ribalda wrote:
> Hi Sakari
> 
> On Wed, 22 Nov 2023 at 11:25, Sakari Ailus <sakari.ailus@iki.fi> wrote:
> >
> > Hi Ricardo,
> >
> > Thank you for the patch.
> >
> > On Tue, Nov 21, 2023 at 07:53:49PM +0000, Ricardo Ribalda wrote:
> > > usb drivers should not call to any usb_() function after the
> > > .disconnect() callback has been triggered.
> > >
> > > If the camera is streaming, the uvc driver will call usb_set_interface or
> > > usb_clear_halt once the device is being released. Let's fix this issue.
> > >
> > > This is probably not the only driver affected with this kind of bug, but
> > > until there is a better way to do it in the core this is the way to
> > > solve this issue.
> > >
> > > When/if a different mechanism is implemented in the core to solve the
> > > lifetime of devices we will adopt it in uvc.
> > >
> > > Trace:
> > > [ 1065.389723] drivers/media/usb/uvc/uvc_driver.c:2248 uvc_disconnect enter
> > > [ 1065.390160] drivers/media/usb/uvc/uvc_driver.c:2264 uvc_disconnect exit
> > > [ 1065.433956] drivers/media/usb/uvc/uvc_v4l2.c:659 uvc_v4l2_release enter
> > > [ 1065.433973] drivers/media/usb/uvc/uvc_video.c:2274 uvc_video_stop_streaming enter
> > > [ 1065.434560] drivers/media/usb/uvc/uvc_video.c:2285 uvc_video_stop_streaming exit
> > > [ 1065.435154] drivers/media/usb/uvc/uvc_v4l2.c:680 uvc_v4l2_release exit
> > > [ 1065.435188] drivers/media/usb/uvc/uvc_driver.c:2248 uvc_disconnect enter
> > >
> > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > ---
> > >  drivers/media/usb/uvc/uvc_driver.c |  2 ++
> > >  drivers/media/usb/uvc/uvc_video.c  | 45 ++++++++++++++++++++++++--------------
> > >  drivers/media/usb/uvc/uvcvideo.h   |  2 ++
> > >  3 files changed, 32 insertions(+), 17 deletions(-)
> > >
> > > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > > index 08fcd2ffa727..413c32867617 100644
> > > --- a/drivers/media/usb/uvc/uvc_driver.c
> > > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > > @@ -2257,6 +2257,8 @@ static void uvc_disconnect(struct usb_interface *intf)
> > >               return;
> > >
> > >       uvc_unregister_video(dev);
> > > +     /* Barrier needed to synchronize with uvc_video_stop_streaming(). */
> > > +     smp_store_release(&dev->disconnected, true);
> > >       kref_put(&dev->ref, uvc_delete);
> > >  }
> > >
> > > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> > > index 28dde08ec6c5..032b44e45b22 100644
> > > --- a/drivers/media/usb/uvc/uvc_video.c
> > > +++ b/drivers/media/usb/uvc/uvc_video.c
> > > @@ -2243,28 +2243,39 @@ int uvc_video_start_streaming(struct uvc_streaming *stream)
> > >       return ret;
> > >  }
> > >
> > > -void uvc_video_stop_streaming(struct uvc_streaming *stream)
> > > +static void uvc_video_halt(struct uvc_streaming *stream)
> > >  {
> > > -     uvc_video_stop_transfer(stream, 1);
> > > +     unsigned int epnum;
> > > +     unsigned int pipe;
> > > +     unsigned int dir;
> > >
> > >       if (stream->intf->num_altsetting > 1) {
> >
> > Doesn't this imply the device is using isochronous mode?
> 
> I haven't changed the behaviour for halt, it is just that git diff is
> being a bit too creative here:
> 
> Basically it is doing:
> 
> void video_halt() {
>    if (is_isoc()) {
>      usb_set_interface(stream->dev->udev, stream->intfnum, 0);
>      return;
>    }
>    usb_clear_halt();
> }
> 
> instead of the old:
> 
> void video_halt() {
>    if (is_isoc()) {
>      usb_set_interface(stream->dev->udev, stream->intfnum, 0);
>    }  else {
>       usb_clear_halt();
>    }
> }
> 
> Thanks!

Oops. I missed the removal of the else branch altogether while reading the
patch.

Shouldn't you also ensure the disconnect callback won't return until the
streaming has been stopped here?

> >
> > >               usb_set_interface(stream->dev->udev, stream->intfnum, 0);
> > > -     } else {
> > > -             /*
> > > -              * UVC doesn't specify how to inform a bulk-based device
> > > -              * when the video stream is stopped. Windows sends a
> > > -              * CLEAR_FEATURE(HALT) request to the video streaming
> > > -              * bulk endpoint, mimic the same behaviour.
> > > -              */
> > > -             unsigned int epnum = stream->header.bEndpointAddress
> > > -                                & USB_ENDPOINT_NUMBER_MASK;
> > > -             unsigned int dir = stream->header.bEndpointAddress
> > > -                              & USB_ENDPOINT_DIR_MASK;
> > > -             unsigned int pipe;
> > > -
> > > -             pipe = usb_sndbulkpipe(stream->dev->udev, epnum) | dir;
> > > -             usb_clear_halt(stream->dev->udev, pipe);
> > > +             return;
> > >       }
> > >
> > > +     /*
> > > +      * UVC doesn't specify how to inform a bulk-based device
> >
> > Then this comment doesn't look right. What about the code? This isn't
> > mentioned in the commit message either.
> >
> > > +      * when the video stream is stopped. Windows sends a
> > > +      * CLEAR_FEATURE(HALT) request to the video streaming
> > > +      * bulk endpoint, mimic the same behaviour.
> > > +      */
> > > +     epnum = stream->header.bEndpointAddress & USB_ENDPOINT_NUMBER_MASK;
> > > +     dir = stream->header.bEndpointAddress & USB_ENDPOINT_DIR_MASK;
> > > +     pipe = usb_sndbulkpipe(stream->dev->udev, epnum) | dir;
> > > +     usb_clear_halt(stream->dev->udev, pipe);
> > > +}
> > > +
> > > +void uvc_video_stop_streaming(struct uvc_streaming *stream)
> > > +{
> > > +     uvc_video_stop_transfer(stream, 1);
> > > +
> > > +     /*
> > > +      * Barrier needed to synchronize with uvc_disconnect().
> > > +      * We cannot call usb_* functions on a disconnected USB device.
> > > +      */
> > > +     if (!smp_load_acquire(&stream->dev->disconnected))
> > > +             uvc_video_halt(stream);
> > > +
> > >       uvc_video_clock_cleanup(stream);
> > >  }
> > > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> > > index 6fb0a78b1b00..4318ce8e31db 100644
> > > --- a/drivers/media/usb/uvc/uvcvideo.h
> > > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > > @@ -559,6 +559,8 @@ struct uvc_device {
> > >       unsigned int users;
> > >       atomic_t nmappings;
> > >
> > > +     bool disconnected;
> > > +
> > >       /* Video control interface */
> > >  #ifdef CONFIG_MEDIA_CONTROLLER
> > >       struct media_device mdev;

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v3 2/3] media: uvcvideo: Do not halt the device after disconnect

[Ricardo Ribalda]

Hi

On Wed, 22 Nov 2023 at 12:04, Sakari Ailus <sakari.ailus@iki.fi> wrote:
>
> Hi Ricardo,
>
> On Wed, Nov 22, 2023 at 11:32:16AM +0100, Ricardo Ribalda wrote:
> > Hi Sakari
> >
> > On Wed, 22 Nov 2023 at 11:25, Sakari Ailus <sakari.ailus@iki.fi> wrote:
> > >
> > > Hi Ricardo,
> > >
> > > Thank you for the patch.
> > >
> > > On Tue, Nov 21, 2023 at 07:53:49PM +0000, Ricardo Ribalda wrote:
> > > > usb drivers should not call to any usb_() function after the
> > > > .disconnect() callback has been triggered.
> > > >
> > > > If the camera is streaming, the uvc driver will call usb_set_interface or
> > > > usb_clear_halt once the device is being released. Let's fix this issue.
> > > >
> > > > This is probably not the only driver affected with this kind of bug, but
> > > > until there is a better way to do it in the core this is the way to
> > > > solve this issue.
> > > >
> > > > When/if a different mechanism is implemented in the core to solve the
> > > > lifetime of devices we will adopt it in uvc.
> > > >
> > > > Trace:
> > > > [ 1065.389723] drivers/media/usb/uvc/uvc_driver.c:2248 uvc_disconnect enter
> > > > [ 1065.390160] drivers/media/usb/uvc/uvc_driver.c:2264 uvc_disconnect exit
> > > > [ 1065.433956] drivers/media/usb/uvc/uvc_v4l2.c:659 uvc_v4l2_release enter
> > > > [ 1065.433973] drivers/media/usb/uvc/uvc_video.c:2274 uvc_video_stop_streaming enter
> > > > [ 1065.434560] drivers/media/usb/uvc/uvc_video.c:2285 uvc_video_stop_streaming exit
> > > > [ 1065.435154] drivers/media/usb/uvc/uvc_v4l2.c:680 uvc_v4l2_release exit
> > > > [ 1065.435188] drivers/media/usb/uvc/uvc_driver.c:2248 uvc_disconnect enter
> > > >
> > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > > ---
> > > >  drivers/media/usb/uvc/uvc_driver.c |  2 ++
> > > >  drivers/media/usb/uvc/uvc_video.c  | 45 ++++++++++++++++++++++++--------------
> > > >  drivers/media/usb/uvc/uvcvideo.h   |  2 ++
> > > >  3 files changed, 32 insertions(+), 17 deletions(-)
> > > >
> > > > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > > > index 08fcd2ffa727..413c32867617 100644
> > > > --- a/drivers/media/usb/uvc/uvc_driver.c
> > > > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > > > @@ -2257,6 +2257,8 @@ static void uvc_disconnect(struct usb_interface *intf)
> > > >               return;
> > > >
> > > >       uvc_unregister_video(dev);
> > > > +     /* Barrier needed to synchronize with uvc_video_stop_streaming(). */
> > > > +     smp_store_release(&dev->disconnected, true);
> > > >       kref_put(&dev->ref, uvc_delete);
> > > >  }
> > > >
> > > > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> > > > index 28dde08ec6c5..032b44e45b22 100644
> > > > --- a/drivers/media/usb/uvc/uvc_video.c
> > > > +++ b/drivers/media/usb/uvc/uvc_video.c
> > > > @@ -2243,28 +2243,39 @@ int uvc_video_start_streaming(struct uvc_streaming *stream)
> > > >       return ret;
> > > >  }
> > > >
> > > > -void uvc_video_stop_streaming(struct uvc_streaming *stream)
> > > > +static void uvc_video_halt(struct uvc_streaming *stream)
> > > >  {
> > > > -     uvc_video_stop_transfer(stream, 1);
> > > > +     unsigned int epnum;
> > > > +     unsigned int pipe;
> > > > +     unsigned int dir;
> > > >
> > > >       if (stream->intf->num_altsetting > 1) {
> > >
> > > Doesn't this imply the device is using isochronous mode?
> >
> > I haven't changed the behaviour for halt, it is just that git diff is
> > being a bit too creative here:
> >
> > Basically it is doing:
> >
> > void video_halt() {
> >    if (is_isoc()) {
> >      usb_set_interface(stream->dev->udev, stream->intfnum, 0);
> >      return;
> >    }
> >    usb_clear_halt();
> > }
> >
> > instead of the old:
> >
> > void video_halt() {
> >    if (is_isoc()) {
> >      usb_set_interface(stream->dev->udev, stream->intfnum, 0);
> >    }  else {
> >       usb_clear_halt();
> >    }
> > }
> >
> > Thanks!
>
> Oops. I missed the removal of the else branch altogether while reading the
> patch.
>
> Shouldn't you also ensure the disconnect callback won't return until the
> streaming has been stopped here?

This patch is just for calls after .disconnect. It will not protect
for concurrent calls.

I have sent a v4 making this explicit. We still need:

media: uvcvideo: Lock video streams and queues while unregistering

or similar.

Thanks!

>
> > >
> > > >               usb_set_interface(stream->dev->udev, stream->intfnum, 0);
> > > > -     } else {
> > > > -             /*
> > > > -              * UVC doesn't specify how to inform a bulk-based device
> > > > -              * when the video stream is stopped. Windows sends a
> > > > -              * CLEAR_FEATURE(HALT) request to the video streaming
> > > > -              * bulk endpoint, mimic the same behaviour.
> > > > -              */
> > > > -             unsigned int epnum = stream->header.bEndpointAddress
> > > > -                                & USB_ENDPOINT_NUMBER_MASK;
> > > > -             unsigned int dir = stream->header.bEndpointAddress
> > > > -                              & USB_ENDPOINT_DIR_MASK;
> > > > -             unsigned int pipe;
> > > > -
> > > > -             pipe = usb_sndbulkpipe(stream->dev->udev, epnum) | dir;
> > > > -             usb_clear_halt(stream->dev->udev, pipe);
> > > > +             return;
> > > >       }
> > > >
> > > > +     /*
> > > > +      * UVC doesn't specify how to inform a bulk-based device
> > >
> > > Then this comment doesn't look right. What about the code? This isn't
> > > mentioned in the commit message either.
> > >
> > > > +      * when the video stream is stopped. Windows sends a
> > > > +      * CLEAR_FEATURE(HALT) request to the video streaming
> > > > +      * bulk endpoint, mimic the same behaviour.
> > > > +      */
> > > > +     epnum = stream->header.bEndpointAddress & USB_ENDPOINT_NUMBER_MASK;
> > > > +     dir = stream->header.bEndpointAddress & USB_ENDPOINT_DIR_MASK;
> > > > +     pipe = usb_sndbulkpipe(stream->dev->udev, epnum) | dir;
> > > > +     usb_clear_halt(stream->dev->udev, pipe);
> > > > +}
> > > > +
> > > > +void uvc_video_stop_streaming(struct uvc_streaming *stream)
> > > > +{
> > > > +     uvc_video_stop_transfer(stream, 1);
> > > > +
> > > > +     /*
> > > > +      * Barrier needed to synchronize with uvc_disconnect().
> > > > +      * We cannot call usb_* functions on a disconnected USB device.
> > > > +      */
> > > > +     if (!smp_load_acquire(&stream->dev->disconnected))
> > > > +             uvc_video_halt(stream);
> > > > +
> > > >       uvc_video_clock_cleanup(stream);
> > > >  }
> > > > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> > > > index 6fb0a78b1b00..4318ce8e31db 100644
> > > > --- a/drivers/media/usb/uvc/uvcvideo.h
> > > > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > > > @@ -559,6 +559,8 @@ struct uvc_device {
> > > >       unsigned int users;
> > > >       atomic_t nmappings;
> > > >
> > > > +     bool disconnected;
> > > > +
> > > >       /* Video control interface */
> > > >  #ifdef CONFIG_MEDIA_CONTROLLER
> > > >       struct media_device mdev;
>
> --
> Kind regards,
>
> Sakari Ailus



-- 
Ricardo Ribalda

Re: [PATCH v3 3/3] media: uvcvideo: Lock video streams and queues while unregistering

[Laurent Pinchart]

CC'ing Dan Williams.

On Wed, Nov 22, 2023 at 10:21:04AM +0000, Sakari Ailus wrote:
> Hi Ricardo,
> 
> On Tue, Nov 21, 2023 at 07:53:50PM +0000, Ricardo Ribalda wrote:
> > From: Guenter Roeck <linux@roeck-us.net>
> > 
> > The call to uvc_disconnect() is not protected by any mutex.
> > This means it can and will be called while other accesses to the video
> > device are in progress. This can cause all kinds of race conditions,
> > including crashes such as the following.
> > 
> > usb 1-4: USB disconnect, device number 3
> > BUG: unable to handle kernel NULL pointer dereference at 0000000000000000
> > PGD 0 P4D 0
> > Oops: 0000 [#1] PREEMPT SMP PTI
> > CPU: 0 PID: 5633 Comm: V4L2CaptureThre Not tainted 4.19.113-08536-g5d29ca36db06 #1
> > Hardware name: GOOGLE Edgar, BIOS Google_Edgar.7287.167.156 03/25/2019
> > RIP: 0010:usb_ifnum_to_if+0x29/0x40
> > Code: <...>
> > RSP: 0018:ffffa46f42a47a80 EFLAGS: 00010246
> > RAX: 0000000000000000 RBX: 0000000000000000 RCX: ffff904a396c9000
> > RDX: ffff904a39641320 RSI: 0000000000000001 RDI: 0000000000000000
> > RBP: ffffa46f42a47a80 R08: 0000000000000002 R09: 0000000000000000
> > R10: 0000000000009975 R11: 0000000000000009 R12: 0000000000000000
> > R13: ffff904a396b3800 R14: ffff904a39e88000 R15: 0000000000000000
> > FS: 00007f396448e700(0000) GS:ffff904a3ba00000(0000) knlGS:0000000000000000
> > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > CR2: 0000000000000000 CR3: 000000016cb46000 CR4: 00000000001006f0
> > Call Trace:
> >  usb_hcd_alloc_bandwidth+0x1ee/0x30f
> >  usb_set_interface+0x1a3/0x2b7
> >  uvc_video_start_transfer+0x29b/0x4b8 [uvcvideo]
> >  uvc_video_start_streaming+0x91/0xdd [uvcvideo]
> >  uvc_start_streaming+0x28/0x5d [uvcvideo]
> >  vb2_start_streaming+0x61/0x143 [videobuf2_common]
> >  vb2_core_streamon+0xf7/0x10f [videobuf2_common]
> >  uvc_queue_streamon+0x2e/0x41 [uvcvideo]
> >  uvc_ioctl_streamon+0x42/0x5c [uvcvideo]
> >  __video_do_ioctl+0x33d/0x42a
> >  video_usercopy+0x34e/0x5ff
> >  ? video_ioctl2+0x16/0x16
> >  v4l2_ioctl+0x46/0x53
> >  do_vfs_ioctl+0x50a/0x76f
> >  ksys_ioctl+0x58/0x83
> >  __x64_sys_ioctl+0x1a/0x1e
> >  do_syscall_64+0x54/0xde
> > 
> > usb_set_interface() should not be called after the USB device has been
> > unregistered. However, in the above case the disconnect happened after
> > v4l2_ioctl() was called, but before the call to usb_ifnum_to_if().
> > 
> > Acquire various mutexes in uvc_unregister_video() to fix the majority
> > (maybe all) of the observed race conditions.
> > 
> > The uvc_device lock prevents races against suspend and resume calls
> > and the poll function.
> > 
> > The uvc_streaming lock prevents races against stream related functions;
> > for the most part, those are ioctls. This lock also requires other
> > functions using this lock to check if a video device is still registered
> > after acquiring it. For example, it was observed that the video device
> > was already unregistered by the time the stream lock was acquired in
> > uvc_ioctl_streamon().
> > 
> > The uvc_queue lock prevents races against queue functions, Most of
> > those are already protected by the uvc_streaming lock, but some
> > are called directly. This is done as added protection; an actual race
> > was not (yet) observed.
> > 
> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Cc: Alan Stern <stern@rowland.harvard.edu>
> > Cc: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> > Reviewed-by: Tomasz Figa <tfiga@chromium.org>
> > Reviewed-by: Sean Paul <seanpaul@chromium.org>
> > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > ---
> >  drivers/media/usb/uvc/uvc_driver.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > index 413c32867617..3408b865d346 100644
> > --- a/drivers/media/usb/uvc/uvc_driver.c
> > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > @@ -1907,14 +1907,22 @@ static void uvc_unregister_video(struct uvc_device *dev)
> >  {
> >  	struct uvc_streaming *stream;
> >  
> > +	mutex_lock(&dev->lock);
> > +
> >  	list_for_each_entry(stream, &dev->streams, list) {
> >  		if (!video_is_registered(&stream->vdev))
> >  			continue;
> >  
> > +		mutex_lock(&stream->mutex);
> > +		mutex_lock(&stream->queue.mutex);
> > +
> >  		video_unregister_device(&stream->vdev);
> >  		video_unregister_device(&stream->meta.vdev);
> >  
> >  		uvc_debugfs_cleanup_stream(stream);
> > +
> > +		mutex_unlock(&stream->queue.mutex);
> > +		mutex_unlock(&stream->mutex);
> >  	}
> >  
> >  	uvc_status_unregister(dev);
> > @@ -1925,6 +1933,7 @@ static void uvc_unregister_video(struct uvc_device *dev)
> >  	if (media_devnode_is_registered(dev->mdev.devnode))
> >  		media_device_unregister(&dev->mdev);
> >  #endif
> > +	mutex_unlock(&dev->lock);
> >  }
> >  
> >  int uvc_register_video_device(struct uvc_device *dev,
> > 
> 
> Instead of acquiring all the possible locks, could you instead take the
> same approach as you do with ISP? It should use refcount_t btw.
> <URL:https://chromium.googlesource.com/chromiumos/third_party/kernel/+/refs/heads/kcam-6.1/drivers/isp/isp-device.c:

The right approach, as I've said countless of times, is to fix this at
the cdev and V4L2 level. Dan Williams has done some ground work on this
a while ago ([1]), and before that I posted an RFC series that
overlapped with Dan's work (with a more naive and less efficient
implementation of refcounting, see [2]).

[1] https://lore.kernel.org/all/161117153776.2853729.6944617921517514510.stgit@dwillia2-desk3.amr.corp.intel.com/
[2] https://lore.kernel.org/linux-media/20171116003349.19235-2-laurent.pinchart+renesas@ideasonboard.com/

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 3/3] media: uvcvideo: Lock video streams and queues while unregistering

[Ricardo Ribalda]

Hi Laurent

On Wed, 22 Nov 2023 at 14:14, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> CC'ing Dan Williams.
>
> On Wed, Nov 22, 2023 at 10:21:04AM +0000, Sakari Ailus wrote:
> > Hi Ricardo,
> >
> > On Tue, Nov 21, 2023 at 07:53:50PM +0000, Ricardo Ribalda wrote:
> > > From: Guenter Roeck <linux@roeck-us.net>
> > >
> > > The call to uvc_disconnect() is not protected by any mutex.
> > > This means it can and will be called while other accesses to the video
> > > device are in progress. This can cause all kinds of race conditions,
> > > including crashes such as the following.
> > >
> > > usb 1-4: USB disconnect, device number 3
> > > BUG: unable to handle kernel NULL pointer dereference at 0000000000000000
> > > PGD 0 P4D 0
> > > Oops: 0000 [#1] PREEMPT SMP PTI
> > > CPU: 0 PID: 5633 Comm: V4L2CaptureThre Not tainted 4.19.113-08536-g5d29ca36db06 #1
> > > Hardware name: GOOGLE Edgar, BIOS Google_Edgar.7287.167.156 03/25/2019
> > > RIP: 0010:usb_ifnum_to_if+0x29/0x40
> > > Code: <...>
> > > RSP: 0018:ffffa46f42a47a80 EFLAGS: 00010246
> > > RAX: 0000000000000000 RBX: 0000000000000000 RCX: ffff904a396c9000
> > > RDX: ffff904a39641320 RSI: 0000000000000001 RDI: 0000000000000000
> > > RBP: ffffa46f42a47a80 R08: 0000000000000002 R09: 0000000000000000
> > > R10: 0000000000009975 R11: 0000000000000009 R12: 0000000000000000
> > > R13: ffff904a396b3800 R14: ffff904a39e88000 R15: 0000000000000000
> > > FS: 00007f396448e700(0000) GS:ffff904a3ba00000(0000) knlGS:0000000000000000
> > > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > > CR2: 0000000000000000 CR3: 000000016cb46000 CR4: 00000000001006f0
> > > Call Trace:
> > >  usb_hcd_alloc_bandwidth+0x1ee/0x30f
> > >  usb_set_interface+0x1a3/0x2b7
> > >  uvc_video_start_transfer+0x29b/0x4b8 [uvcvideo]
> > >  uvc_video_start_streaming+0x91/0xdd [uvcvideo]
> > >  uvc_start_streaming+0x28/0x5d [uvcvideo]
> > >  vb2_start_streaming+0x61/0x143 [videobuf2_common]
> > >  vb2_core_streamon+0xf7/0x10f [videobuf2_common]
> > >  uvc_queue_streamon+0x2e/0x41 [uvcvideo]
> > >  uvc_ioctl_streamon+0x42/0x5c [uvcvideo]
> > >  __video_do_ioctl+0x33d/0x42a
> > >  video_usercopy+0x34e/0x5ff
> > >  ? video_ioctl2+0x16/0x16
> > >  v4l2_ioctl+0x46/0x53
> > >  do_vfs_ioctl+0x50a/0x76f
> > >  ksys_ioctl+0x58/0x83
> > >  __x64_sys_ioctl+0x1a/0x1e
> > >  do_syscall_64+0x54/0xde
> > >
> > > usb_set_interface() should not be called after the USB device has been
> > > unregistered. However, in the above case the disconnect happened after
> > > v4l2_ioctl() was called, but before the call to usb_ifnum_to_if().
> > >
> > > Acquire various mutexes in uvc_unregister_video() to fix the majority
> > > (maybe all) of the observed race conditions.
> > >
> > > The uvc_device lock prevents races against suspend and resume calls
> > > and the poll function.
> > >
> > > The uvc_streaming lock prevents races against stream related functions;
> > > for the most part, those are ioctls. This lock also requires other
> > > functions using this lock to check if a video device is still registered
> > > after acquiring it. For example, it was observed that the video device
> > > was already unregistered by the time the stream lock was acquired in
> > > uvc_ioctl_streamon().
> > >
> > > The uvc_queue lock prevents races against queue functions, Most of
> > > those are already protected by the uvc_streaming lock, but some
> > > are called directly. This is done as added protection; an actual race
> > > was not (yet) observed.
> > >
> > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > Cc: Alan Stern <stern@rowland.harvard.edu>
> > > Cc: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> > > Reviewed-by: Tomasz Figa <tfiga@chromium.org>
> > > Reviewed-by: Sean Paul <seanpaul@chromium.org>
> > > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > ---
> > >  drivers/media/usb/uvc/uvc_driver.c | 9 +++++++++
> > >  1 file changed, 9 insertions(+)
> > >
> > > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > > index 413c32867617..3408b865d346 100644
> > > --- a/drivers/media/usb/uvc/uvc_driver.c
> > > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > > @@ -1907,14 +1907,22 @@ static void uvc_unregister_video(struct uvc_device *dev)
> > >  {
> > >     struct uvc_streaming *stream;
> > >
> > > +   mutex_lock(&dev->lock);
> > > +
> > >     list_for_each_entry(stream, &dev->streams, list) {
> > >             if (!video_is_registered(&stream->vdev))
> > >                     continue;
> > >
> > > +           mutex_lock(&stream->mutex);
> > > +           mutex_lock(&stream->queue.mutex);
> > > +
> > >             video_unregister_device(&stream->vdev);
> > >             video_unregister_device(&stream->meta.vdev);
> > >
> > >             uvc_debugfs_cleanup_stream(stream);
> > > +
> > > +           mutex_unlock(&stream->queue.mutex);
> > > +           mutex_unlock(&stream->mutex);
> > >     }
> > >
> > >     uvc_status_unregister(dev);
> > > @@ -1925,6 +1933,7 @@ static void uvc_unregister_video(struct uvc_device *dev)
> > >     if (media_devnode_is_registered(dev->mdev.devnode))
> > >             media_device_unregister(&dev->mdev);
> > >  #endif
> > > +   mutex_unlock(&dev->lock);
> > >  }
> > >
> > >  int uvc_register_video_device(struct uvc_device *dev,
> > >
> >
> > Instead of acquiring all the possible locks, could you instead take the
> > same approach as you do with ISP? It should use refcount_t btw.
> > <URL:https://chromium.googlesource.com/chromiumos/third_party/kernel/+/refs/heads/kcam-6.1/drivers/isp/isp-device.c:
>
> The right approach, as I've said countless of times, is to fix this at
> the cdev and V4L2 level. Dan Williams has done some ground work on this
> a while ago ([1]), and before that I posted an RFC series that
> overlapped with Dan's work (with a more naive and less efficient
> implementation of refcounting, see [2]).
>
> [1] https://lore.kernel.org/all/161117153776.2853729.6944617921517514510.stgit@dwillia2-desk3.amr.corp.intel.com/
> [2] https://lore.kernel.org/linux-media/20171116003349.19235-2-laurent.pinchart+renesas@ideasonboard.com/

The UVC driver is violating the USB driver API [1] causing crashes and
probably security vulnerabilities. It has to be fixed.

If there was a different way TODAY to do this, I would very happily
implement it differently. But your patchset is 6 years old and Dan's 2
and they have not landed. How many kernel versions is ok to put our
users willingly at risk?

This patch simply serialises unregister_video? How can this patch
affect the viability of your patchset or Dan's patch? If this patch is
not needed in the future we can simply revert it.

When/If there is a better way to implement this, I would very happily
send a follow-up patch to use that framework.

[1] https://www.kernel.org/doc/html/latest/driver-api/usb/callbacks.html#the-disconnect-callback









>
> --
> Regards,
>
> Laurent Pinchart



--
Ricardo Ribalda

Re: [PATCH v3 3/3] media: uvcvideo: Lock video streams and queues while unregistering

[Laurent Pinchart]

Hi Ricardo,

On Wed, Nov 22, 2023 at 02:59:31PM +0100, Ricardo Ribalda wrote:
> On Wed, 22 Nov 2023 at 14:14, Laurent Pinchart wrote:
> > On Wed, Nov 22, 2023 at 10:21:04AM +0000, Sakari Ailus wrote:
> > > On Tue, Nov 21, 2023 at 07:53:50PM +0000, Ricardo Ribalda wrote:
> > > > From: Guenter Roeck <linux@roeck-us.net>
> > > >
> > > > The call to uvc_disconnect() is not protected by any mutex.
> > > > This means it can and will be called while other accesses to the video
> > > > device are in progress. This can cause all kinds of race conditions,
> > > > including crashes such as the following.
> > > >
> > > > usb 1-4: USB disconnect, device number 3
> > > > BUG: unable to handle kernel NULL pointer dereference at 0000000000000000
> > > > PGD 0 P4D 0
> > > > Oops: 0000 [#1] PREEMPT SMP PTI
> > > > CPU: 0 PID: 5633 Comm: V4L2CaptureThre Not tainted 4.19.113-08536-g5d29ca36db06 #1
> > > > Hardware name: GOOGLE Edgar, BIOS Google_Edgar.7287.167.156 03/25/2019
> > > > RIP: 0010:usb_ifnum_to_if+0x29/0x40
> > > > Code: <...>
> > > > RSP: 0018:ffffa46f42a47a80 EFLAGS: 00010246
> > > > RAX: 0000000000000000 RBX: 0000000000000000 RCX: ffff904a396c9000
> > > > RDX: ffff904a39641320 RSI: 0000000000000001 RDI: 0000000000000000
> > > > RBP: ffffa46f42a47a80 R08: 0000000000000002 R09: 0000000000000000
> > > > R10: 0000000000009975 R11: 0000000000000009 R12: 0000000000000000
> > > > R13: ffff904a396b3800 R14: ffff904a39e88000 R15: 0000000000000000
> > > > FS: 00007f396448e700(0000) GS:ffff904a3ba00000(0000) knlGS:0000000000000000
> > > > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > > > CR2: 0000000000000000 CR3: 000000016cb46000 CR4: 00000000001006f0
> > > > Call Trace:
> > > >  usb_hcd_alloc_bandwidth+0x1ee/0x30f
> > > >  usb_set_interface+0x1a3/0x2b7
> > > >  uvc_video_start_transfer+0x29b/0x4b8 [uvcvideo]
> > > >  uvc_video_start_streaming+0x91/0xdd [uvcvideo]
> > > >  uvc_start_streaming+0x28/0x5d [uvcvideo]
> > > >  vb2_start_streaming+0x61/0x143 [videobuf2_common]
> > > >  vb2_core_streamon+0xf7/0x10f [videobuf2_common]
> > > >  uvc_queue_streamon+0x2e/0x41 [uvcvideo]
> > > >  uvc_ioctl_streamon+0x42/0x5c [uvcvideo]
> > > >  __video_do_ioctl+0x33d/0x42a
> > > >  video_usercopy+0x34e/0x5ff
> > > >  ? video_ioctl2+0x16/0x16
> > > >  v4l2_ioctl+0x46/0x53
> > > >  do_vfs_ioctl+0x50a/0x76f
> > > >  ksys_ioctl+0x58/0x83
> > > >  __x64_sys_ioctl+0x1a/0x1e
> > > >  do_syscall_64+0x54/0xde
> > > >
> > > > usb_set_interface() should not be called after the USB device has been
> > > > unregistered. However, in the above case the disconnect happened after
> > > > v4l2_ioctl() was called, but before the call to usb_ifnum_to_if().
> > > >
> > > > Acquire various mutexes in uvc_unregister_video() to fix the majority
> > > > (maybe all) of the observed race conditions.
> > > >
> > > > The uvc_device lock prevents races against suspend and resume calls
> > > > and the poll function.
> > > >
> > > > The uvc_streaming lock prevents races against stream related functions;
> > > > for the most part, those are ioctls. This lock also requires other
> > > > functions using this lock to check if a video device is still registered
> > > > after acquiring it. For example, it was observed that the video device
> > > > was already unregistered by the time the stream lock was acquired in
> > > > uvc_ioctl_streamon().
> > > >
> > > > The uvc_queue lock prevents races against queue functions, Most of
> > > > those are already protected by the uvc_streaming lock, but some
> > > > are called directly. This is done as added protection; an actual race
> > > > was not (yet) observed.
> > > >
> > > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > Cc: Alan Stern <stern@rowland.harvard.edu>
> > > > Cc: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> > > > Reviewed-by: Tomasz Figa <tfiga@chromium.org>
> > > > Reviewed-by: Sean Paul <seanpaul@chromium.org>
> > > > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > > ---
> > > >  drivers/media/usb/uvc/uvc_driver.c | 9 +++++++++
> > > >  1 file changed, 9 insertions(+)
> > > >
> > > > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > > > index 413c32867617..3408b865d346 100644
> > > > --- a/drivers/media/usb/uvc/uvc_driver.c
> > > > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > > > @@ -1907,14 +1907,22 @@ static void uvc_unregister_video(struct uvc_device *dev)
> > > >  {
> > > >     struct uvc_streaming *stream;
> > > >
> > > > +   mutex_lock(&dev->lock);
> > > > +
> > > >     list_for_each_entry(stream, &dev->streams, list) {
> > > >             if (!video_is_registered(&stream->vdev))
> > > >                     continue;
> > > >
> > > > +           mutex_lock(&stream->mutex);
> > > > +           mutex_lock(&stream->queue.mutex);
> > > > +
> > > >             video_unregister_device(&stream->vdev);
> > > >             video_unregister_device(&stream->meta.vdev);
> > > >
> > > >             uvc_debugfs_cleanup_stream(stream);
> > > > +
> > > > +           mutex_unlock(&stream->queue.mutex);
> > > > +           mutex_unlock(&stream->mutex);
> > > >     }
> > > >
> > > >     uvc_status_unregister(dev);
> > > > @@ -1925,6 +1933,7 @@ static void uvc_unregister_video(struct uvc_device *dev)
> > > >     if (media_devnode_is_registered(dev->mdev.devnode))
> > > >             media_device_unregister(&dev->mdev);
> > > >  #endif
> > > > +   mutex_unlock(&dev->lock);
> > > >  }
> > > >
> > > >  int uvc_register_video_device(struct uvc_device *dev,
> > > >
> > >
> > > Instead of acquiring all the possible locks, could you instead take the
> > > same approach as you do with ISP? It should use refcount_t btw.
> > > <URL:https://chromium.googlesource.com/chromiumos/third_party/kernel/+/refs/heads/kcam-6.1/drivers/isp/isp-device.c:
> >
> > The right approach, as I've said countless of times, is to fix this at
> > the cdev and V4L2 level. Dan Williams has done some ground work on this
> > a while ago ([1]), and before that I posted an RFC series that
> > overlapped with Dan's work (with a more naive and less efficient
> > implementation of refcounting, see [2]).
> >
> > [1] https://lore.kernel.org/all/161117153776.2853729.6944617921517514510.stgit@dwillia2-desk3.amr.corp.intel.com/
> > [2] https://lore.kernel.org/linux-media/20171116003349.19235-2-laurent.pinchart+renesas@ideasonboard.com/
> 
> The UVC driver is violating the USB driver API [1] causing crashes and
> probably security vulnerabilities. It has to be fixed.
> 
> If there was a different way TODAY to do this, I would very happily
> implement it differently. But your patchset is 6 years old and Dan's 2
> and they have not landed. How many kernel versions is ok to put our
> users willingly at risk?
> 
> This patch simply serialises unregister_video? How can this patch
> affect the viability of your patchset or Dan's patch? If this patch is
> not needed in the future we can simply revert it.
> 
> When/If there is a better way to implement this, I would very happily
> send a follow-up patch to use that framework.

What I'm asking is that you, or someone else at your time, take over
these patches from Dan and myself and help with the huge backlog we have
in V4L2. You can't expect maintainers to fix everything in their spare
time. Helping there is a good way to lower the pressure on maintainers
and get patches reviewed more quickly.

> [1] https://www.kernel.org/doc/html/latest/driver-api/usb/callbacks.html#the-disconnect-callback

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 3/3] media: uvcvideo: Lock video streams and queues while unregistering

[Ricardo Ribalda]

Hi Laurent

On Wed, 22 Nov 2023 at 15:04, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Ricardo,
>
> On Wed, Nov 22, 2023 at 02:59:31PM +0100, Ricardo Ribalda wrote:
> > On Wed, 22 Nov 2023 at 14:14, Laurent Pinchart wrote:
> > > On Wed, Nov 22, 2023 at 10:21:04AM +0000, Sakari Ailus wrote:
> > > > On Tue, Nov 21, 2023 at 07:53:50PM +0000, Ricardo Ribalda wrote:
> > > > > From: Guenter Roeck <linux@roeck-us.net>
> > > > >
> > > > > The call to uvc_disconnect() is not protected by any mutex.
> > > > > This means it can and will be called while other accesses to the video
> > > > > device are in progress. This can cause all kinds of race conditions,
> > > > > including crashes such as the following.
> > > > >
> > > > > usb 1-4: USB disconnect, device number 3
> > > > > BUG: unable to handle kernel NULL pointer dereference at 0000000000000000
> > > > > PGD 0 P4D 0
> > > > > Oops: 0000 [#1] PREEMPT SMP PTI
> > > > > CPU: 0 PID: 5633 Comm: V4L2CaptureThre Not tainted 4.19.113-08536-g5d29ca36db06 #1
> > > > > Hardware name: GOOGLE Edgar, BIOS Google_Edgar.7287.167.156 03/25/2019
> > > > > RIP: 0010:usb_ifnum_to_if+0x29/0x40
> > > > > Code: <...>
> > > > > RSP: 0018:ffffa46f42a47a80 EFLAGS: 00010246
> > > > > RAX: 0000000000000000 RBX: 0000000000000000 RCX: ffff904a396c9000
> > > > > RDX: ffff904a39641320 RSI: 0000000000000001 RDI: 0000000000000000
> > > > > RBP: ffffa46f42a47a80 R08: 0000000000000002 R09: 0000000000000000
> > > > > R10: 0000000000009975 R11: 0000000000000009 R12: 0000000000000000
> > > > > R13: ffff904a396b3800 R14: ffff904a39e88000 R15: 0000000000000000
> > > > > FS: 00007f396448e700(0000) GS:ffff904a3ba00000(0000) knlGS:0000000000000000
> > > > > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > > > > CR2: 0000000000000000 CR3: 000000016cb46000 CR4: 00000000001006f0
> > > > > Call Trace:
> > > > >  usb_hcd_alloc_bandwidth+0x1ee/0x30f
> > > > >  usb_set_interface+0x1a3/0x2b7
> > > > >  uvc_video_start_transfer+0x29b/0x4b8 [uvcvideo]
> > > > >  uvc_video_start_streaming+0x91/0xdd [uvcvideo]
> > > > >  uvc_start_streaming+0x28/0x5d [uvcvideo]
> > > > >  vb2_start_streaming+0x61/0x143 [videobuf2_common]
> > > > >  vb2_core_streamon+0xf7/0x10f [videobuf2_common]
> > > > >  uvc_queue_streamon+0x2e/0x41 [uvcvideo]
> > > > >  uvc_ioctl_streamon+0x42/0x5c [uvcvideo]
> > > > >  __video_do_ioctl+0x33d/0x42a
> > > > >  video_usercopy+0x34e/0x5ff
> > > > >  ? video_ioctl2+0x16/0x16
> > > > >  v4l2_ioctl+0x46/0x53
> > > > >  do_vfs_ioctl+0x50a/0x76f
> > > > >  ksys_ioctl+0x58/0x83
> > > > >  __x64_sys_ioctl+0x1a/0x1e
> > > > >  do_syscall_64+0x54/0xde
> > > > >
> > > > > usb_set_interface() should not be called after the USB device has been
> > > > > unregistered. However, in the above case the disconnect happened after
> > > > > v4l2_ioctl() was called, but before the call to usb_ifnum_to_if().
> > > > >
> > > > > Acquire various mutexes in uvc_unregister_video() to fix the majority
> > > > > (maybe all) of the observed race conditions.
> > > > >
> > > > > The uvc_device lock prevents races against suspend and resume calls
> > > > > and the poll function.
> > > > >
> > > > > The uvc_streaming lock prevents races against stream related functions;
> > > > > for the most part, those are ioctls. This lock also requires other
> > > > > functions using this lock to check if a video device is still registered
> > > > > after acquiring it. For example, it was observed that the video device
> > > > > was already unregistered by the time the stream lock was acquired in
> > > > > uvc_ioctl_streamon().
> > > > >
> > > > > The uvc_queue lock prevents races against queue functions, Most of
> > > > > those are already protected by the uvc_streaming lock, but some
> > > > > are called directly. This is done as added protection; an actual race
> > > > > was not (yet) observed.
> > > > >
> > > > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > > Cc: Alan Stern <stern@rowland.harvard.edu>
> > > > > Cc: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> > > > > Reviewed-by: Tomasz Figa <tfiga@chromium.org>
> > > > > Reviewed-by: Sean Paul <seanpaul@chromium.org>
> > > > > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > > > ---
> > > > >  drivers/media/usb/uvc/uvc_driver.c | 9 +++++++++
> > > > >  1 file changed, 9 insertions(+)
> > > > >
> > > > > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > > > > index 413c32867617..3408b865d346 100644
> > > > > --- a/drivers/media/usb/uvc/uvc_driver.c
> > > > > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > > > > @@ -1907,14 +1907,22 @@ static void uvc_unregister_video(struct uvc_device *dev)
> > > > >  {
> > > > >     struct uvc_streaming *stream;
> > > > >
> > > > > +   mutex_lock(&dev->lock);
> > > > > +
> > > > >     list_for_each_entry(stream, &dev->streams, list) {
> > > > >             if (!video_is_registered(&stream->vdev))
> > > > >                     continue;
> > > > >
> > > > > +           mutex_lock(&stream->mutex);
> > > > > +           mutex_lock(&stream->queue.mutex);
> > > > > +
> > > > >             video_unregister_device(&stream->vdev);
> > > > >             video_unregister_device(&stream->meta.vdev);
> > > > >
> > > > >             uvc_debugfs_cleanup_stream(stream);
> > > > > +
> > > > > +           mutex_unlock(&stream->queue.mutex);
> > > > > +           mutex_unlock(&stream->mutex);
> > > > >     }
> > > > >
> > > > >     uvc_status_unregister(dev);
> > > > > @@ -1925,6 +1933,7 @@ static void uvc_unregister_video(struct uvc_device *dev)
> > > > >     if (media_devnode_is_registered(dev->mdev.devnode))
> > > > >             media_device_unregister(&dev->mdev);
> > > > >  #endif
> > > > > +   mutex_unlock(&dev->lock);
> > > > >  }
> > > > >
> > > > >  int uvc_register_video_device(struct uvc_device *dev,
> > > > >
> > > >
> > > > Instead of acquiring all the possible locks, could you instead take the
> > > > same approach as you do with ISP? It should use refcount_t btw.
> > > > <URL:https://chromium.googlesource.com/chromiumos/third_party/kernel/+/refs/heads/kcam-6.1/drivers/isp/isp-device.c:
> > >
> > > The right approach, as I've said countless of times, is to fix this at
> > > the cdev and V4L2 level. Dan Williams has done some ground work on this
> > > a while ago ([1]), and before that I posted an RFC series that
> > > overlapped with Dan's work (with a more naive and less efficient
> > > implementation of refcounting, see [2]).
> > >
> > > [1] https://lore.kernel.org/all/161117153776.2853729.6944617921517514510.stgit@dwillia2-desk3.amr.corp.intel.com/
> > > [2] https://lore.kernel.org/linux-media/20171116003349.19235-2-laurent.pinchart+renesas@ideasonboard.com/
> >
> > The UVC driver is violating the USB driver API [1] causing crashes and
> > probably security vulnerabilities. It has to be fixed.
> >
> > If there was a different way TODAY to do this, I would very happily
> > implement it differently. But your patchset is 6 years old and Dan's 2
> > and they have not landed. How many kernel versions is ok to put our
> > users willingly at risk?
> >
> > This patch simply serialises unregister_video? How can this patch
> > affect the viability of your patchset or Dan's patch? If this patch is
> > not needed in the future we can simply revert it.
> >
> > When/If there is a better way to implement this, I would very happily
> > send a follow-up patch to use that framework.
>
> What I'm asking is that you, or someone else at your time, take over
> these patches from Dan and myself and help with the huge backlog we have
> in V4L2. You can't expect maintainers to fix everything in their spare
> time. Helping there is a good way to lower the pressure on maintainers
> and get patches reviewed more quickly.

I do not think that this is a fair comment.
I am already reviewing all the uvc patches in the ML. Most of the time
in my spare time.
How much more help do you need there?

We know for over 4 years that there is a NULL deref issue in the uvc
driver. There is a 10 lines fix for it, that has been both reviewed
and tested.
What else is needed from a driver point of view?

It feels that by your position you are just pressuring to get a global
solution, that is a noble cause. But that is not the responsibility or
the role of a driver maintainer.

>
> > [1] https://www.kernel.org/doc/html/latest/driver-api/usb/callbacks.html#the-disconnect-callback
>
> --
> Regards,
>
> Laurent Pinchart



-- 
Ricardo Ribalda

[PATCH v3 0/3] uvcvideo: Attempt N to land UVC race conditions fixes

[Ricardo Ribalda]

Back in 2020 Guenter published a set of patches to fix some race
conditions on UVC:
https://lore.kernel.org/all/20200917022547.198090-5-linux@roeck-us.net/

That kind of race conditions are not only seen on UVC, but are a common
sin on almost all the kernel, so this is what it was decided back then
that we should try to fix them at higher levels.

After that. A lot of video_is_registered() were added to the core:

```
ribalda@alco:~/work/linux$ git grep is_registered drivers/media/v4l2-core/
drivers/media/v4l2-core/v4l2-compat-ioctl32.c:  if (!video_is_registered(vdev))
drivers/media/v4l2-core/v4l2-dev.c:     if (video_is_registered(vdev))
drivers/media/v4l2-core/v4l2-dev.c:     if (video_is_registered(vdev))
drivers/media/v4l2-core/v4l2-dev.c:     if (video_is_registered(vdev)) {
drivers/media/v4l2-core/v4l2-dev.c:             if (video_is_registered(vdev))
drivers/media/v4l2-core/v4l2-dev.c:     if (!video_is_registered(vdev))
drivers/media/v4l2-core/v4l2-dev.c:     if (video_is_registered(vdev))
drivers/media/v4l2-core/v4l2-dev.c:     if (vdev == NULL || !video_is_registered(vdev)) {
drivers/media/v4l2-core/v4l2-dev.c:             if (video_is_registered(vdev))
drivers/media/v4l2-core/v4l2-dev.c:     if (!vdev || !video_is_registered(vdev))
drivers/media/v4l2-core/v4l2-ioctl.c:   if (!video_is_registered(vfd)) {
drivers/media/v4l2-core/v4l2-subdev.c:  if (video_is_registered(vdev)) {
```

And recently Sakari is trying to land:
https://lore.kernel.org/linux-media/20230201214535.347075-1-sakari.ailus@linux.intel.com/

Which will make obsolete a lot off (all?) of the video_is_registered() checks on
Guenter's patches.

Besides those checks, there were some other valid races fixed on his
patches.

This patchset tries to fix the races still present in our code.

Thanks!

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
Changes in v3: Thanks Hans!
- Stop streaming during uvc_unregister()
- Refactor the uvc_status code
- Link to v2: https://lore.kernel.org/r/20230309-guenter-mini-v2-0-e6410d590d43@chromium.org

Changes in v2:
- Actually send the series to the ML an not only to individuals.
- Link to v1: https://lore.kernel.org/r/20230309-guenter-mini-v1-0-627d10cf6e96@chromium.org

---
Ricardo Ribalda (3):
      media: uvcvideo: stop stream during unregister
      media: uvcvideo: Refactor the status irq API
      media: uvcvideo: Exit early if there is not int_urb

 drivers/media/usb/uvc/uvc_driver.c | 24 ++++++++-------
 drivers/media/usb/uvc/uvc_status.c | 62 +++++++++++++++++++++++++++++++++++---
 drivers/media/usb/uvc/uvc_v4l2.c   | 22 ++++----------
 drivers/media/usb/uvc/uvcvideo.h   | 10 +++---
 4 files changed, 83 insertions(+), 35 deletions(-)
---
base-commit: b14257abe7057def6127f6fb2f14f9adc8acabdb
change-id: 20230309-guenter-mini-89861b084ef1

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