Re: [PATCH] videobuf2: Add missing lock held on vb2_fop_relase

[Ricardo Ribalda Delgado]

Hello Sylwester

On Sat, Oct 19, 2013 at 11:55 AM, Sylwester Nawrocki
<sylvester.nawrocki@xxxxxxxxx> wrote:
> Hi Ricardo,
>
>
> On 10/14/2013 09:41 AM, Ricardo Ribalda Delgado wrote:
>>
>> vb2_fop_relase does not held the lock although it is modifying the
>> queue->owner field.
>
> [...]
>
>> diff --git a/drivers/media/v4l2-core/videobuf2-core.c
>> b/drivers/media/v4l2-core/videobuf2-core.c
>> index 9fc4bab..3a961ee 100644
>> --- a/drivers/media/v4l2-core/videobuf2-core.c
>> +++ b/drivers/media/v4l2-core/videobuf2-core.c
>> @@ -2588,8 +2588,15 @@ int vb2_fop_release(struct file *file)
>>         struct video_device *vdev = video_devdata(file);
>>
>>         if (file->private_data == vdev->queue->owner) {
>> +               struct mutex *lock;
>> +
>> +               lock = vdev->queue->lock ? vdev->queue->lock : vdev->lock;
>> +               if (lock)
>> +                       mutex_lock(lock);
>>                 vb2_queue_release(vdev->queue);
>>                 vdev->queue->owner = NULL;
>> +               if (lock)
>> +                       mutex_unlock(lock);
>>         }
>>         return v4l2_fh_release(file);
>>   }
>
>
> It seems you didn't inspect all users of vb2_fop_release(). There are 3
> drivers
> that don't assign vb2_fop_release() to struct v4l2_file_operations directly
> but
> instead call it from within its own release() handler. Two of them do call
> vb2_fop_release() with the video queue lock already held.
>
> $ git grep -n vb2_fop_rel -- drivers/media/
>
> drivers/media/platform/exynos4-is/fimc-capture.c:552:   ret =
> vb2_fop_release(file);
> drivers/media/platform/exynos4-is/fimc-lite.c:549: vb2_fop_release(file);
>
Very good catch, thanks!

> A rather ugly solution would be to open code the vb2_fop_release() function
> in those driver, like in below patch (untested). Unless there are better
> proposals I would queue the patch as below together with the $subject patch
> upstream.

IMHO this will lead to the same type of mistakes in the future.

 What about creating a function __vb2_fop_release that does exactly
the same as the original function but with an extra parameter bool
lock_held

vb2_fop_release will be a wrapper for that funtion with lock_held== false

drivers that overload the fop_release and need to hold the lock will
call the __ function with lock_held= true

What do you think?

Thanks!

>
>
> From 3617684d759bb021e3cf1d862a91cb6e18d12052 Mon Sep 17 00:00:00 2001
> From: Sylwester Nawrocki <s.nawrocki@xxxxxxxxxxx>
> Date: Sat, 19 Oct 2013 11:48:10 +0200
> Subject: [PATCH] exynos4-is: Do not call vb2_fop_release() with queue lock
> held
>
> Currently vb2_fop_release() function doesn't take the queue lock,
> but it is going to change and then there would happen a deadlock
> in fimc_capture_release() and fimc_lite_release(), since these
> function take the video queue lock prior to calling vb2_fop_release().
>
> To avoid a deadlock open code the vb2_fop_release() function in those
> drivers.
>
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@xxxxxxxxxxx>
> ---
>  drivers/media/platform/exynos4-is/fimc-capture.c |   11 ++++++++---
>  drivers/media/platform/exynos4-is/fimc-lite.c    |    8 +++++++-
>  2 files changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/media/platform/exynos4-is/fimc-capture.c
> b/drivers/media/platform/exynos4-is/fimc-capture.c
> index fb27ff7..e9a5c90 100644
> --- a/drivers/media/platform/exynos4-is/fimc-capture.c
> +++ b/drivers/media/platform/exynos4-is/fimc-capture.c
> @@ -537,6 +537,7 @@ static int fimc_capture_release(struct file *file)
>  {
>         struct fimc_dev *fimc = video_drvdata(file);
>         struct fimc_vid_cap *vc = &fimc->vid_cap;
> +       struct video_device *vdev = &vc->ve.vdev;
>         bool close = v4l2_fh_is_singular_file(file);
>         int ret;
>
> @@ -545,11 +546,15 @@ static int fimc_capture_release(struct file *file)
>         mutex_lock(&fimc->lock);
>
>         if (close && vc->streaming) {
> -               media_entity_pipeline_stop(&vc->ve.vdev.entity);
> +               media_entity_pipeline_stop(&vdev->entity);
>                 vc->streaming = false;
>         }
>
> -       ret = vb2_fop_release(file);
> +       if (file->private_data == vdev->queue->owner) {
> +               vb2_queue_release(vdev->queue);
> +               vdev->queue->owner = NULL;
> +       }
> +       ret = v4l2_fh_release(file);
>
>         if (close) {
>                 clear_bit(ST_CAPT_BUSY, &fimc->state);
> @@ -557,7 +562,7 @@ static int fimc_capture_release(struct file *file)
>                 clear_bit(ST_CAPT_SUSPENDED, &fimc->state);
>
>                 fimc_md_graph_lock(&vc->ve);
> -               vc->ve.vdev.entity.use_count--;
> +               vdev->entity.use_count--;
>                 fimc_md_graph_unlock(&vc->ve);
>         }
>
> diff --git a/drivers/media/platform/exynos4-is/fimc-lite.c
> b/drivers/media/platform/exynos4-is/fimc-lite.c
> index e5798f7..182db3c 100644
> --- a/drivers/media/platform/exynos4-is/fimc-lite.c
> +++ b/drivers/media/platform/exynos4-is/fimc-lite.c
> @@ -528,6 +528,7 @@ static int fimc_lite_release(struct file *file)
>  {
>         struct fimc_lite *fimc = video_drvdata(file);
>         struct media_entity *entity = &fimc->ve.vdev.entity;
> +       struct video_device *vdev = &fimc->ve.vdev;
>
>         mutex_lock(&fimc->lock);
>
> @@ -546,7 +547,12 @@ static int fimc_lite_release(struct file *file)
>                 mutex_unlock(&entity->parent->graph_mutex);
>         }
>
> -       vb2_fop_release(file);
> +       if (file->private_data == vdev->queue->owner) {
> +               vb2_queue_release(vdev->queue);
> +               vdev->queue->owner = NULL;
> +       }
> +       v4l2_fh_release(file);
> +
>         pm_runtime_put(&fimc->pdev->dev);
>         clear_bit(ST_FLITE_SUSPENDED, &fimc->state);
>
> --
> 1.7.4.1



-- 
Ricardo Ribalda
--
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/