[BUG] rmap: fix page_address_in_vma() to walk through anon_vma_chain

[BUG] rmap: fix page_address_in_vma() to walk through anon_vma_chain

[Naoya Horiguchi]

I found a bug on page_address_in_vma() related to anon_vma_chain.

I wrote a patch, but according to a comment in include/linux/rmap.h,
I suspect this doesn't meet lock requirement of anon_vma_chain
(mmap_sem and page_table_lock, see below).

                           mmap_sem      page_table_lock
  mm/ksm.c:
    write_protect_page()   hold          not hold
    replace_page()         hold          not hold
  mm/memory-failure.c:
    add_to_kill()          not hold      hold
  mm/mempolicy.c:
    new_vma_page()         hold          not hold
  mm/swapfile.c:
    unuse_vma()            hold          not hold

Any comments?

Thanks,
Naoya Horiguchi
---
Subject: [BUG] rmap: fix page_address_in_vma() to walk through anon_vma_chain

page_address_in_vma() checks if a given page is associated with a given vma.
Currently it just compares vma->anon_vma and page_anon_vma(page).
But in 2.6.34, a vma can have multiple anon_vmas with anon_vma_chain,
so we have to check all anon_vmas in the "same_vma" chain.
Otherwise, when a page is shared by multiple processes,
some (page,vma) pairs can be misjudged as not-mapped.

Need Work: Meet lock requirement of anon_vma_chain.

Signed-off-by: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Rik van Riel <riel@redhat.com>
Cc: Andi Kleen <andi@firstfloor.org>
---
 mm/rmap.c |   12 ++++++++++--
 1 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/mm/rmap.c b/mm/rmap.c
index 526704e..2e7462b 100644
--- a/mm/rmap.c
+++ b/mm/rmap.c
@@ -340,14 +340,22 @@ vma_address(struct page *page, struct vm_area_struct *vma)
 unsigned long page_address_in_vma(struct page *page, struct vm_area_struct *vma)
 {
 	if (PageAnon(page)) {
-		if (vma->anon_vma != page_anon_vma(page))
-			return -EFAULT;
+		struct anon_vma_chain *avc;
+		/*
+		 * Walking same_vma list needs mmap_sem and page_table_lock.
+		 * Do users of this function meet it?
+		 */
+		list_for_each_entry(avc, &vma->anon_vma_chain, same_vma)
+			if (avc->anon_vma == page_anon_vma(page))
+				goto get_address;
+		return -EFAULT;
 	} else if (page->mapping && !(vma->vm_flags & VM_NONLINEAR)) {
 		if (!vma->vm_file ||
 		    vma->vm_file->f_mapping != page->mapping)
 			return -EFAULT;
 	} else
 		return -EFAULT;
+get_address:
 	return vma_address(page, vma);
 }
 
-- 
1.7.0

[BUG] rmap: fix page_address_in_vma() to walk through anon_vma_chain

[Naoya Horiguchi]

I found a bug on page_address_in_vma() related to anon_vma_chain.

I wrote a patch, but according to a comment in include/linux/rmap.h,
I suspect this doesn't meet lock requirement of anon_vma_chain
(mmap_sem and page_table_lock, see below).

                           mmap_sem      page_table_lock
  mm/ksm.c:
    write_protect_page()   hold          not hold
    replace_page()         hold          not hold
  mm/memory-failure.c:
    add_to_kill()          not hold      hold
  mm/mempolicy.c:
    new_vma_page()         hold          not hold
  mm/swapfile.c:
    unuse_vma()            hold          not hold

Any comments?

Thanks,
Naoya Horiguchi
---
Subject: [BUG] rmap: fix page_address_in_vma() to walk through anon_vma_chain

page_address_in_vma() checks if a given page is associated with a given vma.
Currently it just compares vma->anon_vma and page_anon_vma(page).
But in 2.6.34, a vma can have multiple anon_vmas with anon_vma_chain,
so we have to check all anon_vmas in the "same_vma" chain.
Otherwise, when a page is shared by multiple processes,
some (page,vma) pairs can be misjudged as not-mapped.

Need Work: Meet lock requirement of anon_vma_chain.

