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

[Sakari Ailus <sakari.ailus@linux.intel.com>]

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.

-- 
Regards,

Sakari Ailus