Skip to contentSkip to conversation

videobuf2-core: take mmap_sem before calling __qbuf_userptr

PATCH Superseded Hans Verkuil 3 participants 3 messages Aug 22, 2014 – Aug 26, 2014 Media (V4L/DVB)

v2 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Hans Verkuil

author
#0 lore raw

PATCH v2 1/1 videobuf2-core: take mmap_sem before calling __qbuf_userptr

(Changes since v1: fix the embarrassing bug where mmap_sem wasn't initialized)

Commit f035eb4e976ef5a059e30bc91cfd310ff030a7d3 (videobuf2: fix lockdep warning) unfortunately removed the mmap_sem lock that is needed around the call to __qbuf_userptr. Amazingly nobody noticed this (especially me as the author) until Jan Kara pointed this out to me.

Signed-off-by Hans Verkuil <[email protected]>
Reported-by Jan Kara <[email protected]>
---
 drivers/media/v4l2-core/videobuf2-core.c | 4 ++++
 1 file changed, 4 insertions(+)
drivers/media/v4l2-core/videobuf2-core.c +4 -0
index 5b808e2..a0ab6af 100644
@@ -1591,6 +1591,7 @@ static void __enqueue_in_driver(struct vb2_buffer *vb)
 static int __buf_prepare(struct vb2_buffer *vb, const struct v4l2_buffer *b)
 {
 	struct vb2_queue *q = vb->vb2_queue;
+	struct rw_semaphore *mmap_sem;
 	int ret;
 
 	ret = __verify_length(vb, b);

@@ -1627,7 +1628,10 @@ static int __buf_prepare(struct vb2_buffer *vb, const struct v4l2_buffer *b)
 		ret = __qbuf_mmap(vb, b);
 		break;
 	case V4L2_MEMORY_USERPTR:
+		mmap_sem = &current->mm->mmap_sem;
+		down_read(mmap_sem);
 		ret = __qbuf_userptr(vb, b);
+		up_read(mmap_sem);
 		break;
 	case V4L2_MEMORY_DMABUF:
 		ret = __qbuf_dmabuf(vb, b);
Signature · 1 line
2.0.1
Acked-by on v2 1/1
#1 lore raw

Hi Hans,

Thank you for the patch.

Quoted 10 lines · On Friday 22 August 2014 23:31:31 Hans Verkuil wrote:

(Changes since v1: fix the embarrassing bug where mmap_sem wasn't initialized)

Commit f035eb4e976ef5a059e30bc91cfd310ff030a7d3 (videobuf2: fix lockdep warning) unfortunately removed the mmap_sem lock that is needed around the call to __qbuf_userptr. Amazingly nobody noticed this (especially me as the author) until Jan Kara pointed this out to me.

Signed-off-by Hans Verkuil <[email protected]>
Reported-by Jan Kara <[email protected]>
Acked-by Laurent Pinchart <[email protected]>

but please see below for one small comment.

drivers/media/v4l2-core/videobuf2-core.c static void __enqueue_in_driver(struct vb2_buffer *vb) · line 1594

Quoted 13 lines
---
 drivers/media/v4l2-core/videobuf2-core.c | 4 ++++
 1 file changed, 4 insertions(+)
(quoted diff) +0 -0

b/drivers/media/v4l2-core/videobuf2-core.c index 5b808e2..a0ab6af 100644

drivers/media/v4l2-core/videobuf2-core.c +0 -0
@@ -1591,6 +1591,7 @@ static void __enqueue_in_driver(struct vb2_buffer *vb)

static int __buf_prepare(struct vb2_buffer *vb, const struct v4l2_buffer *b) {

drivers/media/v4l2-core/videobuf2-core.c +1 -0
 	struct vb2_queue *q = vb->vb2_queue;
+	struct rw_semaphore *mmap_sem;

Do we really need the local variable ?

Quoted 14 lines
drivers/media/v4l2-core/videobuf2-core.c +3 -0
 	int ret;
 
 	ret = __verify_length(vb, b);

@@ -1627,7 +1628,10 @@ static int __buf_prepare(struct vb2_buffer *vb, const
 struct v4l2_buffer *b) ret = __qbuf_mmap(vb, b);
 		break;
 	case V4L2_MEMORY_USERPTR:
+		mmap_sem = &current->mm->mmap_sem;
+		down_read(mmap_sem);
 		ret = __qbuf_userptr(vb, b);
+		up_read(mmap_sem);
 		break;
 	case V4L2_MEMORY_DMABUF:
 		ret = __qbuf_dmabuf(vb, b);
Signature · 3 lines
Regards,

Laurent Pinchart
Reviewed-by on v2 1/1
#2 lore raw
In reply to Hans Verkuil
Quoted 9 lines · Hans Verkuil

(Changes since v1: fix the embarrassing bug where mmap_sem wasn't initialized)

Commit f035eb4e976ef5a059e30bc91cfd310ff030a7d3 (videobuf2: fix lockdep warning) unfortunately removed the mmap_sem lock that is needed around the call to __qbuf_userptr. Amazingly nobody noticed this (especially me as the author) until Jan Kara pointed this out to me.

Signed-off-by Hans Verkuil <[email protected]>
Reported-by Jan Kara <[email protected]>

The patch looks good to me. You can add:

Reviewed-by Jan Kara <[email protected]>

Honza

Quoted 30 lines
---
 drivers/media/v4l2-core/videobuf2-core.c | 4 ++++
 1 file changed, 4 insertions(+)
drivers/media/v4l2-core/videobuf2-core.c +4 -0
index 5b808e2..a0ab6af 100644
@@ -1591,6 +1591,7 @@ static void __enqueue_in_driver(struct vb2_buffer *vb)
 static int __buf_prepare(struct vb2_buffer *vb, const struct v4l2_buffer *b)
 {
 	struct vb2_queue *q = vb->vb2_queue;
+	struct rw_semaphore *mmap_sem;
 	int ret;
 
 	ret = __verify_length(vb, b);

@@ -1627,7 +1628,10 @@ static int __buf_prepare(struct vb2_buffer *vb, const struct v4l2_buffer *b)
 		ret = __qbuf_mmap(vb, b);
 		break;
 	case V4L2_MEMORY_USERPTR:
+		mmap_sem = &current->mm->mmap_sem;
+		down_read(mmap_sem);
 		ret = __qbuf_userptr(vb, b);
+		up_read(mmap_sem);
 		break;
 	case V4L2_MEMORY_DMABUF:
 		ret = __qbuf_dmabuf(vb, b);
Signature · 1 line
2.0.1
Signature · 2 lines
Jan Kara <[email protected]>
SUSE Labs, CR