omap_vout: find_vma() needs ->mmap_sem held

PATCH In review Al Viro 4 participants 8 messages Dec 15, 2012 – Jan 7, 2013 Media (V4L/DVB)

Share & tools

Al Viro

author

2012-12-15 20:12 UTC #0 lore raw

PATCH 1/1 omap_vout: find_vma() needs ->mmap_sem held

Walking rbtree while it's modified is a Bad Idea(tm); besides,
the result of find_vma() can be freed just as it's getting returned to caller. Fortunately, it's easy to fix - just take ->mmap_sem a bit earlier (and don't bother with find_vma() at all if virtp >= PAGE_OFFSET - in that case we don't even look at its result).

Cc [email protected] [2.6.35]

Signed-off-by Al Viro <[email protected]>

---

drivers/media/platform/omap/omap_vout.c +6 -4

index 9935040..984512f 100644
@@ -207,19 +207,21 @@ static u32 omap_vout_uservirt_to_phys(u32 virtp)
 	struct vm_area_struct *vma;
 	struct mm_struct *mm = current->mm;
 
-	vma = find_vma(mm, virtp);
 	/* For kernel direct-mapped memory, take the easy way */
-	if (virtp >= PAGE_OFFSET) {
-		physp = virt_to_phys((void *) virtp);
+	if (virtp >= PAGE_OFFSET)
+		return virt_to_phys((void *) virtp);
+
+	down_read(&current->mm->mmap_sem);
+	vma = find_vma(mm, virtp);
 	} else if (vma && (vma->vm_flags & VM_IO) && vma->vm_pgoff) {
 		/* this will catch, kernel-allocated, mmaped-to-usermode
 		   addresses */
 		physp = (vma->vm_pgoff << PAGE_SHIFT) + (virtp - vma->vm_start);
+		up_read(&current->mm->mmap_sem);
 	} else {
 		/* otherwise, use get_user_pages() for general userland pages */
 		int res, nr_pages = 1;
 		struct page *pages;
-		down_read(&current->mm->mmap_sem);
 
 		res = get_user_pages(current, current->mm, virtp, nr_pages, 1,
 				0, &pages, NULL);

Al Viro

author

2012-12-15 20:38 UTC #1 lore raw

Quoted 5 lines · Al Viro

Walking rbtree while it's modified is a Bad Idea(tm); besides,
the result of find_vma() can be freed just as it's getting returned to caller. Fortunately, it's easy to fix - just take ->mmap_sem a bit earlier (and don't bother with find_vma() at all if virtp >= PAGE_OFFSET - in that case we don't even look at its result).

While we are at it, what prevents VIDIOC_PREPARE_BUF calling
v4l_prepare_buf() -> (e.g) vb2_ioctl_prepare_buf() -> vb2_prepare_buf() -> __buf_prepare() -> __qbuf_userptr() -> vb2_vmalloc_get_userptr() -> find_vma(), AFAICS without having taken ->mmap_sem anywhere in process? The code flow is bloody convoluted and depends on a bunch of things done by initialization, so I certainly might've missed something...

Mauro Carvalho Chehab

2013-01-06 13:03 UTC #2 lore raw

Hi Viro,

Em Sat, 15 Dec 2012 20:38:29 +0000
Al Viro <[email protected]> escreveu:

Quoted 7 lines · Al Viro

Quoted 5 lines
Walking rbtree while it's modified is a Bad Idea(tm); besides,
the result of find_vma() can be freed just as it's getting returned to caller. Fortunately, it's easy to fix - just take ->mmap_sem a bit earlier (and don't bother with find_vma() at all if virtp >= PAGE_OFFSET - in that case we don't even look at its result).

Quoted 6 lines

While we are at it, what prevents VIDIOC_PREPARE_BUF calling
v4l_prepare_buf() -> (e.g) vb2_ioctl_prepare_buf() -> vb2_prepare_buf() -> __buf_prepare() -> __qbuf_userptr() -> vb2_vmalloc_get_userptr() -> find_vma(), AFAICS without having taken ->mmap_sem anywhere in process? The code flow is bloody convoluted and depends on a bunch of things done by initialization, so I certainly might've missed something...

This driver is currently missing an active maintainer, as it is for an old hardware (AFAIK, omap is now at version 4, and this is for the first one), but I'm c/c a few developers that might help to test and analyze it.

In any case, /me is assuming that your patch is right (as nobody complained), and I'm applying it right now on my tree. This will hopefully allow some people to test.

Cheers,
Mauro

Laurent Pinchart

2013-01-07 14:01 UTC #3 lore raw

Hi Mauro,

Quoted 12 lines · On Sunday 06 January 2013 11:02:25 Mauro Carvalho Chehab wrote:

Em Sat, 15 Dec 2012 20:38:29 +0000 Al Viro escreveu:
Quoted 11 lines · Al Viro
Quoted 7 lines
Walking rbtree while it's modified is a Bad Idea(tm); besides,
the result of find_vma() can be freed just as it's getting returned to caller. Fortunately, it's easy to fix - just take ->mmap_sem a bit earlier (and don't bother with find_vma() at all if virtp >= PAGE_OFFSET -
in that case we don't even look at its result).
While we are at it, what prevents VIDIOC_PREPARE_BUF calling

Quoted 8 lines

Quoted 5 lines
v4l_prepare_buf() -> (e.g) vb2_ioctl_prepare_buf() -> vb2_prepare_buf() -> __buf_prepare() -> __qbuf_userptr() -> vb2_vmalloc_get_userptr() -> find_vma(), AFAICS without having taken ->mmap_sem anywhere in process? The code flow is bloody convoluted and depends on a bunch of things done by initialization, so I certainly might've missed something...
This driver is currently missing an active maintainer, as it is for an old hardware (AFAIK, omap is now at version 4, and this is for the first one),

The omap_vout driver is for OMAP2+ if I'm not mistaken. I use it with the OMAP3.

Quoted 5 lines

but I'm c/c a few developers that might help to test and analyze it.
In any case, /me is assuming that your patch is right (as nobody complained), and I'm applying it right now on my tree. This will hopefully allow some people to test.

Signature · 3 lines

Regards,

Laurent Pinchart

Laurent Pinchart

2013-01-07 14:04 UTC #4 lore raw

In reply to Mauro Carvalho Chehab

Hi Mauro,

Quoted 18 lines · On Sunday 06 January 2013 11:02:25 Mauro Carvalho Chehab wrote:

Em Sat, 15 Dec 2012 20:38:29 +0000 Al Viro escreveu:
Quoted 9 lines · Al Viro
Quoted 7 lines
Walking rbtree while it's modified is a Bad Idea(tm); besides,
the result of find_vma() can be freed just as it's getting returned to caller. Fortunately, it's easy to fix - just take ->mmap_sem a bit earlier (and don't bother with find_vma() at all if virtp >= PAGE_OFFSET -
in that case we don't even look at its result).
Quoted 7 lines
While we are at it, what prevents VIDIOC_PREPARE_BUF calling
v4l_prepare_buf() -> (e.g) vb2_ioctl_prepare_buf() -> vb2_prepare_buf() -> __buf_prepare() -> __qbuf_userptr() -> vb2_vmalloc_get_userptr() -> find_vma(), AFAICS without having taken ->mmap_sem anywhere in process? The code flow is bloody convoluted and depends on a bunch of things done by initialization, so I certainly might've missed something...

Quoted 7 lines

This driver is currently missing an active maintainer, as it is for an old hardware (AFAIK, omap is now at version 4, and this is for the first one), but I'm c/c a few developers that might help to test and analyze it.
In any case, /me is assuming that your patch is right (as nobody complained), and I'm applying it right now on my tree. This will hopefully allow some people to test.

Please make sure you apply v2 (posted as "Re: [PATCH] omap_vout: find_vma() needs ->mmap_sem held").

Signature · 3 lines

Regards,

Laurent Pinchart

Paul Bolle

2012-12-16 20:01 UTC #5 lore raw

In reply to Al Viro

Quoted 17 lines · Al Viro

Walking rbtree while it's modified is a Bad Idea(tm); besides,
the result of find_vma() can be freed just as it's getting returned to caller. Fortunately, it's easy to fix - just take ->mmap_sem a bit earlier (and don't bother with find_vma() at all if virtp >= PAGE_OFFSET - in that case we don't even look at its result).
Cc [email protected] [2.6.35]
Signed-off-by Al Viro <[email protected]>
---
drivers/media/platform/omap/omap_vout.c +0 -0
index 9935040..984512f 100644
@@ -207,19 +207,21 @@ static u32 omap_vout_uservirt_to_phys(u32 virtp)
 	struct vm_area_struct *vma;
 	struct mm_struct *mm = current->mm;
 

drivers/media/platform/omap/omap_vout.c static u32 omap_vout_uservirt_to_phys(u32 virtp) · line 216

Quoted 10 lines

drivers/media/platform/omap/omap_vout.c +5 -3

-	vma = find_vma(mm, virtp);
 	/* For kernel direct-mapped memory, take the easy way */
-	if (virtp >= PAGE_OFFSET) {
-		physp = virt_to_phys((void *) virtp);
+	if (virtp >= PAGE_OFFSET)
+		return virt_to_phys((void *) virtp);
+
+	down_read(&current->mm->mmap_sem);
+	vma = find_vma(mm, virtp);
 	} else if (vma && (vma->vm_flags & VM_IO) && vma->vm_pgoff) {

Shouldn't that line become

    if (vma && (vma->vm_flags & VM_IO) && vma->vm_pgoff) {

so that this actually compiles?

drivers/media/platform/omap/omap_vout.c static u32 omap_vout_uservirt_to_phys(u32 virtp) · line 227

Quoted 12 lines

drivers/media/platform/omap/omap_vout.c +1 -1

 		/* this will catch, kernel-allocated, mmaped-to-usermode
 		   addresses */
 		physp = (vma->vm_pgoff << PAGE_SHIFT) + (virtp - vma->vm_start);
+		up_read(&current->mm->mmap_sem);
 	} else {
 		/* otherwise, use get_user_pages() for general userland pages */
 		int res, nr_pages = 1;
 		struct page *pages;
-		down_read(&current->mm->mmap_sem);
 
 		res = get_user_pages(current, current->mm, virtp, nr_pages, 1,
 				0, &pages, NULL);

Paul Bolle

Al Viro

author

2012-12-16 20:04 UTC #6 lore raw

Quoted 7 lines · Paul Bolle

Review
Quoted 2 lines
(quoted diff) +1 -0
+	vma = find_vma(mm, virtp);
 	} else if (vma && (vma->vm_flags & VM_IO) && vma->vm_pgoff) {
Shouldn't that line become
    if (vma && (vma->vm_flags & VM_IO) && vma->vm_pgoff) {
so that this actually compiles?

*Do'h*

Yes, it should. Mea culpa...

Signed-off-by Al Viro <[email protected]>

---

drivers/media/platform/omap/omap_vout.c +7 -5

index 9935040..cb564d0 100644
@@ -207,19 +207,21 @@ static u32 omap_vout_uservirt_to_phys(u32 virtp)
 	struct vm_area_struct *vma;
 	struct mm_struct *mm = current->mm;
 
-	vma = find_vma(mm, virtp);
 	/* For kernel direct-mapped memory, take the easy way */
-	if (virtp >= PAGE_OFFSET) {
-		physp = virt_to_phys((void *) virtp);
-	} else if (vma && (vma->vm_flags & VM_IO) && vma->vm_pgoff) {
+	if (virtp >= PAGE_OFFSET)
+		return virt_to_phys((void *) virtp);
+
+	down_read(&current->mm->mmap_sem);
+	vma = find_vma(mm, virtp);
+	if (vma && (vma->vm_flags & VM_IO) && vma->vm_pgoff) {
 		/* this will catch, kernel-allocated, mmaped-to-usermode
 		   addresses */
 		physp = (vma->vm_pgoff << PAGE_SHIFT) + (virtp - vma->vm_start);
+		up_read(&current->mm->mmap_sem);
 	} else {
 		/* otherwise, use get_user_pages() for general userland pages */
 		int res, nr_pages = 1;
 		struct page *pages;
-		down_read(&current->mm->mmap_sem);
 
 		res = get_user_pages(current, current->mm, virtp, nr_pages, 1,
 				0, &pages, NULL);

Laurent Pinchart

Acked-by

2013-01-07 14:13 UTC #7 lore raw

Quoted 9 lines · On Sunday 16 December 2012 20:04:46 Al Viro wrote:

Quoted 8 lines · Paul Bolle
drivers/media/platform/omap/omap_vout.c static u32 omap_vout_uservirt_to_phys(u32 virtp)
Quoted 3 lines
drivers/media/platform/omap/omap_vout.c +1 -0
+	vma = find_vma(mm, virtp);
 
 	} else if (vma && (vma->vm_flags & VM_IO) && vma->vm_pgoff) {
Shouldn't that line become
    if (vma && (vma->vm_flags & VM_IO) && vma->vm_pgoff) {

Quoted 7 lines

Quoted 1 line
so that this actually compiles?
*Do'h*
Yes, it should. Mea culpa...
Signed-off-by Al Viro <[email protected]>

Acked-by Laurent Pinchart <[email protected]>

Quoted 32 lines

---

(quoted diff) +0 -0

b/drivers/media/platform/omap/omap_vout.c index 9935040..cb564d0 100644

drivers/media/platform/omap/omap_vout.c +7 -5

@@ -207,19 +207,21 @@ static u32 omap_vout_uservirt_to_phys(u32 virtp)
 	struct vm_area_struct *vma;
 	struct mm_struct *mm = current->mm;
 
-	vma = find_vma(mm, virtp);
 	/* For kernel direct-mapped memory, take the easy way */
-	if (virtp >= PAGE_OFFSET) {
-		physp = virt_to_phys((void *) virtp);
-	} else if (vma && (vma->vm_flags & VM_IO) && vma->vm_pgoff) {
+	if (virtp >= PAGE_OFFSET)
+		return virt_to_phys((void *) virtp);
+
+	down_read(&current->mm->mmap_sem);
+	vma = find_vma(mm, virtp);
+	if (vma && (vma->vm_flags & VM_IO) && vma->vm_pgoff) {
 		/* this will catch, kernel-allocated, mmaped-to-usermode
 		   addresses */
 		physp = (vma->vm_pgoff << PAGE_SHIFT) + (virtp - vma->vm_start);
+		up_read(&current->mm->mmap_sem);
 	} else {
 		/* otherwise, use get_user_pages() for general userland pages */
 		int res, nr_pages = 1;
 		struct page *pages;
-		down_read(&current->mm->mmap_sem);
 
 		res = get_user_pages(current, current->mm, virtp, nr_pages, 1,
 				0, &pages, NULL);

Signature · 3 lines

Regards,

Laurent Pinchart