2008-03-25 17:49:50

by Daniel Yeisley

[permalink] [raw]
Subject: [PATCH] list_add corruption in slab.c

I've been seeing list_add corruption in slab.c on the ES7000 since the
2.6.24.1 kernel. There are several places where the initkmem_list3
array is access by [somevalue + node]. This also needs to be done in
kmem_cache_init().

Signed-off-by: Dan Yeisley <[email protected]>

---
diff -Naur linux-2.6.25-rc5/mm/slab.c linux-2.6.25-rc5-new/mm/slab.c
--- linux-2.6.25-rc5/mm/slab.c 2008-03-10 01:22:27.000000000 -0400
+++ linux-2.6.25-rc5-new/mm/slab.c 2008-03-20 13:59:24.000000000 -0400
@@ -1481,7 +1481,7 @@
list_add(&cache_cache.next, &cache_chain);
cache_cache.colour_off = cache_line_size();
cache_cache.array[smp_processor_id()] = &initarray_cache.cache;
- cache_cache.nodelists[node] = &initkmem_list3[CACHE_CACHE];
+ cache_cache.nodelists[node] = &initkmem_list3[CACHE_CACHE + node];

/*
* struct kmem_cache size depends on nr_node_ids, which


2008-03-25 18:45:43

by Pekka Enberg

[permalink] [raw]
Subject: Re: [PATCH] list_add corruption in slab.c

Hi Daniel,

On Tue, Mar 25, 2008 at 6:57 PM, Daniel Yeisley <[email protected]> wrote:
> I've been seeing list_add corruption in slab.c on the ES7000 since the
> 2.6.24.1 kernel. There are several places where the initkmem_list3
> array is access by [somevalue + node]. This also needs to be done in
> kmem_cache_init().
>
> Signed-off-by: Dan Yeisley <[email protected]>
>
> ---
> diff -Naur linux-2.6.25-rc5/mm/slab.c linux-2.6.25-rc5-new/mm/slab.c
> --- linux-2.6.25-rc5/mm/slab.c 2008-03-10 01:22:27.000000000 -0400
> +++ linux-2.6.25-rc5-new/mm/slab.c 2008-03-20 13:59:24.000000000 -0400
> @@ -1481,7 +1481,7 @@
> list_add(&cache_cache.next, &cache_chain);
> cache_cache.colour_off = cache_line_size();
> cache_cache.array[smp_processor_id()] = &initarray_cache.cache;
> - cache_cache.nodelists[node] = &initkmem_list3[CACHE_CACHE];
> + cache_cache.nodelists[node] = &initkmem_list3[CACHE_CACHE + node];

Good catch! You'd need to fix up the use of initkmem_list3 farther
down in kmem_init_cache():

> /* 5) Replace the bootstrap kmem_list3's */
> {
> int nid;
>
> for_each_online_node(nid) {
> init_list(&cache_cache, &initkmem_list3[CACHE_CACHE], nid);

Care to send a tested patch that fixes that as well?

Pekka

2008-03-25 21:03:07

by Daniel Yeisley

[permalink] [raw]
Subject: Re: [PATCH] list_add corruption in slab.c

On Tue, 2008-03-25 at 20:45 +0200, Pekka Enberg wrote:
> Hi Daniel,
>
> On Tue, Mar 25, 2008 at 6:57 PM, Daniel Yeisley <[email protected]> wrote:
> > I've been seeing list_add corruption in slab.c on the ES7000 since the
> > 2.6.24.1 kernel. There are several places where the initkmem_list3
> > array is access by [somevalue + node]. This also needs to be done in
> > kmem_cache_init().
> >
> > Signed-off-by: Dan Yeisley <[email protected]>
> >
> > ---
> > diff -Naur linux-2.6.25-rc5/mm/slab.c linux-2.6.25-rc5-new/mm/slab.c
> > --- linux-2.6.25-rc5/mm/slab.c 2008-03-10 01:22:27.000000000 -0400
> > +++ linux-2.6.25-rc5-new/mm/slab.c 2008-03-20 13:59:24.000000000 -0400
> > @@ -1481,7 +1481,7 @@
> > list_add(&cache_cache.next, &cache_chain);
> > cache_cache.colour_off = cache_line_size();
> > cache_cache.array[smp_processor_id()] = &initarray_cache.cache;
> > - cache_cache.nodelists[node] = &initkmem_list3[CACHE_CACHE];
> > + cache_cache.nodelists[node] = &initkmem_list3[CACHE_CACHE + node];
>
> Good catch! You'd need to fix up the use of initkmem_list3 farther
> down in kmem_init_cache():
>
> > /* 5) Replace the bootstrap kmem_list3's */
> > {
> > int nid;
> >
> > for_each_online_node(nid) {
> > init_list(&cache_cache, &initkmem_list3[CACHE_CACHE], nid);
>
> Care to send a tested patch that fixes that as well?
>
> Pekka

I actually saw that initkmem_list reference, but didn't change it since
my original patch fixed my list corruption. Anyway, I made the changed
and tested it. The system booted fine.

Signed-off-by: Dan Yeisley <[email protected]>

---
diff -Nuar linux-2.6.25-rc6/mm/slab.c linux-2.6.25-rc6-new/mm/slab.c
--- linux-2.6.25-rc6/mm/slab.c 2008-03-25 15:39:07.000000000 -0400
+++ linux-2.6.25-rc6-new/mm/slab.c 2008-03-25 15:13:01.000000000 -0400
@@ -1481,7 +1481,7 @@
list_add(&cache_cache.next, &cache_chain);
cache_cache.colour_off = cache_line_size();
cache_cache.array[smp_processor_id()] = &initarray_cache.cache;
- cache_cache.nodelists[node] = &initkmem_list3[CACHE_CACHE];
+ cache_cache.nodelists[node] = &initkmem_list3[CACHE_CACHE + node];

/*
* struct kmem_cache size depends on nr_node_ids, which
@@ -1602,7 +1602,7 @@
int nid;

for_each_online_node(nid) {
- init_list(&cache_cache, &initkmem_list3[CACHE_CACHE], nid);
+ init_list(&cache_cache, &initkmem_list3[CACHE_CACHE + nid], nid);

init_list(malloc_sizes[INDEX_AC].cs_cachep,
&initkmem_list3[SIZE_AC + nid], nid);

2008-03-25 21:15:27

by Pekka Enberg

[permalink] [raw]
Subject: Re: [PATCH] list_add corruption in slab.c

Hi Daniel,

Daniel Yeisley wrote:
> I actually saw that initkmem_list reference, but didn't change it since
> my original patch fixed my list corruption. Anyway, I made the changed
> and tested it. The system booted fine.

Yeah, but the second change is needed; otherwise we forget to fix up
some of the bootstrap caches.

> Signed-off-by: Dan Yeisley <[email protected]>

Reviewed-by: Pekka Enberg <[email protected]>

Mel, as this change is related to the memoryless node fix that went in
2.6.24, any chance you'd give this patch a spin on your machines so we
can get the fix in 2.6.25 and 2.6.24-stable?

Pekka

2008-03-25 21:27:45

by Oliver Pinter

[permalink] [raw]
Subject: Re: [PATCH] list_add corruption in slab.c

Hi Pekka
this patch for 2.6.22?(http://repo.or.cz/w/linux-2.6.22.y-op.git)
--8<-- /* 1) create the cache_cache */ INIT_LIST_HEAD(&cache_chain); list_add(&cache_cache.next, &cache_chain); cache_cache.colour_off = cache_line_size(); cache_cache.array[smp_processor_id()] = &initarray_cache.cache; cache_cache.nodelists[node] = &initkmem_list3[CACHE_CACHE];-->8--from 2.6.22's slab.c
On 3/25/08, Pekka Enberg <[email protected]> wrote:> Hi Daniel,>> Daniel Yeisley wrote:> > I actually saw that initkmem_list reference, but didn't change it since> > my original patch fixed my list corruption. Anyway, I made the changed> > and tested it. The system booted fine.>> Yeah, but the second change is needed; otherwise we forget to fix up> some of the bootstrap caches.>> > Signed-off-by: Dan Yeisley <[email protected]>>> Reviewed-by: Pekka Enberg <[email protected]>>> Mel, as this change is related to the memoryless node fix that went in> 2.6.24, any chance you'd give this patch a spin on your machines so we> can get the fix in 2.6.25 and 2.6.24-stable?>> Pekka> --> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in> the body of a message to [email protected]> More majordomo info at http://vger.kernel.org/majordomo-info.html> Please read the FAQ at http://www.tux.org/lkml/>

-- Thanks,Oliver????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?

2008-03-25 21:41:23

by Pekka Enberg

[permalink] [raw]
Subject: Re: [PATCH] list_add corruption in slab.c

Hi Oliver,

On Tue, Mar 25, 2008 at 11:27 PM, Oliver Pinter <[email protected]> wrote:
> this patch for 2.6.22?

No. It fixes the following commit which is not in nor is it required for 2.6.22:

http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=556a169dab38b5100df6f4a45b655dddd3db94c1

2008-03-25 21:46:37

by Oliver Pinter

[permalink] [raw]
Subject: Re: [PATCH] list_add corruption in slab.c

thanks for the fast answer

On 3/25/08, Pekka Enberg <[email protected]> wrote:
> Hi Oliver,
>
> On Tue, Mar 25, 2008 at 11:27 PM, Oliver Pinter <[email protected]>
> wrote:
> > this patch for 2.6.22?
>
> No. It fixes the following commit which is not in nor is it required for
> 2.6.22:
>
>
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=556a169dab38b5100df6f4a45b655dddd3db94c1
>


--
Thanks,
Oliver

2008-03-26 14:16:44

by Mel Gorman

[permalink] [raw]
Subject: Re: [PATCH] list_add corruption in slab.c

On (25/03/08 23:13), Pekka Enberg didst pronounce:
> Hi Daniel,
>
> Daniel Yeisley wrote:
> >I actually saw that initkmem_list reference, but didn't change it since
> >my original patch fixed my list corruption. Anyway, I made the changed
> >and tested it. The system booted fine.
>
> Yeah, but the second change is needed; otherwise we forget to fix up
> some of the bootstrap caches.
>
> > ???Signed-off-by: Dan Yeisley <[email protected]>
>
> Reviewed-by: Pekka Enberg <[email protected]>
>
> Mel, as this change is related to the memoryless node fix that went in
> 2.6.24, any chance you'd give this patch a spin on your machines so we
> can get the fix in 2.6.25 and 2.6.24-stable?
>

Of course. Tested against 2.6.25-rc5 on normal and memoryless machines
running just kernbench. So far, I have seen no problems.

--
Mel Gorman
Part-time Phd Student Linux Technology Center
University of Limerick IBM Dublin Software Lab