[Slub allocator] There are chances that kmem_cache_cpu->freelist gets lost if the process happens to be rescheduled to a differenet cpu before the local_irq_save() completes in __slab_alloc()

[Slub allocator] There are chances that kmem_cache_cpu->freelist gets lost if the process happens to be rescheduled to a differenet cpu before the local_irq_save() completes in __slab_alloc()

[zhihua che]

Hi, everyone

        I'm reading code of version 3.1.5 and I guess I come across a
memory leak involving with slub allocating process. I image a case
like below.

        Process p0 running on cpu0 is requesting a small piece of
memory by calling slab_alloc(). It finds that the c0->freelist is null
and resorts to __slab_alloc(), where c0 is a pointer of kmem_cache_cpu
corresponding to cpu0. Unfortunately, scheduling happens right the
moment and p0 is now running on a different cpu1. P0 then disables the
local interrupts of cpu1 at the beginning of __slab_alloc() (line
2057),  and retrieves a different pointer of kmem_cache_cpu, c1,
corresponding to cpu1(line 2064). P0 continues executing to line 2116
where c1->freelist is assigned a new value while there is no any check
to it before at all!!! So I think there are chances that memory leak
could happen if scheduling happens right before line 2057.

2057 local_irq_disable();  /* scheduling happens before here */

2064 c = this_cpu_ptr(s->cpu_slab);

2114 load_freelist:
2115         VM_BUG_ON(!page->frozen);
2116         c->freelist = get_freepointer(s, object);  /* memory leak
happens here */

         I wonder if I think in a wrong way.

         Furthermore, I find in the earlier version like 2.6.38.8, the
local interrupts have been disabled in slab_alloc() before the control
enters the __slab_alloc(), so I think the memory leak is not present
in the earlier versions.

Re: [Slub allocator] There are chances that kmem_cache_cpu->freelist gets lost if the process happens to be rescheduled to a differenet cpu before the local_irq_save() completes in __slab_alloc()

[Eric Dumazet]

Le lundi 12 décembre 2011 à 22:28 +0800, zhihua che a écrit :
> Hi, everyone
> 
>         I'm reading code of version 3.1.5 and I guess I come across a
> memory leak involving with slub allocating process. I image a case
> like below.
> 
>         Process p0 running on cpu0 is requesting a small piece of
> memory by calling slab_alloc(). It finds that the c0->freelist is null
> and resorts to __slab_alloc(), where c0 is a pointer of kmem_cache_cpu
> corresponding to cpu0. Unfortunately, scheduling happens right the
> moment and p0 is now running on a different cpu1. P0 then disables the
> local interrupts of cpu1 at the beginning of __slab_alloc() (line
> 2057),  and retrieves a different pointer of kmem_cache_cpu, c1,
> corresponding to cpu1(line 2064). P0 continues executing to line 2116
> where c1->freelist is assigned a new value while there is no any check
> to it before at all!!! So I think there are chances that memory leak
> could happen if scheduling happens right before line 2057.
> 
> 2057 local_irq_disable();  /* scheduling happens before here */
> 
> 2064 c = this_cpu_ptr(s->cpu_slab);
> 
> 2114 load_freelist:
> 2115         VM_BUG_ON(!page->frozen);
> 2116         c->freelist = get_freepointer(s, object);  /* memory leak
> happens here */
> 
>          I wonder if I think in a wrong way.
> 
>          Furthermore, I find in the earlier version like 2.6.38.8, the
> local interrupts have been disabled in slab_alloc() before the control
> enters the __slab_alloc(), so I think the memory leak is not present
> in the earlier versions.
> --

Hi

CC Christoph Lameter on this one

Another bug from this lockless stuff I am afraid.

We miss a deactivate_slab() call or just deliver object if c->freelist
is not null (and NUMA node matches)

Untested patch :

diff --git a/mm/slub.c b/mm/slub.c
index ed3334d..923d238 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -2166,6 +2166,12 @@ redo:
 		goto new_slab;
 	}
 
