mm: fix do_mbind return value

PATCH In review Kazutomo Yoshii 3 participants 7 messages Mar 5, 2015

Subsystems (3)

Memory management MM core MM NUMA placement

lore

Share & tools

lore

Kazutomo Yoshii

author

2015-03-05 02:20 UTC #0 lore raw

PATCH 1/1 mm: fix do_mbind return value

I noticed that numa_alloc_onnode() failed to allocate memory on a specified node in v4.0-rc1. I added a code to check the return value of walk_page_range() in queue_pages_range() so that do_mbind() only returns an error number or zero.

Signed-off-by Kazutomo Yoshii <[email protected]>

---
 mm/mempolicy.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

mm/mempolicy.c +5 -0

index 4721046..ea79171 100644
@@ -644,6 +644,7 @@ queue_pages_range(struct mm_struct *mm, unsigned long start, unsigned long end,
 		.nmask = nodes,
 		.prev = NULL,
 	};
+	int err;
 	struct mm_walk queue_pages_walk = {
 		.hugetlb_entry = queue_pages_hugetlb,
 		.pmd_entry = queue_pages_pte_range,

@@ -652,7 +653,10 @@ queue_pages_range(struct mm_struct *mm, unsigned long start, unsigned long end,
 		.private = &qp,
 	};
 -	return walk_page_range(start, end, &queue_pages_walk);
+	err = walk_page_range(start, end, &queue_pages_walk);
+	if (err < 0)
+		return err;
+	return 0;
 }
  /*

Signature · 1 line

1.8.3.1

David Rientjes

2015-03-05 06:53 UTC #1 lore raw

Quoted 5 lines · Kazutomo Yoshii

I noticed that numa_alloc_onnode() failed to allocate memory on a specified node in v4.0-rc1. I added a code to check the return value of walk_page_range() in queue_pages_range() so that do_mbind() only returns an error number or zero.

I assume this is libnuma-2.0.10?

Quoted 18 lines

Signed-off-by Kazutomo Yoshii <[email protected]>

---
 mm/mempolicy.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

mm/mempolicy.c +1 -0

index 4721046..ea79171 100644
@@ -644,6 +644,7 @@ queue_pages_range(struct mm_struct *mm, unsigned long start, unsigned long end,
 		.nmask = nodes,
 		.prev = NULL,
 	};
+	int err;
 	struct mm_walk queue_pages_walk = {
 		.hugetlb_entry = queue_pages_hugetlb,
 		.pmd_entry = queue_pages_pte_range,

mm/mempolicy.c queue_pages_range(struct mm_struct *mm, unsigned long start, unsigned long end, · line 661

Quoted 9 lines

mm/mempolicy.c +4 -0

@@ -652,7 +653,10 @@ queue_pages_range(struct mm_struct *mm, unsigned long start, unsigned long end,
 		.private = &qp,
 	};
 -	return walk_page_range(start, end, &queue_pages_walk);
+	err = walk_page_range(start, end, &queue_pages_walk);
+	if (err < 0)
+		return err;
+	return 0;
 }
  /*

I'm afraid I don't think this is the right fix, if walk_page_range() returns a positive value then it should be supplied by one of the callbacks in the struct mm_walk, which none of these happen to do. I think this may be a problem with commit 6f4576e3687b ("mempolicy: apply page table walker on queue_pages_range()"), so let's add Naoya to the thread.

Kazutomo Yoshii

author

2015-03-05 07:44 UTC #2 lore raw

Quoted 8 lines · David Rientjes

Quoted 5 lines · Kazutomo Yoshii
I noticed that numa_alloc_onnode() failed to allocate memory on a specified node in v4.0-rc1. I added a code to check the return value of walk_page_range() in queue_pages_range() so that do_mbind() only returns an error number or zero.
I assume this is libnuma-2.0.10?

I used libnuma-2.0.9. Here is a strace output related to numa_alloc_onnode()

mmap(NULL, 4194304, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, 0, 0) = 0x7fe9b8334000 mbind(0x7fe9b8334000, 4194304, MPOL_BIND, 0x1b43bf0, 1025, 0) = 1

I believe mbind() returning a positive number is just a wrong behavior. A tricky part is that libnuma only checks a negative error,
so numa_alloc_onnode() itself didn't fail. I noticed this first when I was checking
memory placement using the proc pagemap.

Quoted 25 lines

Signed-off-by Kazutomo Yoshii <[email protected]>

---
  mm/mempolicy.c | 6 +++++-
  1 file changed, 5 insertions(+), 1 deletion(-)

mm/mempolicy.c +5 -0

index 4721046..ea79171 100644
@@ -644,6 +644,7 @@ queue_pages_range(struct mm_struct *mm, unsigned long start, unsigned long end,
  		.nmask = nodes,
  		.prev = NULL,
  	};
+	int err;
  	struct mm_walk queue_pages_walk = {
  		.hugetlb_entry = queue_pages_hugetlb,
  		.pmd_entry = queue_pages_pte_range,

@@ -652,7 +653,10 @@ queue_pages_range(struct mm_struct *mm, unsigned long start, unsigned long end,
  		.private = &qp,
  	};
  -	return walk_page_range(start, end, &queue_pages_walk);
+	err = walk_page_range(start, end, &queue_pages_walk);
+	if (err < 0)
+		return err;
+	return 0;

Quoted 8 lines

mm/mempolicy.c queue_pages_range(struct mm_struct *mm, unsigned long start, unsigned long end, · line 661
Quoted 2 lines
mm/mempolicy.c +0 -0
  }
   /*
I'm afraid I don't think this is the right fix, if walk_page_range() returns a positive value then it should be supplied by one of the callbacks in the struct mm_walk, which none of these happen to do. I think this may be a problem with commit 6f4576e3687b ("mempolicy: apply page table walker on queue_pages_range()"), so let's add Naoya to the thread.

Thank you for the pointer!
I think queue_pages_test_walk() returns 1.
My fix may not be in a right place but someone needs to fix this.

- kaz

Naoya Horiguchi

2015-03-05 08:02 UTC #3 lore raw

PATCH 1/1 mm: pagewalk: prevent positive return value of walk_page_test() from being passed to callers (Re: [PATCH] mm: fix do_mbind return value)

In reply to David Rientjes

# CCed Andrew and linux-mm

Quoted 36 lines · David Rientjes

Quoted 5 lines · Kazutomo Yoshii
I noticed that numa_alloc_onnode() failed to allocate memory on a specified node in v4.0-rc1. I added a code to check the return value of walk_page_range() in queue_pages_range() so that do_mbind() only returns an error number or zero.
I assume this is libnuma-2.0.10?
Quoted 26 lines
Signed-off-by Kazutomo Yoshii <[email protected]>
---
 mm/mempolicy.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
mm/mempolicy.c +5 -0
index 4721046..ea79171 100644
@@ -644,6 +644,7 @@ queue_pages_range(struct mm_struct *mm, unsigned long start, unsigned long end,
 		.nmask = nodes,
 		.prev = NULL,
 	};
+	int err;
 	struct mm_walk queue_pages_walk = {
 		.hugetlb_entry = queue_pages_hugetlb,
 		.pmd_entry = queue_pages_pte_range,
@@ -652,7 +653,10 @@ queue_pages_range(struct mm_struct *mm, unsigned long start, unsigned long end,
 		.private = &qp,
 	};
 -	return walk_page_range(start, end, &queue_pages_walk);
+	err = walk_page_range(start, end, &queue_pages_walk);
+	if (err < 0)
+		return err;
+	return 0;
 }

Quoted 8 lines

mm/mempolicy.c queue_pages_range(struct mm_struct *mm, unsigned long start, unsigned long end, · line 661
Quoted 1 line
mm/mempolicy.c +0 -0
  /*
I'm afraid I don't think this is the right fix, if walk_page_range() returns a positive value then it should be supplied by one of the callbacks in the struct mm_walk, which none of these happen to do. I think this may be a problem with commit 6f4576e3687b ("mempolicy: apply page table walker on queue_pages_range()"), so let's add Naoya to the thread.

Thank you for reporting/forwarding, Yoshii-san and David.

This bug is in the pagewalk's common path, and the following patch should fix it.

Thanks,
Naoya Horiguchi
---

Quoted 1 line

From 107fa3fb256bddff40a882c90af717af9863aed7 Mon Sep 17 00:00:00 2001

From: Naoya Horiguchi <[email protected]>
Date: Thu, 5 Mar 2015 16:37:37 +0900
Subject: [PATCH] mm: pagewalk: prevent positive return value of
walk_page_test() from being passed to callers

walk_page_test() is purely pagewalk's internal stuff, and its positive return values are not intended to be passed to the callers of pagewalk. However, in the current code if the last vma in the do-while loop in walk_page_range() happens to return a positive value, it leaks outside walk_page_range(). So the user visible effect is invalid/unexpected return value (according to the reporter, mbind() causes it.)

This patch fixes it simply by reinitializing the return value after checked.

Another exposed interface, walk_page_vma(), already returns 0 for such cases so no problem.

Fixes 6f4576e3687b ("mempolicy: apply page table walker on queue_pages_range()")

Reported-by Kazutomo Yoshii <[email protected]>

Signed-off-by Naoya Horiguchi <[email protected]>

---
 mm/pagewalk.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

mm/pagewalk.c +8 -1

index 75c1f2878519..29f2f8b853ae 100644
@@ -265,8 +265,15 @@ int walk_page_range(unsigned long start, unsigned long end,
 			vma = vma->vm_next;
 
 			err = walk_page_test(start, next, walk);
-			if (err > 0)
+			if (err > 0) {
+				/*
+				 * positive return values are purely for
+				 * controlling the pagewalk, so should never
+				 * be passed to the callers.
+				 */
+				err = 0;
 				continue;