Signed-off-by: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Rik van Riel <riel@redhat.com>
Cc: Andi Kleen <andi@firstfloor.org>
---
 mm/rmap.c |   12 ++++++++++--
 1 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/mm/rmap.c b/mm/rmap.c
index 526704e..2e7462b 100644
--- a/mm/rmap.c
+++ b/mm/rmap.c
@@ -340,14 +340,22 @@ vma_address(struct page *page, struct vm_area_struct *vma)
 unsigned long page_address_in_vma(struct page *page, struct vm_area_struct *vma)
 {
 	if (PageAnon(page)) {
-		if (vma->anon_vma != page_anon_vma(page))
-			return -EFAULT;
+		struct anon_vma_chain *avc;
+		/*
+		 * Walking same_vma list needs mmap_sem and page_table_lock.
+		 * Do users of this function meet it?
+		 */
+		list_for_each_entry(avc, &vma->anon_vma_chain, same_vma)
+			if (avc->anon_vma == page_anon_vma(page))
+				goto get_address;
+		return -EFAULT;
 	} else if (page->mapping && !(vma->vm_flags & VM_NONLINEAR)) {
 		if (!vma->vm_file ||
 		    vma->vm_file->f_mapping != page->mapping)
 			return -EFAULT;
 	} else
 		return -EFAULT;
+get_address:
 	return vma_address(page, vma);
 }
 
-- 
1.7.0

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [BUG] rmap: fix page_address_in_vma() to walk through anon_vma_chain

[Naoya Horiguchi]

>                            mmap_sem      page_table_lock
>   mm/ksm.c:
>     write_protect_page()   hold          not hold
>     replace_page()         hold          not hold
>   mm/memory-failure.c:
>     add_to_kill()          not hold      hold
                                           ^^^^
Sorry, I misread here. This is "not hold".

Thanks,
Naoya Horiguchi

Re: [BUG] rmap: fix page_address_in_vma() to walk through anon_vma_chain

[Naoya Horiguchi]

>                            mmap_sem      page_table_lock
>   mm/ksm.c:
>     write_protect_page()   hold          not hold
>     replace_page()         hold          not hold
>   mm/memory-failure.c:
>     add_to_kill()          not hold      hold
                                           ^^^^
Sorry, I misread here. This is "not hold".

Thanks,
Naoya Horiguchi

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [BUG] rmap: fix page_address_in_vma() to walk through anon_vma_chain

[Rik van Riel]

On 04/22/2010 01:42 AM, Naoya Horiguchi wrote:
> I found a bug on page_address_in_vma() related to anon_vma_chain.
>
> I wrote a patch, but according to a comment in include/linux/rmap.h,
> I suspect this doesn't meet lock requirement of anon_vma_chain
> (mmap_sem and page_table_lock, see below).
>
>                             mmap_sem      page_table_lock
>    mm/ksm.c:
>      write_protect_page()   hold          not hold
>      replace_page()         hold          not hold
>    mm/memory-failure.c:
>      add_to_kill()          not hold      hold
>    mm/mempolicy.c:
>      new_vma_page()         hold          not hold
>    mm/swapfile.c:
>      unuse_vma()            hold          not hold
>
> Any comments?

Good catch.

However, for anonymous pages, page_address_in_vma only
ever determined whether the page _could_ be part of the
VMA, never whether it actually was.

The function page_address_in_vma has always given
false positives, which means all of the callers already
check that the page is actually part of the process.

This means we may be able to get away with not verifying
the anon_vma at all.  After all, verifying that the VMA
has the anon_vma mapped does not mean the VMA has this
page...

Doing away with that check gets rid of your locking
conundrum :)

Opinions?

Re: [BUG] rmap: fix page_address_in_vma() to walk through anon_vma_chain

[Rik van Riel]

On 04/22/2010 01:42 AM, Naoya Horiguchi wrote:
> I found a bug on page_address_in_vma() related to anon_vma_chain.
>
> I wrote a patch, but according to a comment in include/linux/rmap.h,
> I suspect this doesn't meet lock requirement of anon_vma_chain
> (mmap_sem and page_table_lock, see below).
>
>                             mmap_sem      page_table_lock
>    mm/ksm.c:
>      write_protect_page()   hold          not hold
>      replace_page()         hold          not hold
>    mm/memory-failure.c:
>      add_to_kill()          not hold      hold
>    mm/mempolicy.c:
>      new_vma_page()         hold          not hold
>    mm/swapfile.c:
>      unuse_vma()            hold          not hold
>
> Any comments?

