[PATCH] media: uvcvideo: Stop stream during unregister

[PATCH] 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 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>
---
This patch was part of the series:
https://patchwork.linuxtv.org/project/linux-media/list/?series=13064

Moved out from it to ease the review.
---
 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 f0febdc08c2d..bee150b852e4 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -1919,11 +1919,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);
 	}

---
base-commit: 81ee62e8d09ee3c7107d11c8bbfd64073ab601ad
change-id: 20240926-uvc_stop_streaming-6e9fd20e97bc

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

Re: [PATCH] media: uvcvideo: Stop stream during unregister

[Hans Verkuil]

Hi Laurent,

Just a reminder: I have extensively reviewed this patch here:

https://lore.kernel.org/linux-media/f4c49ccf-9dc9-475a-8fc9-4ef4c85a729a@xs4all.nl/

and here (specifically checking for mmap() races):

https://lore.kernel.org/linux-media/1a10530f-b4bb-4244-84ff-1f2365ae9b23@xs4all.nl/

To the best of my ability I believe this patch is correct.

Unless you have any additional concerns I plan to take this patch as a fix for
v6.12 on Monday next week.

Alternatively, you can make a PR for 6.12 with this patch that I can pull from.

Regards,

	Hans

On 26/09/2024 07:59, 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@xs4all.nl>
> Suggested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
> This patch was part of the series:
> https://patchwork.linuxtv.org/project/linux-media/list/?series=13064
> 
> Moved out from it to ease the review.
> ---
>  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 f0febdc08c2d..bee150b852e4 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -1919,11 +1919,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);
>  	}
> 
> ---
> base-commit: 81ee62e8d09ee3c7107d11c8bbfd64073ab601ad
> change-id: 20240926-uvc_stop_streaming-6e9fd20e97bc
> 
> Best regards,

Re: [PATCH] media: uvcvideo: Stop stream during unregister

[Laurent Pinchart]

Hi Hans,

On Mon, Oct 07, 2024 at 09:46:47AM +0200, Hans Verkuil wrote:
> Hi Laurent,
> 
> Just a reminder: I have extensively reviewed this patch here:
> 
> https://lore.kernel.org/linux-media/f4c49ccf-9dc9-475a-8fc9-4ef4c85a729a@xs4all.nl/
> 
> and here (specifically checking for mmap() races):
> 
> https://lore.kernel.org/linux-media/1a10530f-b4bb-4244-84ff-1f2365ae9b23@xs4all.nl/
> 
> To the best of my ability I believe this patch is correct.
> 
> Unless you have any additional concerns I plan to take this patch as a fix for
> v6.12 on Monday next week.

I thought we had an agreement that I could submit an alternative fix for
v6.12. Can you therefore delay merging this patch until v6.12-rc6 ?

> Alternatively, you can make a PR for 6.12 with this patch that I can pull from.
> 
> Regards,
> 
> 	Hans
> 
> On 26/09/2024 07:59, 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@xs4all.nl>
> > Suggested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > ---
> > This patch was part of the series:
> > https://patchwork.linuxtv.org/project/linux-media/list/?series=13064
> > 
> > Moved out from it to ease the review.
> > ---
> >  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 f0febdc08c2d..bee150b852e4 100644
> > --- a/drivers/media/usb/uvc/uvc_driver.c
> > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > @@ -1919,11 +1919,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);
> >  	}
> > 
> > ---
> > base-commit: 81ee62e8d09ee3c7107d11c8bbfd64073ab601ad
> > change-id: 20240926-uvc_stop_streaming-6e9fd20e97bc
> > 
> > Best regards,
> 

-- 
Regards,

Laurent Pinchart

Re: [PATCH] media: uvcvideo: Stop stream during unregister

[Hans Verkuil]