+			}
 			if (err < 0)
 				break;
 		}

Signature · 1 line

1.9.3

Naoya Horiguchi

2015-03-05 08:10 UTC #4 lore raw

Re: [PATCH] mm: pagewalk: prevent positive return value of walk_page_test() from being passed to callers (Re: [PATCH] mm: fix do_mbind return value)

On Thu, Mar 05, 2015 at 08:02:27AM +0000, Horiguchi Naoya(堀口直也) wrote:
...

Quoted 14 lines

---
From 107fa3fb256bddff40a882c90af717af9863aed7 Mon Sep 17 00:00:00 2001
From: Naoya Horiguchi <[email protected]>
Date: Thu, 5 Mar 2015 16:37:37 +0900
Subject: [PATCH] mm: pagewalk: prevent positive return value of
walk_page_test() from being passed to callers
walk_page_test() is purely pagewalk's internal stuff, and its positive return values are not intended to be passed to the callers of pagewalk. However, in the current code if the last vma in the do-while loop in walk_page_range() happens to return a positive value, it leaks outside walk_page_range(). So the user visible effect is invalid/unexpected return value (according to the reporter, mbind() causes it.)

Quoted 6 lines

This patch fixes it simply by reinitializing the return value after checked.
Another exposed interface, walk_page_vma(), already returns 0 for such cases so no problem.
Fixes 6f4576e3687b ("mempolicy: apply page table walker on queue_pages_range()")

