Re: [PATCH RESEND v3 1/5] media: uvcvideo: Cancel async worker earlier

[Ezequiel Garcia]

Hi Laurent,

On Wed, 16 Sept 2020 at 23:33, Guenter Roeck <linux@xxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxx>
> Cc: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>
> Cc: Hans Verkuil <hverkuil-cisco@xxxxxxxxx>
> Signed-off-by: Guenter Roeck <linux@xxxxxxxxxxxx>

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