[PATCH] mm/slab.c: check pointer slabp before using it in alloc_slabmgmt()

[PATCH] mm/slab.c: check pointer slabp before using it in alloc_slabmgmt()

[ethan.zhao]

Move the NULL check of slabp to the right place before refer its memeber in
function alloc_slabmgmt().

This bug may be introduced by rewriting of funcion kmemleak_scan_area(),
the first parameter changed from slabp to &slabp->list.

Signed-off-by: ethan.zhao <ethan.kernel@gmail.com>
---
 mm/slab.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/mm/slab.c b/mm/slab.c
index 2580db0..b6d27bc 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -2612,6 +2612,8 @@ static struct slab *alloc_slabmgmt(struct kmem_cache *cachep, void *objp,
 		/* Slab management obj is off-slab. */
 		slabp = kmem_cache_alloc_node(cachep->slabp_cache,
 					      local_flags, nodeid);
+		if (!slabp)
+			return NULL;
 		/*
 		 * If the first object in the slab is leaked (it's allocated
 		 * but no one has a reference to it), we want to make sure
@@ -2620,8 +2622,6 @@ static struct slab *alloc_slabmgmt(struct kmem_cache *cachep, void *objp,
 		 */
 		kmemleak_scan_area(&slabp->list, sizeof(struct list_head),
 				   local_flags);
-		if (!slabp)
-			return NULL;
 	} else {
 		slabp = objp + colour_off;
 		colour_off += cachep->slab_size;
-- 
1.8.3.4 (Apple Git-47)

Re: [PATCH] mm/slab.c: check pointer slabp before using it in alloc_slabmgmt()

[Christoph Lameter]

On Sun, 8 Dec 2013, ethan.zhao wrote:

> Move the NULL check of slabp to the right place before refer its memeber in
> function alloc_slabmgmt().

I am having trouble to find the code you are modifying. Which kernel
release is this? The code you are referring to has been rewritten in the
meantime or this is some other code basis.

Re: [PATCH] mm/slab.c: check pointer slabp before using it in alloc_slabmgmt()

[Ethan Zhao]

Christoph,
    Found in the latest stable release V3.12.3,  yes, changed in
3.12.4. not needed for later release anymore.

Thanks,
Ethan


On Tue, Dec 10, 2013 at 12:11 AM, Christoph Lameter <cl@linux.com> wrote:
> On Sun, 8 Dec 2013, ethan.zhao wrote:
>
>> Move the NULL check of slabp to the right place before refer its memeber in
>> function alloc_slabmgmt().
>
> I am having trouble to find the code you are modifying. Which kernel
> release is this? The code you are referring to has been rewritten in the
> meantime or this is some other code basis.
>

Re: [PATCH] mm/slab.c: check pointer slabp before using it in alloc_slabmgmt()

[Ethan Zhao]

Christoph,

     No, stable 3.12.4 and 3.12.3 need this patch.  3.13RC doesn't
need it anymore.

Thanks,
Ethan

On Tue, Dec 10, 2013 at 3:08 PM, Ethan Zhao <ethan.kernel@gmail.com> wrote:
> Christoph,
>     Found in the latest stable release V3.12.3,  yes, changed in
> 3.12.4. not needed for later release anymore.
>
> Thanks,
> Ethan
>
>
> On Tue, Dec 10, 2013 at 12:11 AM, Christoph Lameter <cl@linux.com> wrote:
>> On Sun, 8 Dec 2013, ethan.zhao wrote:
>>
>>> Move the NULL check of slabp to the right place before refer its memeber in
>>> function alloc_slabmgmt().
>>
>> I am having trouble to find the code you are modifying. Which kernel
>> release is this? The code you are referring to has been rewritten in the
>> meantime or this is some other code basis.
>>

Re: [PATCH] mm/slab.c: check pointer slabp before using it in alloc_slabmgmt()

[Christoph Lameter]

On Tue, 10 Dec 2013, Ethan Zhao wrote:

>      No, stable 3.12.4 and 3.12.3 need this patch.  3.13RC doesn't
> need it anymore.

Hmmm.. Then this commit may have fixed it:

commit 0172f779e4314639a8ed440082cfe9e3450954e8
Author: Joonsoo Kim <iamjoonsoo.kim@lge.com>
Date:   Wed Oct 30 19:04:00 2013 +0900

    slab: fix to calm down kmemleak warning

    After using struct page as slab management, we should not call
    kmemleak_scan_area(), since struct page isn't the tracking object of
    kmemleak. Without this patch and if CONFIG_DEBUG_KMEMLEAK is enabled,
    so many kmemleak warnings are printed.

    Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>
    Signed-off-by: Pekka Enberg <penberg@iki.fi>

diff --git a/mm/slab.c b/mm/slab.c
index af2db76..a8a9349 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -2531,14 +2531,6 @@ static struct freelist *alloc_slabmgmt(struct
kmem_cache *cachep,
                /* Slab management obj is off-slab. */
                freelist = kmem_cache_alloc_node(cachep->freelist_cache,
                                              local_flags, nodeid);
-               /*
-                * If the first object in the slab is leaked (it's allocated
-                * but no one has a reference to it), we want to make sure
-                * kmemleak does not treat the ->s_mem pointer as a reference
-                * to the object. Otherwise we will not report the leak.
-                */
-               kmemleak_scan_area(&page->lru, sizeof(struct list_head),
-                                  local_flags);
                if (!freelist)
                        return NULL;
        } else {

Re: [PATCH] mm/slab.c: check pointer slabp before using it in alloc_slabmgmt()

[Christoph Lameter]

On Sun, 8 Dec 2013, ethan.zhao wrote:

> Move the NULL check of slabp to the right place before refer its memeber in
> function alloc_slabmgmt().

Ok this patch is needed for 2.6.30 to 3.12

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

Re: [PATCH] mm/slab.c: check pointer slabp before using it in alloc_slabmgmt()

[Peter Hurley]

On 12/10/2013 10:35 AM, Christoph Lameter wrote:
> On Tue, 10 Dec 2013, Ethan Zhao wrote:
>
>>       No, stable 3.12.4 and 3.12.3 need this patch.  3.13RC doesn't
>> need it anymore.
>
> Hmmm.. Then this commit may have fixed it:

Cross-threaded from [Re: Slab BUG with DEBUG_* options ]

On 12/08/2013 10:00 AM, Meelis Roos wrote:
 > ldata alloc change + your second slab patch + slab debug: hang on boot
 > after
 > console [tty0] enabled, bootconsole disabled
 > (after that, I should see all the dmesg again on serial but I do not).

Meelis,

Please grab this patch below and see if it fixes your 'hang on boot'
with Christoph's other slab patch + slab debug.

Regards,
Peter Hurley

> commit 0172f779e4314639a8ed440082cfe9e3450954e8
> Author: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> Date:   Wed Oct 30 19:04:00 2013 +0900
>
>      slab: fix to calm down kmemleak warning
>
>      After using struct page as slab management, we should not call
>      kmemleak_scan_area(), since struct page isn't the tracking object of
>      kmemleak. Without this patch and if CONFIG_DEBUG_KMEMLEAK is enabled,
>      so many kmemleak warnings are printed.
>
>      Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>
>      Signed-off-by: Pekka Enberg <penberg@iki.fi>
>
> diff --git a/mm/slab.c b/mm/slab.c
> index af2db76..a8a9349 100644
> --- a/mm/slab.c
> +++ b/mm/slab.c
> @@ -2531,14 +2531,6 @@ static struct freelist *alloc_slabmgmt(struct
> kmem_cache *cachep,
>                  /* Slab management obj is off-slab. */
>                  freelist = kmem_cache_alloc_node(cachep->freelist_cache,
>                                                local_flags, nodeid);
> -               /*
> -                * If the first object in the slab is leaked (it's allocated
> -                * but no one has a reference to it), we want to make sure
> -                * kmemleak does not treat the ->s_mem pointer as a reference
> -                * to the object. Otherwise we will not report the leak.
> -                */
> -               kmemleak_scan_area(&page->lru, sizeof(struct list_head),
> -                                  local_flags);
>                  if (!freelist)
>                          return NULL;
>          } else {
>

Re: [PATCH] mm/slab.c: check pointer slabp before using it in alloc_slabmgmt()

[Meelis Roos]

> > On Tue, 10 Dec 2013, Ethan Zhao wrote:
> > 
> > >       No, stable 3.12.4 and 3.12.3 need this patch.  3.13RC doesn't
> > > need it anymore.
> > 
> > Hmmm.. Then this commit may have fixed it:
> 
> Cross-threaded from [Re: Slab BUG with DEBUG_* options ]
> 
> On 12/08/2013 10:00 AM, Meelis Roos wrote:
> > ldata alloc change + your second slab patch + slab debug: hang on boot
> > after
> > console [tty0] enabled, bootconsole disabled
> > (after that, I should see all the dmesg again on serial but I do not).
> 
> Meelis,
> 
> Please grab this patch below and see if it fixes your 'hang on boot'
> with Christoph's other slab patch + slab debug.

Tried it on top of 3.12 + second slab patch + ldata alloc change, and 
lsab debug conf. No change - still hangs on boot.

-- 
Meelis Roos (mroos@linux.ee)