[PATCH v4 0/4] uvcvideo: Attempt N to land UVC race conditions fixes

[PATCH v4 0/4] 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 in UVC:
https://lore.kernel.org/all/20200917022547.198090-5-linux@roeck-us.net/

That kind of race conditions are not only seen in UVC, but are a common
seen in 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 in
Guenter's patches.

Besides those checks, there were some other valid races fixed in 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 v4: Thanks Sergey and Guenter
- Fix typos
- Move location of mutex_init
- Split patch to make the suspend change explicit
- Link to v3: https://lore.kernel.org/r/20240325-guenter-mini-v3-0-c4bc61d84e03@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 (4):
      media: uvcvideo: stop stream during unregister
      media: uvcvideo: Refactor the status irq API
      media: uvcvideo: Avoid race condition during unregister
      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>

[PATCH v4 1/4] media: uvcvideo: stop stream during unregister

[Ricardo Ribalda]

uvc_unregister_video() can be called asynchronously from
uvc_disconnect(). If the device is still streaming when that happens, a
plethora of race conditions can happen.

Make sure that the device has stopped streaming before exiting this
function.

If the user still holds handles to the driver's file descriptors, any
ioctl will return -ENODEV from the v4l2 core.

This change make uvc more consistent with the rest of the v4l2 drivers
using the vb2_fop_* and vb2_ioctl_* helpers.

Suggested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_driver.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index bbd90123a4e76..17fc945c8deb6 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -1911,8 +1911,19 @@ static void uvc_unregister_video(struct uvc_device *dev)
 		if (!video_is_registered(&stream->vdev))
 			continue;
 
+		/*
+		 * Serialize other access to the stream.
+		 */
+		mutex_lock(&stream->mutex);
+		uvc_queue_streamoff(&stream->queue, stream->type);
 		video_unregister_device(&stream->vdev);
 		video_unregister_device(&stream->meta.vdev);
+		mutex_unlock(&stream->mutex);
+
+		/*
+		 * Now the vdev is not streaming and all the ioctls will
+		 * return -ENODEV
+		 */
 
 		uvc_debugfs_cleanup_stream(stream);
 	}

-- 
2.44.0.396.g6e790dbe36-goog

[PATCH v4 2/4] media: uvcvideo: Refactor the status irq API

[Ricardo Ribalda]

There are two different use-cases of uvc_status():
	- adding/removing a user when the camera is open/closed
	- stopping/starting when the camera is suspended/resumed

Make the API reflect these two use-cases and move all the refcounting
and locking logic to the uvc_status.c file.

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_driver.c | 13 ++-------
 drivers/media/usb/uvc/uvc_status.c | 55 ++++++++++++++++++++++++++++++++++++--
 drivers/media/usb/uvc/uvc_v4l2.c   | 22 +++++----------
 drivers/media/usb/uvc/uvcvideo.h   | 10 ++++---
 4 files changed, 67 insertions(+), 33 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index 17fc945c8deb6..b579644ac0745 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -2116,7 +2116,6 @@ static int uvc_probe(struct usb_interface *intf,
 	INIT_LIST_HEAD(&dev->streams);
 	kref_init(&dev->ref);
 	atomic_set(&dev->nmappings, 0);
-	mutex_init(&dev->lock);
 
 	dev->udev = usb_get_dev(udev);
 	dev->intf = usb_get_intf(intf);
@@ -2282,10 +2281,7 @@ static int uvc_suspend(struct usb_interface *intf, pm_message_t message)
 	/* Controls are cached on the fly so they don't need to be saved. */
 	if (intf->cur_altsetting->desc.bInterfaceSubClass ==
 	    UVC_SC_VIDEOCONTROL) {
-		mutex_lock(&dev->lock);
-		if (dev->users)
-			uvc_status_stop(dev);
-		mutex_unlock(&dev->lock);
+		uvc_status_suspend(dev);
 		return 0;
 	}
 
@@ -2316,12 +2312,7 @@ static int __uvc_resume(struct usb_interface *intf, int reset)
 				return ret;
 		}
 
-		mutex_lock(&dev->lock);
-		if (dev->users)
-			ret = uvc_status_start(dev, GFP_NOIO);
-		mutex_unlock(&dev->lock);
-
-		return ret;
+		return uvc_status_resume(dev);
 	}
 
 	list_for_each_entry(stream, &dev->streams, list) {
diff --git a/drivers/media/usb/uvc/uvc_status.c b/drivers/media/usb/uvc/uvc_status.c
index a78a88c710e24..375a95dd30110 100644
--- a/drivers/media/usb/uvc/uvc_status.c
+++ b/drivers/media/usb/uvc/uvc_status.c
@@ -257,6 +257,8 @@ int uvc_status_init(struct uvc_device *dev)
 	unsigned int pipe;
 	int interval;
 
+	mutex_init(&dev->status_lock);
+
 	if (ep == NULL)
 		return 0;
 
@@ -302,18 +304,22 @@ void uvc_status_cleanup(struct uvc_device *dev)
 	kfree(dev->status);
 }
 
-int uvc_status_start(struct uvc_device *dev, gfp_t flags)
+static int __uvc_status_start(struct uvc_device *dev, gfp_t flags)
 {
+	lockdep_assert_held(&dev->status_lock);
+
 	if (dev->int_urb == NULL)
 		return 0;
 
 	return usb_submit_urb(dev->int_urb, flags);
 }
 
