| 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
| ||
|
Date: Tue, 20 Jun 2017 00:02:58 +1000
From: Daniel Axtens <dja@...ens.net>
To: linux-media@...r.kernel.org, linux-kernel@...r.kernel.org
Cc: Laurent Pinchart <laurent.pinchart@...asonboard.com>,
Dave Stevenson <linux-media@...tevenson.freeserve.co.uk>,
Greg KH <greg@...ah.com>
Subject: Re: [PATCH v2] [media] uvcvideo: Refactor teardown of uvc on USB disconnect
Hi Laurent,
Just checking if this was OK with you - I hadn't heard anything and I
noticed it's not in -next so I just wanted to check to see if there were
any changes you wanted.
Regards,
Daniel
Daniel Axtens <dja@...ens.net> writes:
> Currently, disconnecting a USB webcam while it is in use prints out a
> number of warnings, such as:
>
> WARNING: CPU: 2 PID: 3118 at /build/linux-ezBi1T/linux-4.8.0/fs/sysfs/group.c:237 sysfs_remove_group+0x8b/0x90
> sysfs group ffffffffa7cd0780 not found for kobject 'event13'
>
> This has been noticed before. [0]
>
> This is because of the order in which things are torn down.
>
> If there are no streams active during a USB disconnect:
>
> - uvc_disconnect() is invoked via device_del() through the bus
> notifier mechanism.
>
> - this calls uvc_unregister_video().
>
> - uvc_unregister_video() unregisters the video device for each
> stream,
>
> - because there are no streams open, it calls uvc_delete()
>
> - uvc_delete() calls uvc_status_cleanup(), which cleans up the status
> input device.
>
> - uvc_delete() calls media_device_unregister(), which cleans up the
> media device
>
> - uvc_delete(), uvc_unregister_video() and uvc_disconnect() all
> return, and we end up back in device_del().
>
> - device_del() then cleans up the sysfs folder for the camera with
> dpm_sysfs_remove(). Because uvc_status_cleanup() and
> media_device_unregister() have already been called, this all works
> nicely.
>
> If, on the other hand, there *are* streams active during a USB disconnect:
>
> - uvc_disconnect() is invoked
>
> - this calls uvc_unregister_video()
>
> - uvc_unregister_video() unregisters the video device for each
> stream,
>
> - uvc_unregister_video() and uvc_disconnect() return, and we end up
> back in device_del().
>
> - device_del() then cleans up the sysfs folder for the camera with
> dpm_sysfs_remove(). Because the status input device and the media
> device are children of the USB device, this also deletes their
> sysfs folders.
>
> - Sometime later, the final stream is closed, invoking uvc_release().
>
> - uvc_release() calls uvc_delete()
>
> - uvc_delete() calls uvc_status_cleanup(), which cleans up the status
> input device. Because the sysfs directory has already been removed,
> this causes a WARNing.
>
> - uvc_delete() calls media_device_unregister(), which cleans up the
> media device. Because the sysfs directory has already been removed,
> this causes another WARNing.
>
> To fix this, we need to make sure the devices are always unregistered
> before the end of uvc_disconnect(). To this, move the unregistration
> into the disconnect path:
>
> - split uvc_status_cleanup() into two parts, one on disconnect that
> unregisters and one on delete that frees.
>
> - move v4l2_device_unregister() and media_device_unregister() into
> the disconnect path.
>
> [0]: https://lkml.org/lkml/2016/12/8/657
>
> Cc: Laurent Pinchart <laurent.pinchart@...asonboard.com>
> Cc: Dave Stevenson <linux-media@...tevenson.freeserve.co.uk>
> Cc: Greg KH <greg@...ah.com>
> Signed-off-by: Daniel Axtens <dja@...ens.net>
>
> ---
>
> v2: Move v4l2_device_unregister() as well as media_device_unregister(),
> thanks Laurent.
>
> Tested with cheese and yavta.
>
> Laurent, I know you were concerned about race conditions so I plugged
> and unplugged the camera over a dozen times, with lots of debugging
> turned on.
>
> In particular:
> - KASan was enabled and didn't trigger
> - If I unplugged while yavta was running, and replugged *while yatva
> was still running*, the video camera came up as video2, not video1.
> This indicates that yatva was successfully holding a reference to
> video1 - as it should.
> - No other debugging triggered any warning.
> ---
> drivers/media/usb/uvc/uvc_driver.c | 12 ++++++++----
> drivers/media/usb/uvc/uvc_status.c | 8 ++++++--
> drivers/media/usb/uvc/uvcvideo.h | 1 +
> 3 files changed, 15 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index 46d6be0bb316..0d0541054ce2 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -1812,11 +1812,7 @@ static void uvc_delete(struct uvc_device *dev)
> usb_put_intf(dev->intf);
> usb_put_dev(dev->udev);
>
> - if (dev->vdev.dev)
> - v4l2_device_unregister(&dev->vdev);
> #ifdef CONFIG_MEDIA_CONTROLLER
> - if (media_devnode_is_registered(dev->mdev.devnode))
> - media_device_unregister(&dev->mdev);
> media_device_cleanup(&dev->mdev);
> #endif
>
> @@ -1884,6 +1880,14 @@ static void uvc_unregister_video(struct uvc_device *dev)
> uvc_debugfs_cleanup_stream(stream);
> }
>
> + uvc_status_unregister(dev);
> + if (dev->vdev.dev)
> + v4l2_device_unregister(&dev->vdev);
> +#ifdef CONFIG_MEDIA_CONTROLLER
> + if (media_devnode_is_registered(dev->mdev.devnode))
> + media_device_unregister(&dev->mdev);
> +#endif
> +
> /* Decrement the stream count and call uvc_delete explicitly if there
> * are no stream left.
> */
> diff --git a/drivers/media/usb/uvc/uvc_status.c b/drivers/media/usb/uvc/uvc_status.c
> index f552ab997956..95709b23d3b4 100644
> --- a/drivers/media/usb/uvc/uvc_status.c
> +++ b/drivers/media/usb/uvc/uvc_status.c
> @@ -198,12 +198,16 @@ int uvc_status_init(struct uvc_device *dev)
> return 0;
> }
>
> -void uvc_status_cleanup(struct uvc_device *dev)
> +void uvc_status_unregister(struct uvc_device *dev)
> {
> usb_kill_urb(dev->int_urb);
> + uvc_input_cleanup(dev);
> +}
> +
> +void uvc_status_cleanup(struct uvc_device *dev)
> +{
> usb_free_urb(dev->int_urb);
> kfree(dev->status);
> - uvc_input_cleanup(dev);
> }
>
> int uvc_status_start(struct uvc_device *dev, gfp_t flags)
> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> index 15e415e32c7f..4b4814df35cd 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -712,6 +712,7 @@ void uvc_video_clock_update(struct uvc_streaming *stream,
>
> /* Status */
> extern int uvc_status_init(struct uvc_device *dev);
> +extern void uvc_status_unregister(struct uvc_device *dev);
> extern void uvc_status_cleanup(struct uvc_device *dev);
> extern int uvc_status_start(struct uvc_device *dev, gfp_t flags);
> extern void uvc_status_stop(struct uvc_device *dev);
> --
> 2.11.0
Powered by blists - more mailing lists