Re: Linux 2.6.26-rc9 circular lock with uvcvideo on resume from hibernation

[Laurent Pinchart]

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
> <romano@xxxxxxxxxxxxxxxxxxxxxx>.
>
> Signed-off-by: Roland Dreier <roland@xxxxxxxxxxxxxxxxxx>
> ---
>  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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/