-void uvc_status_stop(struct uvc_device *dev)
+static void __uvc_status_stop(struct uvc_device *dev)
 {
 	struct uvc_ctrl_work *w = &dev->async_ctrl;
 
+	lockdep_assert_held(&dev->status_lock);
+
 	/*
 	 * Prevent the asynchronous control handler from requeing the URB. The
 	 * barrier is needed so the flush_status change is visible to other
@@ -350,3 +356,48 @@ void uvc_status_stop(struct uvc_device *dev)
 	 */
 	smp_store_release(&dev->flush_status, false);
 }
+
+int uvc_status_resume(struct uvc_device *dev)
+{
+	int ret = 0;
+
+	mutex_lock(&dev->status_lock);
+	if (dev->status_users)
+		ret = __uvc_status_start(dev, GFP_NOIO);
+	mutex_unlock(&dev->status_lock);
+
+	return ret;
+}
+
+void uvc_status_suspend(struct uvc_device *dev)
+{
+	mutex_lock(&dev->status_lock);
+	if (dev->status_users)
+		__uvc_status_stop(dev);
+	mutex_unlock(&dev->status_lock);
+}
+
+int uvc_status_get(struct uvc_device *dev)
+{
+	int ret = 0;
+
+	mutex_lock(&dev->status_lock);
+	if (!dev->status_users)
+		ret = __uvc_status_start(dev, GFP_KERNEL);
+	if (!ret)
+		dev->status_users++;
+	mutex_unlock(&dev->status_lock);
+
+	return ret;
+}
+
+void uvc_status_put(struct uvc_device *dev)
+{
+	mutex_lock(&dev->status_lock);
+	if (dev->status_users == 1)
+		__uvc_status_stop(dev);
+	WARN_ON(!dev->status_users);
+	if (dev->status_users)
+		dev->status_users--;
+	mutex_unlock(&dev->status_lock);
+}
diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
index f4988f03640ae..97c5407f66032 100644
--- a/drivers/media/usb/uvc/uvc_v4l2.c
+++ b/drivers/media/usb/uvc/uvc_v4l2.c
@@ -628,20 +628,13 @@ static int uvc_v4l2_open(struct file *file)
 		return -ENOMEM;
 	}
 
