mm: fix invalid node in alloc_migrate_target()

It is incorrect to use next_node to find a target node, it will return MAX_NUMNODES or invalid node. This will lead to crash in buddy system allocation.

...

mm/page_isolation.c +0 -2

@@ -289,11 +289,11 @@ struct page *alloc_migrate_target(struct page *page, unsigned long private,
 	 * now as a simple work-around, we use the next node for destination.
 	 */
 	if (PageHuge(page)) {
-		nodemask_t src = nodemask_of_node(page_to_nid(page));
-		nodemask_t dst;

Quoted 23 lines · On Fri, 25 Mar 2016 14:56:04 +0800 Xishi Qiu <[email protected]> wrote:

It is incorrect to use next_node to find a target node, it will return MAX_NUMNODES or invalid node. This will lead to crash in buddy system allocation.

...

mm/page_isolation.c +4 -4

@@ -289,11 +289,11 @@ struct page *alloc_migrate_target(struct page *page, unsigned long private,
 	 * now as a simple work-around, we use the next node for destination.
 	 */
 	if (PageHuge(page)) {
-		nodemask_t src = nodemask_of_node(page_to_nid(page));
-		nodemask_t dst;
-		nodes_complement(dst, src);
+		int node = next_online_node(page_to_nid(page));
+		if (node == MAX_NUMNODES)
+			node = first_online_node;
 		return alloc_huge_page_node(page_hstate(compound_head(page)),
-					    next_node(page_to_nid(page), dst));
+					    node);
 	}
 

mm/page_isolation.c struct page *alloc_migrate_target(struct · line 296

Quoted 1 line

mm/page_isolation.c +0 -0

 	if (PageHighMem(page))

Indeed. Can you tell us more about this circumstances under which the kernel will crash? I need to decide which kernel version(s) need the patch, but the changelog doesn't contain the info needed to make this decision (it should).

Quoted 25 lines · Andrew Morton

Quoted 23 lines · On Fri, 25 Mar 2016 14:56:04 +0800 Xishi Qiu <[email protected]> wrote:

It is incorrect to use next_node to find a target node, it will return MAX_NUMNODES or invalid node. This will lead to crash in buddy system allocation.

...

mm/page_isolation.c +4 -4

@@ -289,11 +289,11 @@ struct page *alloc_migrate_target(struct page *page, unsigned long private,
  	 * now as a simple work-around, we use the next node for destination.
  	 */
  	if (PageHuge(page)) {
-		nodemask_t src = nodemask_of_node(page_to_nid(page));
-		nodemask_t dst;
-		nodes_complement(dst, src);
+		int node = next_online_node(page_to_nid(page));
+		if (node == MAX_NUMNODES)
+			node = first_online_node;
  		return alloc_huge_page_node(page_hstate(compound_head(page)),
-					    next_node(page_to_nid(page), dst));
+					    node);
  	}
 

Quoted 7 lines

mm/page_isolation.c struct page *alloc_migrate_target(struct · line 296

Quoted 1 line

mm/page_isolation.c +0 -0

  	if (PageHighMem(page))

Indeed. Can you tell us more about this circumstances under which the kernel will crash? I need to decide which kernel version(s) need the patch, but the changelog doesn't contain the info needed to make this decision (it should).

Hi Andrew,

I read the code v4.4, and find the following path maybe trigger the bug.

alloc_migrate_target()

	alloc_huge_page_node()  // the node may be offline or MAX_NUMNODES
		__alloc_buddy_huge_page_no_mpol()
			__alloc_buddy_huge_page()
				__hugetlb_alloc_buddy_huge_page()

					alloc_pages_node()
						__alloc_pages_node()
							VM_BUG_ON(nid < 0 || nid >= MAX_NUMNODES);
							VM_WARN_ON(!node_online(nid));

Thanks,
Xishi Qiu

Quoted 37 lines · Xishi Qiu

Quoted 25 lines · Andrew Morton

Quoted 23 lines · On Fri, 25 Mar 2016 14:56:04 +0800 Xishi Qiu <[email protected]> wrote:

It is incorrect to use next_node to find a target node, it will return MAX_NUMNODES or invalid node. This will lead to crash in buddy system allocation.

...

mm/page_isolation.c +4 -4

@@ -289,11 +289,11 @@ struct page *alloc_migrate_target(struct page *page, unsigned long private,
   	 * now as a simple work-around, we use the next node for destination.
   	 */
   	if (PageHuge(page)) {
-		nodemask_t src = nodemask_of_node(page_to_nid(page));
-		nodemask_t dst;
-		nodes_complement(dst, src);
+		int node = next_online_node(page_to_nid(page));
+		if (node == MAX_NUMNODES)
+			node = first_online_node;
   		return alloc_huge_page_node(page_hstate(compound_head(page)),
-					    next_node(page_to_nid(page), dst));
+					    node);
   	}
 

Quoted 7 lines

mm/page_isolation.c struct page *alloc_migrate_target(struct · line 296

Quoted 1 line

mm/page_isolation.c +0 -0

   	if (PageHighMem(page))

Indeed. Can you tell us more about this circumstances under which the kernel will crash? I need to decide which kernel version(s) need the patch, but the changelog doesn't contain the info needed to make this decision (it should).

Hi Andrew,

Quoted 7 lines

I read the code v4.4, and find the following path maybe trigger the bug.

alloc_migrate_target()

	alloc_huge_page_node()  // the node may be offline or MAX_NUMNODES
		__alloc_buddy_huge_page_no_mpol()
			__alloc_buddy_huge_page()
				__hugetlb_alloc_buddy_huge_page()

The code in this functions seems to come from 099730d67417d ("mm,
hugetlb: use memory policy when available") by Dave Hansen (adding to CC), which was indeed merged in 4.4-rc1.

However, alloc_pages_node() is only called in the block guarded by:

if (!IS_ENABLED(CONFIG_NUMA) || !vma) {

The rather weird "!IS_ENABLED(CONFIG_NUMA)" part comes from immediate followup commit e0ec90ee7e6f ("mm, hugetlbfs: optimize when NUMA=n")

So I doubt the code path here can actually happen. But it's fragile and confusing nevertheless.

Quoted 8 lines

					alloc_pages_node()
						__alloc_pages_node()
							VM_BUG_ON(nid < 0 || nid >= MAX_NUMNODES);
							VM_WARN_ON(!node_online(nid));

Thanks,
Xishi Qiu

Quoted 37 lines · Xishi Qiu

Quoted 25 lines · Andrew Morton

Quoted 23 lines · On Fri, 25 Mar 2016 14:56:04 +0800 Xishi Qiu <[email protected]> wrote:

It is incorrect to use next_node to find a target node, it will return MAX_NUMNODES or invalid node. This will lead to crash in buddy system allocation.

...

mm/page_isolation.c +4 -4

@@ -289,11 +289,11 @@ struct page *alloc_migrate_target(struct page *page, unsigned long private,
       * now as a simple work-around, we use the next node for destination.
       */
      if (PageHuge(page)) {
-        nodemask_t src = nodemask_of_node(page_to_nid(page));
-        nodemask_t dst;
-        nodes_complement(dst, src);
+        int node = next_online_node(page_to_nid(page));
+        if (node == MAX_NUMNODES)
+            node = first_online_node;
          return alloc_huge_page_node(page_hstate(compound_head(page)),
-                        next_node(page_to_nid(page), dst));
+                        node);
      }
 

Quoted 7 lines

mm/page_isolation.c struct page *alloc_migrate_target(struct · line 296

Quoted 1 line

mm/page_isolation.c +0 -0

      if (PageHighMem(page))

Indeed. Can you tell us more about this circumstances under which the kernel will crash? I need to decide which kernel version(s) need the patch, but the changelog doesn't contain the info needed to make this decision (it should).

Hi Andrew,

Quoted 7 lines

I read the code v4.4, and find the following path maybe trigger the bug.

alloc_migrate_target()

    alloc_huge_page_node()  // the node may be offline or MAX_NUMNODES
        __alloc_buddy_huge_page_no_mpol()
            __alloc_buddy_huge_page()
                __hugetlb_alloc_buddy_huge_page()

The code in this functions seems to come from 099730d67417d ("mm, hugetlb: use memory policy when available") by Dave Hansen (adding to CC), which was indeed merged in 4.4-rc1.

However, alloc_pages_node() is only called in the block guarded by:

if (!IS_ENABLED(CONFIG_NUMA) || !vma) {

The rather weird "!IS_ENABLED(CONFIG_NUMA)" part comes from immediate followup commit e0ec90ee7e6f ("mm, hugetlbfs: optimize when NUMA=n")

So I doubt the code path here can actually happen. But it's fragile and confusing nevertheless.

Quoted 8 lines

                    alloc_pages_node()
                        __alloc_pages_node()
                            VM_BUG_ON(nid < 0 || nid >= MAX_NUMNODES);
                            VM_WARN_ON(!node_online(nid));

Thanks,
Xishi Qiu

.

Quoted 6 lines · Vlastimil Babka

The code in this functions seems to come from 099730d67417d ("mm, hugetlb: use memory policy when available") by Dave Hansen (adding to CC), which was indeed merged in 4.4-rc1.

However, alloc_pages_node() is only called in the block guarded by:

if (!IS_ENABLED(CONFIG_NUMA) || !vma) {

Quoted 4 lines

The rather weird "!IS_ENABLED(CONFIG_NUMA)" part comes from immediate followup commit e0ec90ee7e6f ("mm, hugetlbfs: optimize when NUMA=n")

So I doubt the code path here can actually happen. But it's fragile and confusing nevertheless.

Hi Vlastimil

__alloc_buddy_huge_page(h, NULL, addr, nid); // so the vma is NULL

Quoted 14 lines · On Fri, 25 Mar 2016 14:56:04 +0800 Xishi Qiu <[email protected]> wrote:

It is incorrect to use next_node to find a target node, it will return MAX_NUMNODES or invalid node. This will lead to crash in buddy system allocation.

...

mm/page_isolation.c +0 -2

@@ -289,11 +289,11 @@ struct page *alloc_migrate_target(struct page *page, unsigned long private,
  	 * now as a simple work-around, we use the next node for destination.
  	 */
  	if (PageHuge(page)) {
-		nodemask_t src = nodemask_of_node(page_to_nid(page));
-		nodemask_t dst;

mm/page_isolation.c struct page *alloc_migrate_target(struct page *page, unsigned long private, · line 299

Quoted 10 lines

mm/page_isolation.c +4 -2

-		nodes_complement(dst, src);
+		int node = next_online_node(page_to_nid(page));
+		if (node == MAX_NUMNODES)
+			node = first_online_node;
  		return alloc_huge_page_node(page_hstate(compound_head(page)),
-					    next_node(page_to_nid(page), dst));
+					    node);
  	}
 
  	if (PageHighMem(page))

Indeed. Can you tell us more about this circumstances under which the kernel will crash? I need to decide which kernel version(s) need the patch, but the changelog doesn't contain the info needed to make this decision (it should).

next_node() isn't a very useful interface, really. Just about every caller does this:

	node = next_node(node, XXX);
	if (node == MAX_NUMNODES)
		node = first_node(XXX);

so how about we write a function which does that, and stop open-coding the same thing everywhere?

And I think your fix could then use such a function:

	int node = that_new_function(page_to_nid(page), node_online_map);

Also, mm/mempolicy.c:offset_il_node() worries me:

	do {
		nid = next_node(nid, pol->v.nodes);
		c++;
	} while (c <= target);

Can't `nid' hit MAX_NUMNODES?

And can someone please explain mem_cgroup_select_victim_node() to me? How can we hit the "node = numa_node_id()" path? Only if memcg->scan_nodes is empty? is that even valid? The comment seems to have not much to do with the code?

mpol_rebind_nodemask() is similar.

Something like this?

From: Andrew Morton <[email protected]>
Subject: include/linux/nodemask.h: create next_node_in() helper

Lots of code does

	node = next_node(node, XXX);
	if (node == MAX_NUMNODES)
		node = first_node(XXX);

so create next_node_in() to do this and use it in various places.

Cc Xishi Qiu <[email protected]>

Cc Vlastimil Babka <[email protected]>

Quoted 7 lines · Andrew Morton

Also, mm/mempolicy.c:offset_il_node() worries me:

	do {
		nid = next_node(nid, pol->v.nodes);
		c++;
	} while (c <= target);

Quoted 1 line

Can't `nid' hit MAX_NUMNODES?

AFAICS it can. interleave_nid() uses this and the nid is then used e.g. in node_zonelist() where it's used for NODE_DATA(nid). That's quite scary. It also predates git. Why don't we see crashes or KASAN finding this?

Quoted 10 lines · Vlastimil Babka

Quoted 8 lines · Andrew Morton

Also, mm/mempolicy.c:offset_il_node() worries me:

	do {
		nid = next_node(nid, pol->v.nodes);
		c++;
	} while (c <= target);

Can't `nid' hit MAX_NUMNODES?

Quoted 3 lines

AFAICS it can. interleave_nid() uses this and the nid is then used e.g. in node_zonelist() where it's used for NODE_DATA(nid). That's quite scary. It also predates git. Why don't we see crashes or KASAN finding this?

Ah, I see. In offset_il_node(), nid is initialized to -1, and the number of do-while iterations calling next_node() is up to the number of bits set in the pol->v.nodes bitmap, so it can't reach past the last set bit and return MAX_NUMNODES.

Quoted 15 lines · On Thu, 31 Mar 2016 15:13:41 +0200 Vlastimil Babka <[email protected]> wrote:

Quoted 13 lines · Vlastimil Babka

Quoted 8 lines · Andrew Morton

Also, mm/mempolicy.c:offset_il_node() worries me:

	do {
		nid = next_node(nid, pol->v.nodes);
		c++;
	} while (c <= target);

Can't `nid' hit MAX_NUMNODES?

AFAICS it can. interleave_nid() uses this and the nid is then used e.g. in node_zonelist() where it's used for NODE_DATA(nid). That's quite scary. It also predates git. Why don't we see crashes or KASAN finding this?

Quoted 4 lines

Ah, I see. In offset_il_node(), nid is initialized to -1, and the number of do-while iterations calling next_node() is up to the number of bits set in the pol->v.nodes bitmap, so it can't reach past the last set bit and return MAX_NUMNODES.

Gack. offset_il_node() should be dragged out, strangled, shot then burnt.

static unsigned offset_il_node(struct mempolicy *pol,
struct vm_area_struct *vma, unsigned long off)

{
	unsigned nnodes = nodes_weight(pol->v.nodes);
	unsigned target;
	int c;
	int nid = NUMA_NO_NODE;

	if (!nnodes)
		return numa_node_id();
	target = (unsigned int)off % nnodes;
	c = 0;
	do {
		nid = next_node(nid, pol->v.nodes);
		c++;
	} while (c <= target);
	return nid;
}

For starters it is relying upon next_node(-1, ...) behaving like first_node(). Fair enough I guess, but that isn't very clear.

static inline int __next_node(int n, const nodemask_t *srcp)

{
	return min_t(int,MAX_NUMNODES,find_next_bit(srcp->bits, MAX_NUMNODES, n+1));
}

will start from node 0 when it does the n+1.

Also it is relying upon NUMA_NO_NODE having a value of -1. That's just grubby - this code shouldn't "know" that NUMA_NO_NODE==-1. It would have been better to use plain old "-1" here.

Does this look clearer and correct?

/*
* Do static interleaving for a VMA with known offset @n. Returns the n'th
* node in pol->v.nodes (starting from n=0), wrapping around if n exceeds the
* number of present nodes.
*/
static unsigned offset_il_node(struct mempolicy *pol,
struct vm_area_struct *vma, unsigned long n)

{
	unsigned nnodes = nodes_weight(pol->v.nodes);
	unsigned target;
	int i;
	int nid;

	if (!nnodes)
		return numa_node_id();
	target = (unsigned int)n % nnodes;
	nid = first_node(pol->v.nodes);
	for (i = 0; i < target; i++)
		nid = next_node(nid, pol->v.nodes);
	return nid;
}

From: Andrew Morton <[email protected]>
Subject: mm/mempolicy.c:offset_il_node() document and clarify

This code was pretty obscure and was relying upon obscure side-effects of next_node(-1, ...) and was relying upon NUMA_NO_NODE being equal to -1.

Clean that all up and document the function's intent.

Cc Vlastimil Babka <[email protected]>

Cc Xishi Qiu <[email protected]>

Cc Joonsoo Kim <[email protected]>

Cc David Rientjes <[email protected]>

Cc Naoya Horiguchi <[email protected]>

Cc Laura Abbott <[email protected]>

Signed-off-by Andrew Morton <[email protected]>

And can someone please explain mem_cgroup_select_victim_node() to me? How can we hit the "node = numa_node_id()" path? Only if memcg->scan_nodes is empty?

is that even valid?

The comment seems to have not much to do with the code?

It is incorrect to use next_node to find a target node, it will return MAX_NUMNODES or invalid node. This will lead to crash in buddy system allocation.

Signed-off-by Xishi Qiu <[email protected]>

---
  mm/page_isolation.c | 8 ++++----
  1 file changed, 4 insertions(+), 4 deletions(-)

mm/page_isolation.c +0 -2

index 92c4c36..31555b6 100644
@@ -289,11 +289,11 @@ struct page *alloc_migrate_target(struct page *page, unsigned long private,
  	 * now as a simple work-around, we use the next node for destination.
  	 */
  	if (PageHuge(page)) {
-		nodemask_t src = nodemask_of_node(page_to_nid(page));
-		nodemask_t dst;

Quoted 3 lines · Xishi Qiu

It is incorrect to use next_node to find a target node, it will return MAX_NUMNODES or invalid node. This will lead to crash in buddy system allocation.

One possible place of crash is:
alloc_huge_page_node()

    dequeue_huge_page_node()
        [accesses h->hugepage_freelists[nid] with size MAX_NUMANODES]

Quoted 1 line

Signed-off-by Xishi Qiu <[email protected]>

Fixes c8721bbbdd36 ("mm: memory-hotplug: enable memory hotplug to handle

hugepage")

Cc stable

Acked-by Vlastimil Babka <[email protected]>

Quoted 23 lines

---
 mm/page_isolation.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

mm/page_isolation.c +4 -4

index 92c4c36..31555b6 100644
@@ -289,11 +289,11 @@ struct page *alloc_migrate_target(struct page *page, unsigned long private,
 	 * now as a simple work-around, we use the next node for destination.
 	 */
 	if (PageHuge(page)) {
-		nodemask_t src = nodemask_of_node(page_to_nid(page));
-		nodemask_t dst;
-		nodes_complement(dst, src);
+		int node = next_online_node(page_to_nid(page));
+		if (node == MAX_NUMNODES)
+			node = first_online_node;
 		return alloc_huge_page_node(page_hstate(compound_head(page)),
-					    next_node(page_to_nid(page), dst));
+					    node);
 	}
 

mm/page_isolation.c struct page *alloc_migrate_target(struct page *page, unsigned long private, · line 299

Quoted 2 lines

mm/page_isolation.c +0 -0

 	if (PageHighMem(page))