diff mbox series

[v6,1/4] media: uvcvideo: Stop stream during unregister

Message ID 20240614-guenter-mini-v6-1-7b7fdc3b21b3@chromium.org (mailing list archive)
State New
Headers show
Series uvcvideo: Attempt N to land UVC race conditions fixes | expand

Commit Message

Ricardo Ribalda June 14, 2024, 12:41 p.m. UTC
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 occur.

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 makes uvc more consistent with the rest of the v4l2 drivers
using the vb2_fop_* and vb2_ioctl_* helpers.

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

Comments

Laurent Pinchart June 16, 2024, 11:54 p.m. UTC | #1
Hi Ricardo,

Thank you for the patch.

On Fri, Jun 14, 2024 at 12:41:27PM +0000, 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 occur.
> 
> 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 makes uvc more consistent with the rest of the v4l2 drivers
> using the vb2_fop_* and vb2_ioctl_* helpers.

As I've said many times before, this issue needs a fix in the V4L2 core,
ideally with support in the cdev core. It seems I'll have to do it
myself ?

> Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> Suggested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_driver.c | 32 +++++++++++++++++++++++++++++++-
>  1 file changed, 31 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index bbd90123a4e7..55132688e363 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -1908,11 +1908,41 @@ static void uvc_unregister_video(struct uvc_device *dev)
>  	struct uvc_streaming *stream;
>  
>  	list_for_each_entry(stream, &dev->streams, list) {
> +		/* Nothing to do here, continue. */
>  		if (!video_is_registered(&stream->vdev))
>  			continue;
>  
> +		/*
> +		 * For stream->vdev we follow the same logic as:
> +		 * vb2_video_unregister_device().
> +		 */
> +
> +		/* 1. Take a reference to vdev */
> +		get_device(&stream->vdev.dev);
> +
> +		/* 2. Ensure that no new ioctls can be called. */
>  		video_unregister_device(&stream->vdev);
> -		video_unregister_device(&stream->meta.vdev);
> +
> +		/* 3. Wait for old ioctls to finish. */
> +		mutex_lock(&stream->mutex);
> +
> +		/* 4. Stop streaming. */
> +		uvc_queue_release(&stream->queue);
> +
> +		mutex_unlock(&stream->mutex);
> +
> +		put_device(&stream->vdev.dev);
> +
> +		/*
> +		 * For stream->meta.vdev we can directly call:
> +		 * vb2_video_unregister_device().
> +		 */
> +		vb2_video_unregister_device(&stream->meta.vdev);
> +
> +		/*
> +		 * Now both vdevs are not streaming and all the ioctls will
> +		 * return -ENODEV.
> +		 */
>  
>  		uvc_debugfs_cleanup_stream(stream);
>  	}
Ricardo Ribalda June 17, 2024, 7:22 a.m. UTC | #2
Hi Laurent

On Mon, 17 Jun 2024 at 01:54, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Ricardo,
>
> Thank you for the patch.
>
> On Fri, Jun 14, 2024 at 12:41:27PM +0000, 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 occur.
> >
> > 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 makes uvc more consistent with the rest of the v4l2 drivers
> > using the vb2_fop_* and vb2_ioctl_* helpers.
>
> As I've said many times before, this issue needs a fix in the V4L2 core,
> ideally with support in the cdev core. It seems I'll have to do it
> myself ?

 vb2_video_unregister_device() already patched this issue. We are just
porting that solution to UVC, because uvc is not using the vb2
helpers.
I don't see why being more consistent with the rest of the v4l2 driver
makes it a bad thing.

I am reverting the ChromeOS solution for this race condition and
applying this patch instead:
https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/5632459/1
That is going to test this patch in some million devices in some days.

This change is self-contained, fixes a real problem, makes the driver
consistent and will be tested by lots of users. We could land this
patch and help our users until there is a solution in cdev.
I would argue that even with a solution in cdev this is not a bad patch.

> ideally with support in the cdev core. It seems I'll have to do it
> myself ?

I can help reviewing and testing ;)

Regards!


>
> > Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> > Suggested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > ---
> >  drivers/media/usb/uvc/uvc_driver.c | 32 +++++++++++++++++++++++++++++++-
> >  1 file changed, 31 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > index bbd90123a4e7..55132688e363 100644
> > --- a/drivers/media/usb/uvc/uvc_driver.c
> > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > @@ -1908,11 +1908,41 @@ static void uvc_unregister_video(struct uvc_device *dev)
> >       struct uvc_streaming *stream;
> >
> >       list_for_each_entry(stream, &dev->streams, list) {
> > +             /* Nothing to do here, continue. */
> >               if (!video_is_registered(&stream->vdev))
> >                       continue;
> >
> > +             /*
> > +              * For stream->vdev we follow the same logic as:
> > +              * vb2_video_unregister_device().
> > +              */
> > +
> > +             /* 1. Take a reference to vdev */
> > +             get_device(&stream->vdev.dev);
> > +
> > +             /* 2. Ensure that no new ioctls can be called. */
> >               video_unregister_device(&stream->vdev);
> > -             video_unregister_device(&stream->meta.vdev);
> > +
> > +             /* 3. Wait for old ioctls to finish. */
> > +             mutex_lock(&stream->mutex);
> > +
> > +             /* 4. Stop streaming. */
> > +             uvc_queue_release(&stream->queue);
> > +
> > +             mutex_unlock(&stream->mutex);
> > +
> > +             put_device(&stream->vdev.dev);
> > +
> > +             /*
> > +              * For stream->meta.vdev we can directly call:
> > +              * vb2_video_unregister_device().
> > +              */
> > +             vb2_video_unregister_device(&stream->meta.vdev);
> > +
> > +             /*
> > +              * Now both vdevs are not streaming and all the ioctls will
> > +              * return -ENODEV.
> > +              */
> >
> >               uvc_debugfs_cleanup_stream(stream);
> >       }
>
> --
> Regards,
>
> Laurent Pinchart
diff mbox series

Patch

diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index bbd90123a4e7..55132688e363 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -1908,11 +1908,41 @@  static void uvc_unregister_video(struct uvc_device *dev)
 	struct uvc_streaming *stream;
 
 	list_for_each_entry(stream, &dev->streams, list) {
+		/* Nothing to do here, continue. */
 		if (!video_is_registered(&stream->vdev))
 			continue;
 
+		/*
+		 * For stream->vdev we follow the same logic as:
+		 * vb2_video_unregister_device().
+		 */
+
+		/* 1. Take a reference to vdev */
+		get_device(&stream->vdev.dev);
+
+		/* 2. Ensure that no new ioctls can be called. */
 		video_unregister_device(&stream->vdev);
-		video_unregister_device(&stream->meta.vdev);
+
+		/* 3. Wait for old ioctls to finish. */
+		mutex_lock(&stream->mutex);
+
+		/* 4. Stop streaming. */
+		uvc_queue_release(&stream->queue);
+
+		mutex_unlock(&stream->mutex);
+
+		put_device(&stream->vdev.dev);
+
+		/*
+		 * For stream->meta.vdev we can directly call:
+		 * vb2_video_unregister_device().
+		 */
+		vb2_video_unregister_device(&stream->meta.vdev);
+
+		/*
+		 * Now both vdevs are not streaming and all the ioctls will
+		 * return -ENODEV.
+		 */
 
 		uvc_debugfs_cleanup_stream(stream);
 	}