-	mutex_lock(&stream->dev->lock);
-	if (stream->dev->users == 0) {
-		ret = uvc_status_start(stream->dev, GFP_KERNEL);
-		if (ret < 0) {
-			mutex_unlock(&stream->dev->lock);
-			usb_autopm_put_interface(stream->dev->intf);
-			kfree(handle);
-			return ret;
-		}
+	ret = uvc_status_get(stream->dev);
+	if (ret) {
+		usb_autopm_put_interface(stream->dev->intf);
+		kfree(handle);
+		return ret;
 	}
 
-	stream->dev->users++;
-	mutex_unlock(&stream->dev->lock);
-
 	v4l2_fh_init(&handle->vfh, &stream->vdev);
 	v4l2_fh_add(&handle->vfh);
 	handle->chain = stream->chain;
@@ -670,10 +663,7 @@ static int uvc_v4l2_release(struct file *file)
 	kfree(handle);
 	file->private_data = NULL;
 
-	mutex_lock(&stream->dev->lock);
-	if (--stream->dev->users == 0)
-		uvc_status_stop(stream->dev);
-	mutex_unlock(&stream->dev->lock);
+	uvc_status_put(stream->dev);
 
 	usb_autopm_put_interface(stream->dev->intf);
 	return 0;
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index 6fb0a78b1b009..00b600eb058cc 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -555,8 +555,6 @@ struct uvc_device {
 
 	const struct uvc_device_info *info;
 
-	struct mutex lock;		/* Protects users */
-	unsigned int users;
 	atomic_t nmappings;
 
 	/* Video control interface */
@@ -578,6 +576,8 @@ struct uvc_device {
 	struct usb_host_endpoint *int_ep;
 	struct urb *int_urb;
 	struct uvc_status *status;
+	struct mutex status_lock; /* Protects status_users */
+	unsigned int status_users;
 	bool flush_status;
 
 	struct input_dev *input;
@@ -744,8 +744,10 @@ int uvc_register_video_device(struct uvc_device *dev,
 int uvc_status_init(struct uvc_device *dev);
 void uvc_status_unregister(struct uvc_device *dev);
 void uvc_status_cleanup(struct uvc_device *dev);
-int uvc_status_start(struct uvc_device *dev, gfp_t flags);
-void uvc_status_stop(struct uvc_device *dev);
+int uvc_status_resume(struct uvc_device *dev);
+void uvc_status_suspend(struct uvc_device *dev);
+int uvc_status_get(struct uvc_device *dev);
+void uvc_status_put(struct uvc_device *dev);
 
 /* Controls */
 extern const struct uvc_control_mapping uvc_ctrl_power_line_mapping_limited;

-- 
2.44.0.396.g6e790dbe36-goog

[PATCH v4 3/4] media: uvcvideo: Avoid race condition during unregister

[Ricardo Ribalda]

The control events are handled asynchronously by the driver. Once the
control event are handled, the urb is re-submitted.

If we simply kill the urb, there is a chance that a control event is
waiting to be processed, which will re-submit the urb after the device is
disconnected.

uvc_status_suspend() flushes the async controls and stops the urb in a
correct manner.

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

diff --git a/drivers/media/usb/uvc/uvc_status.c b/drivers/media/usb/uvc/uvc_status.c
index 375a95dd30110..8fd8250110e2f 100644
--- a/drivers/media/usb/uvc/uvc_status.c
+++ b/drivers/media/usb/uvc/uvc_status.c
@@ -294,7 +294,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_suspend(dev);
 	uvc_input_unregister(dev);
 }
 

-- 
2.44.0.396.g6e790dbe36-goog

[PATCH v4 4/4] media: uvcvideo: Exit early if there is not int_urb

[Ricardo Ribalda]

If there is no int_urb there is no need to do a clean stop.

Also we avoid calling usb_kill_urb(NULL). It is properly handled by the
usb framework, but it is not polite.

Now that we are at it, fix the code style in uvc_status_start() for
consistency.

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_status.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/media/usb/uvc/uvc_status.c b/drivers/media/usb/uvc/uvc_status.c
index 8fd8250110e2f..9108522beea6a 100644
--- a/drivers/media/usb/uvc/uvc_status.c
+++ b/drivers/media/usb/uvc/uvc_status.c
@@ -308,7 +308,7 @@ static int __uvc_status_start(struct uvc_device *dev, gfp_t flags)
 {
 	lockdep_assert_held(&dev->status_lock);
 
-	if (dev->int_urb == NULL)
+	if (!dev->int_urb)
 		return 0;
 
 	return usb_submit_urb(dev->int_urb, flags);
@@ -320,6 +320,9 @@ static void __uvc_status_stop(struct uvc_device *dev)
 
 	lockdep_assert_held(&dev->status_lock);
 
+	if (!dev->int_urb)
+		return;
+
 	/*
 	 * Prevent the asynchronous control handler from requeing the URB. The
 	 * barrier is needed so the flush_status change is visible to other

-- 
2.44.0.396.g6e790dbe36-goog

Re: [PATCH v4 0/4] uvcvideo: Attempt N to land UVC race conditions fixes

[Sergey Senozhatsky]

On (24/03/27 08:24), Ricardo Ribalda wrote:
> Back in 2020 Guenter published a set of patches to fix some race
> conditions in UVC:
> https://lore.kernel.org/all/20200917022547.198090-5-linux@roeck-us.net/
> 
> That kind of race conditions are not only seen in UVC, but are a common
> seen in 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 in
> Guenter's patches.
> 
> Besides those checks, there were some other valid races fixed in his
> patches.
> 
> This patchset tries to fix the races still present in our code.
> 
> Thanks!
> 
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>

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

Re: [PATCH v4 1/4] media: uvcvideo: stop stream during unregister

[Hans Verkuil]

On 27/03/2024 09:24, Ricardo Ribalda wrote:
> uvc_unregister_video() can be called asynchronously from
> uvc_disconnect(). If the device is still streaming when that happens, a
> plethora of race conditions can happen.
> 
> Make sure that the device has stopped streaming before exiting this
> function.
> 
> If the user still holds handles to the driver's file descriptors, any
> ioctl will return -ENODEV from the v4l2 core.
> 
> This change make uvc more consistent with the rest of the v4l2 drivers
> using the vb2_fop_* and vb2_ioctl_* helpers.
> 
> Suggested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_driver.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index bbd90123a4e76..17fc945c8deb6 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -1911,8 +1911,19 @@ static void uvc_unregister_video(struct uvc_device *dev)
>  		if (!video_is_registered(&stream->vdev))
>  			continue;
>  
> +		/*
> +		 * Serialize other access to the stream.
> +		 */
> +		mutex_lock(&stream->mutex);
> +		uvc_queue_streamoff(&stream->queue, stream->type);
>  		video_unregister_device(&stream->vdev);
>  		video_unregister_device(&stream->meta.vdev);
> +		mutex_unlock(&stream->mutex);

This sequence isn't fool proof. You have to follow the same sequence as
vb2_video_unregister_device(): take a reference to the video device,
then unregister, then take the stream mutex and call vb2_queue_release
for each queue. Finally unlock the mutex and call put_device.

Doing the video_unregister_device first ensures no new ioctls can be
called on that device node. Taking the mutex ensures that any still
running ioctls will finish (since it will sleep until they release the
mutex), and then you can release the queue.

This does require that you call get_device before calling video_unregister_device,
otherwise everything might be released at that point.

Instead of vb2_queue_release() you might have to call uvc_queue_streamoff,
but note that there are two queues here: video and meta. The code above
just calls streamoff for the video device.

For the meta device I think you can just use vb2_video_unregister_device()
directly, since that sets vdev->queue and vdev->queue.lock to the correct
values. That would just leave the video device where you need to do this
manually.

Regards,

	Hans

> +
> +		/*
> +		 * Now the vdev is not streaming and all the ioctls will
> +		 * return -ENODEV
> +		 */
>  
>  		uvc_debugfs_cleanup_stream(stream);
>  	}
> 

Re: [PATCH v4 1/4] media: uvcvideo: stop stream during unregister

[Tomasz Figa]

Hi Ricardo,

On Wed, Mar 27, 2024 at 5:24 PM Ricardo Ribalda <ribalda@chromium.org> wrote:
>
> uvc_unregister_video() can be called asynchronously from
> uvc_disconnect(). If the device is still streaming when that happens, a
> plethora of race conditions can happen.
>
> Make sure that the device has stopped streaming before exiting this
> function.
>
> If the user still holds handles to the driver's file descriptors, any
> ioctl will return -ENODEV from the v4l2 core.
>
> This change make uvc more consistent with the rest of the v4l2 drivers
> using the vb2_fop_* and vb2_ioctl_* helpers.
>
> Suggested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_driver.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>

First of all, thanks for the patch. I have a question about the
problem being fixed here.

Could you point out a specific race condition example that could
happen without this change?
From what I see in __video_do_ioctl((), no ioctls would be executed
anymore after the video node is unregistered.
Since the device is not present either, what asynchronous code paths
could be still triggered?

[1] https://elixir.bootlin.com/linux/latest/source/drivers/media/v4l2-core/v4l2-ioctl.c#L3023

Best regards,
Tomasz

> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index bbd90123a4e76..17fc945c8deb6 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -1911,8 +1911,19 @@ static void uvc_unregister_video(struct uvc_device *dev)
>                 if (!video_is_registered(&stream->vdev))
>                         continue;
>
> +               /*
> +                * Serialize other access to the stream.
> +                */
> +               mutex_lock(&stream->mutex);
> +               uvc_queue_streamoff(&stream->queue, stream->type);
>                 video_unregister_device(&stream->vdev);
>                 video_unregister_device(&stream->meta.vdev);
> +               mutex_unlock(&stream->mutex);
> +
> +               /*
> +                * Now the vdev is not streaming and all the ioctls will
> +                * return -ENODEV
> +                */
>
>                 uvc_debugfs_cleanup_stream(stream);
>         }
>
> --
> 2.44.0.396.g6e790dbe36-goog
>

Re: [PATCH v4 1/4] media: uvcvideo: stop stream during unregister

[Tomasz Figa]

Hi Hans,

On Tue, May 28, 2024 at 4:55 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
>
> On 27/03/2024 09:24, Ricardo Ribalda wrote:
> > uvc_unregister_video() can be called asynchronously from
> > uvc_disconnect(). If the device is still streaming when that happens, a
> > plethora of race conditions can happen.
> >
> > Make sure that the device has stopped streaming before exiting this
> > function.
> >
> > If the user still holds handles to the driver's file descriptors, any
> > ioctl will return -ENODEV from the v4l2 core.
> >
> > This change make uvc more consistent with the rest of the v4l2 drivers
> > using the vb2_fop_* and vb2_ioctl_* helpers.
> >
> > Suggested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > ---
> >  drivers/media/usb/uvc/uvc_driver.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > index bbd90123a4e76..17fc945c8deb6 100644
> > --- a/drivers/media/usb/uvc/uvc_driver.c
> > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > @@ -1911,8 +1911,19 @@ static void uvc_unregister_video(struct uvc_device *dev)
> >               if (!video_is_registered(&stream->vdev))
> >                       continue;
> >
> > +             /*
> > +              * Serialize other access to the stream.
> > +              */
> > +             mutex_lock(&stream->mutex);
> > +             uvc_queue_streamoff(&stream->queue, stream->type);
> >               video_unregister_device(&stream->vdev);
> >               video_unregister_device(&stream->meta.vdev);
> > +             mutex_unlock(&stream->mutex);
>
> This sequence isn't fool proof. You have to follow the same sequence as
> vb2_video_unregister_device(): take a reference to the video device,
> then unregister, then take the stream mutex and call vb2_queue_release
> for each queue. Finally unlock the mutex and call put_device.

vb2_queue_release() will run when the userspace releases the file
handle that owns the vb2 queue [1], so we shouldn't really need to
call it here.

[1] https://elixir.bootlin.com/linux/v6.9.3/source/drivers/media/usb/uvc/uvc_v4l2.c#L655

>
> Doing the video_unregister_device first ensures no new ioctls can be
> called on that device node. Taking the mutex ensures that any still
> running ioctls will finish (since it will sleep until they release the
> mutex), and then you can release the queue.

Actually isn't the only missing thing in the code basically ensuring
that any ioctl currently being executed finishes? Why is the streamoff
or queue release needed?

Best regards,
Tomasz

>
> This does require that you call get_device before calling video_unregister_device,
> otherwise everything might be released at that point.
>
> Instead of vb2_queue_release() you might have to call uvc_queue_streamoff,
> but note that there are two queues here: video and meta. The code above
> just calls streamoff for the video device.
>
> For the meta device I think you can just use vb2_video_unregister_device()
> directly, since that sets vdev->queue and vdev->queue.lock to the correct
> values. That would just leave the video device where you need to do this
> manually.
>
> Regards,
>
>         Hans
>
> > +
> > +             /*
> > +              * Now the vdev is not streaming and all the ioctls will
> > +              * return -ENODEV
> > +              */
> >
> >               uvc_debugfs_cleanup_stream(stream);
> >       }
> >
>

Re: [PATCH v4 1/4] media: uvcvideo: stop stream during unregister

[Hans Verkuil]

On 06/06/2024 12:04, Tomasz Figa wrote:
> Hi Hans,
> 
> On Tue, May 28, 2024 at 4:55 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
>>
>> On 27/03/2024 09:24, Ricardo Ribalda wrote:
>>> uvc_unregister_video() can be called asynchronously from
>>> uvc_disconnect(). If the device is still streaming when that happens, a
>>> plethora of race conditions can happen.
>>>
>>> Make sure that the device has stopped streaming before exiting this
>>> function.
>>>
>>> If the user still holds handles to the driver's file descriptors, any
>>> ioctl will return -ENODEV from the v4l2 core.
>>>
>>> This change make uvc more consistent with the rest of the v4l2 drivers
>>> using the vb2_fop_* and vb2_ioctl_* helpers.
>>>
>>> Suggested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>>> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
>>> ---
>>>  drivers/media/usb/uvc/uvc_driver.c | 11 +++++++++++
>>>  1 file changed, 11 insertions(+)
>>>
>>> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
>>> index bbd90123a4e76..17fc945c8deb6 100644
>>> --- a/drivers/media/usb/uvc/uvc_driver.c
>>> +++ b/drivers/media/usb/uvc/uvc_driver.c
>>> @@ -1911,8 +1911,19 @@ static void uvc_unregister_video(struct uvc_device *dev)
>>>               if (!video_is_registered(&stream->vdev))
>>>                       continue;
>>>
>>> +             /*
>>> +              * Serialize other access to the stream.
>>> +              */
>>> +             mutex_lock(&stream->mutex);
>>> +             uvc_queue_streamoff(&stream->queue, stream->type);
>>>               video_unregister_device(&stream->vdev);
>>>               video_unregister_device(&stream->meta.vdev);
>>> +             mutex_unlock(&stream->mutex);
>>
>> This sequence isn't fool proof. You have to follow the same sequence as
>> vb2_video_unregister_device(): take a reference to the video device,
>> then unregister, then take the stream mutex and call vb2_queue_release
>> for each queue. Finally unlock the mutex and call put_device.
> 
> vb2_queue_release() will run when the userspace releases the file
> handle that owns the vb2 queue [1], so we shouldn't really need to
> call it here.
> 
> [1] https://elixir.bootlin.com/linux/v6.9.3/source/drivers/media/usb/uvc/uvc_v4l2.c#L655
> 
>>
>> Doing the video_unregister_device first ensures no new ioctls can be
>> called on that device node. Taking the mutex ensures that any still
>> running ioctls will finish (since it will sleep until they release the
>> mutex), and then you can release the queue.
> 
> Actually isn't the only missing thing in the code basically ensuring
> that any ioctl currently being executed finishes? Why is the streamoff
> or queue release needed?

See below...

> 
> Best regards,
> Tomasz
> 
>>
>> This does require that you call get_device before calling video_unregister_device,
>> otherwise everything might be released at that point.
>>
>> Instead of vb2_queue_release() you might have to call uvc_queue_streamoff,
>> but note that there are two queues here: video and meta. The code above
>> just calls streamoff for the video device.
>>
>> For the meta device I think you can just use vb2_video_unregister_device()
>> directly, since that sets vdev->queue and vdev->queue.lock to the correct
>> values. That would just leave the video device where you need to do this
>> manually.

Ideally uvc should just use centralized locking (i.e. set vdev->queue.lock)
for the video node, just like it does for the meta device.

If that was the case, then it can just call vb2_video_unregister_device().
The vb2_video_unregister_device helper was added to ensure that no ioctls
are running, and then streaming is stopped and the queue is released.

While it is true that the queue is released automatically when the last user
closes the filehandle, it can get complicated if the application has a file
handle open when the device is unregistered (usually due to it being unplugged).
Without that call to vb2_video_unregister_device() the queue is still active,
and especially if you also have subdevices that are still in streaming mode,
it is hard to make sure nothing is still using the hardware.

vb2_video_unregister_device() provides a clean way of ensuring that when the
device is unregistered all streaming is stopped and the vb2 queue is released.

After that the only file operation that can be used without returning an error
is close().

Since uvc uses its own locking for the video device, it has to do this manually.
It is probably enough to ensure no ioctls are running since uvc doesn't have
subdevices, but I think it makes sense to stop streaming and release the queue
when unregistering the device, it is weird to postpone that until the last fh
is closed.

Regards,

	Hans


>>
>> Regards,
>>
>>         Hans
>>
>>> +
>>> +             /*
>>> +              * Now the vdev is not streaming and all the ioctls will
>>> +              * return -ENODEV
>>> +              */
>>>
>>>               uvc_debugfs_cleanup_stream(stream);
>>>       }
>>>
>>

Re: [PATCH v4 1/4] media: uvcvideo: stop stream during unregister

[Tomasz Figa]

On Thu, Jun 6, 2024 at 8:58 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
>
> On 06/06/2024 12:04, Tomasz Figa wrote:
> > Hi Hans,
> >
> > On Tue, May 28, 2024 at 4:55 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
> >>
> >> On 27/03/2024 09:24, Ricardo Ribalda wrote:
> >>> uvc_unregister_video() can be called asynchronously from
> >>> uvc_disconnect(). If the device is still streaming when that happens, a
> >>> plethora of race conditions can happen.
> >>>
> >>> Make sure that the device has stopped streaming before exiting this
> >>> function.
> >>>
> >>> If the user still holds handles to the driver's file descriptors, any
> >>> ioctl will return -ENODEV from the v4l2 core.
> >>>
> >>> This change make uvc more consistent with the rest of the v4l2 drivers
> >>> using the vb2_fop_* and vb2_ioctl_* helpers.
> >>>
> >>> Suggested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> >>> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> >>> ---
> >>>  drivers/media/usb/uvc/uvc_driver.c | 11 +++++++++++
> >>>  1 file changed, 11 insertions(+)
> >>>
> >>> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> >>> index bbd90123a4e76..17fc945c8deb6 100644
> >>> --- a/drivers/media/usb/uvc/uvc_driver.c
> >>> +++ b/drivers/media/usb/uvc/uvc_driver.c
> >>> @@ -1911,8 +1911,19 @@ static void uvc_unregister_video(struct uvc_device *dev)
> >>>               if (!video_is_registered(&stream->vdev))
> >>>                       continue;
> >>>
> >>> +             /*
> >>> +              * Serialize other access to the stream.
> >>> +              */
> >>> +             mutex_lock(&stream->mutex);
> >>> +             uvc_queue_streamoff(&stream->queue, stream->type);
> >>>               video_unregister_device(&stream->vdev);
> >>>               video_unregister_device(&stream->meta.vdev);
> >>> +             mutex_unlock(&stream->mutex);
> >>
> >> This sequence isn't fool proof. You have to follow the same sequence as
> >> vb2_video_unregister_device(): take a reference to the video device,
> >> then unregister, then take the stream mutex and call vb2_queue_release
> >> for each queue. Finally unlock the mutex and call put_device.
> >
> > vb2_queue_release() will run when the userspace releases the file
> > handle that owns the vb2 queue [1], so we shouldn't really need to
> > call it here.
> >
> > [1] https://elixir.bootlin.com/linux/v6.9.3/source/drivers/media/usb/uvc/uvc_v4l2.c#L655
> >
> >>
> >> Doing the video_unregister_device first ensures no new ioctls can be
> >> called on that device node. Taking the mutex ensures that any still
> >> running ioctls will finish (since it will sleep until they release the
> >> mutex), and then you can release the queue.
> >
> > Actually isn't the only missing thing in the code basically ensuring
> > that any ioctl currently being executed finishes? Why is the streamoff
> > or queue release needed?
>
> See below...
>
> >
> > Best regards,
> > Tomasz
> >
> >>
> >> This does require that you call get_device before calling video_unregister_device,
> >> otherwise everything might be released at that point.
> >>
> >> Instead of vb2_queue_release() you might have to call uvc_queue_streamoff,
> >> but note that there are two queues here: video and meta. The code above
> >> just calls streamoff for the video device.
> >>
> >> For the meta device I think you can just use vb2_video_unregister_device()
> >> directly, since that sets vdev->queue and vdev->queue.lock to the correct
> >> values. That would just leave the video device where you need to do this
> >> manually.
>
> Ideally uvc should just use centralized locking (i.e. set vdev->queue.lock)
> for the video node, just like it does for the meta device.
>
> If that was the case, then it can just call vb2_video_unregister_device().
> The vb2_video_unregister_device helper was added to ensure that no ioctls
> are running, and then streaming is stopped and the queue is released.

Yes, it would be as simple as that if it used standard locking, but
since it does its own stuff, I'm not very confident that the same
logic as in vb2_video_unregister_device() would work fine for it as
well.

>
> While it is true that the queue is released automatically when the last user
> closes the filehandle, it can get complicated if the application has a file
> handle open when the device is unregistered (usually due to it being unplugged).
> Without that call to vb2_video_unregister_device() the queue is still active,
> and especially if you also have subdevices that are still in streaming mode,
> it is hard to make sure nothing is still using the hardware.
>
> vb2_video_unregister_device() provides a clean way of ensuring that when the
> device is unregistered all streaming is stopped and the vb2 queue is released.
>
> After that the only file operation that can be used without returning an error
> is close().
>
> Since uvc uses its own locking for the video device, it has to do this manually.
> It is probably enough to ensure no ioctls are running since uvc doesn't have
> subdevices, but I think it makes sense to stop streaming and release the queue
> when unregistering the device, it is weird to postpone that until the last fh
> is closed.

My comment comes from a concern that releasing objects and changing
internal state in the release callback may break some assumptions in
the operations that will follow once the lock is released, e.g.
close(), leading to some kind of double frees or dereferencing freed
objects. Therefore just preventing new opens and setting the
video_device as unregistered could be potentially a safer option until
the driver is converted to standard locking. (Is there even a plan to
do that?)

Best regards,
Tomasz

>
> Regards,
>
>         Hans
>
>
> >>
> >> Regards,
> >>
> >>         Hans
> >>
> >>> +
> >>> +             /*
> >>> +              * Now the vdev is not streaming and all the ioctls will
> >>> +              * return -ENODEV
> >>> +              */
> >>>
> >>>               uvc_debugfs_cleanup_stream(stream);
> >>>       }
> >>>
> >>
>
>

