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

[Ricardo Ribalda Delgado]

Hello Sylwester

Thanks for your comments. There is a new patch: v4! :)

On Fri, Nov 1, 2013 at 11:36 PM, Sylwester Nawrocki
<sylvester.nawrocki at gmail.com> wrote:
> Hi Ricardo,
>
>
> On 10/31/2013 09:54 PM, Ricardo Ribalda Delgado wrote:
>>
>> From: Ricardo Ribalda<ricardo.ribalda at gmail.com>
>>
>> vb2_fop_relase does not held the lock although it is modifying the
>> queue->owner field.
>>
>> This could lead to race conditions on the vb2_perform_io function
>> when multiple applications are accessing the video device via
>> read/write API:
>
> [...]
>
>> v2: Add bug found by Sylvester Nawrocki
>
>
> "v2: Add fix for a bug found..." ? :)

In Spanish it makes sense. it is fixed now, thanks

>
>
>> fimc-capture and fimc-lite where calling vb2_fop_release with the lock
>> held.
>> Therefore a new __vb2_fop_release function has been created to be used by
>> drivers that overload the release function.
>>
>> v3: Comments by Sylvester Nawrocki and Mauro Carvalho Chehab
>>
>> Use vb2_fop_release_locked instead of __vb2_fop_release
>
>
> Such notes normally go after the scissors line ("---") after Signed-off-by
> lines.

Fixed, thanks!

>
>
>> Signed-off-by: Ricardo Ribalda<ricardo.ribalda at gmail.com>
>> Signed-off-by: Ricardo Ribalda Delgado<ricardo.ribalda at gmail.com>
>
>
> Is this duplication really needed ?

I have different slightly different git configuration in 2 computers. Fixed now

>
>
>> ---
>
>
>>   drivers/media/platform/exynos4-is/fimc-capture.c |  2 +-
>>   drivers/media/platform/exynos4-is/fimc-lite.c    |  2 +-
>>   drivers/media/v4l2-core/videobuf2-core.c         | 24
>> +++++++++++++++++++++++-
>>   include/media/videobuf2-core.h                   |  1 +
>>   4 files changed, 26 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/media/platform/exynos4-is/fimc-capture.c
>> b/drivers/media/platform/exynos4-is/fimc-capture.c
>> index fb27ff7..c3c3b3b 100644
>> --- a/drivers/media/platform/exynos4-is/fimc-capture.c
>> +++ b/drivers/media/platform/exynos4-is/fimc-capture.c
>> @@ -549,7 +549,7 @@ static int fimc_capture_release(struct file *file)
>>                 vc->streaming = false;
>>         }
>>
>> -       ret = vb2_fop_release(file);
>> +       ret = vb2_fop_release_locked(file);
>
>
> I'm personally not happy with such a change. It is still not obvious
> if "locked" means that this function takes the lock internally or it
> should be called with the lock held. How about sticking to the common
> practice and instead naming it __vb2_fop_release() ?

I like the locked prefix, but it is a lost war :P. New version is named as __

>
>>         if (close) {
>>                 clear_bit(ST_CAPT_BUSY,&fimc->state);
>>
>> diff --git a/drivers/media/platform/exynos4-is/fimc-lite.c
>> b/drivers/media/platform/exynos4-is/fimc-lite.c
>> index e5798f7..b8d417f 100644
>> --- a/drivers/media/platform/exynos4-is/fimc-lite.c
>> +++ b/drivers/media/platform/exynos4-is/fimc-lite.c
>> @@ -546,7 +546,7 @@ static int fimc_lite_release(struct file *file)
>>                 mutex_unlock(&entity->parent->graph_mutex);
>>         }
>>
>> -       vb2_fop_release(file);
>> +       vb2_fop_release_locked(file);
>>         pm_runtime_put(&fimc->pdev->dev);
>>         clear_bit(ST_FLITE_SUSPENDED,&fimc->state);
>>
>>
>> diff --git a/drivers/media/v4l2-core/videobuf2-core.c
>> b/drivers/media/v4l2-core/videobuf2-core.c
>> index 594c75e..06e6dbd 100644
>> --- a/drivers/media/v4l2-core/videobuf2-core.c
>> +++ b/drivers/media/v4l2-core/videobuf2-core.c
>> @@ -2619,18 +2619,40 @@ int vb2_fop_mmap(struct file *file, struct
>> vm_area_struct *vma)
>>   }
>>   EXPORT_SYMBOL_GPL(vb2_fop_mmap);
>>
>> -int vb2_fop_release(struct file *file)
>> +int __vb2_fop_release(struct file *file, bool lock_is_held)
>>   {
>>         struct video_device *vdev = video_devdata(file);
>> +       struct mutex *lock;
>>
>>         if (file->private_data == vdev->queue->owner) {
>> +               if (lock_is_held)
>> +                       lock = NULL;
>> +               else
>> +                       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);
>>   }
>> +EXPORT_SYMBOL_GPL(__vb2_fop_release);
>
>
> We don't need to export this function, do we ?

Not really. Fixed

>
>
>> +int vb2_fop_release(struct file *file)
>> +{
>> +       return __vb2_fop_release(file, false);
>> +}
>>   EXPORT_SYMBOL_GPL(vb2_fop_release);
>>
>> +int vb2_fop_release_locked(struct file *file)
>> +{
>> +       return __vb2_fop_release(file, true);
>> +}
>> +EXPORT_SYMBOL_GPL(vb2_fop_release_locked);
>
>
> --
> Thanks,
> Sylwester

Thanks!



-- 
Ricardo Ribalda