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@xxxxxxxxx> > >>> Signed-off-by: Ricardo Ribalda <ribalda@xxxxxxxxxxxx> > >>> --- > >>> 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@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/>. In the meantime vb_video_unregister_device() would seem to be the best choice. -- Regards, Sakari Ailus