This is not a right tag. To be precise, the bug was introduced by commit fafaa4264eba ("pagewalk: improve vma handling"), so

Fixes fafaa4264eba ("pagewalk: improve vma handling")

is right.

Thanks,
Naoya Horiguchi

Quoted 30 lines

Reported-by Kazutomo Yoshii <[email protected]>

Signed-off-by Naoya Horiguchi <[email protected]>

---
 mm/pagewalk.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

mm/pagewalk.c +8 -1

index 75c1f2878519..29f2f8b853ae 100644
@@ -265,8 +265,15 @@ int walk_page_range(unsigned long start, unsigned long end,
 			vma = vma->vm_next;
 
 			err = walk_page_test(start, next, walk);
-			if (err > 0)
+			if (err > 0) {
+				/*
+				 * positive return values are purely for
+				 * controlling the pagewalk, so should never
+				 * be passed to the callers.
+				 */
+				err = 0;
 				continue;
+			}
 			if (err < 0)
 				break;
 		}

Signature · 2 lines

1.9.3
���{.n�+�������+%�����ݶ��w��{.n�+����{��G�����{ay�ʇڙ�,j��f���h��������z_�(�階�ݢj"���m������G������?����&���~��iO��z��v�^�m����������?�I�

Naoya Horiguchi

2015-03-05 08:28 UTC #5 lore raw

Re: [PATCH] mm: pagewalk: prevent positive return value of walk_page_test() from being passed to callers (Re: [PATCH] mm: fix do_mbind return value)

Quoted 23 lines

Quoted 13 lines
From 107fa3fb256bddff40a882c90af717af9863aed7 Mon Sep 17 00:00:00 2001
From: Naoya Horiguchi <[email protected]>
Date: Thu, 5 Mar 2015 16:37:37 +0900
Subject: [PATCH] mm: pagewalk: prevent positive return value of
walk_page_test() from being passed to callers
walk_page_test() is purely pagewalk's internal stuff, and its positive return values are not intended to be passed to the callers of pagewalk. However, in the current code if the last vma in the do-while loop in walk_page_range() happens to return a positive value, it leaks outside walk_page_range(). So the user visible effect is invalid/unexpected return value (according to the reporter, mbind() causes it.)
Quoted 6 lines
This patch fixes it simply by reinitializing the return value after checked.
Another exposed interface, walk_page_vma(), already returns 0 for such cases so no problem.
Fixes 6f4576e3687b ("mempolicy: apply page table walker on queue_pages_range()")
This is not a right tag. To be precise, the bug was introduced by commit fafaa4264eba ("pagewalk: improve vma handling"), so

Quoted 8 lines

Fixes fafaa4264eba ("pagewalk: improve vma handling")
is right.
Thanks,
Naoya Horiguchi
Quoted 1 line
Reported-by Kazutomo Yoshii <[email protected]>

Ah, I might be a kind of rude, the original idea was posted by Yoshii-san, and I changed it, so I may as well add his Signed-off-by (additional to Reported-by) ?

David Rientjes

Acked-by on v1 1/1

2015-03-05 20:34 UTC #6 lore raw

Re: [PATCH] mm: pagewalk: prevent positive return value of walk_page_test() from being passed to callers (Re: [PATCH] mm: fix do_mbind return value)

In reply to Naoya Horiguchi

Quoted 7 lines · Naoya Horiguchi

walk_page_test() is purely pagewalk's internal stuff, and its positive return values are not intended to be passed to the callers of pagewalk. However, in the current code if the last vma in the do-while loop in walk_page_range() happens to return a positive value, it leaks outside walk_page_range(). So the user visible effect is invalid/unexpected return value (according to the reporter, mbind() causes it.)

Quoted 8 lines

This patch fixes it simply by reinitializing the return value after checked.
Another exposed interface, walk_page_vma(), already returns 0 for such cases so no problem.
Fixes 6f4576e3687b ("mempolicy: apply page table walker on queue_pages_range()")
Reported-by Kazutomo Yoshii <[email protected]>
Signed-off-by Naoya Horiguchi <[email protected]>

Acked-by David Rientjes <[email protected]>

This is exactly what I had in mind, thanks for fixing it up so fast!

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