linux-kernel - Re: [PATCH] media: uvcvideo: Stop stream during unregister

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <33e4c073-4edc-4128-b698-a90de185f1a9@xs4all.nl>
Date: Wed, 23 Oct 2024 10:42:11 +0200
From: Hans Verkuil <hverkuil-cisco@...all.nl>
To: Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc: Ricardo Ribalda <ribalda@...omium.org>,
 Mauro Carvalho Chehab <mchehab@...nel.org>, linux-media@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH] media: uvcvideo: Stop stream during unregister

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@...all.nl>
>>>>>> Suggested-by: Hans Verkuil <hverkuil-cisco@...all.nl>
>>>>>> Signed-off-by: Ricardo Ribalda <ribalda@...omium.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
>>
> 
>