+#ifdef CONFIG_PREEMPT
+	object = c->freelist;
+	if (object)
+		goto load_freelist;
+#endif
+
 	stat(s, ALLOC_SLOWPATH);
 
 	do {

Re: [Slub allocator] There are chances that kmem_cache_cpu->freelist gets lost if the process happens to be rescheduled to a differenet cpu before the local_irq_save() completes in __slab_alloc()

[Christoph Lameter]

On Mon, 12 Dec 2011, Eric Dumazet wrote:

> We miss a deactivate_slab() call or just deliver object if c->freelist
> is not null (and NUMA node matches)
>
> Untested patch :
>
> diff --git a/mm/slub.c b/mm/slub.c
> index ed3334d..923d238 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -2166,6 +2166,12 @@ redo:
>  		goto new_slab;
>  	}
>
> +#ifdef CONFIG_PREEMPT
> +	object = c->freelist;
> +	if (object)
> +		goto load_freelist;
> +#endif
> +
>  	stat(s, ALLOC_SLOWPATH);
>
>  	do {

Correct. Issue was introduced in 2.6.39.

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

Re: [Slub allocator] There are chances that kmem_cache_cpu->freelist gets lost if the process happens to be rescheduled to a differenet cpu before the local_irq_save() completes in __slab_alloc()

[Eric Dumazet]

Le lundi 12 décembre 2011 à 09:50 -0600, Christoph Lameter a écrit :

> Correct. Issue was introduced in 2.6.39.
> 
> Acked-by: Christoph Lameter <cl@linux.com>
> 

Indeed, I reproduced the leak with hackbench and lot of threads.

Thanks

[PATCH] slub: fix a possible memleak in __slab_alloc()

Zhihua Che reported a possible memleak in slub allocator on
CONFIG_PREEMPT=y builds.

It is possible current thread migrates right before disabling irqs in
__slab_alloc(). We must check again c->freelist, and perform a normal
allocation instead of scratching c->freelist.

Many thanks to Zhihua Che for spotting this bug, introduced in 2.6.39

Reported-by: zhihua che <zhihua.che@gmail.com>
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
Acked-by: Christoph Lameter <cl@linux.com>
CC: Pekka Enberg <penberg@cs.helsinki.fi>
CC: stable@vger.kernel.org
---
 mm/slub.c |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/mm/slub.c b/mm/slub.c
index ed3334d..923d238 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -2166,6 +2166,12 @@ redo:
 		goto new_slab;
 	}
 