Re: [PATCH v4 1/4] media: uvcvideo: stop stream during unregister

[Laurent Pinchart]

Hi Tomasz,

On Thu, Jun 06, 2024 at 06:57:50PM +0900, Tomasz Figa wrote:
> On Wed, Mar 27, 2024 at 5:24 PM Ricardo Ribalda wrote:
> >
> > uvc_unregister_video() can be called asynchronously from
> > uvc_disconnect(). If the device is still streaming when that happens, a
> > plethora of race conditions can happen.
> >
> > Make sure that the device has stopped streaming before exiting this
> > function.
> >
> > If the user still holds handles to the driver's file descriptors, any
> > ioctl will return -ENODEV from the v4l2 core.
> >
> > This change make uvc more consistent with the rest of the v4l2 drivers
> > using the vb2_fop_* and vb2_ioctl_* helpers.
> >
> > Suggested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > ---
> >  drivers/media/usb/uvc/uvc_driver.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> 
> First of all, thanks for the patch. I have a question about the
> problem being fixed here.
> 
> Could you point out a specific race condition example that could
> happen without this change?
> From what I see in __video_do_ioctl((), no ioctls would be executed
> anymore after the video node is unregistered.
> Since the device is not present either, what asynchronous code paths
> could be still triggered?

I believe the issue is that some ioctls can be in progress while the
device is unregistered. I'll let Ricardo confirm.

I've tried to explain multiple times before that this should be handled
in the V4L2 core, ideally with fixes in the cdev core too, as this issue
affects all cdev drivers. I've pointed to related patches that have been
posted for the cdev core. They need to be wrapped in V4L2 functions to
make them easier to use for drivers. If we don't want to depend on those
cdev changes, we can implement the "wrappers" with fixes limited to
V4L2 until the cdev changes get merged (assuming someone would resurect
them).

> [1] https://elixir.bootlin.com/linux/latest/source/drivers/media/v4l2-core/v4l2-ioctl.c#L3023
> 
> > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > index bbd90123a4e76..17fc945c8deb6 100644
> > --- a/drivers/media/usb/uvc/uvc_driver.c
> > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > @@ -1911,8 +1911,19 @@ static void uvc_unregister_video(struct uvc_device *dev)
> >                 if (!video_is_registered(&stream->vdev))
> >                         continue;
> >
> > +               /*
> > +                * Serialize other access to the stream.
> > +                */
> > +               mutex_lock(&stream->mutex);
> > +               uvc_queue_streamoff(&stream->queue, stream->type);
> >                 video_unregister_device(&stream->vdev);
> >                 video_unregister_device(&stream->meta.vdev);
> > +               mutex_unlock(&stream->mutex);
> > +
> > +               /*
> > +                * Now the vdev is not streaming and all the ioctls will
> > +                * return -ENODEV
> > +                */
> >
> >                 uvc_debugfs_cleanup_stream(stream);
> >         }

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 1/4] media: uvcvideo: stop stream during unregister

