[PATCH] videobuf2-core: take mmap_sem before calling __qbuf_userptr

[PATCH] videobuf2-core: take mmap_sem before calling __qbuf_userptr

[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 <hans.verkuil@cisco.com>
Reported-by: Jan Kara <jack@suse.cz>
---
 drivers/media/v4l2-core/videobuf2-core.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index 5b808e2..2f6ac7e 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -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);
-- 
2.0.1

Re: [PATCH] videobuf2-core: take mmap_sem before calling __qbuf_userptr

[Laurent Pinchart]

Hi Hans,

Thank you for the patch.

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 <hans.verkuil@cisco.com>
> Reported-by: Jan Kara <jack@suse.cz>
> ---
>  drivers/media/v4l2-core/videobuf2-core.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/videobuf2-core.c
> b/drivers/media/v4l2-core/videobuf2-core.c index 5b808e2..2f6ac7e 100644
> --- a/drivers/media/v4l2-core/videobuf2-core.c
> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> @@ -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;

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 :-)

>  	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);

-- 
Regards,

Laurent Pinchart

Re: [PATCH] videobuf2-core: take mmap_sem before calling __qbuf_userptr

[Jan Kara]

On Fri 22-08-14 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 <hans.verkuil@cisco.com>
> Reported-by: Jan Kara <jack@suse.cz>
...
> @@ -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
> +		down_read(mmap_sem);
>  		ret = __qbuf_userptr(vb, b);
> +		up_read(mmap_sem);
>  		break;
>  	case V4L2_MEMORY_DMABUF:
>  		ret = __qbuf_dmabuf(vb, b);
> -- 
> 2.0.1
> 
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: [PATCH] videobuf2-core: take mmap_sem before calling __qbuf_userptr

[Hans Verkuil]

On 08/22/2014 07:34 PM, Jan Kara wrote:
> On Fri 22-08-14 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 <hans.verkuil@cisco.com>
>> Reported-by: Jan Kara <jack@suse.cz>
> ...
>> @@ -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

> 
> 								Honza
>> +		down_read(mmap_sem);
>>  		ret = __qbuf_userptr(vb, b);
>> +		up_read(mmap_sem);
>>  		break;
>>  	case V4L2_MEMORY_DMABUF:
>>  		ret = __qbuf_dmabuf(vb, b);
>> -- 
>> 2.0.1
>>