Good catch.

However, for anonymous pages, page_address_in_vma only
ever determined whether the page _could_ be part of the
VMA, never whether it actually was.

The function page_address_in_vma has always given
false positives, which means all of the callers already
check that the page is actually part of the process.

This means we may be able to get away with not verifying
the anon_vma at all.  After all, verifying that the VMA
has the anon_vma mapped does not mean the VMA has this
page...

Doing away with that check gets rid of your locking
conundrum :)

Opinions?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [BUG] rmap: fix page_address_in_vma() to walk through anon_vma_chain

[Naoya Horiguchi]

> However, for anonymous pages, page_address_in_vma only
> ever determined whether the page _could_ be part of the
> VMA, never whether it actually was.
>
> The function page_address_in_vma has always given
> false positives, which means all of the callers already
> check that the page is actually part of the process.

I see.

> This means we may be able to get away with not verifying
> the anon_vma at all.  After all, verifying that the VMA
> has the anon_vma mapped does not mean the VMA has this
> page...
> 
> Doing away with that check gets rid of your locking
> conundrum :)

I get it, thank you :)
I'll rewrite fix patch based on your comments.

Thanks,
Naoya Horiguchi

Re: [BUG] rmap: fix page_address_in_vma() to walk through anon_vma_chain

[Naoya Horiguchi]

> However, for anonymous pages, page_address_in_vma only
> ever determined whether the page _could_ be part of the
> VMA, never whether it actually was.
>
> The function page_address_in_vma has always given
> false positives, which means all of the callers already
> check that the page is actually part of the process.

I see.

> This means we may be able to get away with not verifying
> the anon_vma at all.  After all, verifying that the VMA
> has the anon_vma mapped does not mean the VMA has this
> page...
> 
> Doing away with that check gets rid of your locking
> conundrum :)

I get it, thank you :)
I'll rewrite fix patch based on your comments.

Thanks,
Naoya Horiguchi

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH] [BUGFIX] rmap: remove anon_vma check in page_address_in_vma()

[Naoya Horiguchi]

Currently page_address_in_vma() compares vma->anon_vma and page_anon_vma(page)
for parameter check, but in 2.6.34 a vma can have multiple anon_vmas with
anon_vma_chain, so current check does not work. (For anonymous page shared by
multiple processes, some verified (page,vma) pairs return -EFAULT wrongly.)

We can go to checking all anon_vmas in the "same_vma" chain, but it needs
to meet lock requirement. Instead, we can remove anon_vma check safely
because page_address_in_vma() assumes that page and vma are already checked
to belong to the identical process.

Signed-off-by: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Rik van Riel <riel@redhat.com>
Cc: Andi Kleen <andi@firstfloor.org>
---
 mm/rmap.c |    9 ++++-----
 1 files changed, 4 insertions(+), 5 deletions(-)

