Re: [PATCH v4 1/4] media: uvcvideo: stop stream during unregister

[Hans Verkuil <hverkuil-cisco@xs4all.nl>]

On 17/06/2024 09:56, Sakari Ailus wrote:
> Hi Hans,
> 
> On Mon, Jun 17, 2024 at 09:27:43AM +0200, Hans Verkuil wrote:
>> On 17/06/2024 01:58, Laurent Pinchart wrote:
>>> Hi Tomasz,
>>>
>>> On Thu, Jun 06, 2024 at 06:57:50PM +0900, Tomasz Figa wrote:
>>>> On Wed, Mar 27, 2024 at 5:24 PM 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 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@xs4all.nl>
>>>>> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
>>>>> ---
>>>>>  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?
>>>
>>> I believe the issue is that some ioctls can be in progress while the
>>> device is unregistered. I'll let Ricardo confirm.
>>>
>>> I've tried to explain multiple times before that this should be handled
>>> in the V4L2 core, ideally with fixes in the cdev core too, as this issue
>>> affects all cdev drivers. I've pointed to related patches that have been
>>> posted for the cdev core. They need to be wrapped in V4L2 functions to
>>> make them easier to use for drivers. If we don't want to depend on those
>>> cdev changes, we can implement the "wrappers" with fixes limited to
>>> V4L2 until the cdev changes get merged (assuming someone would resurect
>>> them).
>>
>> But there is already a V4L2 wrapper for that: vb2_video_unregister_device().
>> It safely unregisters the video device, ensuring any in-flight ioctls finish
>> first, and it stops any video streaming.
>>
>> The only reason it can't be used in uvc for the video stream is that that
>> vb2_queue doesn't set the lock field (i.e. uses the core V4L2 serialization
>> mechanism). The metadata stream *does* set that field, so for that stream this
>> function can be used.
>>
>> While it would be nice to have this fixed in the cdev core part, that will
>> take very long, and we have a perfectly fine V4L2 helper for this already.
> 
> It might not take *that* long to get there but it won't happen unless
> someone does it. Dan Williams posted a patch but his immediate problem was
> solved differently so there it remains
> <URL:https://lore.kernel.org/all/161117153248.2853729.2452425259045172318.stgit@dwillia2-desk3.amr.corp.intel.com/>.
> 
> In the meantime vb_video_unregister_device() would seem to be the best
> choice.

Also note that even if these cdev improvements ever land, that doesn't remove
the need for vb2_video_unregister_device, since that also explicitly stops
any streaming that is in progress. Which is something you really want to do
when the device is unbound.

Regards,

	Hans