[Hans Verkuil]

On 17/06/2024 01:58, Laurent Pinchart wrote:
> Hi Tomasz,
> 
> On Thu, Jun 06, 2024 at 06:57:50PM +0900, Tomasz Figa wrote:
>> On Wed, Mar 27, 2024 at 5:24 PM Ricardo Ribalda wrote:
>>>
>>> uvc_unregister_video() can be called asynchronously from
>>> uvc_disconnect(). If the device is still streaming when that happens, a
>>> plethora of race conditions can happen.
>>>
>>> Make sure that the device has stopped streaming before exiting this
>>> function.
>>>
>>> If the user still holds handles to the driver's file descriptors, any
>>> ioctl will return -ENODEV from the v4l2 core.
>>>
>>> This change make uvc more consistent with the rest of the v4l2 drivers
>>> using the vb2_fop_* and vb2_ioctl_* helpers.
>>>
>>> Suggested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>>> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
>>> ---
>>>  drivers/media/usb/uvc/uvc_driver.c | 11 +++++++++++
>>>  1 file changed, 11 insertions(+)
>>
>> First of all, thanks for the patch. I have a question about the
>> problem being fixed here.
>>
>> Could you point out a specific race condition example that could
>> happen without this change?
>> From what I see in __video_do_ioctl((), no ioctls would be executed
>> anymore after the video node is unregistered.
>> Since the device is not present either, what asynchronous code paths
>> could be still triggered?
> 
> I believe the issue is that some ioctls can be in progress while the
> device is unregistered. I'll let Ricardo confirm.
> 
> I've tried to explain multiple times before that this should be handled
> in the V4L2 core, ideally with fixes in the cdev core too, as this issue
> affects all cdev drivers. I've pointed to related patches that have been
> posted for the cdev core. They need to be wrapped in V4L2 functions to
> make them easier to use for drivers. If we don't want to depend on those
> cdev changes, we can implement the "wrappers" with fixes limited to
> V4L2 until the cdev changes get merged (assuming someone would resurect
> them).

But there is already a V4L2 wrapper for that: vb2_video_unregister_device().
It safely unregisters the video device, ensuring any in-flight ioctls finish
first, and it stops any video streaming.

The only reason it can't be used in uvc for the video stream is that that
vb2_queue doesn't set the lock field (i.e. uses the core V4L2 serialization
mechanism). The metadata stream *does* set that field, so for that stream this
function can be used.

