| Message ID | 20240614-guenter-mini-v6-1-7b7fdc3b21b3@chromium.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | uvcvideo: Attempt N to land UVC race conditions fixes | expand |
Hi Ricardo, Thank you for the patch. On Fri, Jun 14, 2024 at 12:41:27PM +0000, 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. As I've said many times before, this issue needs a fix in the V4L2 core, ideally with support in the cdev core. It seems I'll have to do it myself ? > Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > Suggested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > --- > 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 bbd90123a4e7..55132688e363 100644 > --- a/drivers/media/usb/uvc/uvc_driver.c > +++ b/drivers/media/usb/uvc/uvc_driver.c > @@ -1908,11 +1908,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); > }
Hi Laurent On Mon, 17 Jun 2024 at 01:54, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Ricardo, > > Thank you for the patch. > > On Fri, Jun 14, 2024 at 12:41:27PM +0000, 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. > > As I've said many times before, this issue needs a fix in the V4L2 core, > ideally with support in the cdev core. It seems I'll have to do it > myself ? vb2_video_unregister_device() already patched this issue. We are just porting that solution to UVC, because uvc is not using the vb2 helpers. I don't see why being more consistent with the rest of the v4l2 driver makes it a bad thing. I am reverting the ChromeOS solution for this race condition and applying this patch instead: https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/5632459/1 That is going to test this patch in some million devices in some days. This change is self-contained, fixes a real problem, makes the driver consistent and will be tested by lots of users. We could land this patch and help our users until there is a solution in cdev. I would argue that even with a solution in cdev this is not a bad patch. > ideally with support in the cdev core. It seems I'll have to do it > myself ? I can help reviewing and testing ;) Regards! > > > Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > > Suggested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > > --- > > 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 bbd90123a4e7..55132688e363 100644 > > --- a/drivers/media/usb/uvc/uvc_driver.c > > +++ b/drivers/media/usb/uvc/uvc_driver.c > > @@ -1908,11 +1908,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); > > } > > -- > Regards, > > Laurent Pinchart
Hi Laurent, We discussed this patch last week, and you thought that there was still a race condition if the uvc device was unplugged while an application was in the VIDIOC_DQBUF call waiting for a buffer to arrive (so the vb2 wait_prepare op is called, which unlocks the serialization mutex). I'll go through the code below, explaining why that isn't an issue. On 14/06/2024 14:41, 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> > --- > 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 bbd90123a4e7..55132688e363 100644 > --- a/drivers/media/usb/uvc/uvc_driver.c > +++ b/drivers/media/usb/uvc/uvc_driver.c > @@ -1908,11 +1908,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); This ensures that the device refcount won't go to 0 if video_unregister_device is called (which calls put_device). But note that if an application called VIDIOC_DQBUF and is waiting for a buffer, then that open filehandle also called get_device(). So while that application is waiting, the device refcount will never go to 0. > + > + /* 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); If VIDIOC_DQBUF is waiting for a buffer to arrive, then indeed we can take this lock here. So in that case this won't wait for that specific ioctl to finish. > + > + /* 4. Stop streaming. */ > + uvc_queue_release(&stream->queue); This will __vb2_queue_cancel() which will stop streaming and wake up the wait for buffers in VIDIOC_DQBUF. It will try to lock this mutex again, and sleeps while waiting for the mutex to become available. > + > + mutex_unlock(&stream->mutex); At this point it can take the mutex again. But since q->streaming is now false, (due to the __vb2_queue_cancel call) this will return an error which is returned to userspace. > + > + put_device(&stream->vdev.dev); This releases the reference we took earlier. If the application has already closed the filehandle, then this will release all memory. If the application still has the fh open, then only when it closes that fh will the memory be released. Conclusion: there is no race condition here, this is handled correctly by the core. > + > + /* > + * For stream->meta.vdev we can directly call: > + * vb2_video_unregister_device(). > + */ > + vb2_video_unregister_device(&stream->meta.vdev); Perhaps a patch adding more comments to the vb2_video_unregister_device() function might help document this sequence better. Regards, Hans > + > + /* > + * Now both vdevs are not streaming and all the ioctls will > + * return -ENODEV. > + */ > > uvc_debugfs_cleanup_stream(stream); > } >
Hi Hans, On Wed, Sep 25, 2024 at 11:50:41AM +0200, Hans Verkuil wrote: > On 25/09/2024 10:32, Hans Verkuil wrote: > > Hi Laurent, > > > > We discussed this patch last week, and you thought that there was still > > a race condition if the uvc device was unplugged while an application was > > in the VIDIOC_DQBUF call waiting for a buffer to arrive (so the vb2 wait_prepare > > op is called, which unlocks the serialization mutex). > > > > I'll go through the code below, explaining why that isn't an issue. > > Update: I added an extra check for this scenario to the test-media script to make > sure we catch any potential regressions in how this is handled in the core. I'm recovering from Vienna and I'll review your explanation towards the end of the week. Ricardo, patches 2/4 to 4/4 are not controversial. 2/4 needs a new version to address small issues. As far as I understand, they don't depend on 1/4. Would you submit a new version of them that I can merge right away ? > > On 14/06/2024 14:41, 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> > >> --- > >> 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 bbd90123a4e7..55132688e363 100644 > >> --- a/drivers/media/usb/uvc/uvc_driver.c > >> +++ b/drivers/media/usb/uvc/uvc_driver.c > >> @@ -1908,11 +1908,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); > > > > This ensures that the device refcount won't go to 0 if video_unregister_device > > is called (which calls put_device). > > > > But note that if an application called VIDIOC_DQBUF and is waiting for a buffer, > > then that open filehandle also called get_device(). So while that application is > > waiting, the device refcount will never go to 0. > > > >> + > >> + /* 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); > > > > If VIDIOC_DQBUF is waiting for a buffer to arrive, then indeed we can take this > > lock here. So in that case this won't wait for that specific ioctl to finish. > > > >> + > >> + /* 4. Stop streaming. */ > >> + uvc_queue_release(&stream->queue); > > > > This will __vb2_queue_cancel() which will stop streaming and wake up the wait for > > buffers in VIDIOC_DQBUF. It will try to lock this mutex again, and sleeps while > > waiting for the mutex to become available. > > > >> + > >> + mutex_unlock(&stream->mutex); > > > > At this point it can take the mutex again. But since q->streaming is now false, > > (due to the __vb2_queue_cancel call) this will return an error which is returned > > to userspace. > > > >> + > >> + put_device(&stream->vdev.dev); > > > > This releases the reference we took earlier. If the application has already closed > > the filehandle, then this will release all memory. If the application still has the > > fh open, then only when it closes that fh will the memory be released. > > > > Conclusion: there is no race condition here, this is handled correctly by the core. > > > >> + > >> + /* > >> + * For stream->meta.vdev we can directly call: > >> + * vb2_video_unregister_device(). > >> + */ > >> + vb2_video_unregister_device(&stream->meta.vdev); > > > > Perhaps a patch adding more comments to the vb2_video_unregister_device() > > function might help document this sequence better. > > > > Regards, > > > > Hans > > > >> + > >> + /* > >> + * Now both vdevs are not streaming and all the ioctls will > >> + * return -ENODEV. > >> + */ > >> > >> uvc_debugfs_cleanup_stream(stream); > >> } > >>
On 25/09/2024 10:32, Hans Verkuil wrote: > Hi Laurent, > > We discussed this patch last week, and you thought that there was still > a race condition if the uvc device was unplugged while an application was > in the VIDIOC_DQBUF call waiting for a buffer to arrive (so the vb2 wait_prepare > op is called, which unlocks the serialization mutex). > > I'll go through the code below, explaining why that isn't an issue. > > On 14/06/2024 14:41, 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> >> --- >> 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 bbd90123a4e7..55132688e363 100644 >> --- a/drivers/media/usb/uvc/uvc_driver.c >> +++ b/drivers/media/usb/uvc/uvc_driver.c >> @@ -1908,11 +1908,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); > > This ensures that the device refcount won't go to 0 if video_unregister_device > is called (which calls put_device). > > But note that if an application called VIDIOC_DQBUF and is waiting for a buffer, > then that open filehandle also called get_device(). So while that application is > waiting, the device refcount will never go to 0. > >> + >> + /* 2. Ensure that no new ioctls can be called. */ >> video_unregister_device(&stream->vdev); After calling video_unregister_device, the /dev/videoX device disappears, so nobody can open it again, and any file operations other than close() will return an error. >> - video_unregister_device(&stream->meta.vdev); >> + >> + /* 3. Wait for old ioctls to finish. */ >> + mutex_lock(&stream->mutex); > > If VIDIOC_DQBUF is waiting for a buffer to arrive, then indeed we can take this > lock here. So in that case this won't wait for that specific ioctl to finish. > >> + >> + /* 4. Stop streaming. */ >> + uvc_queue_release(&stream->queue); > > This will __vb2_queue_cancel() which will stop streaming and wake up the wait for > buffers in VIDIOC_DQBUF. It will try to lock this mutex again, and sleeps while > waiting for the mutex to become available. vb2_core_queue_release() calls __vb2_queue_cancel and also __vb2_queue_free(), and that last call is done with mmap_lock held. This ensures that it safely serializes with mmap() calls: either mmap() succeeded before the queue buffers are freed (and so a mapping will still exist for a buffer), or the queue buffers were freed first and mmap will return an error since the buffer it wanted to mmap is already gone. So the handling of mmap() is also safe to the best of my knowledge. I remembered we talked briefly about this as well during the conference, so I wanted to describe this as well. Regards, Hans > >> + >> + mutex_unlock(&stream->mutex); > > At this point it can take the mutex again. But since q->streaming is now false, > (due to the __vb2_queue_cancel call) this will return an error which is returned > to userspace. > >> + >> + put_device(&stream->vdev.dev); > > This releases the reference we took earlier. If the application has already closed > the filehandle, then this will release all memory. If the application still has the > fh open, then only when it closes that fh will the memory be released. > > Conclusion: there is no race condition here, this is handled correctly by the core. > >> + >> + /* >> + * For stream->meta.vdev we can directly call: >> + * vb2_video_unregister_device(). >> + */ >> + vb2_video_unregister_device(&stream->meta.vdev); > > Perhaps a patch adding more comments to the vb2_video_unregister_device() > function might help document this sequence better. > > Regards, > > Hans > >> + >> + /* >> + * Now both vdevs are not streaming and all the ioctls will >> + * return -ENODEV. >> + */ >> >> uvc_debugfs_cleanup_stream(stream); >> } >> > >
diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c index bbd90123a4e7..55132688e363 100644 --- a/drivers/media/usb/uvc/uvc_driver.c +++ b/drivers/media/usb/uvc/uvc_driver.c @@ -1908,11 +1908,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); }