| Message ID | 20200917022547.198090-2-linux@roeck-us.net |
|---|---|
| State | Superseded |
| Headers | show |
| Series | media: uvcvideo: Fix race conditions | expand |
Hi Laurent, On Wed, 16 Sept 2020 at 23:33, Guenter Roeck <linux@roeck-us.net> wrote: > > So far the asynchronous control worker was canceled only in > uvc_ctrl_cleanup_device. This is much later than the call to > uvc_disconnect. However, after the call to uvc_disconnect returns, > there must be no more USB activity. This can result in all kinds > of problems in the USB code. One observed example: > > URB ffff993e83d0bc00 submitted while active > WARNING: CPU: 0 PID: 4046 at drivers/usb/core/urb.c:364 usb_submit_urb+0x4ba/0x55e > Modules linked in: <...> > CPU: 0 PID: 4046 Comm: kworker/0:35 Not tainted 4.19.139 #18 > Hardware name: Google Phaser/Phaser, BIOS Google_Phaser.10952.0.0 08/09/2018 > Workqueue: events uvc_ctrl_status_event_work [uvcvideo] > RIP: 0010:usb_submit_urb+0x4ba/0x55e > Code: <...> > RSP: 0018:ffffb08d471ebde8 EFLAGS: 00010246 > RAX: a6da85d923ea5d00 RBX: ffff993e71985928 RCX: 0000000000000000 > RDX: ffff993f37a1de90 RSI: ffff993f37a153d0 RDI: ffff993f37a153d0 > RBP: ffffb08d471ebe28 R08: 000000000000003b R09: 001424bf85822e96 > R10: 0000001000000000 R11: ffffffff975a4398 R12: ffff993e83d0b000 > R13: ffff993e83d0bc00 R14: 0000000000000000 R15: 00000000fffffff0 > FS: 0000000000000000(0000) GS:ffff993f37a00000(0000) knlGS:0000000000000000 > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > CR2: 00000000ec9c0000 CR3: 000000025b160000 CR4: 0000000000340ef0 > Call Trace: > uvc_ctrl_status_event_work+0xd6/0x107 [uvcvideo] > process_one_work+0x19b/0x4c5 > worker_thread+0x10d/0x286 > kthread+0x138/0x140 > ? process_one_work+0x4c5/0x4c5 > ? kthread_associate_blkcg+0xc1/0xc1 > ret_from_fork+0x1f/0x40 > > Introduce new function uvc_ctrl_stop_device() to cancel the worker > and call it from uvc_unregister_video() to solve the problem. > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Cc: Alan Stern <stern@rowland.harvard.edu> > Cc: Hans Verkuil <hverkuil-cisco@xs4all.nl> > Signed-off-by: Guenter Roeck <linux@roeck-us.net> If I understand correctly, this patch is correct and can be merged independently of the rest in this series. I think it could also be fixed with a check in uvc_ctrl_status_event_work, to prevent pending workers from accessing the device, which is now disconnected. Something like this (untested): @@ -1325,6 +1325,10 @@ static void uvc_ctrl_status_event_work(struct work_struct *work) uvc_ctrl_status_event(w->chain, w->ctrl, w->data); + /* Don't submit URBs if the device was disconnected */ + if (!usb_get_intfdata(dev->intf)) + return; + In any case, it'd be nice to fix this case now, and pospone taking care of the other race conditions in the core, as you suggested in the cover letter. Thanks! Ezequiel
diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c index e399b9fad757..130c56e0063d 100644 --- a/drivers/media/usb/uvc/uvc_ctrl.c +++ b/drivers/media/usb/uvc/uvc_ctrl.c @@ -2340,14 +2340,17 @@ static void uvc_ctrl_cleanup_mappings(struct uvc_device *dev, } } -void uvc_ctrl_cleanup_device(struct uvc_device *dev) +void uvc_ctrl_stop_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); +} + +void uvc_ctrl_cleanup_device(struct uvc_device *dev) +{ + struct uvc_entity *entity; + unsigned int i; /* Free controls and control mappings for all entities. */ list_for_each_entry(entity, &dev->entities, list) { diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c index 431d86e1c94b..bfba67a69185 100644 --- a/drivers/media/usb/uvc/uvc_driver.c +++ b/drivers/media/usb/uvc/uvc_driver.c @@ -1952,6 +1952,7 @@ static void uvc_unregister_video(struct uvc_device *dev) } uvc_status_unregister(dev); + uvc_ctrl_stop_device(dev); if (dev->vdev.dev) v4l2_device_unregister(&dev->vdev); diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h index 6ab972c643e3..543afcd9fd26 100644 --- a/drivers/media/usb/uvc/uvcvideo.h +++ b/drivers/media/usb/uvc/uvcvideo.h @@ -830,6 +830,7 @@ int uvc_query_v4l2_menu(struct uvc_video_chain *chain, int uvc_ctrl_add_mapping(struct uvc_video_chain *chain, const struct uvc_control_mapping *mapping); int uvc_ctrl_init_device(struct uvc_device *dev); +void uvc_ctrl_stop_device(struct uvc_device *dev); void uvc_ctrl_cleanup_device(struct uvc_device *dev); int uvc_ctrl_restore_values(struct uvc_device *dev); bool uvc_ctrl_status_event(struct urb *urb, struct uvc_video_chain *chain,
So far the asynchronous control worker was canceled only in uvc_ctrl_cleanup_device. This is much later than the call to uvc_disconnect. However, after the call to uvc_disconnect returns, there must be no more USB activity. This can result in all kinds of problems in the USB code. One observed example: URB ffff993e83d0bc00 submitted while active WARNING: CPU: 0 PID: 4046 at drivers/usb/core/urb.c:364 usb_submit_urb+0x4ba/0x55e Modules linked in: <...> CPU: 0 PID: 4046 Comm: kworker/0:35 Not tainted 4.19.139 #18 Hardware name: Google Phaser/Phaser, BIOS Google_Phaser.10952.0.0 08/09/2018 Workqueue: events uvc_ctrl_status_event_work [uvcvideo] RIP: 0010:usb_submit_urb+0x4ba/0x55e Code: <...> RSP: 0018:ffffb08d471ebde8 EFLAGS: 00010246 RAX: a6da85d923ea5d00 RBX: ffff993e71985928 RCX: 0000000000000000 RDX: ffff993f37a1de90 RSI: ffff993f37a153d0 RDI: ffff993f37a153d0 RBP: ffffb08d471ebe28 R08: 000000000000003b R09: 001424bf85822e96 R10: 0000001000000000 R11: ffffffff975a4398 R12: ffff993e83d0b000 R13: ffff993e83d0bc00 R14: 0000000000000000 R15: 00000000fffffff0 FS: 0000000000000000(0000) GS:ffff993f37a00000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 00000000ec9c0000 CR3: 000000025b160000 CR4: 0000000000340ef0 Call Trace: uvc_ctrl_status_event_work+0xd6/0x107 [uvcvideo] process_one_work+0x19b/0x4c5 worker_thread+0x10d/0x286 kthread+0x138/0x140 ? process_one_work+0x4c5/0x4c5 ? kthread_associate_blkcg+0xc1/0xc1 ret_from_fork+0x1f/0x40 Introduce new function uvc_ctrl_stop_device() to cancel the worker and call it from uvc_unregister_video() to solve the problem. Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Cc: Alan Stern <stern@rowland.harvard.edu> Cc: Hans Verkuil <hverkuil-cisco@xs4all.nl> Signed-off-by: Guenter Roeck <linux@roeck-us.net> --- drivers/media/usb/uvc/uvc_ctrl.c | 11 +++++++---- drivers/media/usb/uvc/uvc_driver.c | 1 + drivers/media/usb/uvc/uvcvideo.h | 1 + 3 files changed, 9 insertions(+), 4 deletions(-)