While it would be nice to have this fixed in the cdev core part, that will
take very long, and we have a perfectly fine V4L2 helper for this already.

Regards,

	Hans

> 
>> [1] https://elixir.bootlin.com/linux/latest/source/drivers/media/v4l2-core/v4l2-ioctl.c#L3023
>>
>>> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
>>> index bbd90123a4e76..17fc945c8deb6 100644
>>> --- a/drivers/media/usb/uvc/uvc_driver.c
>>> +++ b/drivers/media/usb/uvc/uvc_driver.c
>>> @@ -1911,8 +1911,19 @@ static void uvc_unregister_video(struct uvc_device *dev)
>>>                 if (!video_is_registered(&stream->vdev))
>>>                         continue;
>>>
>>> +               /*
>>> +                * Serialize other access to the stream.
>>> +                */
>>> +               mutex_lock(&stream->mutex);
>>> +               uvc_queue_streamoff(&stream->queue, stream->type);
>>>                 video_unregister_device(&stream->vdev);
>>>                 video_unregister_device(&stream->meta.vdev);
>>> +               mutex_unlock(&stream->mutex);
>>> +
>>> +               /*
>>> +                * Now the vdev is not streaming and all the ioctls will
>>> +                * return -ENODEV
>>> +                */
>>>
>>>                 uvc_debugfs_cleanup_stream(stream);
>>>         }
> 

