Skip to contentSkip to conversation

videobuf2-core: take mmap_sem before calling __qbuf_userptr

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

v1 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Hans Verkuil

author
#0 lore raw

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

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 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 | 3 +++
 1 file changed, 3 insertions(+)
drivers/media/v4l2-core/videobuf2-core.c +3 -0
index 5b808e2..2f6ac7e 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,9 @@ static int __buf_prepare(struct vb2_buffer *vb, const struct v4l2_buffer *b)
 		ret = __qbuf_mmap(vb, b);
 		break;
 	case V4L2_MEMORY_USERPTR:
+		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
#1 lore raw

Hi Hans,

Thank you for the patch.

Quoted 18 lines · On Friday 22 August 2014 18:01:09 Hans Verkuil wrote:

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 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 | 3 +++
 1 file changed, 3 insertions(+)
(quoted diff) +0 -0

b/drivers/media/v4l2-core/videobuf2-core.c index 5b808e2..2f6ac7e 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 static void __enqueue_in_driver(struct vb2_buffer *vb) · line 1594

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

I don't want to to be included in the category of the "amazing reviewers who don't notice obvious issues" twice in a row, so I'll point out the obvious lack of initialization of the mmap_sem variable :-)

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

@@ -1627,7 +1628,9 @@ static int __buf_prepare(struct vb2_buffer *vb, const
 struct v4l2_buffer *b) ret = __qbuf_mmap(vb, b);
 		break;
 	case V4L2_MEMORY_USERPTR:
+		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
#2 lore raw
In reply to Hans Verkuil
Quoted 7 lines · Hans Verkuil

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 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 static int __buf_prepare(struct vb2_buffer *vb, const struct v4l2_buffer *b) · line 1630

Quoted 4 lines
drivers/media/v4l2-core/videobuf2-core.c +0 -0
@@ -1627,7 +1628,9 @@ static int __buf_prepare(struct vb2_buffer *vb, const struct v4l2_buffer *b)
 		ret = __qbuf_mmap(vb, b);
 		break;
 	case V4L2_MEMORY_USERPTR:

I guess you are missing something like:

mmap_sem = &current->mm->mmap_sem;

Honza

Quoted 9 lines
drivers/media/v4l2-core/videobuf2-core.c +2 -0
+		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
author
#3 lore raw
Quoted 8 lines · Jan Kara
Quoted 7 lines · Hans Verkuil

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 until Jan Kara pointed this out to me.

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

...

drivers/media/v4l2-core/videobuf2-core.c static int __buf_prepare(struct vb2_buffer *vb, const struct v4l2_buffer *b) · line 1630

Quoted 4 lines
drivers/media/v4l2-core/videobuf2-core.c +0 -0
@@ -1627,7 +1628,9 @@ static int __buf_prepare(struct vb2_buffer *vb, const struct v4l2_buffer *b)
 		ret = __qbuf_mmap(vb, b);
 		break;
 	case V4L2_MEMORY_USERPTR:

I guess you are missing something like:

mmap_sem = &current->mm->mmap_sem;

Insert <red face>.

Oops, I'll repost.

Regards,

Hans

Quoted 11 lines

Honza

Quoted 9 lines
drivers/media/v4l2-core/videobuf2-core.c +2 -0
+		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