| 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
| ||
|
Message-ID: <CAPybu_3ngoGuV+emsX4F5CpLYeevpNbCNHONwAY7JAxm=hLvrg@mail.gmail.com>
Date: Tue, 29 Oct 2013 11:08:22 +0100
From: Ricardo Ribalda Delgado <ricardo.ribalda@...il.com>
To: Sylwester Nawrocki <sylvester.nawrocki@...il.com>
Cc: Kyungmin Park <kyungmin.park@...sung.com>,
Sylwester Nawrocki <s.nawrocki@...sung.com>,
Mauro Carvalho Chehab <m.chehab@...sung.com>,
Kukjin Kim <kgene.kim@...sung.com>,
Pawel Osciak <pawel@...iak.com>,
Marek Szyprowski <m.szyprowski@...sung.com>,
linux-media <linux-media@...r.kernel.org>,
"moderated list:ARM/S5P EXYNOS AR..."
<linux-arm-kernel@...ts.infradead.org>,
"moderated list:ARM/S5P EXYNOS AR..."
<linux-samsung-soc@...r.kernel.org>,
LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] videobuf2: Add missing lock held on vb2_fop_relase
Hello
Anybody has a comment here? If not I will post a patch with the
modifications propossed by Sylwester.
Thanks!
On Sat, Oct 19, 2013 at 10:08 PM, Ricardo Ribalda Delgado
<ricardo.ribalda@...il.com> wrote:
> Hello Sylwester
>
>
> On Sat, Oct 19, 2013 at 8:27 PM, Sylwester Nawrocki
> <sylvester.nawrocki@...il.com> wrote:
>> On 10/19/2013 06:07 PM, Ricardo Ribalda wrote:
>> [...]
>>>
>>> ---
>>> drivers/media/platform/exynos4-is/fimc-capture.c | 2 +-
>>> drivers/media/platform/exynos4-is/fimc-lite.c | 2 +-
>>> drivers/media/usb/em28xx/em28xx-video.c | 2 +-
>>> drivers/media/v4l2-core/videobuf2-core.c | 18 +++++++++++++++++-
>>> include/media/videobuf2-core.h | 2 ++
>>> 5 files changed, 22 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..c38d247c 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(file, true);
>>>
>>> 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..021d804 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(file, true);
>>> pm_runtime_put(&fimc->pdev->dev);
>>> clear_bit(ST_FLITE_SUSPENDED,&fimc->state);
>>>
>>>
>>> diff --git a/drivers/media/usb/em28xx/em28xx-video.c
>>> b/drivers/media/usb/em28xx/em28xx-video.c
>>> index 9d10334..6a5c147 100644
>>> --- a/drivers/media/usb/em28xx/em28xx-video.c
>>> +++ b/drivers/media/usb/em28xx/em28xx-video.c
>>> @@ -1664,7 +1664,7 @@ static int em28xx_v4l2_close(struct file *filp)
>>> em28xx_videodbg("users=%d\n", dev->users);
>>>
>>> mutex_lock(&dev->lock);
>>> - vb2_fop_release(filp);
>>> + __vb2_fop_release(filp, false);
>>
>>
>> I believe no modifications are needed for this driver.
>>
>>
>>> if (dev->users == 1) {
>>> /* the device is already disconnect,
>>> diff --git a/drivers/media/v4l2-core/videobuf2-core.c
>>> b/drivers/media/v4l2-core/videobuf2-core.c
>>> index 594c75e..ce309a8 100644
>>> --- a/drivers/media/v4l2-core/videobuf2-core.c
>>> +++ b/drivers/media/v4l2-core/videobuf2-core.c
>>> @@ -2619,16 +2619,32 @@ 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);
>>> +
>>> +int vb2_fop_release(struct file *file)
>>> +{
>>> + return __vb2_fop_release(file, false);
>>> +}
>>> EXPORT_SYMBOL_GPL(vb2_fop_release);
>>
>>
>> It might be better to make it something like:
>>
>
> The rationale behind my patch (and probably not properly commented) is
> that the vb2_fop_release must be used ONLY as a file operantion
> handler.
>
> If the user makes its own function for relase the __vb2_fop_release
> function must be used and the infrastructure must be notified about
> the status of he lock (he is on his own).
>
> I believe my approach is simpler because It has only two functions
> (instead of 3) and the user understand the difference of the two
> functions just by looking at the arguments. In the future we could
> even check statically that vb2_fop_release is not called inside a
> driver.
>
> Anyway, this is just a detail :), the most important part is that the
> oops is fixed, and that all the drivers that worked keep working.
>
> Lets wait for more comments and then lets post a new patch (with two
> functions and better documentation, or three functions).
>
> Thank you very much for you comments!!!
>
>> static int _vb2_fop_release(struct file *file, bool locked)
>>
>> {
>> struct video_device *vdev = video_devdata(file);
>> struct mutex *lock;
>>
>> if (file->private_data == vdev->queue->owner) {
>> lock = vdev->queue->lock ?
>> vdev->queue->lock : vdev->lock;
>>
>> if (lock && !locked)
>>
>> mutex_lock(lock);
>>
>> vb2_queue_release(vdev->queue);
>> vdev->queue->owner = NULL;
>> if (lock && !locked)
>> mutex_unlock(lock);
>> }
>> return v4l2_fh_release(file);
>> }
>>
>> int vb2_fop_release(struct file *file)
>> {
>> return _vb2_fop_release(file, false);
>> }
>> EXPORT_SYMBOL_GPL(vb2_fop_release);
>>
>> /*
>> * This function should be used instead of vb2_fop_release()
>> * if the caller already holds the video queue mutex.
>> */
>> int __vb2_fop_release(struct file *file)
>> {
>> return _vb2_fop_release(file, true);
>> }
>> EXPORT_SYMBOL_GPL(__vb2_fop_release);
>>
>> since __vb2_fop_release(file, false); is basically useless, it is same
>> as vb2_fop_release(file);
>>
>>
>>> ssize_t vb2_fop_write(struct file *file, char __user *buf,
>>> diff --git a/include/media/videobuf2-core.h
>>> b/include/media/videobuf2-core.h
>>> index 6781258..cd1e4d5 100644
>>> --- a/include/media/videobuf2-core.h
>>> +++ b/include/media/videobuf2-core.h
>>> @@ -491,6 +491,8 @@ int vb2_ioctl_expbuf(struct file *file, void *priv,
>>>
>>> int vb2_fop_mmap(struct file *file, struct vm_area_struct *vma);
>>> int vb2_fop_release(struct file *file);
>>> +/* must be used if the lock is held. */
>>
>>
>> Let's put any comments at the function body, not here.
>>
>>> +int __vb2_fop_release(struct file *file, bool lock_is_held);
>>
>>
>> int __vb2_fop_release(struct file *file);
>>
>>
>>> ssize_t vb2_fop_write(struct file *file, char __user *buf,
>>> size_t count, loff_t *ppos);
>>> ssize_t vb2_fop_read(struct file *file, char __user *buf,
>>
>>
>> Thanks,
>> Sylwester
>
>
>
> --
> Ricardo Ribalda
--
Ricardo Ribalda
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists