[PATCH] slab: Fix nodeid bounds check for non-contiguous node IDs

The bounds check for nodeid in ____cache_alloc_node gives false
positives on machines where the node IDs are not contiguous, leading
to a panic at boot time.  For example, on a POWER8 machine the node
IDs are typically 0, 1, 16 and 17.  This means that num_online_nodes()
returns 4, so when ____cache_alloc_node is called with nodeid = 16 the
VM_BUG_ON triggers.

To fix this, we instead compare the nodeid with MAX_NUMNODES, and
additionally make sure it isn't negative (since nodeid is an int).
The check is there mainly to protect the array dereference in the
get_node() call in the next line, and the array being dereferenced is
of size MAX_NUMNODES.  If the nodeid is in range but invalid, the
BUG_ON in the next line will catch that.

Signed-off-by: Paul Mackerras <redacted>
---

diff --git a/mm/slab.c b/mm/slab.c
index eb2b2ea..f34e053 100644
--- a/mm/slab.c
+++ b/mm/slab.c

@@ -3076,7 +3076,7 @@ static void *____cache_alloc_node(struct kmem_cache *cachep, gfp_t flags,
 	void *obj;
 	int x;
 
-	VM_BUG_ON(nodeid > num_online_nodes());
+	VM_BUG_ON(nodeid < 0 || nodeid >= MAX_NUMNODES);
 	n = get_node(cachep, nodeid);
 	BUG_ON(!n);
 

(2014/12/01 7:16), Paul Mackerras wrote:

The bounds check for nodeid in ____cache_alloc_node gives false
positives on machines where the node IDs are not contiguous, leading
to a panic at boot time.  For example, on a POWER8 machine the node
IDs are typically 0, 1, 16 and 17.  This means that num_online_nodes()
returns 4, so when ____cache_alloc_node is called with nodeid = 16 the
VM_BUG_ON triggers.

Do you have the call trace? If you have it, please add it in the description.

To fix this, we instead compare the nodeid with MAX_NUMNODES, and
additionally make sure it isn't negative (since nodeid is an int).
The check is there mainly to protect the array dereference in the
get_node() call in the next line, and the array being dereferenced is
of size MAX_NUMNODES.  If the nodeid is in range but invalid, the
BUG_ON in the next line will catch that.

Signed-off-by: Paul Mackerras <redacted>

Do you need to backport it into -stable kernels?

-	VM_BUG_ON(nodeid > num_online_nodes());
+	VM_BUG_ON(nodeid < 0 || nodeid >= MAX_NUMNODES);

How about use:
	VM_BUG_ON(!node_online(nodeid));

When allocating the memory, the node of the memory being allocated must be
online. But your code cannot check the condition.

Thanks,
Yasuaki Ishimatsu

  	n = get_node(cachep, nodeid);
  	BUG_ON(!n);


--
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>

On Mon, Dec 01, 2014 at 09:14:40AM +0900, Yasuaki Ishimatsu wrote:

(2014/12/01 7:16), Paul Mackerras wrote:

quoted

The bounds check for nodeid in ____cache_alloc_node gives false
positives on machines where the node IDs are not contiguous, leading
to a panic at boot time.  For example, on a POWER8 machine the node
IDs are typically 0, 1, 16 and 17.  This means that num_online_nodes()
returns 4, so when ____cache_alloc_node is called with nodeid = 16 the
VM_BUG_ON triggers.

Do you have the call trace? If you have it, please add it in the description.

I can get it easily enough.

quoted

To fix this, we instead compare the nodeid with MAX_NUMNODES, and
additionally make sure it isn't negative (since nodeid is an int).
The check is there mainly to protect the array dereference in the
get_node() call in the next line, and the array being dereferenced is
of size MAX_NUMNODES.  If the nodeid is in range but invalid, the
BUG_ON in the next line will catch that.

Signed-off-by: Paul Mackerras <redacted>

Do you need to backport it into -stable kernels?

It does need to go to stable, yes, for 3.10 and later.

quoted

---

diff --git a/mm/slab.c b/mm/slab.c
index eb2b2ea..f34e053 100644
--- a/mm/slab.c
+++ b/mm/slab.c

@@ -3076,7 +3076,7 @@ static void *____cache_alloc_node(struct kmem_cache *cachep, gfp_t flags,
 	void *obj;
 	int x;

quoted

-	VM_BUG_ON(nodeid > num_online_nodes());
+	VM_BUG_ON(nodeid < 0 || nodeid >= MAX_NUMNODES);

How about use:
	VM_BUG_ON(!node_online(nodeid));

That would not be better, since node_online() doesn't bounds-check its
argument.

When allocating the memory, the node of the memory being allocated must be
online. But your code cannot check the condition.

The following two lines:

quoted

 	n = get_node(cachep, nodeid);
 	BUG_ON(!n);

effectively check that condition already, as I tried to explain in the
commit message.

Regards,
Paul.

(2014/12/01 9:42), Paul Mackerras wrote:

On Mon, Dec 01, 2014 at 09:14:40AM +0900, Yasuaki Ishimatsu wrote:

quoted

(2014/12/01 7:16), Paul Mackerras wrote:

quoted

The bounds check for nodeid in ____cache_alloc_node gives false
positives on machines where the node IDs are not contiguous, leading
to a panic at boot time.  For example, on a POWER8 machine the node
IDs are typically 0, 1, 16 and 17.  This means that num_online_nodes()
returns 4, so when ____cache_alloc_node is called with nodeid = 16 the
VM_BUG_ON triggers.

Do you have the call trace? If you have it, please add it in the description.

I can get it easily enough.

quoted

quoted

To fix this, we instead compare the nodeid with MAX_NUMNODES, and
additionally make sure it isn't negative (since nodeid is an int).
The check is there mainly to protect the array dereference in the
get_node() call in the next line, and the array being dereferenced is
of size MAX_NUMNODES.  If the nodeid is in range but invalid, the
BUG_ON in the next line will catch that.

Signed-off-by: Paul Mackerras <redacted>

Do you need to backport it into -stable kernels?

It does need to go to stable, yes, for 3.10 and later.

quoted

quoted

---

diff --git a/mm/slab.c b/mm/slab.c
index eb2b2ea..f34e053 100644
--- a/mm/slab.c
+++ b/mm/slab.c

@@ -3076,7 +3076,7 @@ static void *____cache_alloc_node(struct kmem_cache *cachep, gfp_t flags,
  	void *obj;
  	int x;

quoted

quoted

-	VM_BUG_ON(nodeid > num_online_nodes());
+	VM_BUG_ON(nodeid < 0 || nodeid >= MAX_NUMNODES);

How about use:
	VM_BUG_ON(!node_online(nodeid));

That would not be better, since node_online() doesn't bounds-check its
argument.

Ah. You are right.

quoted

When allocating the memory, the node of the memory being allocated must be
online. But your code cannot check the condition.

The following two lines:

quoted

quoted

  	n = get_node(cachep, nodeid);
  	BUG_ON(!n);

effectively check that condition already, as I tried to explain in the
commit message.

O.K. I understood.

Thansk,
Yasuaki Ishimatsu

Regards,
Paul.