Fix 2.5.3pre reiserfs BUG() at boot time

kmem_cache_create() ("reiserfs_inode_cache") but kmem_cache_create() only allows 19 character names and BUG()s for longer names.

please apply this too then.

I would prefer instead just avoiding the copy altogether, and just save the name pointer - with no length restrictions.

Right now the code has the comment

   /* Copy name over so we don't have problems with unloaded modules */

but that was written before "kmem_cache_destroy()" existed, and we should long ago have fixed any modules that don't properly destroy their caches when they exit (and yes, I know the difference between "should" and "did", but that's not an excuse for a bad interface).

Quoted 2 lines · John Levon

please apply this too then.

I would prefer instead just avoiding the copy altogether, and just save the name pointer - with no length restrictions.

Right now the code has the comment

   /* Copy name over so we don't have problems with unloaded modules */

but that was written before "kmem_cache_destroy()" existed, and we should long ago have fixed any modules that don't properly destroy their caches when they exit (and yes, I know the difference between "should" and "did", but that's not an excuse for a bad interface).

(quoted diff) +0 -0

@@ -810,11 +832,8 @@
 		struct list_head *p;
 
 		list_for_each(p, &cache_chain) {

So, what exactly does the above do now (hint: p and pc are both local so they cannot be referenced anywhere else)? It used to check that you weren't trying to add two caches with the same name. This isn't possible with caches from broken modules anymore as they have no name.

When calling kmem_cache_destroy() on a non-empty slab we should just malloc some memory with the old cache name + "_leaked" for the name pointer. At least then we have a sane chance of figuring out what caused the problem, instead of having a bunch of "broken" entries in the table, and remove the above "broken" check entirely (we will always have a name).

Quoted 5 lines · Andreas Dilger

When calling kmem_cache_destroy() on a non-empty slab we should just malloc some memory with the old cache name + "_leaked" for the name pointer. At least then we have a sane chance of figuring out what caused the problem, instead of having a bunch of "broken" entries in the table, and remove the above "broken" check entirely (we will always have a name).

I don't like this because it complicates the code too much. "broken" should be enough to debug it.

Well, the code says "this should never happen" ;-)

But anyway, it is probably a temporary memory allocation failure. The code handles this case.

Friday 25 January 2002 23:59, Jeff Garzik scrisse:

Quoted 4 lines

Well, the code says "this should never happen" ;-)

But anyway, it is probably a temporary memory allocation failure. The code handles this case.

Yes, but I think that isn't normal to have to do a cold reboot to have the machine again working on the network. It is, maybe that the code doesn't handle so well this case. Do you suggest a kernel upgrade?

Quoted 4 lines · Andreas Dilger

So, what exactly does the above do now (hint: p and pc are both local so they cannot be referenced anywhere else)? It used to check that you weren't trying to add two caches with the same name. This isn't possible with caches from broken modules anymore as they have no name.

I have fixed the loop now to check for names again.

Quoted 5 lines

When calling kmem_cache_destroy() on a non-empty slab we should just malloc some memory with the old cache name + "_leaked" for the name pointer. At least then we have a sane chance of figuring out what caused the problem, instead of having a bunch of "broken" entries in the table, and remove the above "broken" check entirely (we will always have a name).

I don't like this because it complicates the code too much. "broken" should be enough to debug it.

New patch appended. Linus please apply if you didn't already.

-Andi

Index: mm/slab.c
===================================================================
RCS file: /cvs/linux/mm/slab.c,v
retrieving revision 1.66
diff -u -u -r1.66 slab.c

slab.c 2002/01/25 22:14:40 renamed from slab.c 2002/01/08 16:00:20 1.66 +43 -16

@@ -186,8 +186,6 @@
  * manages a cache.
  */
 
-#define CACHE_NAMELEN  20      /* max name length for a slab cache */
-
 struct kmem_cache_s {
 /* 1) each alloc & free */
        /* full, partial first, then free */

@@ -225,7 +223,7 @@
        unsigned long           failures;
 
 /* 3) cache creation/removal */
-       char                    name[CACHE_NAMELEN];
+       const char                      *name;
        struct list_head        next;
 #ifdef CONFIG_SMP
 /* 4) per-cpu data */

@@ -335,6 +333,7 @@
        kmem_cache_t    *cs_dmacachep;
 } cache_sizes_t;
 
+/* These are the default caches for kmalloc. Custom caches can have other sizes. */
 static cache_sizes_t cache_sizes[] = {
 #if PAGE_SIZE == 4096
        {    32,        NULL, NULL},

@@ -353,6 +352,26 @@
        {131072,        NULL, NULL},
        {     0,        NULL, NULL}
 };
