| Message ID | 20240327-guenter-mini-v4-1-49955c198eae@chromium.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | uvcvideo: Attempt N to land UVC race conditions fixes | expand |
Hi Ricardo, On Wed, Mar 27, 2024 at 5:24 PM Ricardo Ribalda <ribalda@chromium.org> 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?
On 06/06/2024 12:04, Tomasz Figa wrote: > Hi Hans, > > On Tue, May 28, 2024 at 4:55 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote: >> >> On 27/03/2024 09:24, 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(+) >>> >>> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c >>> index bbd90123a4e76..17fc945c8deb6 100644 >>> --- a/drivers/media/usb/uvc/uvc_driver.c >>> +++ b/drivers/media/usb/uvc/uvc_driver.c >>> @@ -1911,8 +1911,19 @@ static void uvc_unregister_video(struct uvc_device *dev) >>> if (!video_is_registered(&stream->vdev)) >>> continue; >>> >>> + /* >>> + * Serialize other access to the stream. >>> + */ >>> + mutex_lock(&stream->mutex); >>> + uvc_queue_streamoff(&stream->queue, stream->type); >>> video_unregister_device(&stream->vdev); >>> video_unregister_device(&stream->meta.vdev); >>> + mutex_unlock(&stream->mutex); >> >> This sequence isn't fool proof. You have to follow the same sequence as >> vb2_video_unregister_device(): take a reference to the video device, >> then unregister, then take the stream mutex and call vb2_queue_release >> for each queue. Finally unlock the mutex and call put_device. > > vb2_queue_release() will run when the userspace releases the file > handle that owns the vb2 queue [1], so we shouldn't really need to > call it here. > > [1] https://elixir.bootlin.com/linux/v6.9.3/source/drivers/media/usb/uvc/uvc_v4l2.c#L655 > >> >> Doing the video_unregister_device first ensures no new ioctls can be >> called on that device node. Taking the mutex ensures that any still >> running ioctls will finish (since it will sleep until they release the >> mutex), and then you can release the queue. > > Actually isn't the only missing thing in the code basically ensuring > that any ioctl currently being executed finishes? Why is the streamoff > or queue release needed? See below... > > Best regards, > Tomasz > >> >> This does require that you call get_device before calling video_unregister_device, >> otherwise everything might be released at that point. >> >> Instead of vb2_queue_release() you might have to call uvc_queue_streamoff, >> but note that there are two queues here: video and meta. The code above >> just calls streamoff for the video device. >> >> For the meta device I think you can just use vb2_video_unregister_device() >> directly, since that sets vdev->queue and vdev->queue.lock to the correct >> values. That would just leave the video device where you need to do this >> manually. Ideally uvc should just use centralized locking (i.e. set vdev->queue.lock) for the video node, just like it does for the meta device. If that was the case, then it can just call vb2_video_unregister_device(). The vb2_video_unregister_device helper was added to ensure that no ioctls are running, and then streaming is stopped and the queue is released. While it is true that the queue is released automatically when the last user closes the filehandle, it can get complicated if the application has a file handle open when the device is unregistered (usually due to it being unplugged). Without that call to vb2_video_unregister_device() the queue is still active, and especially if you also have subdevices that are still in streaming mode, it is hard to make sure nothing is still using the hardware. vb2_video_unregister_device() provides a clean way of ensuring that when the device is unregistered all streaming is stopped and the vb2 queue is released. After that the only file operation that can be used without returning an error is close(). Since uvc uses its own locking for the video device, it has to do this manually. It is probably enough to ensure no ioctls are running since uvc doesn't have subdevices, but I think it makes sense to stop streaming and release the queue when unregistering the device, it is weird to postpone that until the last fh is closed. Regards, Hans >> >> Regards, >> >> Hans >> >>> + >>> + /* >>> + * Now the vdev is not streaming and all the ioctls will >>> + * return -ENODEV >>> + */ >>> >>> uvc_debugfs_cleanup_stream(stream); >>> } >>> >>
On Thu, Jun 6, 2024 at 8:58 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote: > > On 06/06/2024 12:04, Tomasz Figa wrote: > > Hi Hans, > > > > On Tue, May 28, 2024 at 4:55 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote: > >> > >> On 27/03/2024 09:24, 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(+) > >>> > >>> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c > >>> index bbd90123a4e76..17fc945c8deb6 100644 > >>> --- a/drivers/media/usb/uvc/uvc_driver.c > >>> +++ b/drivers/media/usb/uvc/uvc_driver.c > >>> @@ -1911,8 +1911,19 @@ static void uvc_unregister_video(struct uvc_device *dev) > >>> if (!video_is_registered(&stream->vdev)) > >>> continue; > >>> > >>> + /* > >>> + * Serialize other access to the stream. > >>> + */ > >>> + mutex_lock(&stream->mutex); > >>> + uvc_queue_streamoff(&stream->queue, stream->type); > >>> video_unregister_device(&stream->vdev); > >>> video_unregister_device(&stream->meta.vdev); > >>> + mutex_unlock(&stream->mutex); > >> > >> This sequence isn't fool proof. You have to follow the same sequence as > >> vb2_video_unregister_device(): take a reference to the video device, > >> then unregister, then take the stream mutex and call vb2_queue_release > >> for each queue. Finally unlock the mutex and call put_device. > > > > vb2_queue_release() will run when the userspace releases the file > > handle that owns the vb2 queue [1], so we shouldn't really need to > > call it here. > > > > [1] https://elixir.bootlin.com/linux/v6.9.3/source/drivers/media/usb/uvc/uvc_v4l2.c#L655 > > > >> > >> Doing the video_unregister_device first ensures no new ioctls can be > >> called on that device node. Taking the mutex ensures that any still > >> running ioctls will finish (since it will sleep until they release the > >> mutex), and then you can release the queue. > > > > Actually isn't the only missing thing in the code basically ensuring > > that any ioctl currently being executed finishes? Why is the streamoff > > or queue release needed? > > See below... > > > > > Best regards, > > Tomasz > > > >> > >> This does require that you call get_device before calling video_unregister_device, > >> otherwise everything might be released at that point. > >> > >> Instead of vb2_queue_release() you might have to call uvc_queue_streamoff, > >> but note that there are two queues here: video and meta. The code above > >> just calls streamoff for the video device. > >> > >> For the meta device I think you can just use vb2_video_unregister_device() > >> directly, since that sets vdev->queue and vdev->queue.lock to the correct > >> values. That would just leave the video device where you need to do this > >> manually. > > Ideally uvc should just use centralized locking (i.e. set vdev->queue.lock) > for the video node, just like it does for the meta device. > > If that was the case, then it can just call vb2_video_unregister_device(). > The vb2_video_unregister_device helper was added to ensure that no ioctls > are running, and then streaming is stopped and the queue is released. Yes, it would be as simple as that if it used standard locking, but since it does its own stuff, I'm not very confident that the same logic as in vb2_video_unregister_device() would work fine for it as well. > > While it is true that the queue is released automatically when the last user > closes the filehandle, it can get complicated if the application has a file > handle open when the device is unregistered (usually due to it being unplugged). > Without that call to vb2_video_unregister_device() the queue is still active, > and especially if you also have subdevices that are still in streaming mode, > it is hard to make sure nothing is still using the hardware. > > vb2_video_unregister_device() provides a clean way of ensuring that when the > device is unregistered all streaming is stopped and the vb2 queue is released. > > After that the only file operation that can be used without returning an error > is close(). > > Since uvc uses its own locking for the video device, it has to do this manually. > It is probably enough to ensure no ioctls are running since uvc doesn't have > subdevices, but I think it makes sense to stop streaming and release the queue > when unregistering the device, it is weird to postpone that until the last fh > is closed. My comment comes from a concern that releasing objects and changing internal state in the release callback may break some assumptions in the operations that will follow once the lock is released, e.g. close(), leading to some kind of double frees or dereferencing freed objects. Therefore just preventing new opens and setting the video_device as unregistered could be potentially a safer option until the driver is converted to standard locking. (Is there even a plan to do that?) Best regards, Tomasz > > Regards, > > Hans > > > >> > >> Regards, > >> > >> Hans > >> > >>> + > >>> + /* > >>> + * Now the vdev is not streaming and all the ioctls will > >>> + * return -ENODEV > >>> + */ > >>> > >>> uvc_debugfs_cleanup_stream(stream); > >>> } > >>> > >> > >
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). > [1] https://elixir.bootlin.com/linux/latest/source/drivers/media/v4l2-core/v4l2-ioctl.c#L3023 > > > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c > > index bbd90123a4e76..17fc945c8deb6 100644 > > --- a/drivers/media/usb/uvc/uvc_driver.c > > +++ b/drivers/media/usb/uvc/uvc_driver.c > > @@ -1911,8 +1911,19 @@ static void uvc_unregister_video(struct uvc_device *dev) > > if (!video_is_registered(&stream->vdev)) > > continue; > > > > + /* > > + * Serialize other access to the stream. > > + */ > > + mutex_lock(&stream->mutex); > > + uvc_queue_streamoff(&stream->queue, stream->type); > > video_unregister_device(&stream->vdev); > > video_unregister_device(&stream->meta.vdev); > > + mutex_unlock(&stream->mutex); > > + > > + /* > > + * Now the vdev is not streaming and all the ioctls will > > + * return -ENODEV > > + */ > > > > uvc_debugfs_cleanup_stream(stream); > > }
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. Regards, Hans > >> [1] https://elixir.bootlin.com/linux/latest/source/drivers/media/v4l2-core/v4l2-ioctl.c#L3023 >> >>> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c >>> index bbd90123a4e76..17fc945c8deb6 100644 >>> --- a/drivers/media/usb/uvc/uvc_driver.c >>> +++ b/drivers/media/usb/uvc/uvc_driver.c >>> @@ -1911,8 +1911,19 @@ static void uvc_unregister_video(struct uvc_device *dev) >>> if (!video_is_registered(&stream->vdev)) >>> continue; >>> >>> + /* >>> + * Serialize other access to the stream. >>> + */ >>> + mutex_lock(&stream->mutex); >>> + uvc_queue_streamoff(&stream->queue, stream->type); >>> video_unregister_device(&stream->vdev); >>> video_unregister_device(&stream->meta.vdev); >>> + mutex_unlock(&stream->mutex); >>> + >>> + /* >>> + * Now the vdev is not streaming and all the ioctls will >>> + * return -ENODEV >>> + */ >>> >>> uvc_debugfs_cleanup_stream(stream); >>> } >
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.
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
diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c index bbd90123a4e76..17fc945c8deb6 100644 --- a/drivers/media/usb/uvc/uvc_driver.c +++ b/drivers/media/usb/uvc/uvc_driver.c @@ -1911,8 +1911,19 @@ static void uvc_unregister_video(struct uvc_device *dev) if (!video_is_registered(&stream->vdev)) continue; + /* + * Serialize other access to the stream. + */ + mutex_lock(&stream->mutex); + uvc_queue_streamoff(&stream->queue, stream->type); video_unregister_device(&stream->vdev); video_unregister_device(&stream->meta.vdev); + mutex_unlock(&stream->mutex); + + /* + * Now the vdev is not streaming and all the ioctls will + * return -ENODEV + */ uvc_debugfs_cleanup_stream(stream); }
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(+)