On 07/10/2024 16:44, Laurent Pinchart wrote:
> Hi Hans,
> 
> On Mon, Oct 07, 2024 at 09:46:47AM +0200, Hans Verkuil wrote:
>> Hi Laurent,
>>
>> Just a reminder: I have extensively reviewed this patch here:
>>
>> https://lore.kernel.org/linux-media/f4c49ccf-9dc9-475a-8fc9-4ef4c85a729a@xs4all.nl/
>>
>> and here (specifically checking for mmap() races):
>>
>> https://lore.kernel.org/linux-media/1a10530f-b4bb-4244-84ff-1f2365ae9b23@xs4all.nl/
>>
>> To the best of my ability I believe this patch is correct.
>>
>> Unless you have any additional concerns I plan to take this patch as a fix for
>> v6.12 on Monday next week.
> 
> I thought we had an agreement that I could submit an alternative fix for
> v6.12. Can you therefore delay merging this patch until v6.12-rc6 ?

Correct, if there is indeed something wrong with this patch and an alternative
fix is needed (or at least should be considered).

But I see nothing wrong with this patch after careful analysis. If you disagree
with my analysis, and you think I missed a possible race condition, then that's
a reason to wait for a better fix. Otherwise there is no point in waiting any longer.

Regards,

	Hans

> 
>> Alternatively, you can make a PR for 6.12 with this patch that I can pull from.
>>
>> Regards,
>>
>> 	Hans
>>
>> On 26/09/2024 07:59, 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@xs4all.nl>
>>> Suggested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>>> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
>>> ---
>>> This patch was part of the series:
>>> https://patchwork.linuxtv.org/project/linux-media/list/?series=13064
>>>
>>> Moved out from it to ease the review.
>>> ---
>>>  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 f0febdc08c2d..bee150b852e4 100644
>>> --- a/drivers/media/usb/uvc/uvc_driver.c
>>> +++ b/drivers/media/usb/uvc/uvc_driver.c
>>> @@ -1919,11 +1919,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);
>>>  	}
>>>
>>> ---
>>> base-commit: 81ee62e8d09ee3c7107d11c8bbfd64073ab601ad
>>> change-id: 20240926-uvc_stop_streaming-6e9fd20e97bc
>>>
>>> Best regards,
>>
> 

Re: [PATCH] media: uvcvideo: Stop stream during unregister

[Laurent Pinchart]

Hi Hans,

On Mon, Oct 07, 2024 at 04:53:30PM +0200, Hans Verkuil wrote:
> On 07/10/2024 16:44, Laurent Pinchart wrote:
> > On Mon, Oct 07, 2024 at 09:46:47AM +0200, Hans Verkuil wrote:
> >> Hi Laurent,
> >>
> >> Just a reminder: I have extensively reviewed this patch here:
> >>
> >> https://lore.kernel.org/linux-media/f4c49ccf-9dc9-475a-8fc9-4ef4c85a729a@xs4all.nl/
> >>
> >> and here (specifically checking for mmap() races):
> >>
> >> https://lore.kernel.org/linux-media/1a10530f-b4bb-4244-84ff-1f2365ae9b23@xs4all.nl/
> >>
> >> To the best of my ability I believe this patch is correct.
> >>
> >> Unless you have any additional concerns I plan to take this patch as a fix for
> >> v6.12 on Monday next week.
> > 
> > I thought we had an agreement that I could submit an alternative fix for
> > v6.12. Can you therefore delay merging this patch until v6.12-rc6 ?
> 
> Correct, if there is indeed something wrong with this patch and an alternative
> fix is needed (or at least should be considered).
> 
> But I see nothing wrong with this patch after careful analysis. If you disagree
> with my analysis, and you think I missed a possible race condition, then that's
> a reason to wait for a better fix. Otherwise there is no point in waiting any longer.

I'm in Montréal this week for the GStreamer conference and XDC. I'll
reply to your last e-mail early next week, let's make a decision then.
Surely this can wait until -rc4 before being merged ?

