kmalloc zero size changes break i386

qemu testing and booting test machines with i386 kernels wasn't very successfull with recent git kernels. I got either BUGs because of failing sysfs initialization or oopses in kmalloc, but no user land.

I bisected it down to this commit.

To reproduce: try to boot a 386 defconfig kernel, compiled with gcc 4.1, in qemu

-Andi

6cb8f91320d3e720351c21741da795fed580b21b is first bad commit commit 6cb8f91320d3e720351c21741da795fed580b21b

Author: Christoph Lameter <[email protected]>
Date:   Tue Jul 17 04:03:22 2007 -0700

Slab allocators: consistent ZERO_SIZE_PTR support and NULL result semantics

Define ZERO_OR_NULL_PTR macro to be able to remove the checks from the allocators. Move ZERO_SIZE_PTR related stuff into slab.h.

Make ZERO_SIZE_PTR work for all slab allocators and get rid of the WARN_ON_ONCE(size == 0) that is still remaining in SLAB.

Make slub return NULL like the other allocators if a too large memory segmen
t
is requested via __kmalloc.

Hi Andi,

[snip]

I have i386 defconfig kernel + qemu + busybox userland image + GCC 4.1.2 booting ok here. Did you manage to capture the oops? Is the userland image available somewhere?

Pekka

I think the oops below is related -- Michael reports that avoiding kmalloc(0) in the mlx4_ib driver makes it go away.

From: "Michael S. Tsirkin" <[email protected]>
Subject: oops on mlx4 modprobe
To: [email protected], Roland Dreier <[email protected]>
Date: Thu, 19 Jul 2007 11:47:51 +0300
Reply-To: "Michael S. Tsirkin" <[email protected]>

I got the following when loading mlx4_ib on git
589f1e81bde732dd0b1bc5d01b6bddd4bcb4527b

[ 1350.668590] Unable to handle kernel NULL pointer dereference at 0000000000000028 RIP:
[ 1350.674068]  [<ffffffff8027b373>] __kmalloc+0x51/0xaf
[ 1350.682159] PGD 0
[ 1350.684378] Oops: 0000 [1] SMP
[ 1350.687735] CPU 3
[ 1350.689950] Modules linked in: ib_ipoib ib_cm ib_sa ib_uverbs ib_umad mlx4_ib mlx4_core ib_mthca ib_mad ib_core piix ata_piix
[ 1350.701777] Pid: 5391, comm: ipoib Not tainted 2.6.22-x86_64-git #119
[ 1350.708400] RIP: 0010:[<ffffffff8027b373>]  [<ffffffff8027b373>] __kmalloc+0x51/0xaf
[ 1350.716536] RSP: 0018:ffff81007c655ba0  EFLAGS: 00010046
[ 1350.722034] RAX: 0000000000000003 RBX: 0000000000000246 RCX: 0000000000000040
[ 1350.729352] RDX: ffff81007ed15000 RSI: 00000000000000d0 RDI: 0000000000000000
[ 1350.736669] RBP: ffff81007c655bc0 R08: 00000000fffffff0 R09: ffff810075779d80
[ 1350.743985] R10: 0000000000000001 R11: 0000000005b8d800 R12: 00000000000000d0
[ 1350.751302] R13: 0000000000000010 R14: ffff81007ed7cc78 R15: ffff81007dbad800
[ 1350.758620] FS:  0000000000000000(0000) GS:ffff81007ff2b340(0000) knlGS:0000000000000000
[ 1350.767089] CS:  0010 DS: 0018 ES: 0018 CR0: 000000008005003b
[ 1350.773021] CR2: 0000000000000028 CR3: 0000000075ca6000 CR4: 00000000000006e0
[ 1350.780338] Process ipoib (pid: 5391, threadinfo ffff81007c654000, task ffff81007c5d8040)
[ 1350.788895] Stack:  ffff81007ed7cc00 0000000000000000 ffff81007ed7cc00 ffff81007ed7cd20
[ 1350.797331]  ffff81007c655c40 ffffffff88063cb6 ffff81006ae20b80 000000006ae20c30
[ 1350.805151]  ffff81007c655df0 ffff81007e3ba380 00000000000000d0 ffff81007ffa7c80
[ 1350.812587] Call Trace:
[ 1350.815619]  [<ffffffff88063cb6>] :mlx4_ib:create_qp_common+0x558/0x736
[ 1350.822421]  [<ffffffff88064c2e>] :mlx4_ib:mlx4_ib_create_qp+0x62/0x11f
[ 1350.829223]  [<ffffffff880999d2>] :ib_ipoib:ipoib_cm_tx_completion+0x0/0x2bb
[ 1350.836461]  [<ffffffff8800eca9>] :ib_core:ib_create_qp+0x18/0x94
[ 1350.842743]  [<ffffffff8809a281>] :ib_ipoib:ipoib_cm_tx_start+0x216/0x651
[ 1350.849714]  [<ffffffff80244382>] queue_work+0x3f/0x4a
[ 1350.855043]  [<ffffffff88080e63>] :ib_sa:ib_sa_join_multicast+0x292/0x2df
[ 1350.862030]  [<ffffffff8809a06b>] :ib_ipoib:ipoib_cm_tx_start+0x0/0x651
[ 1350.868829]  [<ffffffff80243cd4>] run_workqueue+0x85/0x10f
[ 1350.874501]  [<ffffffff80244695>] worker_thread+0x0/0xe7
[ 1350.880000]  [<ffffffff80244771>] worker_thread+0xdc/0xe7
[ 1350.885585]  [<ffffffff80247747>] autoremove_wake_function+0x0/0x38
[ 1350.892036]  [<ffffffff80247622>] kthread+0x49/0x77
[ 1350.897102]  [<ffffffff8020caa8>] child_rip+0xa/0x12
[ 1350.902254]  [<ffffffff802475d9>] kthread+0x0/0x77
[ 1350.907231]  [<ffffffff8020ca9e>] child_rip+0x0/0x12
[ 1350.912384]
[ 1350.914068]
[ 1350.914068] Code: 49 8b 54 c5 00 83 3a 00 74 16 8b 02 c7 42 0c 01 00 00 00 ff
[ 1350.923599] RIP  [<ffffffff8027b373>] __kmalloc+0x51/0xaf
[ 1350.929195]  RSP <ffff81007c655ba0>
[ 1350.932873] CR2: 0000000000000028

