Hi Romano and Roland,
On Monday 07 July 2008, Roland Dreier wrote:
> > [ 1132.942569] khubd/2264 is trying to acquire lock:
> > [ 1132.942614] (videodev_lock){--..}, at: [<f89e4595>]
> > video_unregister_device+0x15/0x60 [videodev] [ 1132.942810]
> > [ 1132.942811] but task is already holding lock:
> > [ 1132.942890] (&uvc_driver.open_mutex){--..}, at: [<f8a35c09>]
> > uvc_disconnect+0x29/0x50 [uvcvideo]
>
> Thanks very much for the report (and for testing development kernels
> with lockdep enabled!).
I second this. Thanks a lot.
> I think the patch below should fix this.
>
> Laurent -- if this patch looks good to you, please forward on for
> merging.
>
> Thanks,
> Roland
>
> ---
>
> [PATCH] uvc: Fix possible AB-BA deadlock with videodev_lock and open_mutex
>
> The uvcvideo driver's uvc_v4l2_open() method is called from videodev's
> video_open() function, which means it is called with the videodev_lock
> mutex held. uvc_v4l2_open() then takes uvc_driver.open_mutex to check
> dev->state and avoid racing against a device disconnect, which means
> that open_mutex must nest inside videodev_lock.
The open_mutex lock's main purpose is to prevent uvc_delete() being called
from kref_put() while an open operation is in progress. Protected dev->state
is a side effect which might not be required.
> However uvc_disconnect() takes the open_mutex around setting
> dev->state and also around putting its device reference. However, if
> uvc_disconnect() ends up dropping the last reference, it will call
> uvc_delete(), which calls into the videodev code to unregister its
> device, and this will end up taking videodev_lock. This opens a
> (unlikely in practice) window for an AB-BA deadlock and also causes a
> lockdep warning because of the lock misordering.
I agree with this analysis.
> Fortunately there is no apparent reason to hold open_mutex when doing
> kref_put() in uvc_disconnect(): if uvc_v4l2_open() runs before the
> state is set to UVC_DEV_DISCONNECTED, then it will take another
> reference to the device and kref_put() won't call uvc_delete; if
> uvc_v4l2_open() runs after the state is set, it will run before
> uvc_delete(), see the state, and return immediately -- uvc_delete()
> does uvc_unregister_video() (and hence video_unregister_device(),
> which is synchronized with videodev_lock) as its first thing, so there
> is no risk of use-after-free in uvc_v4l2_open().
This introduces a race condition, see below.
> Bug diagnosed based on a lockdep warning reported by Romano Giannetti
> <[EMAIL PROTECTED]>.
>
> Signed-off-by: Roland Dreier <[EMAIL PROTECTED]>
> ---
> drivers/media/video/uvc/uvc_driver.c | 5 ++---
> 1 files changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/video/uvc/uvc_driver.c
> b/drivers/media/video/uvc/uvc_driver.c index 60ced58..5d60cb3 100644
> --- a/drivers/media/video/uvc/uvc_driver.c
> +++ b/drivers/media/video/uvc/uvc_driver.c
> @@ -1634,11 +1634,10 @@ static void uvc_disconnect(struct usb_interface
> *intf) * chance to increase the reference count (kref_get).
> */
> mutex_lock(&uvc_driver.open_mutex);
> -
> dev->state |= UVC_DEV_DISCONNECTED;
> - kref_put(&dev->kref, uvc_delete);
> -
> mutex_unlock(&uvc_driver.open_mutex);
> +
> + kref_put(&dev->kref, uvc_delete);
> }
>
> static int uvc_suspend(struct usb_interface *intf, pm_message_t message)
uvc_disconnect() | uvc_v4l2_open()
... |
mutex_lock(&uvc_driver.open_mutex); |
dev->state |= UVC_DEV_DISCONNECTED; |
mutex_unlock(&uvc_driver.open_mutex); |
|
| mutex_lock(&uvc_driver.open_mutex);
| vdev = video_devdata(file);
| video = video_get_drvdata(vdev);
|
kref_put(&dev->kref, uvc_delete); |
|
| if (video->dev->state...)
kref_put() in uvc_disconnect() will call uvc_delete(), which will in turn free
the video structure. uvc_v4l2_open() will then dereference freed memory when
testing the device state.
I proper fix will involve videodev modifications. Fortunately, David
Ellingsworth posted a videodev patch to address the same kind of issue a few
days ago. Search the video4linux mailing list for a thread called "[PATCH]
videodev: fix sysfs kobj ref count". I'm currently reviewing his patch. As
soon as a proper fix is applied to videodev I will probably drop the
open_mutex lock acquisition in uvc_disconnect().
In the meantime, and for kernel versions that don't include the videodev
patch, I don't know what to choose between a race condition that can lead to
freed memory dereference, and a AB-BA deadlock. Any opinion on that ?
Best regards,
Laurent Pinchart
_______________________________________________
Linux-uvc-devel mailing list
[email protected]
https://lists.berlios.de/mailman/listinfo/linux-uvc-devel
