[PATCH] slub: avoid potential NULL dereference or corruption

[PATCH] slub: avoid potential NULL dereference or corruption

[Eric Dumazet]

show_slab_objects() can trigger NULL dereferences or memory corruption.

Another cpu can change its c->page to NULL or c->node to NUMA_NO_NODE
while we use them.

Use ACCESS_ONCE(c->page) and ACCESS_ONCE(c->node) to make sure this
cannot happen.

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
 mm/slub.c |   22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/mm/slub.c b/mm/slub.c
index 7d2a996..e8e6714 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -4436,29 +4436,33 @@ static ssize_t show_slab_objects(struct kmem_cache *s,
 		for_each_possible_cpu(cpu) {
 			struct kmem_cache_cpu *c = per_cpu_ptr(s->cpu_slab, cpu);
 			struct page *page;
+			int node;
 
-			if (!c || c->node < 0)
+			if (!c)
 				continue;
-
-			if (c->page) {
-					if (flags & SO_TOTAL)
-						x = c->page->objects;
+			node = ACCESS_ONCE(c->node);
+			if (node < 0)
+				continue;
+			page = ACCESS_ONCE(c->page);
+			if (page) {
+				if (flags & SO_TOTAL)
+					x = page->objects;
 				else if (flags & SO_OBJECTS)
-					x = c->page->inuse;
+					x = page->inuse;
 				else
 					x = 1;
 
 				total += x;
-				nodes[c->node] += x;
+				nodes[node] += x;
 			}
 			page = c->partial;
 
 			if (page) {
 				x = page->pobjects;
                                 total += x;
-                                nodes[c->node] += x;
+                                nodes[node] += x;
 			}
-			per_cpu[c->node]++;
+			per_cpu[node]++;
 		}
 	}
 

Re: [PATCH] slub: avoid potential NULL dereference or corruption

[Christoph Lameter]

On Tue, 22 Nov 2011, Eric Dumazet wrote:

> diff --git a/mm/slub.c b/mm/slub.c
> index 7d2a996..e8e6714 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -4436,29 +4436,33 @@ static ssize_t show_slab_objects(struct kmem_cache *s,
>  		for_each_possible_cpu(cpu) {
>  			struct kmem_cache_cpu *c = per_cpu_ptr(s->cpu_slab, cpu);
>  			struct page *page;
> +			int node;
>
> -			if (!c || c->node < 0)
> +			if (!c)
>  				continue;

Drop the check. c can never be NULL these days.

Otherwiswe it looks okay.

[PATCH V2] slub: avoid potential NULL dereference or corruption

[Eric Dumazet]

show_slab_objects() can trigger NULL dereferences or memory corruption.

Another cpu can change its c->page to NULL or c->node to NUMA_NO_NODE
while we use them.

Use ACCESS_ONCE(c->page) and ACCESS_ONCE(c->node) to make sure this
cannot happen.

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
V2: remove a not needed "if (!c)" test

 mm/slub.c |   21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/mm/slub.c b/mm/slub.c
index 7d2a996..9deef7d 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -4435,30 +4435,31 @@ static ssize_t show_slab_objects(struct kmem_cache *s,
 
 		for_each_possible_cpu(cpu) {
 			struct kmem_cache_cpu *c = per_cpu_ptr(s->cpu_slab, cpu);
+			int node = ACCESS_ONCE(c->node);
 			struct page *page;
 
-			if (!c || c->node < 0)
+			if (node < 0)
 				continue;
-
-			if (c->page) {
-					if (flags & SO_TOTAL)
-						x = c->page->objects;
+			page = ACCESS_ONCE(c->page);
+			if (page) {
+				if (flags & SO_TOTAL)
+					x = page->objects;
 				else if (flags & SO_OBJECTS)
-					x = c->page->inuse;
+					x = page->inuse;
 				else
 					x = 1;
 
 				total += x;
-				nodes[c->node] += x;
+				nodes[node] += x;
 			}
 			page = c->partial;
 
 			if (page) {
 				x = page->pobjects;
-                                total += x;
-                                nodes[c->node] += x;
+				total += x;
+				nodes[node] += x;
 			}
-			per_cpu[c->node]++;
+			per_cpu[node]++;
 		}
 	}
 

Re: [PATCH V2] slub: avoid potential NULL dereference or corruption

[Christoph Lameter]

On Tue, 22 Nov 2011, Eric Dumazet wrote:

> show_slab_objects() can trigger NULL dereferences or memory corruption.

Acked-by: Christoph Lameter <cl@linux.com>

Re: [PATCH V2] slub: avoid potential NULL dereference or corruption

[David Rientjes]

On Tue, 22 Nov 2011, Eric Dumazet wrote:

> show_slab_objects() can trigger NULL dereferences or memory corruption.
> 
> Another cpu can change its c->page to NULL or c->node to NUMA_NO_NODE
> while we use them.
> 
> Use ACCESS_ONCE(c->page) and ACCESS_ONCE(c->node) to make sure this
> cannot happen.
> 
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> 

Acked-by: David Rientjes <rientjes@google.com>

Re: [PATCH V2] slub: avoid potential NULL dereference or corruption

[Pekka Enberg]

On Tue, 22 Nov 2011, Eric Dumazet wrote:
> show_slab_objects() can trigger NULL dereferences or memory corruption.
>
> Another cpu can change its c->page to NULL or c->node to NUMA_NO_NODE
> while we use them.
>
> Use ACCESS_ONCE(c->page) and ACCESS_ONCE(c->node) to make sure this
> cannot happen.
>
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>

Did someone test this patch? Does it fix any of the reported issues?

> ---
> V2: remove a not needed "if (!c)" test
>
> mm/slub.c |   21 +++++++++++----------
> 1 file changed, 11 insertions(+), 10 deletions(-)
>
> diff --git a/mm/slub.c b/mm/slub.c
> index 7d2a996..9deef7d 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -4435,30 +4435,31 @@ static ssize_t show_slab_objects(struct kmem_cache *s,
>
> 		for_each_possible_cpu(cpu) {
> 			struct kmem_cache_cpu *c = per_cpu_ptr(s->cpu_slab, cpu);
> +			int node = ACCESS_ONCE(c->node);
> 			struct page *page;
>
> -			if (!c || c->node < 0)
> +			if (node < 0)
> 				continue;
> -
> -			if (c->page) {
> -					if (flags & SO_TOTAL)
> -						x = c->page->objects;
> +			page = ACCESS_ONCE(c->page);
> +			if (page) {
> +				if (flags & SO_TOTAL)
> +					x = page->objects;
> 				else if (flags & SO_OBJECTS)
> -					x = c->page->inuse;
> +					x = page->inuse;
> 				else
> 					x = 1;
>
> 				total += x;
> -				nodes[c->node] += x;
> +				nodes[node] += x;
> 			}
> 			page = c->partial;
>
> 			if (page) {
> 				x = page->pobjects;
> -                                total += x;
> -                                nodes[c->node] += x;
> +				total += x;
> +				nodes[node] += x;
> 			}
> -			per_cpu[c->node]++;
> +			per_cpu[node]++;
> 		}
> 	}
>
>
>
>

Re: [PATCH V2] slub: avoid potential NULL dereference or corruption

[Christoph Lameter]

On Wed, 23 Nov 2011, Pekka Enberg wrote:

> On Tue, 22 Nov 2011, Eric Dumazet wrote:
> > show_slab_objects() can trigger NULL dereferences or memory corruption.
> >
> > Another cpu can change its c->page to NULL or c->node to NUMA_NO_NODE
> > while we use them.
> >
> > Use ACCESS_ONCE(c->page) and ACCESS_ONCE(c->node) to make sure this
> > cannot happen.
> >
> > Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
>
> Did someone test this patch? Does it fix any of the reported issues?

It does not fix any current issues but it is safe against potential
compiler refetching of variables that we have already checked against
bad values.

I thought I acked it already?

Re: [PATCH V2] slub: avoid potential NULL dereference or corruption

[Pekka Enberg]

On Wed, 2011-11-23 at 09:17 -0600, Christoph Lameter wrote:
> On Wed, 23 Nov 2011, Pekka Enberg wrote:
> 
> > On Tue, 22 Nov 2011, Eric Dumazet wrote:
> > > show_slab_objects() can trigger NULL dereferences or memory corruption.
> > >
> > > Another cpu can change its c->page to NULL or c->node to NUMA_NO_NODE
> > > while we use them.
> > >
> > > Use ACCESS_ONCE(c->page) and ACCESS_ONCE(c->node) to make sure this
> > > cannot happen.
> > >
> > > Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> >
> > Did someone test this patch? Does it fix any of the reported issues?
> 
> It does not fix any current issues but it is safe against potential
> compiler refetching of variables that we have already checked against
> bad values.
> 
> I thought I acked it already?

Yes, you did. I'm just trying to figure out which ones are going
straight to Linus and which ones can wait for v3.3.

			Pekka

Re: [PATCH V2] slub: avoid potential NULL dereference or corruption

[David Rientjes]

On Wed, 23 Nov 2011, Pekka Enberg wrote:

> Yes, you did. I'm just trying to figure out which ones are going
> straight to Linus and which ones can wait for v3.3.
> 

I think it's 3.2-rc material since it can trigger a NULL pointer by 
reading a file from userspace, it's not limited only to debugging.

Re: [PATCH V2] slub: avoid potential NULL dereference or corruption

[Pekka Enberg]

On Wed, 23 Nov 2011, Pekka Enberg wrote:
>> Yes, you did. I'm just trying to figure out which ones are going
>> straight to Linus and which ones can wait for v3.3.

On Wed, 23 Nov 2011, David Rientjes wrote:
> I think it's 3.2-rc material since it can trigger a NULL pointer by
> reading a file from userspace, it's not limited only to debugging.

Applied, thanks!