Ok, I think I see it: I think the mm/slab.c conversion of kmalloc(0) is totally broken.

The problem? It returns ZERO_SIZE_PTR from __find_general_cachep(), not from __kmalloc(). So anythign that uses __find_general_cachep() will get an invalid cachep pointer, which was not the point.

We're deprecating SLAB, and a lot of people are already using SLUB, which hid this.

Does something like this fix it?

Christoph, please go over this and see if there are other cases like that.

Linus

---

index 88bc633..4bc4bc0 100644
@@ -775,8 +775,6 @@ static inline struct kmem_cache *__find_general_cachep(size_t size,
 	 */
 	BUG_ON(malloc_sizes[INDEX_AC].cs_cachep == NULL);
 #endif
-	if (!size)
-		return ZERO_SIZE_PTR;
 
 	while (size > csizep->cs_size)
 		csizep++;

@@ -3684,6 +3682,9 @@ static __always_inline void *__do_kmalloc(size_t size, gfp_t flags,
 {
 	struct kmem_cache *cachep;
 
+	if (!size)
+		return ZERO_SIZE_PTR;
+
 	/* If you want to save a few bytes .text space: replace
 	 * __ with kmem_.
 	 * Then kmalloc uses the uninlined functions instead of the inline

I wondered about that too but I didn't spot any callers that would actually break. Andi? Roland?

__do_kmalloc_node probably.

Pekka

Actually, here's a better version, I think.

Andi, does this patch fix your problem?

Linus

---
 mm/slab.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

index 88bc633..c3feeaa 100644
@@ -3690,8 +3690,8 @@ static __always_inline void *__do_kmalloc(size_t size, gfp_t flags,
 	 * functions.
 	 */
 	cachep = __find_general_cachep(size, flags);
-	if (unlikely(cachep == NULL))
-		return NULL;
+	if (unlikely(ZERO_OR_NULL_PTR(cachep)))
+		return cachep;
 	return __cache_alloc(cachep, flags, caller);
 }
 

No, unfortunately not.

e.g. I see it in a git checkout (with head 589f1e81bde732dd0b1bc5d01b6bddd4bcb4527b), but not in plain -git12, but when I readd my x86 patchkit to -git12 it happens again. I also switched to slub in the config and I also still see it. Trying to bisect that right now.

-Andi

Hi Roland,

There's some heavy-duty function inlining going on in__kmalloc so could you please work out the exact location of the oops as described in Documentation/BUG-HUNTING (look for the "use GDB to translate" part).

And to double-check, this is SLAB with CONFIG_DEBUG_SLAB enabled? Do you see this with SLUB?

Pekka

And, of course, please check if a5c96d8a1c67f31ef48935a78da2d2076513842b fixes it.

Pekka

The comparison with ZERO_SIZE_PTR in ZERO_OR_NULL_PTR() needs to be <= (not just <) so that ZERO_OR_NULL_PTR(ZERO_SIZE_PTR) is 1.

---
I finally had time to reproduce Michael's crash and debug it. Linus's patch is part of the story -- it doesn't help unless ZERO_OR_NULL_PTR() is true. With this change the crash goes away, and it seems pretty obviously correct to me (unless I'm seriously confused).

 include/linux/slab.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

index 7d0ecc1..d859354 100644
@@ -40,7 +40,7 @@
  */
 #define ZERO_SIZE_PTR ((void *)16)
 
-#define ZERO_OR_NULL_PTR(x) ((unsigned long)(x) < \
+#define ZERO_OR_NULL_PTR(x) ((unsigned long)(x) <= \
 				(unsigned long)ZERO_SIZE_PTR)
 
 /*

The sysfs crashes are all over; the first I saw is the BUG_ON() in kernel_param_sysfs_setup(); but it was changing during the bisect and in other similar sysfs BUG_ON()s too.

During one bisect state I also had a crash in kmalloc itself. I right now don't have it anymore; do you want me to restart the bisect go recreate it?

x86-64 kernels BTW work just fine; just something seems to be broken with i386.

Unfortunately newsetup seems to have broken argument passing to qemu so it's a bit difficult to get more out of it.

Userland is not reached yet. You can just use a dummy dd if=/dev/zero of=.... bs=1M count=1 file.

-Andi

Can you send in the oopses and BUGs? The bug is almost certainly not the commit you point to, but some bad kernel code that that commit just shows.

Linus