Re: [PATCH v4 1/4] media: uvcvideo: stop stream during unregister

[Sakari Ailus]

Hi Hans,

On Mon, Jun 17, 2024 at 09:27:43AM +0200, Hans Verkuil wrote:
> On 17/06/2024 01:58, Laurent Pinchart wrote:
> > Hi Tomasz,
> > 
> > On Thu, Jun 06, 2024 at 06:57:50PM +0900, Tomasz Figa wrote:
> >> On Wed, Mar 27, 2024 at 5:24 PM Ricardo Ribalda wrote:
> >>>
> >>> uvc_unregister_video() can be called asynchronously from
> >>> uvc_disconnect(). If the device is still streaming when that happens, a
> >>> plethora of race conditions can happen.
> >>>
> >>> Make sure that the device has stopped streaming before exiting this
> >>> function.
> >>>
> >>> If the user still holds handles to the driver's file descriptors, any
> >>> ioctl will return -ENODEV from the v4l2 core.
> >>>
> >>> This change make uvc more consistent with the rest of the v4l2 drivers
> >>> using the vb2_fop_* and vb2_ioctl_* helpers.
> >>>
> >>> Suggested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> >>> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> >>> ---
> >>>  drivers/media/usb/uvc/uvc_driver.c | 11 +++++++++++
> >>>  1 file changed, 11 insertions(+)
> >>
> >> First of all, thanks for the patch. I have a question about the
> >> problem being fixed here.
> >>
> >> Could you point out a specific race condition example that could
> >> happen without this change?
> >> From what I see in __video_do_ioctl((), no ioctls would be executed
> >> anymore after the video node is unregistered.
> >> Since the device is not present either, what asynchronous code paths
> >> could be still triggered?
> > 
> > I believe the issue is that some ioctls can be in progress while the
> > device is unregistered. I'll let Ricardo confirm.
> > 
> > I've tried to explain multiple times before that this should be handled
> > in the V4L2 core, ideally with fixes in the cdev core too, as this issue
> > affects all cdev drivers. I've pointed to related patches that have been
> > posted for the cdev core. They need to be wrapped in V4L2 functions to
> > make them easier to use for drivers. If we don't want to depend on those
> > cdev changes, we can implement the "wrappers" with fixes limited to
> > V4L2 until the cdev changes get merged (assuming someone would resurect
> > them).
> 
> But there is already a V4L2 wrapper for that: vb2_video_unregister_device().
> It safely unregisters the video device, ensuring any in-flight ioctls finish
> first, and it stops any video streaming.
> 
> The only reason it can't be used in uvc for the video stream is that that
> vb2_queue doesn't set the lock field (i.e. uses the core V4L2 serialization
> mechanism). The metadata stream *does* set that field, so for that stream this
> function can be used.
> 
> While it would be nice to have this fixed in the cdev core part, that will
> take very long, and we have a perfectly fine V4L2 helper for this already.