> >> Alternatively, you can make a PR for 6.12 with this patch that I can pull from.
> >>
> >> Regards,
> >>
> >> 	Hans
> >>
> >> On 26/09/2024 07:59, 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@xs4all.nl>
> >>> Suggested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> >>> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> >>> ---
> >>> This patch was part of the series:
> >>> https://patchwork.linuxtv.org/project/linux-media/list/?series=13064
> >>>
> >>> Moved out from it to ease the review.
> >>> ---
> >>>  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 f0febdc08c2d..bee150b852e4 100644
> >>> --- a/drivers/media/usb/uvc/uvc_driver.c
> >>> +++ b/drivers/media/usb/uvc/uvc_driver.c
> >>> @@ -1919,11 +1919,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);
> >>>  	}
> >>>
> >>> ---
> >>> base-commit: 81ee62e8d09ee3c7107d11c8bbfd64073ab601ad
> >>> change-id: 20240926-uvc_stop_streaming-6e9fd20e97bc

-- 
Regards,

Laurent Pinchart

Re: [PATCH] media: uvcvideo: Stop stream during unregister

[Hans Verkuil]

Hi Laurent,

On 10/10/2024 20:23, Laurent Pinchart wrote:
> Hi Hans,
> 
> On Mon, Oct 07, 2024 at 04:53:30PM +0200, Hans Verkuil wrote:
>> On 07/10/2024 16:44, Laurent Pinchart wrote:
>>> On Mon, Oct 07, 2024 at 09:46:47AM +0200, Hans Verkuil wrote:
>>>> Hi Laurent,
>>>>
>>>> Just a reminder: I have extensively reviewed this patch here:
>>>>
>>>> https://lore.kernel.org/linux-media/f4c49ccf-9dc9-475a-8fc9-4ef4c85a729a@xs4all.nl/
>>>>
>>>> and here (specifically checking for mmap() races):
>>>>
>>>> https://lore.kernel.org/linux-media/1a10530f-b4bb-4244-84ff-1f2365ae9b23@xs4all.nl/
>>>>
>>>> To the best of my ability I believe this patch is correct.
>>>>
>>>> Unless you have any additional concerns I plan to take this patch as a fix for
>>>> v6.12 on Monday next week.
>>>
>>> I thought we had an agreement that I could submit an alternative fix for
>>> v6.12. Can you therefore delay merging this patch until v6.12-rc6 ?
>>
>> Correct, if there is indeed something wrong with this patch and an alternative
>> fix is needed (or at least should be considered).
>>
>> But I see nothing wrong with this patch after careful analysis. If you disagree
>> with my analysis, and you think I missed a possible race condition, then that's
>> a reason to wait for a better fix. Otherwise there is no point in waiting any longer.
> 
> I'm in Montréal this week for the GStreamer conference and XDC. I'll
> reply to your last e-mail early next week, let's make a decision then.
> Surely this can wait until -rc4 before being merged ?

Sure, no problem.

Enjoy Montréal!

	Hans

> 
>>>> Alternatively, you can make a PR for 6.12 with this patch that I can pull from.
>>>>
>>>> Regards,
>>>>
>>>> 	Hans
>>>>
>>>> On 26/09/2024 07:59, 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@xs4all.nl>
>>>>> Suggested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>>>>> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
>>>>> ---
>>>>> This patch was part of the series:
>>>>> https://patchwork.linuxtv.org/project/linux-media/list/?series=13064
>>>>>
>>>>> Moved out from it to ease the review.
>>>>> ---
>>>>>  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 f0febdc08c2d..bee150b852e4 100644
>>>>> --- a/drivers/media/usb/uvc/uvc_driver.c
>>>>> +++ b/drivers/media/usb/uvc/uvc_driver.c
>>>>> @@ -1919,11 +1919,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);
>>>>>  	}
>>>>>
>>>>> ---
>>>>> base-commit: 81ee62e8d09ee3c7107d11c8bbfd64073ab601ad
>>>>> change-id: 20240926-uvc_stop_streaming-6e9fd20e97bc
> 

Re: [PATCH] media: uvcvideo: Stop stream during unregister

[Hans Verkuil]

Hi Laurent,

