Re: [PATCH] mm: fix invalid node in alloc_migrate_target()

From: Vlastimil Babka
Date: Tue Mar 29 2016 - 09:06:28 EST

On 03/25/2016 08:22 PM, Andrew Morton wrote:
On Fri, 25 Mar 2016 14:56:04 +0800 Xishi Qiu <qiuxishi@xxxxxxxxxx> 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.


--- a/mm/page_isolation.c
+++ b/mm/page_isolation.c
@@ -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);

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?

Good idea.

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

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?

I understand the comment that it's valid to be empty and the comment lists reasons why that can happen (with somewhat broken language). Note that I didn't verify these reasons:
- we call this when hitting memcg limit, not when adding pages to LRU, as adding to LRU means it would contain the given LRU's node
- adding to unevictable LRU means it's not added to scan_nodes (probably because scanning unevictable lru would be useless)
- for other reasons (which?) it might have pages not on LRU and it's so small there are no other pages that would be on LRU

mpol_rebind_nodemask() is similar.

Something like this?

From: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
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 <qiuxishi@xxxxxxxxxx>
Cc: Vlastimil Babka <vbabka@xxxxxxx>

Acked-by: Vlastimil Babka <vbabka@xxxxxxx>

Patch doesn't address offset_il_node() which is good, because if it's indeed buggy, it's serious and needs a non-cleanup patch.