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@xxxxxxxxxxxx> 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@xxxxxxxxx>
> Signed-off-by: Ricardo Ribalda <ribalda@xxxxxxxxxxxx>
> ---
>  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
>