Back in 2020 Guenter published a set of patches to fix some race
conditions on UVC.
https://lore.kernel.org/all/[email protected]/
That kind of race conditions are not only seen on UVC, but are a common
sin on almost all the kernel, so this is what it was decided back then
that we should try to fix them at higher levels.
After that. A lot of video_is_registered() were added to the core:
```
ribalda@alco:~/work/linux$ git grep is_registered drivers/media/v4l2-core/
drivers/media/v4l2-core/v4l2-compat-ioctl32.c: if (!video_is_registered(vdev))
drivers/media/v4l2-core/v4l2-dev.c: if (video_is_registered(vdev))
drivers/media/v4l2-core/v4l2-dev.c: if (video_is_registered(vdev))
drivers/media/v4l2-core/v4l2-dev.c: if (video_is_registered(vdev)) {
drivers/media/v4l2-core/v4l2-dev.c: if (video_is_registered(vdev))
drivers/media/v4l2-core/v4l2-dev.c: if (!video_is_registered(vdev))
drivers/media/v4l2-core/v4l2-dev.c: if (video_is_registered(vdev))
drivers/media/v4l2-core/v4l2-dev.c: if (vdev == NULL || !video_is_registered(vdev)) {
drivers/media/v4l2-core/v4l2-dev.c: if (video_is_registered(vdev))
drivers/media/v4l2-core/v4l2-dev.c: if (!vdev || !video_is_registered(vdev))
drivers/media/v4l2-core/v4l2-ioctl.c: if (!video_is_registered(vfd)) {
drivers/media/v4l2-core/v4l2-subdev.c: if (video_is_registered(vdev)) {
```
And recently Sakari is trying to land:
https://lore.kernel.org/linux-media/[email protected]/
Which will make obsolete a lot of (all?) of the video_is_registered() checks on
Guenter's patches.
Besides those checks, there were some other valid races fixed on his
patches.
This series is just a rebase of what I think is missing on UVC even
if we fixed v4l2/core with all the video_is_register() checks removed.
I have tested the series with lockdep and a loop of authorize/de-authorize
while steaming.
Thanks!
Signed-off-by: Ricardo Ribalda <[email protected]>
---
Changes in v4 Thanks Sergey!:
- Reorder patches
- Improve commit messages
- Do not process async work on exit.
- Link to v3: https://lore.kernel.org/r/[email protected]
Changes in v3:
- Introduce media: uvcvideo: Do not halt the device after disconnect
- Introduce media: uvcvideo: Always use uvc_status_stop()
- Link to v2: https://lore.kernel.org/r/[email protected]
Changes in v2:
- Actually send the series to the ML an not only to individuals.
- Link to v1: https://lore.kernel.org/r/[email protected]
---
Guenter Roeck (1):
media: uvcvideo: Lock video streams and queues while unregistering
Ricardo Ribalda (2):
media: uvcvideo: Always use uvc_status_stop()
media: uvcvideo: Do not use usb_* functions after .disconnect
drivers/media/usb/uvc/uvc_ctrl.c | 4 ----
drivers/media/usb/uvc/uvc_driver.c | 13 ++++++++++-
drivers/media/usb/uvc/uvc_status.c | 8 +++----
drivers/media/usb/uvc/uvc_v4l2.c | 2 +-
drivers/media/usb/uvc/uvc_video.c | 45 ++++++++++++++++++++++++--------------
drivers/media/usb/uvc/uvcvideo.h | 4 +++-
6 files changed, 48 insertions(+), 28 deletions(-)
---
base-commit: 98b1cc82c4affc16f5598d4fa14b1858671b2263
change-id: 20230309-guenter-mini-89861b084ef1
Best regards,
--
Ricardo Ribalda <[email protected]>
From: Guenter Roeck <[email protected]>
The call to uvc_disconnect() is not protected by any mutex.
This means it can and will be called while other accesses to the video
device are in progress. This can cause all kinds of race conditions,
including crashes such as the following.
usb 1-4: USB disconnect, device number 3
BUG: unable to handle kernel NULL pointer dereference at 0000000000000000
PGD 0 P4D 0
Oops: 0000 [#1] PREEMPT SMP PTI
CPU: 0 PID: 5633 Comm: V4L2CaptureThre Not tainted 4.19.113-08536-g5d29ca36db06 #1
Hardware name: GOOGLE Edgar, BIOS Google_Edgar.7287.167.156 03/25/2019
RIP: 0010:usb_ifnum_to_if+0x29/0x40
Code: <...>
RSP: 0018:ffffa46f42a47a80 EFLAGS: 00010246
RAX: 0000000000000000 RBX: 0000000000000000 RCX: ffff904a396c9000
RDX: ffff904a39641320 RSI: 0000000000000001 RDI: 0000000000000000
RBP: ffffa46f42a47a80 R08: 0000000000000002 R09: 0000000000000000
R10: 0000000000009975 R11: 0000000000000009 R12: 0000000000000000
R13: ffff904a396b3800 R14: ffff904a39e88000 R15: 0000000000000000
FS: 00007f396448e700(0000) GS:ffff904a3ba00000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000000000 CR3: 000000016cb46000 CR4: 00000000001006f0
Call Trace:
usb_hcd_alloc_bandwidth+0x1ee/0x30f
usb_set_interface+0x1a3/0x2b7
uvc_video_start_transfer+0x29b/0x4b8 [uvcvideo]
uvc_video_start_streaming+0x91/0xdd [uvcvideo]
uvc_start_streaming+0x28/0x5d [uvcvideo]
vb2_start_streaming+0x61/0x143 [videobuf2_common]
vb2_core_streamon+0xf7/0x10f [videobuf2_common]
uvc_queue_streamon+0x2e/0x41 [uvcvideo]
uvc_ioctl_streamon+0x42/0x5c [uvcvideo]
__video_do_ioctl+0x33d/0x42a
video_usercopy+0x34e/0x5ff
? video_ioctl2+0x16/0x16
v4l2_ioctl+0x46/0x53
do_vfs_ioctl+0x50a/0x76f
ksys_ioctl+0x58/0x83
__x64_sys_ioctl+0x1a/0x1e
do_syscall_64+0x54/0xde
usb_set_interface() should not be called after the USB device has been
unregistered. However, in the above case the disconnect happened after
v4l2_ioctl() was called, but before the call to usb_ifnum_to_if().
Acquire various mutexes in uvc_unregister_video() to fix the majority
(maybe all) of the observed race conditions.
The uvc_device lock prevents races against suspend and resume calls
and the poll function.
The uvc_streaming lock prevents races against stream related functions;
for the most part, those are ioctls. This lock also requires other
functions using this lock to check if a video device is still registered
after acquiring it. For example, it was observed that the video device
was already unregistered by the time the stream lock was acquired in
uvc_ioctl_streamon().
The uvc_queue lock prevents races against queue functions, Most of
those are already protected by the uvc_streaming lock, but some
are called directly. This is done as added protection; an actual race
was not (yet) observed.
Cc: Laurent Pinchart <[email protected]>
Cc: Alan Stern <[email protected]>
Cc: Hans Verkuil <[email protected]>
Reviewed-by: Tomasz Figa <[email protected]>
Reviewed-by: Sean Paul <[email protected]>
Signed-off-by: Guenter Roeck <[email protected]>
Reviewed-by: Sergey Senozhatsky <[email protected]>
Signed-off-by: Ricardo Ribalda <[email protected]>
---
drivers/media/usb/uvc/uvc_driver.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index 08fcd2ffa727..ded2cb6ce14f 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -1907,14 +1907,22 @@ static void uvc_unregister_video(struct uvc_device *dev)
{
struct uvc_streaming *stream;
+ mutex_lock(&dev->lock);
+
list_for_each_entry(stream, &dev->streams, list) {
if (!video_is_registered(&stream->vdev))
continue;
+ mutex_lock(&stream->mutex);
+ mutex_lock(&stream->queue.mutex);
+
video_unregister_device(&stream->vdev);
video_unregister_device(&stream->meta.vdev);
uvc_debugfs_cleanup_stream(stream);
+
+ mutex_unlock(&stream->queue.mutex);
+ mutex_unlock(&stream->mutex);
}
uvc_status_unregister(dev);
@@ -1925,6 +1933,7 @@ static void uvc_unregister_video(struct uvc_device *dev)
if (media_devnode_is_registered(dev->mdev.devnode))
media_device_unregister(&dev->mdev);
#endif
+ mutex_unlock(&dev->lock);
}
int uvc_register_video_device(struct uvc_device *dev,
--
2.43.0.rc1.413.gea7ed67945-goog
The only thread safe way to stop the status handler is with uvc_status.
Let's remove all the code paths partially stopping uvc_status.
Reviewed-by: Sakari Ailus <[email protected]>
Signed-off-by: Ricardo Ribalda <[email protected]>
---
drivers/media/usb/uvc/uvc_ctrl.c | 4 ----
drivers/media/usb/uvc/uvc_status.c | 2 +-
2 files changed, 1 insertion(+), 5 deletions(-)
diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index e59a463c2761..8e22a07e3e7b 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -2765,10 +2765,6 @@ void uvc_ctrl_cleanup_device(struct uvc_device *dev)
struct uvc_entity *entity;
unsigned int i;
- /* Can be uninitialized if we are aborting on probe error. */
- if (dev->async_ctrl.work.func)
- cancel_work_sync(&dev->async_ctrl.work);
-
/* Free controls and control mappings for all entities. */
list_for_each_entry(entity, &dev->entities, list) {
for (i = 0; i < entity->ncontrols; ++i) {
diff --git a/drivers/media/usb/uvc/uvc_status.c b/drivers/media/usb/uvc/uvc_status.c
index a78a88c710e2..0208612a9f12 100644
--- a/drivers/media/usb/uvc/uvc_status.c
+++ b/drivers/media/usb/uvc/uvc_status.c
@@ -292,7 +292,7 @@ int uvc_status_init(struct uvc_device *dev)
void uvc_status_unregister(struct uvc_device *dev)
{
- usb_kill_urb(dev->int_urb);
+ uvc_status_stop(dev);
uvc_input_unregister(dev);
}
--
2.43.0.rc1.413.gea7ed67945-goog