[PATCH] slab.c: fix offslab_limit bug

mm/slab.c's offlab_limit logic is totally broken.

Firstly, "offslab_limit" is a global variable while it should either be
calculated in situ or should be passed in as a parameter.

Secondly, the more serious problem with it is that the condition for
calculating it:

               if (!(OFF_SLAB(sizes->cs_cachep))) {
                       offslab_limit = sizes->cs_size - sizeof(struct slab);
                       offslab_limit /= sizeof(kmem_bufctl_t);

is in total disconnect with the condition that makes use of it:

               /* More than offslab_limit objects will cause problems */
               if ((flags & CFLGS_OFF_SLAB) && num > offslab_limit)
                       break;

but due to offslab_limit being a global variable this breakage was
hidden.

Up until lockdep came along and perturbed the slab sizes sufficiently so
that the first off-slab cache would still see a (non-calculated) zero
value for offslab_limit and would panic with:

  kmem_cache_create: couldn't create cache size-512.

  Call Trace:
   [<ffffffff8020a5b9>] show_trace+0x96/0x1c8
   [<ffffffff8020a8f0>] dump_stack+0x13/0x15
   [<ffffffff8022994f>] panic+0x39/0x21a
   [<ffffffff80270814>] kmem_cache_create+0x5a0/0x5d0
   [<ffffffff80aced62>] kmem_cache_init+0x193/0x379
   [<ffffffff80abf779>] start_kernel+0x17f/0x218
   [<ffffffff80abf263>] _sinittext+0x263/0x26a

  Kernel panic - not syncing: kmem_cache_create(): failed to create slab `size-512'

Paolo Ornati's config on x86_64 managed to trigger it.

The fix is to move the calculation to the place that makes use of it.
This also makes slab.o 54 bytes smaller.

Btw., the check itself is quite silly. Its intention is to test whether
the number of objects per slab would be higher than the number of slab
control pointers possible. In theory it could be triggered: if someone
tried to allocate 4-byte objects cache and explicitly requested with
CFLGS_OFF_SLAB. So i kept the check.

Out of historic interest i checked how old this bug was and it's
ancient, 10 years old! It is the oldest hidden and then truly triggering
bugs i ever saw being fixed in the kernel!

Signed-off-by: Ingo Molnar <mingo@elte.hu>
Signed-off-by: Linus Torvalds <torvalds@osdl.org>

diff --git a/mm/slab.c b/mm/slab.c
index d31a06bfbea5c78998e3c68a5a484b98b3e4579f..f1b644eb39d816b3865400caf8d589d458756a17 100644 (file)
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -207,11 +207,6 @@ typedef unsigned int kmem_bufctl_t;
 #define        BUFCTL_ACTIVE   (((kmem_bufctl_t)(~0U))-2)
 #define        SLAB_LIMIT      (((kmem_bufctl_t)(~0U))-3)
 
-/* Max number of objs-per-slab for caches which use off-slab slabs.
- * Needed to avoid a possible looping condition in cache_grow().
- */
-static unsigned long offslab_limit;
-
 /*
  * struct slab
  *
@@ -1356,12 +1351,6 @@ void __init kmem_cache_init(void)
                                        NULL, NULL);
                }
 
-               /* Inc off-slab bufctl limit until the ceiling is hit. */
-               if (!(OFF_SLAB(sizes->cs_cachep))) {
-                       offslab_limit = sizes->cs_size - sizeof(struct slab);
-                       offslab_limit /= sizeof(kmem_bufctl_t);
-               }
-
                sizes->cs_dmacachep = kmem_cache_create(names->name_dma,
                                        sizes->cs_size,
                                        ARCH_KMALLOC_MINALIGN,
@@ -1780,6 +1769,7 @@ static void set_up_list3s(struct kmem_cache *cachep, int index)
 static size_t calculate_slab_order(struct kmem_cache *cachep,
                        size_t size, size_t align, unsigned long flags)
 {
+       unsigned long offslab_limit;
        size_t left_over = 0;
        int gfporder;
 
@@ -1791,9 +1781,18 @@ static size_t calculate_slab_order(struct kmem_cache *cachep,
                if (!num)
                        continue;
 
-               /* More than offslab_limit objects will cause problems */
-               if ((flags & CFLGS_OFF_SLAB) && num > offslab_limit)
-                       break;
+               if (flags & CFLGS_OFF_SLAB) {
+                       /*
+                        * Max number of objs-per-slab for caches which
+                        * use off-slab slabs. Needed to avoid a possible
+                        * looping condition in cache_grow().
+                        */
+                       offslab_limit = size - sizeof(struct slab);
+                       offslab_limit /= sizeof(kmem_bufctl_t);
+
+                       if (num > offslab_limit)
+                               break;
+               }
 
                /* Found something acceptable - save it away */
                cachep->num = num;