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

2013-11-02 Thread 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.nawro...@gmail.com wrote:
 Hi Ricardo,


 On 10/31/2013 09:54 PM, Ricardo Ribalda Delgado wrote:

 From: Ricardo Ribaldaricardo.riba...@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 Ribaldaricardo.riba...@gmail.com
 Signed-off-by: Ricardo Ribalda Delgadoricardo.riba...@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
--
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 v3]] videobuf2: Add missing lock held on vb2_fop_relase

2013-11-01 Thread Sylwester Nawrocki 

Hi Ricardo,

On 10/31/2013 09:54 PM, Ricardo Ribalda Delgado wrote:

From: Ricardo Ribaldaricardo.riba...@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... ? :)


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.


Signed-off-by: Ricardo Ribaldaricardo.riba...@gmail.com
Signed-off-by: Ricardo Ribalda Delgadoricardo.riba...@gmail.com


Is this duplication really needed ?


---



  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() ?


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 ?


+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
--
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

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

2013-10-31 Thread Ricardo Ribalda Delgado 
From: Ricardo Ribalda ricardo.riba...@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:

[ 308.297741] BUG: unable to handle kernel NULL pointer dereference at
0260
[ 308.297759] IP: [a07a9fd2] 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:[a07a9fd2] [a07a9fd2]
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] [a07aa3c4] vb2_read+0x14/0x20 [videobuf2_core]
[ 308.298198] [a07aa494] vb2_fop_read+0xc4/0x120 [videobuf2_core]
[ 308.298252] [8154ee9e] v4l2_read+0x7e/0xc0
[ 308.298296] [8116e639] vfs_read+0xa9/0x160
[ 308.298312] [8116e882] SyS_read+0x52/0xb0
[ 308.298328] [81784179] 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 [a07a9fd2] vb2_perform_fileio+0x372/0x610
[videobuf2_core]
[ 308.298507] RSP 88014dc03df8
[ 308.298514] CR2: 0260
[ 308.298526] ---[ end trace e8f01717c96d1e41 ]---

v2: Add bug found 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

Signed-off-by: Ricardo Ribalda ricardo.riba...@gmail.com
Signed-off-by: Ricardo Ribalda Delgado ricardo.riba...@gmail.com
---
 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);
 
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