[patch] mm/slab.c: fix offslab_limit bug

[patch] mm/slab.c: fix offslab_limit bug

[Ingo Molnar]

Subject: slab.c: fix offslab_limit bug
From: Ingo Molnar <mingo@elte.hu>

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>
---
 mm/slab.c |   27 +++++++++++++--------------
 1 file changed, 13 insertions(+), 14 deletions(-)

Index: linux/mm/slab.c
===================================================================
--- linux.orig/mm/slab.c
+++ linux/mm/slab.c
@@ -209,11 +209,6 @@ typedef unsigned long 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
  *
@@ -1398,12 +1393,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,
@@ -1841,6 +1830,7 @@ static void set_up_list3s(struct kmem_ca
 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;
 
@@ -1852,9 +1842,18 @@ static size_t calculate_slab_order(struc
 		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;

[patch] mm/slab.c: fix early init assumption

[Ingo Molnar]

Subject: mm/slab.c: fix early init assumption
From: Ingo Molnar <mingo@elte.hu>

the SLAB bootstrap code assumes that the first two kmalloc caches
created (the INDEX_AC and INDEX_L3 kmalloc caches) wont be off-slab.
But due to AC and L3 structure size increase in lockdep, one of them
ended up being off-slab, and subsequently crashing with:

Unable to handle kernel NULL pointer dereference at 0000000000000000 RIP:
 [<ffffffff80267478>] kmem_cache_alloc+0x26/0x7d

the fix is to introduce a bootstrap flag and to use it to prevent
off-slab caches being created so early during bootup.

(the calculation for off-slab caches is quite complex so i didnt want
to complicate things with introducing yet another INDEX_ calculation,
the flag approach is simpler and smaller.)

Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 mm/slab.c |   12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

Index: linux/mm/slab.c
===================================================================
--- linux.orig/mm/slab.c
+++ linux/mm/slab.c
@@ -333,6 +333,8 @@ static __always_inline int index_of(cons
 	return 0;
 }
 
+static int slab_early_init = 1;
+
 #define INDEX_AC index_of(sizeof(struct arraycache_init))
 #define INDEX_L3 index_of(sizeof(struct kmem_list3))
 
@@ -1377,6 +1379,8 @@ void __init kmem_cache_init(void)
 				NULL, NULL);
 	}
 
+	slab_early_init = 0;
+
 	while (sizes->cs_size != ULONG_MAX) {
 		/*
 		 * For performance, all the general caches are L1 aligned.
@@ -2128,8 +2132,12 @@ kmem_cache_create (const char *name, siz
 #endif
 #endif
 
-	/* Determine if the slab management is 'on' or 'off' slab. */
-	if (size >= (PAGE_SIZE >> 3))
+	/*
+	 * Determine if the slab management is 'on' or 'off' slab.
+	 * (bootstrapping cannot cope with offslab caches so dont do
+	 *  it too early on.)
+	 */
+	if ((size >= (PAGE_SIZE >> 3)) && !slab_early_init)
 		/*
 		 * Size is large, assume best to place the slab management obj
 		 * off-slab (should allow better packing of objs).