Re: [PATCH v4] videobuf2: Add missing lock held on vb2_fop_relase
Here are the patches
https://patchwork.linuxtv.org/patch/20668/
https://patchwork.linuxtv.org/patch/20669/
Thanks!
On Wed, Nov 6, 2013 at 9:26 AM, Ricardo Ribalda Delgado
wrote:
> Hello Hans and Sywester
>
> I have just posted a new patch. I think it fits the suggestions from
> both of you, please take a look to it and please post any comment.
>
> I will also send a patch about the em28xx, to swap the lock order.
>
> Thanks for your comments
>
> On Mon, Nov 4, 2013 at 4:19 PM, Hans Verkuil wrote:
>> On 11/04/2013 03:24 PM, Sylwester Nawrocki wrote:
>>> On 04/11/13 15:12, Hans Verkuil wrote:
On 11/04/2013 02:54 PM, Ricardo Ribalda Delgado wrote:
>> Hello Hans
>>
>> Thanks for your comments.
>>
>> Please take a look to v4 of this patch
>> https://patchwork.linuxtv.org/patch/20529/
>>
>> On Mon, Nov 4, 2013 at 1:37 PM, Hans Verkuil wrote:
On 11/02/2013 10:53 AM, Ricardo Ribalda Delgado wrote:
>> From: Ricardo Ribalda
>>
>> 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:
It's also called directly by drivers/media/usb/em28xx/em28xx-video.c!
>>
>> em28xx-video does not hold the lock, therefore it can call the normal
>> function. On v2 we made a internal function that should be called if
>> the funciton is called directly by the driver. Please take a look to
>> the old comments. https://patchwork.linuxtv.org/patch/20460/
static int em28xx_v4l2_close(struct file *filp)
{
struct em28xx_fh *fh = filp->private_data;
struct em28xx*dev = fh->dev;
int errCode;
em28xx_videodbg("users=%d\n", dev->users);
mutex_lock(&dev->lock);
vb2_fop_release(filp);
...
vb2_fop_release(filp) will, with your patch, also try to get dev->lock.
Sylwester's comment re em28xx is incorrect.
>>>
>>> dev->lock is not used as the video queue lock:
>>>
>>> $ git grep "lock =" drivers/media/usb/em28xx/
>>> ...
>>> drivers/media/usb/em28xx/em28xx-video.c:dev->vdev->queue->lock =
>>> &dev->vb_queue_lock;
>>> drivers/media/usb/em28xx/em28xx-video.c:
>>> dev->vbi_dev->queue->lock = &dev->vb_vbi_queue_lock;
>>>
>>> There is a separate mutex for the video queue which needs to be acquired
>>> independently.
>>
>> Darn, I missed that one. I was looking for it in em28xx_vdev_init(), which is
>> where I expected the queue->lock to be set, if there was any.
>>
>> That said, wouldn't it be a good idea to swap the order:
>>
>> vb2_fop_release(filp);
>> mutex_lock(&dev->lock);
>>
>> I don't believe there is a good reason for nesting mutexes here.
>>
>> Regards,
>>
>> Hans
>>
>>>
>>> --
>>> Regards,
>>> Sylwester
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-media" in
>>> the body of a message to majord...@vger.kernel.org
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>
>>
>
>
>
> --
> Ricardo Ribalda
--
Ricardo Ribalda
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4] videobuf2: Add missing lock held on vb2_fop_relase
Hello Hans and Sywester
I have just posted a new patch. I think it fits the suggestions from
both of you, please take a look to it and please post any comment.
I will also send a patch about the em28xx, to swap the lock order.
Thanks for your comments
On Mon, Nov 4, 2013 at 4:19 PM, Hans Verkuil wrote:
> On 11/04/2013 03:24 PM, Sylwester Nawrocki wrote:
>> On 04/11/13 15:12, Hans Verkuil wrote:
>>> On 11/04/2013 02:54 PM, Ricardo Ribalda Delgado wrote:
> Hello Hans
>
> Thanks for your comments.
>
> Please take a look to v4 of this patch
> https://patchwork.linuxtv.org/patch/20529/
>
> On Mon, Nov 4, 2013 at 1:37 PM, Hans Verkuil wrote:
>>> On 11/02/2013 10:53 AM, Ricardo Ribalda Delgado wrote:
> From: Ricardo Ribalda
>
> 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:
>>>
>>> It's also called directly by drivers/media/usb/em28xx/em28xx-video.c!
>>>
>
> em28xx-video does not hold the lock, therefore it can call the normal
> function. On v2 we made a internal function that should be called if
> the funciton is called directly by the driver. Please take a look to
> the old comments. https://patchwork.linuxtv.org/patch/20460/
>>>
>>> static int em28xx_v4l2_close(struct file *filp)
>>> {
>>> struct em28xx_fh *fh = filp->private_data;
>>> struct em28xx*dev = fh->dev;
>>> int errCode;
>>>
>>> em28xx_videodbg("users=%d\n", dev->users);
>>>
>>> mutex_lock(&dev->lock);
>>> vb2_fop_release(filp);
>>> ...
>>>
>>> vb2_fop_release(filp) will, with your patch, also try to get dev->lock.
>>>
>>> Sylwester's comment re em28xx is incorrect.
>>
>> dev->lock is not used as the video queue lock:
>>
>> $ git grep "lock =" drivers/media/usb/em28xx/
>> ...
>> drivers/media/usb/em28xx/em28xx-video.c:dev->vdev->queue->lock =
>> &dev->vb_queue_lock;
>> drivers/media/usb/em28xx/em28xx-video.c:
>> dev->vbi_dev->queue->lock = &dev->vb_vbi_queue_lock;
>>
>> There is a separate mutex for the video queue which needs to be acquired
>> independently.
>
> Darn, I missed that one. I was looking for it in em28xx_vdev_init(), which is
> where I expected the queue->lock to be set, if there was any.
>
> That said, wouldn't it be a good idea to swap the order:
>
> vb2_fop_release(filp);
> mutex_lock(&dev->lock);
>
> I don't believe there is a good reason for nesting mutexes here.
>
> Regards,
>
> Hans
>
>>
>> --
>> Regards,
>> Sylwester
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-media" in
>> the body of a message to majord...@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>
--
Ricardo Ribalda
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4] videobuf2: Add missing lock held on vb2_fop_relase
On 11/04/2013 03:24 PM, Sylwester Nawrocki wrote:
> On 04/11/13 15:12, Hans Verkuil wrote:
>> On 11/04/2013 02:54 PM, Ricardo Ribalda Delgado wrote:
Hello Hans
Thanks for your comments.
Please take a look to v4 of this patch
https://patchwork.linuxtv.org/patch/20529/
On Mon, Nov 4, 2013 at 1:37 PM, Hans Verkuil wrote:
>> On 11/02/2013 10:53 AM, Ricardo Ribalda Delgado wrote:
From: Ricardo Ribalda
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:
>>
>> It's also called directly by drivers/media/usb/em28xx/em28xx-video.c!
>>
em28xx-video does not hold the lock, therefore it can call the normal
function. On v2 we made a internal function that should be called if
the funciton is called directly by the driver. Please take a look to
the old comments. https://patchwork.linuxtv.org/patch/20460/
>>
>> static int em28xx_v4l2_close(struct file *filp)
>> {
>> struct em28xx_fh *fh = filp->private_data;
>> struct em28xx*dev = fh->dev;
>> int errCode;
>>
>> em28xx_videodbg("users=%d\n", dev->users);
>>
>> mutex_lock(&dev->lock);
>> vb2_fop_release(filp);
>> ...
>>
>> vb2_fop_release(filp) will, with your patch, also try to get dev->lock.
>>
>> Sylwester's comment re em28xx is incorrect.
>
> dev->lock is not used as the video queue lock:
>
> $ git grep "lock =" drivers/media/usb/em28xx/
> ...
> drivers/media/usb/em28xx/em28xx-video.c:dev->vdev->queue->lock =
> &dev->vb_queue_lock;
> drivers/media/usb/em28xx/em28xx-video.c:
> dev->vbi_dev->queue->lock = &dev->vb_vbi_queue_lock;
>
> There is a separate mutex for the video queue which needs to be acquired
> independently.
Darn, I missed that one. I was looking for it in em28xx_vdev_init(), which is
where I expected the queue->lock to be set, if there was any.
That said, wouldn't it be a good idea to swap the order:
vb2_fop_release(filp);
mutex_lock(&dev->lock);
I don't believe there is a good reason for nesting mutexes here.
Regards,
Hans
>
> --
> Regards,
> Sylwester
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4] videobuf2: Add missing lock held on vb2_fop_relase
On 04/11/13 15:12, Hans Verkuil wrote:
> On 11/04/2013 02:54 PM, Ricardo Ribalda Delgado wrote:
>> > Hello Hans
>> >
>> > Thanks for your comments.
>> >
>> > Please take a look to v4 of this patch
>> > https://patchwork.linuxtv.org/patch/20529/
>> >
>> > On Mon, Nov 4, 2013 at 1:37 PM, Hans Verkuil wrote:
>>> >> On 11/02/2013 10:53 AM, Ricardo Ribalda Delgado wrote:
>>> From: Ricardo Ribalda
>>>
>>> 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:
>>> >>
>>> >> It's also called directly by drivers/media/usb/em28xx/em28xx-video.c!
>>> >>
>> >
>> > em28xx-video does not hold the lock, therefore it can call the normal
>> > function. On v2 we made a internal function that should be called if
>> > the funciton is called directly by the driver. Please take a look to
>> > the old comments. https://patchwork.linuxtv.org/patch/20460/
>
> static int em28xx_v4l2_close(struct file *filp)
> {
> struct em28xx_fh *fh = filp->private_data;
> struct em28xx*dev = fh->dev;
> int errCode;
>
> em28xx_videodbg("users=%d\n", dev->users);
>
> mutex_lock(&dev->lock);
> vb2_fop_release(filp);
> ...
>
> vb2_fop_release(filp) will, with your patch, also try to get dev->lock.
>
> Sylwester's comment re em28xx is incorrect.
dev->lock is not used as the video queue lock:
$ git grep "lock =" drivers/media/usb/em28xx/
...
drivers/media/usb/em28xx/em28xx-video.c:dev->vdev->queue->lock =
&dev->vb_queue_lock;
drivers/media/usb/em28xx/em28xx-video.c:
dev->vbi_dev->queue->lock = &dev->vb_vbi_queue_lock;
There is a separate mutex for the video queue which needs to be acquired
independently.
--
Regards,
Sylwester
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4] videobuf2: Add missing lock held on vb2_fop_relase
On 11/04/2013 02:54 PM, Ricardo Ribalda Delgado wrote:
> Hello Hans
>
> Thanks for your comments.
>
> Please take a look to v4 of this patch
> https://patchwork.linuxtv.org/patch/20529/
>
> On Mon, Nov 4, 2013 at 1:37 PM, Hans Verkuil wrote:
>> On 11/02/2013 10:53 AM, Ricardo Ribalda Delgado wrote:
>>> From: Ricardo Ribalda
>>>
>>> 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:
>>
>> It's also called directly by drivers/media/usb/em28xx/em28xx-video.c!
>>
>
> em28xx-video does not hold the lock, therefore it can call the normal
> function. On v2 we made a internal function that should be called if
> the funciton is called directly by the driver. Please take a look to
> the old comments. https://patchwork.linuxtv.org/patch/20460/
static int em28xx_v4l2_close(struct file *filp)
{
struct em28xx_fh *fh = filp->private_data;
struct em28xx*dev = fh->dev;
int errCode;
em28xx_videodbg("users=%d\n", dev->users);
mutex_lock(&dev->lock);
vb2_fop_release(filp);
...
vb2_fop_release(filp) will, with your patch, also try to get dev->lock.
Sylwester's comment re em28xx is incorrect.
>
>>>
>>> [ 308.297741] BUG: unable to handle kernel NULL pointer dereference at
>>> 0260
>>> [ 308.297759] IP: [] vb2_perform_fileio+0x372/0x610
>>> [videobuf2_core]
>>> [ 308.297794] PGD 159719067 PUD 158119067 PMD 0
>>> [ 308.297812] Oops: #1 SMP
>>> [ 308.297826] Modules linked in: qt5023_video videobuf2_dma_sg
>>> qtec_xform videobuf2_vmalloc videobuf2_memops videobuf2_core
>>> qtec_white qtec_mem gpio_xilinx qtec_cmosis qtec_pcie fglrx(PO)
>>> spi_xilinx spi_bitbang qt5023
>>> [ 308.297888] CPU: 1 PID: 2189 Comm: java Tainted: P O 3.11.0-qtec-standard
>>> #1
>>> [ 308.297919] Hardware name: QTechnology QT5022/QT5022, BIOS
>>> PM_2.1.0.309 X64 05/23/2013
>>> [ 308.297952] task: 8801564e1690 ti: 88014dc02000 task.ti:
>>> 88014dc02000
>>> [ 308.297962] RIP: 0010:[] []
>>> vb2_perform_fileio+0x372/0x610 [videobuf2_core]
>>> [ 308.297985] RSP: 0018:88014dc03df8 EFLAGS: 00010202
>>> [ 308.297995] RAX: RBX: 880158a23000 RCX:
>>> dead00100100
>>> [ 308.298003] RDX: RSI: dead00200200 RDI:
>>>
>>> [ 308.298012] RBP: 88014dc03e58 R08: R09:
>>> 0001
>>> [ 308.298020] R10: ea00051e8380 R11: 88014dc03fd8 R12:
>>> 880158a23070
>>> [ 308.298029] R13: 8801549040b8 R14: 00198000 R15:
>>> 01887e60
>>> [ 308.298040] FS: 7f65130d5700() GS:88015ed0()
>>> knlGS:
>>> [ 308.298049] CS: 0010 DS: ES: CR0: 80050033
>>> [ 308.298057] CR2: 0260 CR3: 00015963 CR4:
>>> 07e0
>>> [ 308.298064] Stack:
>>> [ 308.298071] 880156416c00 00198000
>>> 88010001
>>> [ 308.298087] 88014dc03f50 810a79ca 00020001
>>> 880154904718
>>> [ 308.298101] 880156416c00 00198000 880154904338
>>> 88014dc03f50
>>> [ 308.298116] Call Trace:
>>> [ 308.298143] [] vb2_read+0x14/0x20 [videobuf2_core]
>>> [ 308.298198] [] vb2_fop_read+0xc4/0x120 [videobuf2_core]
>>> [ 308.298252] [] v4l2_read+0x7e/0xc0
>>> [ 308.298296] [] vfs_read+0xa9/0x160
>>> [ 308.298312] [] SyS_read+0x52/0xb0
>>> [ 308.298328] [] tracesys+0xd0/0xd5
>>> [ 308.298335] Code: e5 d6 ff ff 83 3d be 24 00 00 04 89 c2 4c 8b 45 b0
>>> 44 8b 4d b8 0f 8f 20 02 00 00 85 d2 75 32 83 83 78 03 00 00 01 4b 8b
>>> 44 c5 48 <8b> 88 60 02 00 00 85 c9 0f 84 b0 00 00 00 8b 40 58 89 c2 41
>>> 89
>>> [ 308.298487] RIP [] vb2_perform_fileio+0x372/0x610
>>> [videobuf2_core]
>>> [ 308.298507] RSP
>>> [ 308.298514] CR2: 0260
>>> [ 308.298526] ---[ end trace e8f01717c96d1e41 ]---
>>>
>>> Signed-off-by: Ricardo Ribalda
>>> ---
>>> v2: Comments by Sylvester Nawrocki
>>>
>>> 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
>>>
>>> v4: Comments by Sylvester Nawrocki
>>>
>>> Rename vb2_fop_release_locked to __vb2_fop_release and fix patch format
>>>
>>> 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 | 23
>>> ++-
>>> include/media/videobuf2-core.h | 1 +
>>> 4 files changed, 25 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/media/platform/exynos4-is/fimc-capture.c
>>> b/drive
Re: [PATCH v4] videobuf2: Add missing lock held on vb2_fop_relase
Hi,
On 04/11/13 14:54, Ricardo Ribalda Delgado wrote:
> Hello Hans
>
> Thanks for your comments.
>
> Please take a look to v4 of this patch
> https://patchwork.linuxtv.org/patch/20529/
We're discussing v4 actually ;)
> On Mon, Nov 4, 2013 at 1:37 PM, Hans Verkuil wrote:
>> On 11/02/2013 10:53 AM, Ricardo Ribalda Delgado wrote:
>>> From: Ricardo Ribalda
>>>
>>> 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:
>>
>> It's also called directly by drivers/media/usb/em28xx/em28xx-video.c!
>>
>
> em28xx-video does not hold the lock, therefore it can call the normal
> function. On v2 we made a internal function that should be called if
> the funciton is called directly by the driver. Please take a look to
> the old comments. https://patchwork.linuxtv.org/patch/20460/
[...]
>>> -int vb2_fop_release(struct file *file)
>>> +static 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);
>>> }
>>> +
>>> +int vb2_fop_release(struct file *file)
>>> +{
>>> + return _vb2_fop_release(file, false);
>>> +}
>>> EXPORT_SYMBOL_GPL(vb2_fop_release);
>>>
>>> +int __vb2_fop_release(struct file *file)
>>> +{
>>> + return _vb2_fop_release(file, true);
>>> +}
>>> +EXPORT_SYMBOL_GPL(__vb2_fop_release);
>>
>> Sorry for introducing yet another opinion, but I think this is very
>> confusing.
>
> It is confusing the lock_held parameter or the __ naming for unlocked
> versions?
>
>>
>> I would do this:
>>
>> static int __vb2_fop_release(struct file *file, struct mutex *lock)
>> {
>> struct video_device *vdev = video_devdata(file);
>>
>> if (file->private_data == vdev->queue->owner) {
>> if (lock)
>> mutex_lock(lock);
>> vb2_queue_release(vdev->queue);
>> vdev->queue->owner = NULL;
>> if (lock)
>> mutex_unlock(lock);
>> }
>> return v4l2_fh_release(file);
>> }
>>
>> int vb2_fop_release(struct file *file)
>> {
>> struct video_device *vdev = video_devdata(file);
>> struct mutex *lock = vdev->queue->lock ?
>> vdev->queue->lock : vdev->lock;
>>
>> return __vb2_fop_release(file, lock);
>> }
>> EXPORT_SYMBOL_GPL(vb2_fop_release);
>>
>> int vb2_fop_release_unlock(struct file *file)
>> {
>> return __vb2_fop_release(file, NULL);
>> }
>> EXPORT_SYMBOL_GPL(vb2_fop_release_unlock);
>>
>> Optionally, __vb2_fop_release can be exported and then vb2_fop_release_unlock
>> isn't necessary.
>>
>
> i dont have any strong opinion in any direction. All I really care is
> that the oops is fixed :).
>
> If your concern about the patch is the is_lock_held function, I can
> make a patch with the params on your proposal and the __naming as
> Sylvester suggested, so everyone is happy.
>
> Sylvester, Hanshat do you think?
I like Hans' proposal, probably it's better to export __vb2_fop_release(),
so it can be also used in cases like em28xx, to make things a bit more
clear. But I'm fine with vb2_fop_release_unlock() as well, don't really
have strong preference. It's up to you.
--
Thanks,
Sylwester
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4] videobuf2: Add missing lock held on vb2_fop_relase
Hello Hans
Thanks for your comments.
Please take a look to v4 of this patch
https://patchwork.linuxtv.org/patch/20529/
On Mon, Nov 4, 2013 at 1:37 PM, Hans Verkuil wrote:
> On 11/02/2013 10:53 AM, Ricardo Ribalda Delgado wrote:
>> From: Ricardo Ribalda
>>
>> 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:
>
> It's also called directly by drivers/media/usb/em28xx/em28xx-video.c!
>
em28xx-video does not hold the lock, therefore it can call the normal
function. On v2 we made a internal function that should be called if
the funciton is called directly by the driver. Please take a look to
the old comments. https://patchwork.linuxtv.org/patch/20460/
>>
>> [ 308.297741] BUG: unable to handle kernel NULL pointer dereference at
>> 0260
>> [ 308.297759] IP: [] vb2_perform_fileio+0x372/0x610
>> [videobuf2_core]
>> [ 308.297794] PGD 159719067 PUD 158119067 PMD 0
>> [ 308.297812] Oops: #1 SMP
>> [ 308.297826] Modules linked in: qt5023_video videobuf2_dma_sg
>> qtec_xform videobuf2_vmalloc videobuf2_memops videobuf2_core
>> qtec_white qtec_mem gpio_xilinx qtec_cmosis qtec_pcie fglrx(PO)
>> spi_xilinx spi_bitbang qt5023
>> [ 308.297888] CPU: 1 PID: 2189 Comm: java Tainted: P O 3.11.0-qtec-standard
>> #1
>> [ 308.297919] Hardware name: QTechnology QT5022/QT5022, BIOS
>> PM_2.1.0.309 X64 05/23/2013
>> [ 308.297952] task: 8801564e1690 ti: 88014dc02000 task.ti:
>> 88014dc02000
>> [ 308.297962] RIP: 0010:[] []
>> vb2_perform_fileio+0x372/0x610 [videobuf2_core]
>> [ 308.297985] RSP: 0018:88014dc03df8 EFLAGS: 00010202
>> [ 308.297995] RAX: RBX: 880158a23000 RCX:
>> dead00100100
>> [ 308.298003] RDX: RSI: dead00200200 RDI:
>>
>> [ 308.298012] RBP: 88014dc03e58 R08: R09:
>> 0001
>> [ 308.298020] R10: ea00051e8380 R11: 88014dc03fd8 R12:
>> 880158a23070
>> [ 308.298029] R13: 8801549040b8 R14: 00198000 R15:
>> 01887e60
>> [ 308.298040] FS: 7f65130d5700() GS:88015ed0()
>> knlGS:
>> [ 308.298049] CS: 0010 DS: ES: CR0: 80050033
>> [ 308.298057] CR2: 0260 CR3: 00015963 CR4:
>> 07e0
>> [ 308.298064] Stack:
>> [ 308.298071] 880156416c00 00198000
>> 88010001
>> [ 308.298087] 88014dc03f50 810a79ca 00020001
>> 880154904718
>> [ 308.298101] 880156416c00 00198000 880154904338
>> 88014dc03f50
>> [ 308.298116] Call Trace:
>> [ 308.298143] [] vb2_read+0x14/0x20 [videobuf2_core]
>> [ 308.298198] [] vb2_fop_read+0xc4/0x120 [videobuf2_core]
>> [ 308.298252] [] v4l2_read+0x7e/0xc0
>> [ 308.298296] [] vfs_read+0xa9/0x160
>> [ 308.298312] [] SyS_read+0x52/0xb0
>> [ 308.298328] [] tracesys+0xd0/0xd5
>> [ 308.298335] Code: e5 d6 ff ff 83 3d be 24 00 00 04 89 c2 4c 8b 45 b0
>> 44 8b 4d b8 0f 8f 20 02 00 00 85 d2 75 32 83 83 78 03 00 00 01 4b 8b
>> 44 c5 48 <8b> 88 60 02 00 00 85 c9 0f 84 b0 00 00 00 8b 40 58 89 c2 41
>> 89
>> [ 308.298487] RIP [] vb2_perform_fileio+0x372/0x610
>> [videobuf2_core]
>> [ 308.298507] RSP
>> [ 308.298514] CR2: 0260
>> [ 308.298526] ---[ end trace e8f01717c96d1e41 ]---
>>
>> Signed-off-by: Ricardo Ribalda
>> ---
>> v2: Comments by Sylvester Nawrocki
>>
>> 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
>>
>> v4: Comments by Sylvester Nawrocki
>>
>> Rename vb2_fop_release_locked to __vb2_fop_release and fix patch format
>>
>> 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 | 23
>> ++-
>> include/media/videobuf2-core.h | 1 +
>> 4 files changed, 25 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..8192fe0 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);
>>
>> 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 e
Re: [PATCH v4] videobuf2: Add missing lock held on vb2_fop_relase
On 11/02/2013 10:53 AM, Ricardo Ribalda Delgado wrote:
> From: Ricardo Ribalda
>
> 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:
It's also called directly by drivers/media/usb/em28xx/em28xx-video.c!
>
> [ 308.297741] BUG: unable to handle kernel NULL pointer dereference at
> 0260
> [ 308.297759] IP: [] vb2_perform_fileio+0x372/0x610
> [videobuf2_core]
> [ 308.297794] PGD 159719067 PUD 158119067 PMD 0
> [ 308.297812] Oops: #1 SMP
> [ 308.297826] Modules linked in: qt5023_video videobuf2_dma_sg
> qtec_xform videobuf2_vmalloc videobuf2_memops videobuf2_core
> qtec_white qtec_mem gpio_xilinx qtec_cmosis qtec_pcie fglrx(PO)
> spi_xilinx spi_bitbang qt5023
> [ 308.297888] CPU: 1 PID: 2189 Comm: java Tainted: P O 3.11.0-qtec-standard #1
> [ 308.297919] Hardware name: QTechnology QT5022/QT5022, BIOS
> PM_2.1.0.309 X64 05/23/2013
> [ 308.297952] task: 8801564e1690 ti: 88014dc02000 task.ti:
> 88014dc02000
> [ 308.297962] RIP: 0010:[] []
> vb2_perform_fileio+0x372/0x610 [videobuf2_core]
> [ 308.297985] RSP: 0018:88014dc03df8 EFLAGS: 00010202
> [ 308.297995] RAX: RBX: 880158a23000 RCX:
> dead00100100
> [ 308.298003] RDX: RSI: dead00200200 RDI:
>
> [ 308.298012] RBP: 88014dc03e58 R08: R09:
> 0001
> [ 308.298020] R10: ea00051e8380 R11: 88014dc03fd8 R12:
> 880158a23070
> [ 308.298029] R13: 8801549040b8 R14: 00198000 R15:
> 01887e60
> [ 308.298040] FS: 7f65130d5700() GS:88015ed0()
> knlGS:
> [ 308.298049] CS: 0010 DS: ES: CR0: 80050033
> [ 308.298057] CR2: 0260 CR3: 00015963 CR4:
> 07e0
> [ 308.298064] Stack:
> [ 308.298071] 880156416c00 00198000
> 88010001
> [ 308.298087] 88014dc03f50 810a79ca 00020001
> 880154904718
> [ 308.298101] 880156416c00 00198000 880154904338
> 88014dc03f50
> [ 308.298116] Call Trace:
> [ 308.298143] [] vb2_read+0x14/0x20 [videobuf2_core]
> [ 308.298198] [] vb2_fop_read+0xc4/0x120 [videobuf2_core]
> [ 308.298252] [] v4l2_read+0x7e/0xc0
> [ 308.298296] [] vfs_read+0xa9/0x160
> [ 308.298312] [] SyS_read+0x52/0xb0
> [ 308.298328] [] tracesys+0xd0/0xd5
> [ 308.298335] Code: e5 d6 ff ff 83 3d be 24 00 00 04 89 c2 4c 8b 45 b0
> 44 8b 4d b8 0f 8f 20 02 00 00 85 d2 75 32 83 83 78 03 00 00 01 4b 8b
> 44 c5 48 <8b> 88 60 02 00 00 85 c9 0f 84 b0 00 00 00 8b 40 58 89 c2 41
> 89
> [ 308.298487] RIP [] vb2_perform_fileio+0x372/0x610
> [videobuf2_core]
> [ 308.298507] RSP
> [ 308.298514] CR2: 0260
> [ 308.298526] ---[ end trace e8f01717c96d1e41 ]---
>
> Signed-off-by: Ricardo Ribalda
> ---
> v2: Comments by Sylvester Nawrocki
>
> 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
>
> v4: Comments by Sylvester Nawrocki
>
> Rename vb2_fop_release_locked to __vb2_fop_release and fix patch format
>
> 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 | 23 ++-
> include/media/videobuf2-core.h | 1 +
> 4 files changed, 25 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..8192fe0 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);
>
> 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..cbe51cd 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);
> 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 59