On 11/10/2024 08:52, Hans Verkuil wrote:
> Hi Laurent,
> 
> On 10/10/2024 20:23, Laurent Pinchart wrote:
>> Hi Hans,
>>
>> On Mon, Oct 07, 2024 at 04:53:30PM +0200, Hans Verkuil wrote:
>>> On 07/10/2024 16:44, Laurent Pinchart wrote:
>>>> On Mon, Oct 07, 2024 at 09:46:47AM +0200, Hans Verkuil wrote:
>>>>> Hi Laurent,
>>>>>
>>>>> Just a reminder: I have extensively reviewed this patch here:
>>>>>
>>>>> https://lore.kernel.org/linux-media/f4c49ccf-9dc9-475a-8fc9-4ef4c85a729a@xs4all.nl/
>>>>>
>>>>> and here (specifically checking for mmap() races):
>>>>>
>>>>> https://lore.kernel.org/linux-media/1a10530f-b4bb-4244-84ff-1f2365ae9b23@xs4all.nl/
>>>>>
>>>>> To the best of my ability I believe this patch is correct.
>>>>>
>>>>> Unless you have any additional concerns I plan to take this patch as a fix for
>>>>> v6.12 on Monday next week.
>>>>
>>>> I thought we had an agreement that I could submit an alternative fix for
>>>> v6.12. Can you therefore delay merging this patch until v6.12-rc6 ?
>>>
>>> Correct, if there is indeed something wrong with this patch and an alternative
>>> fix is needed (or at least should be considered).
>>>
>>> But I see nothing wrong with this patch after careful analysis. If you disagree
>>> with my analysis, and you think I missed a possible race condition, then that's
>>> a reason to wait for a better fix. Otherwise there is no point in waiting any longer.
>>
>> I'm in Montréal this week for the GStreamer conference and XDC. I'll
>> reply to your last e-mail early next week, let's make a decision then.
>> Surely this can wait until -rc4 before being merged ?

Since I have had no reply, and after consulting with Mauro, I merged this patch.
I added a Fixes tag and CC to stable, so this should also end up in stable kernels
from 5.10 onwards (it won't apply to older kernels since they do not have the
vb2_video_unregister_device function).

Regards,

	Hans

> 
> Sure, no problem.
> 
> Enjoy Montréal!
> 
> 	Hans
> 
>>
>>>>> Alternatively, you can make a PR for 6.12 with this patch that I can pull from.
>>>>>
>>>>> Regards,
>>>>>
>>>>> 	Hans
>>>>>
>>>>> On 26/09/2024 07:59, 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@xs4all.nl>
>>>>>> Suggested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>>>>>> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
>>>>>> ---
>>>>>> This patch was part of the series:
>>>>>> https://patchwork.linuxtv.org/project/linux-media/list/?series=13064
>>>>>>
>>>>>> Moved out from it to ease the review.
>>>>>> ---
>>>>>>  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 f0febdc08c2d..bee150b852e4 100644
>>>>>> --- a/drivers/media/usb/uvc/uvc_driver.c
>>>>>> +++ b/drivers/media/usb/uvc/uvc_driver.c
>>>>>> @@ -1919,11 +1919,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);
>>>>>>  	}
>>>>>>
>>>>>> ---
>>>>>> base-commit: 81ee62e8d09ee3c7107d11c8bbfd64073ab601ad
>>>>>> change-id: 20240926-uvc_stop_streaming-6e9fd20e97bc
>>
> 
> 

Re: [PATCH] media: uvcvideo: Stop stream during unregister

[Mauro Carvalho Chehab]

Em Thu, 26 Sep 2024 05:59:06 +0000
Ricardo Ribalda <ribalda@chromium.org> escreveu:

> 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>

I didn't test it, but the patch looks OK on my eyes.

Reviewed-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>

> ---
> This patch was part of the series:
> https://patchwork.linuxtv.org/project/linux-media/list/?series=13064
> 
> Moved out from it to ease the review.
> ---
>  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 f0febdc08c2d..bee150b852e4 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -1919,11 +1919,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);
>  	}
> 
> ---
> base-commit: 81ee62e8d09ee3c7107d11c8bbfd64073ab601ad
> change-id: 20240926-uvc_stop_streaming-6e9fd20e97bc
> 
> Best regards,



Thanks,
Mauro