It might not take *that* long to get there but it won't happen unless
someone does it. Dan Williams posted a patch but his immediate problem was
solved differently so there it remains
<URL:https://lore.kernel.org/all/161117153248.2853729.2452425259045172318.stgit@dwillia2-desk3.amr.corp.intel.com/>.

In the meantime vb_video_unregister_device() would seem to be the best
choice.

-- 
Regards,

Sakari Ailus

Re: [PATCH v4 1/4] media: uvcvideo: stop stream during unregister

[Hans Verkuil]

On 17/06/2024 09:56, Sakari Ailus wrote:
> Hi Hans,
> 
> On Mon, Jun 17, 2024 at 09:27:43AM +0200, Hans Verkuil wrote:
>> On 17/06/2024 01:58, Laurent Pinchart wrote:
>>> Hi Tomasz,
>>>
>>> On Thu, Jun 06, 2024 at 06:57:50PM +0900, Tomasz Figa wrote:
>>>> On Wed, Mar 27, 2024 at 5:24 PM Ricardo Ribalda wrote:
>>>>>
>>>>> uvc_unregister_video() can be called asynchronously from
>>>>> uvc_disconnect(). If the device is still streaming when that happens, a
>>>>> plethora of race conditions can happen.
>>>>>
>>>>> Make sure that the device has stopped streaming before exiting this
>>>>> function.
>>>>>
>>>>> If the user still holds handles to the driver's file descriptors, any
>>>>> ioctl will return -ENODEV from the v4l2 core.
>>>>>
>>>>> This change make uvc more consistent with the rest of the v4l2 drivers
>>>>> using the vb2_fop_* and vb2_ioctl_* helpers.
>>>>>
>>>>> Suggested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>>>>> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
>>>>> ---
>>>>>  drivers/media/usb/uvc/uvc_driver.c | 11 +++++++++++
>>>>>  1 file changed, 11 insertions(+)
>>>>
>>>> First of all, thanks for the patch. I have a question about the
>>>> problem being fixed here.
>>>>
>>>> Could you point out a specific race condition example that could
>>>> happen without this change?
>>>> From what I see in __video_do_ioctl((), no ioctls would be executed
>>>> anymore after the video node is unregistered.
>>>> Since the device is not present either, what asynchronous code paths
>>>> could be still triggered?
>>>
>>> I believe the issue is that some ioctls can be in progress while the
>>> device is unregistered. I'll let Ricardo confirm.
>>>
>>> I've tried to explain multiple times before that this should be handled
>>> in the V4L2 core, ideally with fixes in the cdev core too, as this issue
>>> affects all cdev drivers. I've pointed to related patches that have been
>>> posted for the cdev core. They need to be wrapped in V4L2 functions to
>>> make them easier to use for drivers. If we don't want to depend on those
>>> cdev changes, we can implement the "wrappers" with fixes limited to
>>> V4L2 until the cdev changes get merged (assuming someone would resurect
>>> them).
>>
>> But there is already a V4L2 wrapper for that: vb2_video_unregister_device().
>> It safely unregisters the video device, ensuring any in-flight ioctls finish
>> first, and it stops any video streaming.
>>
>> The only reason it can't be used in uvc for the video stream is that that
>> vb2_queue doesn't set the lock field (i.e. uses the core V4L2 serialization
>> mechanism). The metadata stream *does* set that field, so for that stream this
>> function can be used.
>>
>> While it would be nice to have this fixed in the cdev core part, that will
>> take very long, and we have a perfectly fine V4L2 helper for this already.
> 
> It might not take *that* long to get there but it won't happen unless
> someone does it. Dan Williams posted a patch but his immediate problem was
> solved differently so there it remains
> <URL:https://lore.kernel.org/all/161117153248.2853729.2452425259045172318.stgit@dwillia2-desk3.amr.corp.intel.com/>.
> 
> In the meantime vb_video_unregister_device() would seem to be the best
> choice.

Also note that even if these cdev improvements ever land, that doesn't remove
the need for vb2_video_unregister_device, since that also explicitly stops
any streaming that is in progress. Which is something you really want to do
when the device is unbound.

Regards,

	Hans