+/* Must match cache_sizes above. Out of line to keep cache footprint low. */
+#define CN(x) { x, x ## "(DMA)" }
+static char cache_names[][2][18] = {
+#if PAGE_SIZE == 4096
+       CN("size-32"),
+#endif
+       CN("size-64"),
+       CN("size-128"),
+       CN("size-256"),
+       CN("size-512"),
+       CN("size-1024"),
+       CN("size-2048"),
+       CN("size-4096"),
+       CN("size-8192"),
+       CN("size-16384"),
+       CN("size-32768"),
+       CN("size-65536"),
+       CN("size-131072")
+};
+#undef CN
 
 /* internal cache of cache description objs */
 static kmem_cache_t cache_cache = {

@@ -437,7 +456,6 @@
 void __init kmem_cache_sizes_init(void)
 {
        cache_sizes_t *sizes = cache_sizes;
-       char name[20];
        /*
         * Fragmentation resistance on low memory - only use bigger
         * page orders on machines with more than 32MB of memory.

@@ -450,9 +468,9 @@
                 * eliminates "false sharing".
                 * Note for systems short on memory removing the alignment will
                 * allow tighter packing of the smaller caches. */
-               sprintf(name,"size-%Zd",sizes->cs_size);
                if (!(sizes->cs_cachep =
-                       kmem_cache_create(name, sizes->cs_size,
+                       kmem_cache_create(cache_names[sizes-cache_sizes][0],
+                                         sizes->cs_size,
                                        0, SLAB_HWCACHE_ALIGN, NULL, NULL))) {
                        BUG();
                }

@@ -462,9 +480,10 @@
                        offslab_limit = sizes->cs_size-sizeof(slab_t);
                        offslab_limit /= 2;
                }
-               sprintf(name, "size-%Zd(DMA)",sizes->cs_size);
-               sizes->cs_dmacachep = kmem_cache_create(name, sizes->cs_size, 0,
-                             SLAB_CACHE_DMA|SLAB_HWCACHE_ALIGN, NULL, NULL);
+               sizes->cs_dmacachep = kmem_cache_create(
+                   cache_names[sizes-cache_sizes][1],
+                       sizes->cs_size, 0,
+                       SLAB_CACHE_DMA|SLAB_HWCACHE_ALIGN, NULL, NULL);
                if (!sizes->cs_dmacachep)
                        BUG();
                sizes++;

@@ -604,6 +623,11 @@
  * Cannot be called within a int, but can be interrupted.
  * The @ctor is run when new pages are allocated by the cache
  * and the @dtor is run before the pages are handed back.
+ *
+ * @name must be valid until the cache is destroyed. This implies that
+ * the module calling this has to destroy the cache before getting
+ * unloaded.
+ *
  * The flags are
  *
  * %SLAB_POISON - Poison the slab with a known test pattern (a5a5a5a5)

@@ -632,7 +656,6 @@
         * Sanity checks... these are all serious usage bugs.
         */
        if ((!name) ||
-               ((strlen(name) >= CACHE_NAMELEN - 1)) ||
                in_interrupt() ||
                (size < BYTES_PER_WORD) ||
                (size > (1<<MAX_OBJ_ORDER)*PAGE_SIZE) ||

@@ -797,8 +820,7 @@
                cachep->slabp_cache = kmem_find_general_cachep(slab_size,0);
        cachep->ctor = ctor;
        cachep->dtor = dtor;
-       /* Copy name over so we don't have problems with unloaded modules */
-       strcpy(cachep->name, name);
+       cachep->name = name;
 
 #ifdef CONFIG_SMP
        if (g_cpucache_up)

@@ -811,10 +833,11 @@
 
                list_for_each(p, &cache_chain) {
                        kmem_cache_t *pc = list_entry(p, kmem_cache_t, next);
-
-                       /* The name field is constant - no lock needed. */
-                       if (!strcmp(pc->name, name))
-                               BUG();
+                       char tmp;
+                       if (get_user(tmp,pc->name))
+                               continue;
+                       if (!strcmp(pc->name,name))
+                               BUG();
                }
        }
 

@@ -1878,6 +1901,7 @@
                unsigned long   num_objs;
                unsigned long   active_slabs = 0;
                unsigned long   num_slabs;
+               const char *name;
                cachep = list_entry(p, kmem_cache_t, next);
 
                spin_lock_irq(&cachep->spinlock);

@@ -1906,8 +1930,15 @@
                num_slabs+=active_slabs;
                num_objs = num_slabs*cachep->num;
 
+               name = cachep->name;
+               {
+               char tmp;

2.5.3-pre5 + this patch still can't boot my system. I haven't had
time to copy down oops at boot, will do if needed.

Quoted 3 lines · Alessandro Suardi

2.5.3-pre5 + this patch still can't boot my system. I haven't had
time to copy down oops at boot, will do if needed.

Please do. I cannot see anything in the patch that should prevent bootup though, so I would also recommend a make clean and recompile first just to make sure it isn't a broken build.

Quoted 5 lines · Andi Kleen

Quoted 3 lines · Alessandro Suardi

2.5.3-pre5 + this patch still can't boot my system. I haven't had
time to copy down oops at boot, will do if needed.

Quoted 3 lines

Please do. I cannot see anything in the patch that should prevent bootup though, so I would also recommend a make clean and recompile first just to make sure it isn't a broken build.

Probably the kmem_cache_create 'name too long' bug that Viro pointed out to me. fs/reiserfs/super.c:init_inodecache(). Change the name passed to kmem_cache_create to something shorter.

Quoted 10 lines · Jens Axboe

Quoted 8 lines · Andi Kleen

Quoted 3 lines · Alessandro Suardi

2.5.3-pre5 + this patch still can't boot my system. I haven't had
time to copy down oops at boot, will do if needed.

Please do. I cannot see anything in the patch that should prevent bootup though, so I would also recommend a make clean and recompile first just to make sure it isn't a broken build.

Quoted 3 lines

Probably the kmem_cache_create 'name too long' bug that Viro pointed out to me. fs/reiserfs/super.c:init_inodecache(). Change the name passed to kmem_cache_create to something shorter.

The patch he tried removed the check of the name length ;)

Quoted 3 lines · Alessandro Suardi

2.5.3-pre5 + this patch still can't boot my system. I haven't had
time to copy down oops at boot, will do if needed.

Please do. I cannot see anything in the patch that should prevent bootup though, so I would also recommend a make clean and recompile first just to make sure it isn't a broken build.