FIX [1/2] slub: Do not dereference NULL pointer in node_match

PATCH Christoph Lameter 4 participants 9 messages Jan 23, 2013 – Feb 1, 2013 1 message not in the archive

Subsystems (2)

Memory management Slab allocator

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Christoph Lameter

author

2013-01-23 21:45 UTC #1 lore raw

The variables accessed in slab_alloc are volatile and therefore the page pointer passed to node_match can be NULL. The processing of data in slab_alloc is tentative until either the cmpxhchg succeeds or the __slab_alloc slowpath is invoked. Both are able to perform the same allocation from the freelist.

Check for the NULL pointer in node_match.

A false positive will lead to a retry of the loop in __slab_alloc.

Signed-off-by Christoph Lameter <[email protected]>

mm/slub.c +1 -1

Index: linux/mm/slub.c
===================================================================
@@ -2041,7 +2041,7 @@ static void flush_all(struct kmem_cache
 static inline int node_match(struct page *page, int node)
 {
 #ifdef CONFIG_NUMA
-	if (node != NUMA_NO_NODE && page_to_nid(page) != node)
+	if (!page || (node != NUMA_NO_NODE && page_to_nid(page) != node))
 		return 0;
 #endif
 	return 1;

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

Simon Jeons

2013-01-24 00:53 UTC #2 lore raw

Quoted 9 lines · Christoph Lameter

The variables accessed in slab_alloc are volatile and therefore the page pointer passed to node_match can be NULL. The processing of data in slab_alloc is tentative until either the cmpxhchg succeeds or the __slab_alloc slowpath is invoked. Both are able to perform the same allocation from the freelist.
Check for the NULL pointer in node_match.
A false positive will lead to a retry of the loop in __slab_alloc.

Hi Christoph,

Since page_to_nid(NULL) will trigger bug, then how can run into __slab_alloc?

Quoted 18 lines

Signed-off-by Christoph Lameter <[email protected]>

Index: linux/mm/slub.c
===================================================================

mm/slub.c +1 -2

@@ -2041,7 +2041,7 @@ static void flush_all(struct kmem_cache
 static inline int node_match(struct page *page, int node)
 {
 #ifdef CONFIG_NUMA
-	if (node != NUMA_NO_NODE && page_to_nid(page) != node)
+	if (!page || (node != NUMA_NO_NODE && page_to_nid(page) != node))
 		return 0;
 #endif
 	return 1;
 
--

Quoted 4 lines

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>

Christoph Lameter

author

2013-01-24 15:14 UTC #3 lore raw

Quoted 7 lines · Simon Jeons

Quoted 6 lines · Christoph Lameter
The variables accessed in slab_alloc are volatile and therefore the page pointer passed to node_match can be NULL. The processing of data in slab_alloc is tentative until either the cmpxhchg succeeds or the __slab_alloc slowpath is invoked. Both are able to perform the same allocation from the freelist.

Quoted 8 lines

Quoted 3 lines
Check for the NULL pointer in node_match.
A false positive will lead to a retry of the loop in __slab_alloc.
Hi Christoph,
Since page_to_nid(NULL) will trigger bug, then how can run into __slab_alloc?

page = NULL

->

node_match(NULL, xx) = 0

call into __slab_alloc.

__slab_alloc() will check for !c->page which requires the assignment of a new per cpu slab page.

Simon Jeons

2013-01-25 08:12 UTC #4 lore raw

Quoted 22 lines · Christoph Lameter

Quoted 7 lines · Simon Jeons
Quoted 6 lines · Christoph Lameter
The variables accessed in slab_alloc are volatile and therefore the page pointer passed to node_match can be NULL. The processing of data in slab_alloc is tentative until either the cmpxhchg succeeds or the __slab_alloc slowpath is invoked. Both are able to perform the same allocation from the freelist.
Quoted 8 lines
Quoted 3 lines
Check for the NULL pointer in node_match.
A false positive will lead to a retry of the loop in __slab_alloc.
Hi Christoph,
Since page_to_nid(NULL) will trigger bug, then how can run into __slab_alloc?
page = NULL
	 ->

Quoted 9 lines

node_match(NULL, xx) = 0
->
call into __slab_alloc.
__slab_alloc() will check for !c->page which requires the assignment of a new per cpu slab page.

But there are dereference in page_to_nid path, function page_to_section:
return (page->flags >> SECTIONS_PGSHIFT) & SECTIONS_MASK;

Christoph Lameter

author

2013-01-25 14:53 UTC #5 lore raw

Quoted 5 lines · Simon Jeons

Quoted 5 lines
node_match(NULL, xx) = 0
->

Quoted 8 lines

Quoted 5 lines
call into __slab_alloc.
__slab_alloc() will check for !c->page which requires the assignment of a new per cpu slab page.
But there are dereference in page_to_nid path, function page_to_section:
return (page->flags >> SECTIONS_PGSHIFT) & SECTIONS_MASK;

node_match() checks for NULL and will not invoke page_to_nid for a NULL pointer.

Pekka Enberg

2013-02-01 10:23 UTC #6 lore raw

In reply to Christoph Lameter

Quoted 11 lines · Christoph Lameter

The variables accessed in slab_alloc are volatile and therefore the page pointer passed to node_match can be NULL. The processing of data in slab_alloc is tentative until either the cmpxhchg succeeds or the __slab_alloc slowpath is invoked. Both are able to perform the same allocation from the freelist.
Check for the NULL pointer in node_match.
A false positive will lead to a retry of the loop in __slab_alloc.
Signed-off-by Christoph Lameter <[email protected]>

Steven, how did you trigger the problem - i.e. is this -rt only problem? Does the patch work for you?

Quoted 15 lines

Index: linux/mm/slub.c
===================================================================
mm/slub.c 2013-01-18 08:47:40.579126371 -0600 renamed from mm/slub.c 2013-01-18 08:47:29.198954250 -0600 +1 -2
@@ -2041,7 +2041,7 @@ static void flush_all(struct kmem_cache
 static inline int node_match(struct page *page, int node)
 {
 #ifdef CONFIG_NUMA
-       if (node != NUMA_NO_NODE && page_to_nid(page) != node)
+       if (!page || (node != NUMA_NO_NODE && page_to_nid(page) != node))
                return 0;
 #endif
        return 1;
 
--

Quoted 4 lines

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>

Steven Rostedt

2013-02-01 11:51 UTC #7 lore raw

Quoted 7 lines · Pekka Enberg

Quoted 6 lines · Christoph Lameter
The variables accessed in slab_alloc are volatile and therefore the page pointer passed to node_match can be NULL. The processing of data in slab_alloc is tentative until either the cmpxhchg succeeds or the __slab_alloc slowpath is invoked. Both are able to perform the same allocation from the freelist.

Quoted 8 lines

Quoted 5 lines
Check for the NULL pointer in node_match.
A false positive will lead to a retry of the loop in __slab_alloc.
Signed-off-by Christoph Lameter <[email protected]>
Steven, how did you trigger the problem - i.e. is this -rt only problem? Does the patch work for you?

I haven't tested Christoph's version yet. I've only tested my own. But I'll take his and run them through tests as well. This bug is not easy to hit.

It is not a -rt only bug, and yes it probably should go to stable. The race is extremely small, but -rt creates scenarios that may only be hit by 1000 CPU core machines. Because of the preemptive nature of -rt, -rt is much more susceptible to race conditions than mainline. But these are real bugs for mainline too. It may only trigger once a year, where in -rt it will trigger once a week.

-- Steve

Quoted 15 lines

Quoted 15 lines
Index: linux/mm/slub.c
===================================================================
mm/slub.c 2013-01-18 08:47:40.579126371 -0600 renamed from mm/slub.c 2013-01-18 08:47:29.198954250 -0600 +1 -1
@@ -2041,7 +2041,7 @@ static void flush_all(struct kmem_cache
 static inline int node_match(struct page *page, int node)
 {
 #ifdef CONFIG_NUMA
-       if (node != NUMA_NO_NODE && page_to_nid(page) != node)
+       if (!page || (node != NUMA_NO_NODE && page_to_nid(page) != node))
                return 0;
 #endif
        return 1;

Quoted 5 lines

Quoted 5 lines
--
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>

Christoph Lameter

author

2013-01-23 21:45 UTC #8 lore raw

FIX [2/2] slub: tid must be retrieved from the percpu area of the current processor.

In reply to a message not in the archive

As Steven Rostedt has pointer out: Rescheduling could occur on a differnet processor after the determination of the per cpu pointer and before the tid is retrieved. This could result in allocation from the wrong node in slab_alloc.

The effect is much more severe in slab_free() where we could free to the freelist of the wrong page.

The window for something like that occurring is pretty small but it is possible.

Signed-off-by Christoph Lameter <[email protected]>

mm/slub.c +9 -3

Index: linux/mm/slub.c
===================================================================
@@ -2331,13 +2331,18 @@ static __always_inline void *slab_alloc_
 
 	s = memcg_kmem_get_cache(s, gfpflags);
 redo:
-
 	/*
 	 * Must read kmem_cache cpu data via this cpu ptr. Preemption is
 	 * enabled. We may switch back and forth between cpus while
 	 * reading from one cpu area. That does not matter as long
 	 * as we end up on the original cpu again when doing the cmpxchg.
+	 *
+	 * Preemption is disabled for the retrieval of the tid because that
+	 * must occur from the current processor. We cannot allow rescheduling
+	 * on a different processor between the determination of the pointer
+	 * and the retrieval of the tid.
 	 */
+	preempt_disable();
 	c = __this_cpu_ptr(s->cpu_slab);
 
 	/*

@@ -2347,7 +2352,7 @@ redo:
 	 * linked list in between.
 	 */
 	tid = c->tid;
-	barrier();
+	preempt_enable();
 
 	object = c->freelist;
 	page = c->page;

@@ -2594,10 +2599,11 @@ redo:
 	 * data is retrieved via this pointer. If we are on the same cpu
 	 * during the cmpxchg then the free will succedd.
 	 */
+	preempt_disable();
 	c = __this_cpu_ptr(s->cpu_slab);
 
 	tid = c->tid;
-	barrier();
+	preempt_enable();
 
 	if (likely(page == c->page)) {
 		set_freepointer(s, object, c->freelist);

Pekka Enberg

2013-02-01 10:24 UTC #9 lore raw

Re: FIX [2/2] slub: tid must be retrieved from the percpu area of the current processor.

Quoted 10 lines · Christoph Lameter

As Steven Rostedt has pointer out: Rescheduling could occur on a differnet processor after the determination of the per cpu pointer and before the tid is retrieved. This could result in allocation from the wrong node in slab_alloc.
The effect is much more severe in slab_free() where we could free to the freelist of the wrong page.
The window for something like that occurring is pretty small but it is possible.
Signed-off-by Christoph Lameter <[email protected]>

Okay, makes sense. Has anyone triggered this in practice? Do we want to tag this for -stable?

Quoted 53 lines

Index: linux/mm/slub.c
===================================================================

mm/slub.c 2013-01-23 15:24:47.656868067 -0600 renamed from mm/slub.c 2013-01-23 15:06:39.805154107 -0600 +9 -4

@@ -2331,13 +2331,18 @@ static __always_inline void *slab_alloc_
 
        s = memcg_kmem_get_cache(s, gfpflags);
 redo:
-
        /*
         * Must read kmem_cache cpu data via this cpu ptr. Preemption is
         * enabled. We may switch back and forth between cpus while
         * reading from one cpu area. That does not matter as long
         * as we end up on the original cpu again when doing the cmpxchg.
+        *
+        * Preemption is disabled for the retrieval of the tid because that
+        * must occur from the current processor. We cannot allow rescheduling
+        * on a different processor between the determination of the pointer
+        * and the retrieval of the tid.
         */
+       preempt_disable();
        c = __this_cpu_ptr(s->cpu_slab);
 
        /*

@@ -2347,7 +2352,7 @@ redo:
         * linked list in between.
         */
        tid = c->tid;
-       barrier();
+       preempt_enable();
 
        object = c->freelist;
        page = c->page;

@@ -2594,10 +2599,11 @@ redo:
         * data is retrieved via this pointer. If we are on the same cpu
         * during the cmpxchg then the free will succedd.
         */
+       preempt_disable();
        c = __this_cpu_ptr(s->cpu_slab);
 
        tid = c->tid;
-       barrier();
+       preempt_enable();
 
        if (likely(page == c->page)) {
                set_freepointer(s, object, c->freelist);
 
--

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>