| Message ID | 20240926-uvc_stop_streaming-v1-1-038180fafe5f@chromium.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | media: uvcvideo: Stop stream during unregister | expand |
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. 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@xs4all.nl> > Suggested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.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 > > Best regards,
Hi Hans, 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 ? > 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@xs4all.nl> > > Suggested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.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 > > > > Best regards, >
On 07/10/2024 16:44, Laurent Pinchart wrote: > Hi Hans, > > 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. Regards, 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@xs4all.nl> >>> Suggested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> >>> Signed-off-by: Ricardo Ribalda <ribalda@chromium.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 >>> >>> Best regards, >> >
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 ? > >> 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@xs4all.nl> > >>> Suggested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > >>> Signed-off-by: Ricardo Ribalda <ribalda@chromium.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
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 ? 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@xs4all.nl> >>>>> Suggested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> >>>>> Signed-off-by: Ricardo Ribalda <ribalda@chromium.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 >
Em Thu, 26 Sep 2024 05:59:06 +0000 Ricardo Ribalda <ribalda@chromium.org> escreveu: > 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> I didn't test it, but the patch looks OK on my eyes. Reviewed-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.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 > > Best regards, Thanks, Mauro
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@xs4all.nl> >>>>>> Suggested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> >>>>>> Signed-off-by: Ricardo Ribalda <ribalda@chromium.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 >> > >
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); }