diff --git v2.6.34-rc5:mm/rmap.c v2.6.34-rc5:mm/rmap.c
index 526704e..486fd0a 100644
--- v2.6.34-rc5:mm/rmap.c
+++ v2.6.34-rc5:mm/rmap.c
@@ -335,14 +335,13 @@ vma_address(struct page *page, struct vm_area_struct *vma)
 
 /*
  * At what user virtual address is page expected in vma?
- * checking that the page matches the vma.
+ * Caller should check the page is actually part of the vma.
  */
 unsigned long page_address_in_vma(struct page *page, struct vm_area_struct *vma)
 {
-	if (PageAnon(page)) {
-		if (vma->anon_vma != page_anon_vma(page))
-			return -EFAULT;
-	} else if (page->mapping && !(vma->vm_flags & VM_NONLINEAR)) {
+	if (PageAnon(page))
+		;
+	else if (page->mapping && !(vma->vm_flags & VM_NONLINEAR)) {
 		if (!vma->vm_file ||
 		    vma->vm_file->f_mapping != page->mapping)
 			return -EFAULT;
-- 
1.7.0

[PATCH] [BUGFIX] rmap: remove anon_vma check in page_address_in_vma()

[Naoya Horiguchi]

Currently page_address_in_vma() compares vma->anon_vma and page_anon_vma(page)
for parameter check, but in 2.6.34 a vma can have multiple anon_vmas with
anon_vma_chain, so current check does not work. (For anonymous page shared by
multiple processes, some verified (page,vma) pairs return -EFAULT wrongly.)

We can go to checking all anon_vmas in the "same_vma" chain, but it needs
to meet lock requirement. Instead, we can remove anon_vma check safely
because page_address_in_vma() assumes that page and vma are already checked
to belong to the identical process.

Signed-off-by: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Rik van Riel <riel@redhat.com>
Cc: Andi Kleen <andi@firstfloor.org>
---
 mm/rmap.c |    9 ++++-----
 1 files changed, 4 insertions(+), 5 deletions(-)

diff --git v2.6.34-rc5:mm/rmap.c v2.6.34-rc5:mm/rmap.c
index 526704e..486fd0a 100644
--- v2.6.34-rc5:mm/rmap.c
+++ v2.6.34-rc5:mm/rmap.c
@@ -335,14 +335,13 @@ vma_address(struct page *page, struct vm_area_struct *vma)
 
 /*
  * At what user virtual address is page expected in vma?
- * checking that the page matches the vma.
+ * Caller should check the page is actually part of the vma.
  */
 unsigned long page_address_in_vma(struct page *page, struct vm_area_struct *vma)
 {
-	if (PageAnon(page)) {
-		if (vma->anon_vma != page_anon_vma(page))
-			return -EFAULT;
-	} else if (page->mapping && !(vma->vm_flags & VM_NONLINEAR)) {
+	if (PageAnon(page))
+		;
+	else if (page->mapping && !(vma->vm_flags & VM_NONLINEAR)) {
 		if (!vma->vm_file ||
 		    vma->vm_file->f_mapping != page->mapping)
 			return -EFAULT;
-- 
1.7.0

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] [BUGFIX] rmap: remove anon_vma check in page_address_in_vma()

[Rik van Riel]

On 04/22/2010 10:08 PM, Naoya Horiguchi wrote:
> Currently page_address_in_vma() compares vma->anon_vma and page_anon_vma(page)
> for parameter check, but in 2.6.34 a vma can have multiple anon_vmas with
> anon_vma_chain, so current check does not work. (For anonymous page shared by
> multiple processes, some verified (page,vma) pairs return -EFAULT wrongly.)
>
> We can go to checking all anon_vmas in the "same_vma" chain, but it needs
> to meet lock requirement. Instead, we can remove anon_vma check safely
> because page_address_in_vma() assumes that page and vma are already checked
> to belong to the identical process.
>
> Signed-off-by: Naoya Horiguchi<n-horiguchi@ah.jp.nec.com>
> Cc: Andrew Morton<akpm@linux-foundation.org>
> Cc: Rik van Riel<riel@redhat.com>
> Cc: Andi Kleen<andi@firstfloor.org>

Reviewed-by: Rik van Riel <riel@redhat.com>

Re: [PATCH] [BUGFIX] rmap: remove anon_vma check in page_address_in_vma()

[Rik van Riel]

On 04/22/2010 10:08 PM, Naoya Horiguchi wrote:
> Currently page_address_in_vma() compares vma->anon_vma and page_anon_vma(page)
> for parameter check, but in 2.6.34 a vma can have multiple anon_vmas with
> anon_vma_chain, so current check does not work. (For anonymous page shared by
> multiple processes, some verified (page,vma) pairs return -EFAULT wrongly.)
>
> We can go to checking all anon_vmas in the "same_vma" chain, but it needs
> to meet lock requirement. Instead, we can remove anon_vma check safely
> because page_address_in_vma() assumes that page and vma are already checked
> to belong to the identical process.
>
> Signed-off-by: Naoya Horiguchi<n-horiguchi@ah.jp.nec.com>
> Cc: Andrew Morton<akpm@linux-foundation.org>
> Cc: Rik van Riel<riel@redhat.com>
> Cc: Andi Kleen<andi@firstfloor.org>

Reviewed-by: Rik van Riel <riel@redhat.com>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>