* [PATCH v5 1/3] media: uvcvideo: Lock video streams and queues while unregistering
2023-11-22 11:45 [PATCH v5 0/3] uvcvideo: Attempt N to land UVC race conditions fixes Ricardo Ribalda
@ 2023-11-22 11:45 ` Ricardo Ribalda
2024-03-21 15:47 ` Hans Verkuil
2023-11-22 11:45 ` [PATCH v5 2/3] media: uvcvideo: Always use uvc_status_stop() Ricardo Ribalda
2023-11-22 11:45 ` [PATCH v5 3/3] media: uvcvideo: Do not use usb_* functions after .disconnect Ricardo Ribalda
2 siblings, 1 reply; 11+ messages in thread
From: Ricardo Ribalda @ 2023-11-22 11:45 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Guenter Roeck, Tomasz Figa, Laurent Pinchart, Alan Stern,
Hans Verkuil, linux-media, linux-kernel, Sean Paul,
Ricardo Ribalda, Sakari Ailus, Sergey Senozhatsky
From: Guenter Roeck <linux@roeck-us.net>
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 <laurent.pinchart@ideasonboard.com>
Cc: Alan Stern <stern@rowland.harvard.edu>
Cc: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Reviewed-by: Tomasz Figa <tfiga@chromium.org>
Reviewed-by: Sean Paul <seanpaul@chromium.org>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org>
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
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
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH v5 1/3] media: uvcvideo: Lock video streams and queues while unregistering
2023-11-22 11:45 ` [PATCH v5 1/3] media: uvcvideo: Lock video streams and queues while unregistering Ricardo Ribalda
@ 2024-03-21 15:47 ` Hans Verkuil
2024-03-25 16:37 ` Ricardo Ribalda
0 siblings, 1 reply; 11+ messages in thread
From: Hans Verkuil @ 2024-03-21 15:47 UTC (permalink / raw)
To: Ricardo Ribalda, Mauro Carvalho Chehab
Cc: Guenter Roeck, Tomasz Figa, Laurent Pinchart, Alan Stern,
linux-media, linux-kernel, Sean Paul, Sakari Ailus,
Sergey Senozhatsky
Hi Ricardo,
On 22/11/2023 12:45 pm, Ricardo Ribalda wrote:
> From: Guenter Roeck <linux@roeck-us.net>
>
> 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 <laurent.pinchart@ideasonboard.com>
> Cc: Alan Stern <stern@rowland.harvard.edu>
> Cc: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> Reviewed-by: Tomasz Figa <tfiga@chromium.org>
> Reviewed-by: Sean Paul <seanpaul@chromium.org>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
> 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);
Part of the problem here is that video_unregister_device does not stop streaming.
For 'normal' drivers that leave most of the locking to the core framework and
that use the standard vb2_fop_* and vb2_ioctl_* helpers, instead of calling
video_unregister_device, they call vb2_video_unregister_device(). This will
safely unregister the device and stop streaming at the same time.
Since after the device is unregistered the only file operation that is accepted
is close(), it will be impossible to restart streaming.
In other words, this guarantees that when the disconnect function exits there
is nothing streaming anymore.
This makes it much easier to deal with disconnects, and I think this should
happen here as well. I wonder if this will obsolete patch 3/3 and perhaps
2/3.
I don't think taking the queue.mutex is needed, especially if you stop
streaming here.
Regards,
Hans
> }
>
> 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,
>
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v5 1/3] media: uvcvideo: Lock video streams and queues while unregistering
2024-03-21 15:47 ` Hans Verkuil
@ 2024-03-25 16:37 ` Ricardo Ribalda
0 siblings, 0 replies; 11+ messages in thread
From: Ricardo Ribalda @ 2024-03-25 16:37 UTC (permalink / raw)
To: Hans Verkuil
Cc: Mauro Carvalho Chehab, Guenter Roeck, Tomasz Figa,
Laurent Pinchart, Alan Stern, linux-media, linux-kernel,
Sean Paul, Sakari Ailus, Sergey Senozhatsky
Hi Hans
On Thu, 21 Mar 2024 at 16:47, Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
>
> Hi Ricardo,
>
> On 22/11/2023 12:45 pm, Ricardo Ribalda wrote:
> > From: Guenter Roeck <linux@roeck-us.net>
> >
> > 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 <laurent.pinchart@ideasonboard.com>
> > Cc: Alan Stern <stern@rowland.harvard.edu>
> > Cc: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> > Reviewed-by: Tomasz Figa <tfiga@chromium.org>
> > Reviewed-by: Sean Paul <seanpaul@chromium.org>
> > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > ---
> > 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);
>
> Part of the problem here is that video_unregister_device does not stop streaming.
>
> For 'normal' drivers that leave most of the locking to the core framework and
> that use the standard vb2_fop_* and vb2_ioctl_* helpers, instead of calling
> video_unregister_device, they call vb2_video_unregister_device(). This will
> safely unregister the device and stop streaming at the same time.
>
> Since after the device is unregistered the only file operation that is accepted
> is close(), it will be impossible to restart streaming.
>
> In other words, this guarantees that when the disconnect function exits there
> is nothing streaming anymore.
I have tested this approach and it seems to work :)
I have updated and sent for review, and in the meantime I will
continue torturing my device to figure out if we are missing any other
race.
Thanks!
>
> This makes it much easier to deal with disconnects, and I think this should
> happen here as well. I wonder if this will obsolete patch 3/3 and perhaps
> 2/3.
>
> I don't think taking the queue.mutex is needed, especially if you stop
> streaming here.
>
> Regards,
>
> Hans
>
> > }
> >
> > 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,
> >
>
--
Ricardo Ribalda
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v5 2/3] media: uvcvideo: Always use uvc_status_stop()
2023-11-22 11:45 [PATCH v5 0/3] uvcvideo: Attempt N to land UVC race conditions fixes Ricardo Ribalda
2023-11-22 11:45 ` [PATCH v5 1/3] media: uvcvideo: Lock video streams and queues while unregistering Ricardo Ribalda
@ 2023-11-22 11:45 ` Ricardo Ribalda
2023-11-22 13:32 ` Laurent Pinchart
2023-11-22 11:45 ` [PATCH v5 3/3] media: uvcvideo: Do not use usb_* functions after .disconnect Ricardo Ribalda
2 siblings, 1 reply; 11+ messages in thread
From: Ricardo Ribalda @ 2023-11-22 11:45 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Guenter Roeck, Tomasz Figa, Laurent Pinchart, Alan Stern,
Hans Verkuil, linux-media, linux-kernel, Sean Paul,
Ricardo Ribalda, Sakari Ailus, Sakari Ailus
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 <sakari.ailus@linux.intel.com>
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
drivers/media/usb/uvc/uvc_ctrl.c | 4 ----
drivers/media/usb/uvc/uvc_driver.c | 2 +-
drivers/media/usb/uvc/uvc_status.c | 8 ++++----
drivers/media/usb/uvc/uvc_v4l2.c | 2 +-
drivers/media/usb/uvc/uvcvideo.h | 2 +-
5 files changed, 7 insertions(+), 11 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_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index ded2cb6ce14f..d5dbf2644272 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -2282,7 +2282,7 @@ static int uvc_suspend(struct usb_interface *intf, pm_message_t message)
UVC_SC_VIDEOCONTROL) {
mutex_lock(&dev->lock);
if (dev->users)
- uvc_status_stop(dev);
+ uvc_status_stop(dev, true);
mutex_unlock(&dev->lock);
return 0;
}
diff --git a/drivers/media/usb/uvc/uvc_status.c b/drivers/media/usb/uvc/uvc_status.c
index a78a88c710e2..9c5da1244999 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, false);
uvc_input_unregister(dev);
}
@@ -310,7 +310,7 @@ int uvc_status_start(struct uvc_device *dev, gfp_t flags)
return usb_submit_urb(dev->int_urb, flags);
}
-void uvc_status_stop(struct uvc_device *dev)
+void uvc_status_stop(struct uvc_device *dev, bool run_async_work)
{
struct uvc_ctrl_work *w = &dev->async_ctrl;
@@ -326,7 +326,7 @@ void uvc_status_stop(struct uvc_device *dev)
* Cancel any pending asynchronous work. If any status event was queued,
* process it synchronously.
*/
- if (cancel_work_sync(&w->work))
+ if (cancel_work_sync(&w->work) && run_async_work)
uvc_ctrl_status_event(w->chain, w->ctrl, w->data);
/* Kill the urb. */
@@ -338,7 +338,7 @@ void uvc_status_stop(struct uvc_device *dev)
* cancelled before returning or it could then race with a future
* uvc_status_start() call.
*/
- if (cancel_work_sync(&w->work))
+ if (cancel_work_sync(&w->work) && run_async_work)
uvc_ctrl_status_event(w->chain, w->ctrl, w->data);
/*
diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
index f4988f03640a..f90206263ff4 100644
--- a/drivers/media/usb/uvc/uvc_v4l2.c
+++ b/drivers/media/usb/uvc/uvc_v4l2.c
@@ -672,7 +672,7 @@ static int uvc_v4l2_release(struct file *file)
mutex_lock(&stream->dev->lock);
if (--stream->dev->users == 0)
- uvc_status_stop(stream->dev);
+ uvc_status_stop(stream->dev, false);
mutex_unlock(&stream->dev->lock);
usb_autopm_put_interface(stream->dev->intf);
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index 6fb0a78b1b00..ba8f8c1f2c83 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -745,7 +745,7 @@ int uvc_status_init(struct uvc_device *dev);
void uvc_status_unregister(struct uvc_device *dev);
void uvc_status_cleanup(struct uvc_device *dev);
int uvc_status_start(struct uvc_device *dev, gfp_t flags);
-void uvc_status_stop(struct uvc_device *dev);
+void uvc_status_stop(struct uvc_device *dev, bool run_async_work);
/* Controls */
extern const struct uvc_control_mapping uvc_ctrl_power_line_mapping_limited;
--
2.43.0.rc1.413.gea7ed67945-goog
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH v5 2/3] media: uvcvideo: Always use uvc_status_stop()
2023-11-22 11:45 ` [PATCH v5 2/3] media: uvcvideo: Always use uvc_status_stop() Ricardo Ribalda
@ 2023-11-22 13:32 ` Laurent Pinchart
0 siblings, 0 replies; 11+ messages in thread
From: Laurent Pinchart @ 2023-11-22 13:32 UTC (permalink / raw)
To: Ricardo Ribalda
Cc: Mauro Carvalho Chehab, Guenter Roeck, Tomasz Figa, Alan Stern,
Hans Verkuil, linux-media, linux-kernel, Sean Paul, Sakari Ailus
Hi Ricardo,
Thank you for the patch.
On Wed, Nov 22, 2023 at 11:45:48AM +0000, Ricardo Ribalda wrote:
> The only thread safe way to stop the status handler is with uvc_status.
The commit message should explain why.
> Let's remove all the code paths partially stopping uvc_status.
And should explain what the implications are.
> Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
> drivers/media/usb/uvc/uvc_ctrl.c | 4 ----
> drivers/media/usb/uvc/uvc_driver.c | 2 +-
> drivers/media/usb/uvc/uvc_status.c | 8 ++++----
> drivers/media/usb/uvc/uvc_v4l2.c | 2 +-
> drivers/media/usb/uvc/uvcvideo.h | 2 +-
> 5 files changed, 7 insertions(+), 11 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_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index ded2cb6ce14f..d5dbf2644272 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -2282,7 +2282,7 @@ static int uvc_suspend(struct usb_interface *intf, pm_message_t message)
> UVC_SC_VIDEOCONTROL) {
> mutex_lock(&dev->lock);
> if (dev->users)
> - uvc_status_stop(dev);
> + uvc_status_stop(dev, true);
> mutex_unlock(&dev->lock);
> return 0;
> }
> diff --git a/drivers/media/usb/uvc/uvc_status.c b/drivers/media/usb/uvc/uvc_status.c
> index a78a88c710e2..9c5da1244999 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, false);
> uvc_input_unregister(dev);
> }
>
> @@ -310,7 +310,7 @@ int uvc_status_start(struct uvc_device *dev, gfp_t flags)
> return usb_submit_urb(dev->int_urb, flags);
> }
>
> -void uvc_status_stop(struct uvc_device *dev)
> +void uvc_status_stop(struct uvc_device *dev, bool run_async_work)
> {
> struct uvc_ctrl_work *w = &dev->async_ctrl;
>
> @@ -326,7 +326,7 @@ void uvc_status_stop(struct uvc_device *dev)
> * Cancel any pending asynchronous work. If any status event was queued,
> * process it synchronously.
> */
> - if (cancel_work_sync(&w->work))
> + if (cancel_work_sync(&w->work) && run_async_work)
> uvc_ctrl_status_event(w->chain, w->ctrl, w->data);
>
> /* Kill the urb. */
> @@ -338,7 +338,7 @@ void uvc_status_stop(struct uvc_device *dev)
> * cancelled before returning or it could then race with a future
> * uvc_status_start() call.
> */
> - if (cancel_work_sync(&w->work))
> + if (cancel_work_sync(&w->work) && run_async_work)
> uvc_ctrl_status_event(w->chain, w->ctrl, w->data);
>
> /*
> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> index f4988f03640a..f90206263ff4 100644
> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> @@ -672,7 +672,7 @@ static int uvc_v4l2_release(struct file *file)
>
> mutex_lock(&stream->dev->lock);
> if (--stream->dev->users == 0)
> - uvc_status_stop(stream->dev);
> + uvc_status_stop(stream->dev, false);
> mutex_unlock(&stream->dev->lock);
>
> usb_autopm_put_interface(stream->dev->intf);
> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> index 6fb0a78b1b00..ba8f8c1f2c83 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -745,7 +745,7 @@ int uvc_status_init(struct uvc_device *dev);
> void uvc_status_unregister(struct uvc_device *dev);
> void uvc_status_cleanup(struct uvc_device *dev);
> int uvc_status_start(struct uvc_device *dev, gfp_t flags);
> -void uvc_status_stop(struct uvc_device *dev);
> +void uvc_status_stop(struct uvc_device *dev, bool run_async_work);
>
> /* Controls */
> extern const struct uvc_control_mapping uvc_ctrl_power_line_mapping_limited;
>
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v5 3/3] media: uvcvideo: Do not use usb_* functions after .disconnect
2023-11-22 11:45 [PATCH v5 0/3] uvcvideo: Attempt N to land UVC race conditions fixes Ricardo Ribalda
2023-11-22 11:45 ` [PATCH v5 1/3] media: uvcvideo: Lock video streams and queues while unregistering Ricardo Ribalda
2023-11-22 11:45 ` [PATCH v5 2/3] media: uvcvideo: Always use uvc_status_stop() Ricardo Ribalda
@ 2023-11-22 11:45 ` Ricardo Ribalda
2023-11-22 13:33 ` Laurent Pinchart
2 siblings, 1 reply; 11+ messages in thread
From: Ricardo Ribalda @ 2023-11-22 11:45 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Guenter Roeck, Tomasz Figa, Laurent Pinchart, Alan Stern,
Hans Verkuil, linux-media, linux-kernel, Sean Paul,
Ricardo Ribalda, Sakari Ailus
usb drivers should not call to any I/O function after the
.disconnect() callback has been triggered.
https://www.kernel.org/doc/html/latest/driver-api/usb/callbacks.html#the-disconnect-callback
If an application is receiving frames form a camera and the device is
disconnected: the device will call close() after the usb .disconnect()
callback has been called. The streamoff path will call usb_set_interface
or usb_clear_halt, which is not allowed.
This patch only solves the calls to close() *after* .disconnect() is
being called.
Trace:
[ 1065.389723] drivers/media/usb/uvc/uvc_driver.c:2248 uvc_disconnect enter
[ 1065.390160] drivers/media/usb/uvc/uvc_driver.c:2264 uvc_disconnect exit
[ 1065.433956] drivers/media/usb/uvc/uvc_v4l2.c:659 uvc_v4l2_release enter
[ 1065.433973] drivers/media/usb/uvc/uvc_video.c:2274 uvc_video_stop_streaming enter
[ 1065.434560] drivers/media/usb/uvc/uvc_video.c:2285 uvc_video_stop_streaming exit
[ 1065.435154] drivers/media/usb/uvc/uvc_v4l2.c:680 uvc_v4l2_release exit
[ 1065.435188] drivers/media/usb/uvc/uvc_driver.c:2248 uvc_disconnect enter
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
drivers/media/usb/uvc/uvc_driver.c | 2 ++
drivers/media/usb/uvc/uvc_video.c | 45 ++++++++++++++++++++++++--------------
drivers/media/usb/uvc/uvcvideo.h | 2 ++
3 files changed, 32 insertions(+), 17 deletions(-)
diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index d5dbf2644272..d78640d422f4 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -2266,6 +2266,8 @@ static void uvc_disconnect(struct usb_interface *intf)
return;
uvc_unregister_video(dev);
+ /* Barrier needed to pair with uvc_video_stop_streaming(). */
+ smp_store_release(&dev->disconnected, true);
kref_put(&dev->ref, uvc_delete);
}
diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
index 28dde08ec6c5..f5ef375088de 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -2243,28 +2243,39 @@ int uvc_video_start_streaming(struct uvc_streaming *stream)
return ret;
}
-void uvc_video_stop_streaming(struct uvc_streaming *stream)
+static void uvc_video_halt(struct uvc_streaming *stream)
{
- uvc_video_stop_transfer(stream, 1);
+ unsigned int epnum;
+ unsigned int pipe;
+ unsigned int dir;
if (stream->intf->num_altsetting > 1) {
usb_set_interface(stream->dev->udev, stream->intfnum, 0);
- } else {
- /*
- * UVC doesn't specify how to inform a bulk-based device
- * when the video stream is stopped. Windows sends a
- * CLEAR_FEATURE(HALT) request to the video streaming
- * bulk endpoint, mimic the same behaviour.
- */
- unsigned int epnum = stream->header.bEndpointAddress
- & USB_ENDPOINT_NUMBER_MASK;
- unsigned int dir = stream->header.bEndpointAddress
- & USB_ENDPOINT_DIR_MASK;
- unsigned int pipe;
-
- pipe = usb_sndbulkpipe(stream->dev->udev, epnum) | dir;
- usb_clear_halt(stream->dev->udev, pipe);
+ return;
}
+ /*
+ * UVC doesn't specify how to inform a bulk-based device
+ * when the video stream is stopped. Windows sends a
+ * CLEAR_FEATURE(HALT) request to the video streaming
+ * bulk endpoint, mimic the same behaviour.
+ */
+ epnum = stream->header.bEndpointAddress & USB_ENDPOINT_NUMBER_MASK;
+ dir = stream->header.bEndpointAddress & USB_ENDPOINT_DIR_MASK;
+ pipe = usb_sndbulkpipe(stream->dev->udev, epnum) | dir;
+ usb_clear_halt(stream->dev->udev, pipe);
+}
+
+void uvc_video_stop_streaming(struct uvc_streaming *stream)
+{
+ uvc_video_stop_transfer(stream, 1);
+
+ /*
+ * Barrier needed to pair with uvc_disconnect().
+ * We cannot call usb_* functions on a disconnected USB device.
+ */
+ if (!smp_load_acquire(&stream->dev->disconnected))
+ uvc_video_halt(stream);
+
uvc_video_clock_cleanup(stream);
}
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index ba8f8c1f2c83..5b1a3643de05 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -559,6 +559,8 @@ struct uvc_device {
unsigned int users;
atomic_t nmappings;
+ bool disconnected;
+
/* Video control interface */
#ifdef CONFIG_MEDIA_CONTROLLER
struct media_device mdev;
--
2.43.0.rc1.413.gea7ed67945-goog
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH v5 3/3] media: uvcvideo: Do not use usb_* functions after .disconnect
2023-11-22 11:45 ` [PATCH v5 3/3] media: uvcvideo: Do not use usb_* functions after .disconnect Ricardo Ribalda
@ 2023-11-22 13:33 ` Laurent Pinchart
2023-11-22 14:59 ` Ricardo Ribalda
0 siblings, 1 reply; 11+ messages in thread
From: Laurent Pinchart @ 2023-11-22 13:33 UTC (permalink / raw)
To: Ricardo Ribalda
Cc: Mauro Carvalho Chehab, Guenter Roeck, Tomasz Figa, Alan Stern,
Hans Verkuil, linux-media, linux-kernel, Sean Paul, Sakari Ailus
On Wed, Nov 22, 2023 at 11:45:49AM +0000, Ricardo Ribalda wrote:
> usb drivers should not call to any I/O function after the
> .disconnect() callback has been triggered.
> https://www.kernel.org/doc/html/latest/driver-api/usb/callbacks.html#the-disconnect-callback
>
> If an application is receiving frames form a camera and the device is
> disconnected: the device will call close() after the usb .disconnect()
> callback has been called. The streamoff path will call usb_set_interface
> or usb_clear_halt, which is not allowed.
>
> This patch only solves the calls to close() *after* .disconnect() is
> being called.
>
> Trace:
> [ 1065.389723] drivers/media/usb/uvc/uvc_driver.c:2248 uvc_disconnect enter
> [ 1065.390160] drivers/media/usb/uvc/uvc_driver.c:2264 uvc_disconnect exit
> [ 1065.433956] drivers/media/usb/uvc/uvc_v4l2.c:659 uvc_v4l2_release enter
> [ 1065.433973] drivers/media/usb/uvc/uvc_video.c:2274 uvc_video_stop_streaming enter
> [ 1065.434560] drivers/media/usb/uvc/uvc_video.c:2285 uvc_video_stop_streaming exit
> [ 1065.435154] drivers/media/usb/uvc/uvc_v4l2.c:680 uvc_v4l2_release exit
> [ 1065.435188] drivers/media/usb/uvc/uvc_driver.c:2248 uvc_disconnect enter
>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
> drivers/media/usb/uvc/uvc_driver.c | 2 ++
> drivers/media/usb/uvc/uvc_video.c | 45 ++++++++++++++++++++++++--------------
> drivers/media/usb/uvc/uvcvideo.h | 2 ++
> 3 files changed, 32 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index d5dbf2644272..d78640d422f4 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -2266,6 +2266,8 @@ static void uvc_disconnect(struct usb_interface *intf)
> return;
>
> uvc_unregister_video(dev);
> + /* Barrier needed to pair with uvc_video_stop_streaming(). */
> + smp_store_release(&dev->disconnected, true);
I can't think this would be such a hot path that we really need barriers
in the driver.
> kref_put(&dev->ref, uvc_delete);
> }
>
> diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> index 28dde08ec6c5..f5ef375088de 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -2243,28 +2243,39 @@ int uvc_video_start_streaming(struct uvc_streaming *stream)
> return ret;
> }
>
> -void uvc_video_stop_streaming(struct uvc_streaming *stream)
> +static void uvc_video_halt(struct uvc_streaming *stream)
> {
> - uvc_video_stop_transfer(stream, 1);
> + unsigned int epnum;
> + unsigned int pipe;
> + unsigned int dir;
>
> if (stream->intf->num_altsetting > 1) {
> usb_set_interface(stream->dev->udev, stream->intfnum, 0);
> - } else {
> - /*
> - * UVC doesn't specify how to inform a bulk-based device
> - * when the video stream is stopped. Windows sends a
> - * CLEAR_FEATURE(HALT) request to the video streaming
> - * bulk endpoint, mimic the same behaviour.
> - */
> - unsigned int epnum = stream->header.bEndpointAddress
> - & USB_ENDPOINT_NUMBER_MASK;
> - unsigned int dir = stream->header.bEndpointAddress
> - & USB_ENDPOINT_DIR_MASK;
> - unsigned int pipe;
> -
> - pipe = usb_sndbulkpipe(stream->dev->udev, epnum) | dir;
> - usb_clear_halt(stream->dev->udev, pipe);
> + return;
> }
>
> + /*
> + * UVC doesn't specify how to inform a bulk-based device
> + * when the video stream is stopped. Windows sends a
> + * CLEAR_FEATURE(HALT) request to the video streaming
> + * bulk endpoint, mimic the same behaviour.
> + */
> + epnum = stream->header.bEndpointAddress & USB_ENDPOINT_NUMBER_MASK;
> + dir = stream->header.bEndpointAddress & USB_ENDPOINT_DIR_MASK;
> + pipe = usb_sndbulkpipe(stream->dev->udev, epnum) | dir;
> + usb_clear_halt(stream->dev->udev, pipe);
> +}
> +
> +void uvc_video_stop_streaming(struct uvc_streaming *stream)
> +{
> + uvc_video_stop_transfer(stream, 1);
> +
> + /*
> + * Barrier needed to pair with uvc_disconnect().
> + * We cannot call usb_* functions on a disconnected USB device.
> + */
> + if (!smp_load_acquire(&stream->dev->disconnected))
> + uvc_video_halt(stream);
> +
> uvc_video_clock_cleanup(stream);
> }
> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> index ba8f8c1f2c83..5b1a3643de05 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -559,6 +559,8 @@ struct uvc_device {
> unsigned int users;
> atomic_t nmappings;
>
> + bool disconnected;
> +
> /* Video control interface */
> #ifdef CONFIG_MEDIA_CONTROLLER
> struct media_device mdev;
>
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v5 3/3] media: uvcvideo: Do not use usb_* functions after .disconnect
2023-11-22 13:33 ` Laurent Pinchart
@ 2023-11-22 14:59 ` Ricardo Ribalda
2024-03-22 14:19 ` Laurent Pinchart
0 siblings, 1 reply; 11+ messages in thread
From: Ricardo Ribalda @ 2023-11-22 14:59 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Mauro Carvalho Chehab, Guenter Roeck, Tomasz Figa, Alan Stern,
Hans Verkuil, linux-media, linux-kernel, Sean Paul, Sakari Ailus
Hi Laurent
On Wed, 22 Nov 2023 at 14:33, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> On Wed, Nov 22, 2023 at 11:45:49AM +0000, Ricardo Ribalda wrote:
> > usb drivers should not call to any I/O function after the
> > .disconnect() callback has been triggered.
> > https://www.kernel.org/doc/html/latest/driver-api/usb/callbacks.html#the-disconnect-callback
> >
> > If an application is receiving frames form a camera and the device is
> > disconnected: the device will call close() after the usb .disconnect()
> > callback has been called. The streamoff path will call usb_set_interface
> > or usb_clear_halt, which is not allowed.
> >
> > This patch only solves the calls to close() *after* .disconnect() is
> > being called.
> >
> > Trace:
> > [ 1065.389723] drivers/media/usb/uvc/uvc_driver.c:2248 uvc_disconnect enter
> > [ 1065.390160] drivers/media/usb/uvc/uvc_driver.c:2264 uvc_disconnect exit
> > [ 1065.433956] drivers/media/usb/uvc/uvc_v4l2.c:659 uvc_v4l2_release enter
> > [ 1065.433973] drivers/media/usb/uvc/uvc_video.c:2274 uvc_video_stop_streaming enter
> > [ 1065.434560] drivers/media/usb/uvc/uvc_video.c:2285 uvc_video_stop_streaming exit
> > [ 1065.435154] drivers/media/usb/uvc/uvc_v4l2.c:680 uvc_v4l2_release exit
> > [ 1065.435188] drivers/media/usb/uvc/uvc_driver.c:2248 uvc_disconnect enter
> >
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > ---
> > drivers/media/usb/uvc/uvc_driver.c | 2 ++
> > drivers/media/usb/uvc/uvc_video.c | 45 ++++++++++++++++++++++++--------------
> > drivers/media/usb/uvc/uvcvideo.h | 2 ++
> > 3 files changed, 32 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > index d5dbf2644272..d78640d422f4 100644
> > --- a/drivers/media/usb/uvc/uvc_driver.c
> > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > @@ -2266,6 +2266,8 @@ static void uvc_disconnect(struct usb_interface *intf)
> > return;
> >
> > uvc_unregister_video(dev);
> > + /* Barrier needed to pair with uvc_video_stop_streaming(). */
> > + smp_store_release(&dev->disconnected, true);
>
> I can't think this would be such a hot path that we really need barriers
> in the driver.
Using the same variable from two contexts without any sync feels weird.
Your concern is that there will be a big penalty by using the
barriers? This is only used in stop_streaming and the shutdown path.
Regards!
>
> > kref_put(&dev->ref, uvc_delete);
> > }
> >
> > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> > index 28dde08ec6c5..f5ef375088de 100644
> > --- a/drivers/media/usb/uvc/uvc_video.c
> > +++ b/drivers/media/usb/uvc/uvc_video.c
> > @@ -2243,28 +2243,39 @@ int uvc_video_start_streaming(struct uvc_streaming *stream)
> > return ret;
> > }
> >
> > -void uvc_video_stop_streaming(struct uvc_streaming *stream)
> > +static void uvc_video_halt(struct uvc_streaming *stream)
> > {
> > - uvc_video_stop_transfer(stream, 1);
> > + unsigned int epnum;
> > + unsigned int pipe;
> > + unsigned int dir;
> >
> > if (stream->intf->num_altsetting > 1) {
> > usb_set_interface(stream->dev->udev, stream->intfnum, 0);
> > - } else {
> > - /*
> > - * UVC doesn't specify how to inform a bulk-based device
> > - * when the video stream is stopped. Windows sends a
> > - * CLEAR_FEATURE(HALT) request to the video streaming
> > - * bulk endpoint, mimic the same behaviour.
> > - */
> > - unsigned int epnum = stream->header.bEndpointAddress
> > - & USB_ENDPOINT_NUMBER_MASK;
> > - unsigned int dir = stream->header.bEndpointAddress
> > - & USB_ENDPOINT_DIR_MASK;
> > - unsigned int pipe;
> > -
> > - pipe = usb_sndbulkpipe(stream->dev->udev, epnum) | dir;
> > - usb_clear_halt(stream->dev->udev, pipe);
> > + return;
> > }
> >
> > + /*
> > + * UVC doesn't specify how to inform a bulk-based device
> > + * when the video stream is stopped. Windows sends a
> > + * CLEAR_FEATURE(HALT) request to the video streaming
> > + * bulk endpoint, mimic the same behaviour.
> > + */
> > + epnum = stream->header.bEndpointAddress & USB_ENDPOINT_NUMBER_MASK;
> > + dir = stream->header.bEndpointAddress & USB_ENDPOINT_DIR_MASK;
> > + pipe = usb_sndbulkpipe(stream->dev->udev, epnum) | dir;
> > + usb_clear_halt(stream->dev->udev, pipe);
> > +}
> > +
> > +void uvc_video_stop_streaming(struct uvc_streaming *stream)
> > +{
> > + uvc_video_stop_transfer(stream, 1);
> > +
> > + /*
> > + * Barrier needed to pair with uvc_disconnect().
> > + * We cannot call usb_* functions on a disconnected USB device.
> > + */
> > + if (!smp_load_acquire(&stream->dev->disconnected))
> > + uvc_video_halt(stream);
> > +
> > uvc_video_clock_cleanup(stream);
> > }
> > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> > index ba8f8c1f2c83..5b1a3643de05 100644
> > --- a/drivers/media/usb/uvc/uvcvideo.h
> > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > @@ -559,6 +559,8 @@ struct uvc_device {
> > unsigned int users;
> > atomic_t nmappings;
> >
> > + bool disconnected;
> > +
> > /* Video control interface */
> > #ifdef CONFIG_MEDIA_CONTROLLER
> > struct media_device mdev;
> >
>
> --
> Regards,
>
> Laurent Pinchart
--
Ricardo Ribalda
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v5 3/3] media: uvcvideo: Do not use usb_* functions after .disconnect
2023-11-22 14:59 ` Ricardo Ribalda
@ 2024-03-22 14:19 ` Laurent Pinchart
2024-03-22 14:31 ` Ricardo Ribalda
0 siblings, 1 reply; 11+ messages in thread
From: Laurent Pinchart @ 2024-03-22 14:19 UTC (permalink / raw)
To: Ricardo Ribalda
Cc: Mauro Carvalho Chehab, Guenter Roeck, Tomasz Figa, Alan Stern,
Hans Verkuil, linux-media, linux-kernel, Sean Paul, Sakari Ailus
Hi Ricardo,
On Wed, Nov 22, 2023 at 03:59:10PM +0100, Ricardo Ribalda wrote:
> On Wed, 22 Nov 2023 at 14:33, Laurent Pinchart wrote:
> > On Wed, Nov 22, 2023 at 11:45:49AM +0000, Ricardo Ribalda wrote:
> > > usb drivers should not call to any I/O function after the
> > > .disconnect() callback has been triggered.
> > > https://www.kernel.org/doc/html/latest/driver-api/usb/callbacks.html#the-disconnect-callback
> > >
> > > If an application is receiving frames form a camera and the device is
> > > disconnected: the device will call close() after the usb .disconnect()
> > > callback has been called. The streamoff path will call usb_set_interface
> > > or usb_clear_halt, which is not allowed.
> > >
> > > This patch only solves the calls to close() *after* .disconnect() is
> > > being called.
> > >
> > > Trace:
> > > [ 1065.389723] drivers/media/usb/uvc/uvc_driver.c:2248 uvc_disconnect enter
> > > [ 1065.390160] drivers/media/usb/uvc/uvc_driver.c:2264 uvc_disconnect exit
> > > [ 1065.433956] drivers/media/usb/uvc/uvc_v4l2.c:659 uvc_v4l2_release enter
> > > [ 1065.433973] drivers/media/usb/uvc/uvc_video.c:2274 uvc_video_stop_streaming enter
> > > [ 1065.434560] drivers/media/usb/uvc/uvc_video.c:2285 uvc_video_stop_streaming exit
> > > [ 1065.435154] drivers/media/usb/uvc/uvc_v4l2.c:680 uvc_v4l2_release exit
> > > [ 1065.435188] drivers/media/usb/uvc/uvc_driver.c:2248 uvc_disconnect enter
> > >
> > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > ---
> > > drivers/media/usb/uvc/uvc_driver.c | 2 ++
> > > drivers/media/usb/uvc/uvc_video.c | 45 ++++++++++++++++++++++++--------------
> > > drivers/media/usb/uvc/uvcvideo.h | 2 ++
> > > 3 files changed, 32 insertions(+), 17 deletions(-)
> > >
> > > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > > index d5dbf2644272..d78640d422f4 100644
> > > --- a/drivers/media/usb/uvc/uvc_driver.c
> > > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > > @@ -2266,6 +2266,8 @@ static void uvc_disconnect(struct usb_interface *intf)
> > > return;
> > >
> > > uvc_unregister_video(dev);
> > > + /* Barrier needed to pair with uvc_video_stop_streaming(). */
> > > + smp_store_release(&dev->disconnected, true);
> >
> > I can't think this would be such a hot path that we really need barriers
> > in the driver.
>
> Using the same variable from two contexts without any sync feels weird.
>
> Your concern is that there will be a big penalty by using the
> barriers? This is only used in stop_streaming and the shutdown path.
What I meant is that lockless concurrency is much harder to get right
than locked concurrency. Unless there's an important reason not to use
lock (which would I think need to be related to performance, and I don't
see that being an issue here), I think using locks will be less
error-prone and more maintainable. That's the KISS approach to
concurrency (even if it doesn't directly address lockless concurrency, I
like the approach advocated by Sima in
https://blog.ffwll.ch/2022/07/locking-engineering.html).
> > > kref_put(&dev->ref, uvc_delete);
> > > }
> > >
> > > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> > > index 28dde08ec6c5..f5ef375088de 100644
> > > --- a/drivers/media/usb/uvc/uvc_video.c
> > > +++ b/drivers/media/usb/uvc/uvc_video.c
> > > @@ -2243,28 +2243,39 @@ int uvc_video_start_streaming(struct uvc_streaming *stream)
> > > return ret;
> > > }
> > >
> > > -void uvc_video_stop_streaming(struct uvc_streaming *stream)
> > > +static void uvc_video_halt(struct uvc_streaming *stream)
> > > {
> > > - uvc_video_stop_transfer(stream, 1);
> > > + unsigned int epnum;
> > > + unsigned int pipe;
> > > + unsigned int dir;
> > >
> > > if (stream->intf->num_altsetting > 1) {
> > > usb_set_interface(stream->dev->udev, stream->intfnum, 0);
> > > - } else {
> > > - /*
> > > - * UVC doesn't specify how to inform a bulk-based device
> > > - * when the video stream is stopped. Windows sends a
> > > - * CLEAR_FEATURE(HALT) request to the video streaming
> > > - * bulk endpoint, mimic the same behaviour.
> > > - */
> > > - unsigned int epnum = stream->header.bEndpointAddress
> > > - & USB_ENDPOINT_NUMBER_MASK;
> > > - unsigned int dir = stream->header.bEndpointAddress
> > > - & USB_ENDPOINT_DIR_MASK;
> > > - unsigned int pipe;
> > > -
> > > - pipe = usb_sndbulkpipe(stream->dev->udev, epnum) | dir;
> > > - usb_clear_halt(stream->dev->udev, pipe);
> > > + return;
> > > }
> > >
> > > + /*
> > > + * UVC doesn't specify how to inform a bulk-based device
> > > + * when the video stream is stopped. Windows sends a
> > > + * CLEAR_FEATURE(HALT) request to the video streaming
> > > + * bulk endpoint, mimic the same behaviour.
> > > + */
> > > + epnum = stream->header.bEndpointAddress & USB_ENDPOINT_NUMBER_MASK;
> > > + dir = stream->header.bEndpointAddress & USB_ENDPOINT_DIR_MASK;
> > > + pipe = usb_sndbulkpipe(stream->dev->udev, epnum) | dir;
> > > + usb_clear_halt(stream->dev->udev, pipe);
> > > +}
> > > +
> > > +void uvc_video_stop_streaming(struct uvc_streaming *stream)
> > > +{
> > > + uvc_video_stop_transfer(stream, 1);
> > > +
> > > + /*
> > > + * Barrier needed to pair with uvc_disconnect().
> > > + * We cannot call usb_* functions on a disconnected USB device.
> > > + */
> > > + if (!smp_load_acquire(&stream->dev->disconnected))
> > > + uvc_video_halt(stream);
> > > +
> > > uvc_video_clock_cleanup(stream);
> > > }
> > > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> > > index ba8f8c1f2c83..5b1a3643de05 100644
> > > --- a/drivers/media/usb/uvc/uvcvideo.h
> > > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > > @@ -559,6 +559,8 @@ struct uvc_device {
> > > unsigned int users;
> > > atomic_t nmappings;
> > >
> > > + bool disconnected;
> > > +
> > > /* Video control interface */
> > > #ifdef CONFIG_MEDIA_CONTROLLER
> > > struct media_device mdev;
> > >
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v5 3/3] media: uvcvideo: Do not use usb_* functions after .disconnect
2024-03-22 14:19 ` Laurent Pinchart
@ 2024-03-22 14:31 ` Ricardo Ribalda
0 siblings, 0 replies; 11+ messages in thread
From: Ricardo Ribalda @ 2024-03-22 14:31 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Mauro Carvalho Chehab, Guenter Roeck, Tomasz Figa, Alan Stern,
Hans Verkuil, linux-media, linux-kernel, Sean Paul, Sakari Ailus
Hi Laurent
On Fri, 22 Mar 2024 at 15:19, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Ricardo,
>
> On Wed, Nov 22, 2023 at 03:59:10PM +0100, Ricardo Ribalda wrote:
> > On Wed, 22 Nov 2023 at 14:33, Laurent Pinchart wrote:
> > > On Wed, Nov 22, 2023 at 11:45:49AM +0000, Ricardo Ribalda wrote:
> > > > usb drivers should not call to any I/O function after the
> > > > .disconnect() callback has been triggered.
> > > > https://www.kernel.org/doc/html/latest/driver-api/usb/callbacks.html#the-disconnect-callback
> > > >
> > > > If an application is receiving frames form a camera and the device is
> > > > disconnected: the device will call close() after the usb .disconnect()
> > > > callback has been called. The streamoff path will call usb_set_interface
> > > > or usb_clear_halt, which is not allowed.
> > > >
> > > > This patch only solves the calls to close() *after* .disconnect() is
> > > > being called.
> > > >
> > > > Trace:
> > > > [ 1065.389723] drivers/media/usb/uvc/uvc_driver.c:2248 uvc_disconnect enter
> > > > [ 1065.390160] drivers/media/usb/uvc/uvc_driver.c:2264 uvc_disconnect exit
> > > > [ 1065.433956] drivers/media/usb/uvc/uvc_v4l2.c:659 uvc_v4l2_release enter
> > > > [ 1065.433973] drivers/media/usb/uvc/uvc_video.c:2274 uvc_video_stop_streaming enter
> > > > [ 1065.434560] drivers/media/usb/uvc/uvc_video.c:2285 uvc_video_stop_streaming exit
> > > > [ 1065.435154] drivers/media/usb/uvc/uvc_v4l2.c:680 uvc_v4l2_release exit
> > > > [ 1065.435188] drivers/media/usb/uvc/uvc_driver.c:2248 uvc_disconnect enter
> > > >
> > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > > ---
> > > > drivers/media/usb/uvc/uvc_driver.c | 2 ++
> > > > drivers/media/usb/uvc/uvc_video.c | 45 ++++++++++++++++++++++++--------------
> > > > drivers/media/usb/uvc/uvcvideo.h | 2 ++
> > > > 3 files changed, 32 insertions(+), 17 deletions(-)
> > > >
> > > > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > > > index d5dbf2644272..d78640d422f4 100644
> > > > --- a/drivers/media/usb/uvc/uvc_driver.c
> > > > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > > > @@ -2266,6 +2266,8 @@ static void uvc_disconnect(struct usb_interface *intf)
> > > > return;
> > > >
> > > > uvc_unregister_video(dev);
> > > > + /* Barrier needed to pair with uvc_video_stop_streaming(). */
> > > > + smp_store_release(&dev->disconnected, true);
> > >
> > > I can't think this would be such a hot path that we really need barriers
> > > in the driver.
> >
> > Using the same variable from two contexts without any sync feels weird.
> >
> > Your concern is that there will be a big penalty by using the
> > barriers? This is only used in stop_streaming and the shutdown path.
>
> What I meant is that lockless concurrency is much harder to get right
> than locked concurrency. Unless there's an important reason not to use
> lock (which would I think need to be related to performance, and I don't
> see that being an issue here), I think using locks will be less
> error-prone and more maintainable. That's the KISS approach to
> concurrency (even if it doesn't directly address lockless concurrency, I
> like the approach advocated by Sima in
> https://blog.ffwll.ch/2022/07/locking-engineering.html).
Gotcha, :)
I thought you wanted to remove the locking all together.
Something like:
> + if (!stream->dev->disconnected)
> + uvc_video_halt(stream);
(Which will likely work btw :P)
>
> > > > kref_put(&dev->ref, uvc_delete);
> > > > }
> > > >
> > > > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> > > > index 28dde08ec6c5..f5ef375088de 100644
> > > > --- a/drivers/media/usb/uvc/uvc_video.c
> > > > +++ b/drivers/media/usb/uvc/uvc_video.c
> > > > @@ -2243,28 +2243,39 @@ int uvc_video_start_streaming(struct uvc_streaming *stream)
> > > > return ret;
> > > > }
> > > >
> > > > -void uvc_video_stop_streaming(struct uvc_streaming *stream)
> > > > +static void uvc_video_halt(struct uvc_streaming *stream)
> > > > {
> > > > - uvc_video_stop_transfer(stream, 1);
> > > > + unsigned int epnum;
> > > > + unsigned int pipe;
> > > > + unsigned int dir;
> > > >
> > > > if (stream->intf->num_altsetting > 1) {
> > > > usb_set_interface(stream->dev->udev, stream->intfnum, 0);
> > > > - } else {
> > > > - /*
> > > > - * UVC doesn't specify how to inform a bulk-based device
> > > > - * when the video stream is stopped. Windows sends a
> > > > - * CLEAR_FEATURE(HALT) request to the video streaming
> > > > - * bulk endpoint, mimic the same behaviour.
> > > > - */
> > > > - unsigned int epnum = stream->header.bEndpointAddress
> > > > - & USB_ENDPOINT_NUMBER_MASK;
> > > > - unsigned int dir = stream->header.bEndpointAddress
> > > > - & USB_ENDPOINT_DIR_MASK;
> > > > - unsigned int pipe;
> > > > -
> > > > - pipe = usb_sndbulkpipe(stream->dev->udev, epnum) | dir;
> > > > - usb_clear_halt(stream->dev->udev, pipe);
> > > > + return;
> > > > }
> > > >
> > > > + /*
> > > > + * UVC doesn't specify how to inform a bulk-based device
> > > > + * when the video stream is stopped. Windows sends a
> > > > + * CLEAR_FEATURE(HALT) request to the video streaming
> > > > + * bulk endpoint, mimic the same behaviour.
> > > > + */
> > > > + epnum = stream->header.bEndpointAddress & USB_ENDPOINT_NUMBER_MASK;
> > > > + dir = stream->header.bEndpointAddress & USB_ENDPOINT_DIR_MASK;
> > > > + pipe = usb_sndbulkpipe(stream->dev->udev, epnum) | dir;
> > > > + usb_clear_halt(stream->dev->udev, pipe);
> > > > +}
> > > > +
> > > > +void uvc_video_stop_streaming(struct uvc_streaming *stream)
> > > > +{
> > > > + uvc_video_stop_transfer(stream, 1);
> > > > +
> > > > + /*
> > > > + * Barrier needed to pair with uvc_disconnect().
> > > > + * We cannot call usb_* functions on a disconnected USB device.
> > > > + */
> > > > + if (!smp_load_acquire(&stream->dev->disconnected))
> > > > + uvc_video_halt(stream);
> > > > +
> > > > uvc_video_clock_cleanup(stream);
> > > > }
> > > > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> > > > index ba8f8c1f2c83..5b1a3643de05 100644
> > > > --- a/drivers/media/usb/uvc/uvcvideo.h
> > > > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > > > @@ -559,6 +559,8 @@ struct uvc_device {
> > > > unsigned int users;
> > > > atomic_t nmappings;
> > > >
> > > > + bool disconnected;
> > > > +
> > > > /* Video control interface */
> > > > #ifdef CONFIG_MEDIA_CONTROLLER
> > > > struct media_device mdev;
> > > >
>
> --
> Regards,
>
> Laurent Pinchart
--
Ricardo Ribalda
^ permalink raw reply [flat|nested] 11+ messages in thread