linux-kernel - Re: [PATCH 2/2] [media] uvcvideo: Kill video URBs on disconnect

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <9757129.ouSjzrobER@avalon>
Date:   Mon, 17 Apr 2017 15:49:46 +0300
From:   Laurent Pinchart <laurent.pinchart@...asonboard.com>
To:     Daniel Axtens <dja@...ens.net>
Cc:     linux-media@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] [media] uvcvideo: Kill video URBs on disconnect

Hi Daniel,

Thank you for the patch.

On Monday 17 Apr 2017 18:52:40 Daniel Axtens wrote:
> When an in-use webcam is disconnected, I noticed the following
> messages:
> 
>   uvcvideo: Failed to resubmit video URB (-19).
> 
> -19 is -ENODEV, which does make sense given that the device has
> disappeared.
> 
> We could put a case for -ENODEV like we have with -ENOENT, -ECONNRESET
> and -ESHUTDOWN, but the usb_unlink_urb() API documentation says that
> 'The disconnect function should synchronize with a driver's I/O
> routines to insure that all URB-related activity has completed before
> it returns.' So we should make an effort to proactively kill URBs in
> the disconnect path instead.
> 
> Call uvc_enable_video() (specifying 0 to disable) in the disconnect
> path, which kills and frees URBs.
> 
> Cc: Laurent Pinchart <laurent.pinchart@...asonboard.com>
> Signed-off-by: Daniel Axtens <dja@...ens.net>
> 
> ---
> 
> Before this patch, yavta -c hangs when a camera is disconnected, but
> with this patch it exits immediately after the camera is
> disconnected. I'm not sure if this is acceptable - Laurent?

I assume that the error message is caused by a race between disconnection and 
URB handling. When the device is disconnected I believe the USB core will 
cancel all in-progress URBs (I'm not very familiar with that part of the USB 
core anymore, so please don't consider this or any further related statement 
as true without double-checking), resulting in the URB completion handler 
being called with an error status. The completion handler should then avoid 
resubmitting the URB. However, if the completion handler is in progress when 
the device is disconnected, it won't notice that the device got disconnected, 
and will try to resubmit the URB.

I'm not sure to see how this patch will fix the problem. If the URB completion 
handler is in progress when the device is being disconnected, won't it call 
usb_submit_urb() regardless of whether you call usb_kill_urb() in the 
disconnect handler, resulting in an error message being printed ?

> ---
>  drivers/media/usb/uvc/uvc_driver.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/media/usb/uvc/uvc_driver.c
> b/drivers/media/usb/uvc/uvc_driver.c index 2390592f78e0..647e3d8a1256
> 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -1877,6 +1877,8 @@ static void uvc_unregister_video(struct uvc_device
> *dev) if (!video_is_registered(&stream->vdev))
>  			continue;
> 
> +		uvc_video_enable(stream, 0);
> +
>  		video_unregister_device(&stream->vdev);
> 
>  		uvc_debugfs_cleanup_stream(stream);

-- 
Regards,

Laurent Pinchart