+#ifdef CONFIG_PREEMPT
+	object = c->freelist;
+	if (object)
+		goto load_freelist;
+#endif
+
 	stat(s, ALLOC_SLOWPATH);
 
 	do {

Re: [Slub allocator] There are chances that kmem_cache_cpu->freelist gets lost if the process happens to be rescheduled to a differenet cpu before the local_irq_save() completes in __slab_alloc()

[Christoph Lameter]

On Mon, 12 Dec 2011, Eric Dumazet wrote:

> Indeed, I reproduced the leak with hackbench and lot of threads.

Ran it here and did not see anything. But obviously this is an issue.
Strange that no one noticed earlier. Usage of CONFIG_PREEMPT must not be
that frequent.

Re: [Slub allocator] There are chances that kmem_cache_cpu->freelist gets lost if the process happens to be rescheduled to a differenet cpu before the local_irq_save() completes in __slab_alloc()

[Eric Dumazet]

Le lundi 12 décembre 2011 à 12:53 -0600, Christoph Lameter a écrit :
> On Mon, 12 Dec 2011, Eric Dumazet wrote:
> 
> > Indeed, I reproduced the leak with hackbench and lot of threads.
> 
> Ran it here and did not see anything. But obviously this is an issue.
> Strange that no one noticed earlier. Usage of CONFIG_PREEMPT must not be
> that frequent.
> 

Yes, probably :(

All I did was using hackbench with 4000 threads on my 16 cpu machine,

while :; do hackbench 100 process 2000 ; done

I got about one leak (freelist chain, so several objects leaked) per
second.

Re: [Slub allocator] There are chances that kmem_cache_cpu->freelist gets lost if the process happens to be rescheduled to a differenet cpu before the local_irq_save() completes in __slab_alloc()

[Eric Dumazet]

Le lundi 12 décembre 2011 à 18:39 +0100, Eric Dumazet a écrit :
> Le lundi 12 décembre 2011 à 09:50 -0600, Christoph Lameter a écrit :
> 
> > Correct. Issue was introduced in 2.6.39.
> > 
> > Acked-by: Christoph Lameter <cl@linux.com>
> > 
> 
> Indeed, I reproduced the leak with hackbench and lot of threads.
> 
> Thanks
> 
> [PATCH] slub: fix a possible memleak in __slab_alloc()
> 
> Zhihua Che reported a possible memleak in slub allocator on
> CONFIG_PREEMPT=y builds.
> 
> It is possible current thread migrates right before disabling irqs in
> __slab_alloc(). We must check again c->freelist, and perform a normal
> allocation instead of scratching c->freelist.
> 
> Many thanks to Zhihua Che for spotting this bug, introduced in 2.6.39
> 
> Reported-by: zhihua che <zhihua.che@gmail.com>
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> Acked-by: Christoph Lameter <cl@linux.com>
> CC: Pekka Enberg <penberg@cs.helsinki.fi>
> CC: stable@vger.kernel.org
> ---
>  mm/slub.c |    6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/mm/slub.c b/mm/slub.c
> index ed3334d..923d238 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -2166,6 +2166,12 @@ redo:
>  		goto new_slab;
>  	}
>  
> +#ifdef CONFIG_PREEMPT
> +	object = c->freelist;
> +	if (object)
> +		goto load_freelist;
> +#endif
> +
>  	stat(s, ALLOC_SLOWPATH);
>  
>  	do {
> 

Thinking again about the issue, I believe its not a CONFIG_PREEMPT only
one.

We can be interrupted and the IRQ handler can free an object an populate
freelist too. So the check must always be done.

Thanks

[PATCH v2] slub: fix a possible memleak in __slab_alloc()

Zhihua Che reported a possible memleak in slub allocator on
CONFIG_PREEMPT=y builds.

It is possible current thread migrates right before disabling irqs in
__slab_alloc(). We must check again c->freelist, and perform a normal
allocation instead of scratching c->freelist.

Many thanks to Zhihua Che for spotting this bug, introduced in 2.6.39

V2: Its also possible an IRQ freed one (or several) object(s) and
populated c->freelist, so its not a CONFIG_PREEMPT only problem.

Reported-by: zhihua che <zhihua.che@gmail.com>
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
CC: Christoph Lameter <cl@linux.com>
CC: Pekka Enberg <penberg@cs.helsinki.fi>
---
 mm/slub.c |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/mm/slub.c b/mm/slub.c
index ed3334d..1a919f0 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -2166,6 +2166,11 @@ redo:
 		goto new_slab;
 	}
 
+	/* must check again c->freelist in case of cpu migration or IRQ */
+	object = c->freelist;
+	if (object)
+		goto load_freelist;
+
 	stat(s, ALLOC_SLOWPATH);
 
 	do {

Re: [Slub allocator] There are chances that kmem_cache_cpu->freelist gets lost if the process happens to be rescheduled to a differenet cpu before the local_irq_save() completes in __slab_alloc()

[zhihua che]

2011/12/13 Eric Dumazet <eric.dumazet@gmail.com>:
> Le lundi 12 décembre 2011 à 18:39 +0100, Eric Dumazet a écrit :
>> Le lundi 12 décembre 2011 à 09:50 -0600, Christoph Lameter a écrit :
>>
>> > Correct. Issue was introduced in 2.6.39.
>> >
>> > Acked-by: Christoph Lameter <cl@linux.com>
>> >
>>
>> Indeed, I reproduced the leak with hackbench and lot of threads.
>>
>> Thanks
>>
>> [PATCH] slub: fix a possible memleak in __slab_alloc()
>>
>> Zhihua Che reported a possible memleak in slub allocator on
>> CONFIG_PREEMPT=y builds.
>>
>> It is possible current thread migrates right before disabling irqs in
>> __slab_alloc(). We must check again c->freelist, and perform a normal
>> allocation instead of scratching c->freelist.
>>
>> Many thanks to Zhihua Che for spotting this bug, introduced in 2.6.39
>>
>> Reported-by: zhihua che <zhihua.che@gmail.com>
>> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
>> Acked-by: Christoph Lameter <cl@linux.com>
>> CC: Pekka Enberg <penberg@cs.helsinki.fi>
>> CC: stable@vger.kernel.org
>> ---
>>  mm/slub.c |    6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/mm/slub.c b/mm/slub.c
>> index ed3334d..923d238 100644
>> --- a/mm/slub.c
>> +++ b/mm/slub.c
>> @@ -2166,6 +2166,12 @@ redo:
>>               goto new_slab;
>>       }
>>
>> +#ifdef CONFIG_PREEMPT
>> +     object = c->freelist;
>> +     if (object)
>> +             goto load_freelist;
>> +#endif
>> +
>>       stat(s, ALLOC_SLOWPATH);
>>
>>       do {
>>
>
> Thinking again about the issue, I believe its not a CONFIG_PREEMPT only
> one.
>
> We can be interrupted and the IRQ handler can free an object an populate
> freelist too. So the check must always be done.
>
> Thanks
>
> [PATCH v2] slub: fix a possible memleak in __slab_alloc()
>
> Zhihua Che reported a possible memleak in slub allocator on
> CONFIG_PREEMPT=y builds.
>
> It is possible current thread migrates right before disabling irqs in
> __slab_alloc(). We must check again c->freelist, and perform a normal
> allocation instead of scratching c->freelist.
>
> Many thanks to Zhihua Che for spotting this bug, introduced in 2.6.39
>
> V2: Its also possible an IRQ freed one (or several) object(s) and
> populated c->freelist, so its not a CONFIG_PREEMPT only problem.
>
> Reported-by: zhihua che <zhihua.che@gmail.com>
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> CC: Christoph Lameter <cl@linux.com>
> CC: Pekka Enberg <penberg@cs.helsinki.fi>
> ---
>  mm/slub.c |    5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/mm/slub.c b/mm/slub.c
> index ed3334d..1a919f0 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -2166,6 +2166,11 @@ redo:
>                goto new_slab;
>        }
>
> +       /* must check again c->freelist in case of cpu migration or IRQ */
> +       object = c->freelist;
> +       if (object)
> +               goto load_freelist;
> +
>        stat(s, ALLOC_SLOWPATH);
>
>        do {
>
>

I'm glad and excited I can help. :-)

Re: [Slub allocator] There are chances that kmem_cache_cpu->freelist gets lost if the process happens to be rescheduled to a differenet cpu before the local_irq_save() completes in __slab_alloc()

[Christoph Lameter]

On Tue, 13 Dec 2011, Eric Dumazet wrote:

> We can be interrupted and the IRQ handler can free an object an populate
> freelist too. So the check must always be done.

True. So a small memory leak that remained undetected. Hmm.... Wonder how
one can create some diagnostics to allow detection of this in the future.

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

Re: [Slub allocator] There are chances that kmem_cache_cpu->freelist gets lost if the process happens to be rescheduled to a differenet cpu before the local_irq_save() completes in __slab_alloc()

[Pekka Enberg]

On Tue, 13 Dec 2011, Eric Dumazet wrote:
>> We can be interrupted and the IRQ handler can free an object an populate
>> freelist too. So the check must always be done.

On Tue, 13 Dec 2011, Christoph Lameter wrote:
> True. So a small memory leak that remained undetected. Hmm.... Wonder how
> one can create some diagnostics to allow detection of this in the future.
>
> Acked-by: Christoph Lameter <cl@linux.com>

I queued this for 3.3. Thanks guys!

 			Pekka