On 25/09/2024 10:32, Hans Verkuil wrote:
> Hi Laurent,
>
> We discussed this patch last week, and you thought that there was still
> a race condition if the uvc device was unplugged while an application was
> in the VIDIOC_DQBUF call waiting for a buffer to arrive (so the vb2 wait_prepare
> op is called, which unlocks the serialization mutex).
>
> I'll go through the code below, explaining why that isn't an issue.
Update: I added an extra check for this scenario to the test-media script to make
sure we catch any potential regressions in how this is handled in the core.
Regards,
Hans
>
> On 14/06/2024 14:41, 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.
>>
>> Reviewed-by: Hans Verkuil <hverkuil-cisco@xxxxxxxxx>
>> Suggested-by: Hans Verkuil <hverkuil-cisco@xxxxxxxxx>
>> Signed-off-by: Ricardo Ribalda <ribalda@xxxxxxxxxxxx>
>> ---
>> 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);
>
> This ensures that the device refcount won't go to 0 if video_unregister_device
> is called (which calls put_device).
>
> But note that if an application called VIDIOC_DQBUF and is waiting for a buffer,
> then that open filehandle also called get_device(). So while that application is
> waiting, the device refcount will never go to 0.
>
>> +
>> + /* 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);
>
> If VIDIOC_DQBUF is waiting for a buffer to arrive, then indeed we can take this
> lock here. So in that case this won't wait for that specific ioctl to finish.
>
>> +
>> + /* 4. Stop streaming. */
>> + uvc_queue_release(&stream->queue);
>
> This will __vb2_queue_cancel() which will stop streaming and wake up the wait for
> buffers in VIDIOC_DQBUF. It will try to lock this mutex again, and sleeps while
> waiting for the mutex to become available.
>
>> +
>> + mutex_unlock(&stream->mutex);
>
> At this point it can take the mutex again. But since q->streaming is now false,
> (due to the __vb2_queue_cancel call) this will return an error which is returned
> to userspace.
>
>> +
>> + put_device(&stream->vdev.dev);
>
> This releases the reference we took earlier. If the application has already closed
> the filehandle, then this will release all memory. If the application still has the
> fh open, then only when it closes that fh will the memory be released.
>
> Conclusion: there is no race condition here, this is handled correctly by the core.
>
>> +
>> + /*
>> + * For stream->meta.vdev we can directly call:
>> + * vb2_video_unregister_device().
>> + */
>> + vb2_video_unregister_device(&stream->meta.vdev);
>
> Perhaps a patch adding more comments to the vb2_video_unregister_device()
> function might help document this sequence better.
>
> Regards,
>
> Hans
>
>> +
>> + /*
>> + * Now both vdevs are not streaming and all the ioctls will
>> + * return -ENODEV.
>> + */
>>
>> uvc_debugfs_cleanup_stream(stream);
>> }
>>
>
>
