kmalloc zero size changes break i386

kmalloc zero size changes break i386

[Andi Kleen]

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 <clameter@sgi.com>
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.
    
    Signed-off-by: Christoph Lameter <clameter@sgi.com>
    Acked-by: Pekka Enberg <penberg@cs.helsinki.fi>
    Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
    Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

Re: kmalloc zero size changes break i386

[Pekka Enberg]

Hi Andi,

On 7/19/07, Andi Kleen <ak@suse.de> wrote:
> 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

[snip]

> 6cb8f91320d3e720351c21741da795fed580b21b is first bad commit
> commit 6cb8f91320d3e720351c21741da795fed580b21b
> Author: Christoph Lameter <clameter@sgi.com>
> Date:   Tue Jul 17 04:03:22 2007 -0700
>
>     Slab allocators: consistent ZERO_SIZE_PTR support and NULL result semantics

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

Re: kmalloc zero size changes break i386

[Roland Dreier]

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" <mst@dev.mellanox.co.il>
Subject: oops on mlx4 modprobe
To: general@lists.openfabrics.org, Roland Dreier <rolandd@cisco.com>
Date: Thu, 19 Jul 2007 11:47:51 +0300
Reply-To: "Michael S. Tsirkin" <mst@dev.mellanox.co.il>

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

Re: kmalloc zero size changes break i386

[Linus Torvalds]

On Thu, 19 Jul 2007, Andi Kleen wrote:
> 
> 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.

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

Re: kmalloc zero size changes break i386

[Andi Kleen]

On Thursday 19 July 2007 16:08:34 Pekka Enberg wrote:
> Hi Andi,
> 
> On 7/19/07, Andi Kleen <ak@suse.de> wrote:
> > 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
> 
> [snip]
> 
> > 6cb8f91320d3e720351c21741da795fed580b21b is first bad commit
> > commit 6cb8f91320d3e720351c21741da795fed580b21b
> > Author: Christoph Lameter <clameter@sgi.com>
> > Date:   Tue Jul 17 04:03:22 2007 -0700
> >
> >     Slab allocators: consistent ZERO_SIZE_PTR support and NULL result semantics
> 
> I have i386 defconfig kernel + qemu + busybox userland image + GCC
> 4.1.2 booting ok here. Did you manage to capture the oops? 

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.

> Is the 
> userland image available somewhere?

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

-Andi

Re: kmalloc zero size changes break i386

[Linus Torvalds]

On Thu, 19 Jul 2007, Roland Dreier wrote:
>
> I think the oops below is related -- Michael reports that avoiding
> kmalloc(0) in the mlx4_ib driver makes it go away.

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

---
diff --git a/mm/slab.c b/mm/slab.c
index 88bc633..4bc4bc0 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -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

Re: kmalloc zero size changes break i386

[Pekka Enberg]

Linus Torvalds wrote:
> 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.
 >
 > Does something like this fix it?

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

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

__do_kmalloc_node probably.


				Pekka

Re: kmalloc zero size changes break i386

[Linus Torvalds]

On Thu, 19 Jul 2007, Linus Torvalds wrote:
> 
> Does something like this fix it?
> 
> Christoph, please go over this and see if there are other cases like that.

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(-)

diff --git a/mm/slab.c b/mm/slab.c
index 88bc633..c3feeaa 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -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);
 }
 

Re: kmalloc zero size changes break i386

[Andi Kleen]

On Thursday 19 July 2007 21:19:29 Linus Torvalds wrote:
> 
> On Thu, 19 Jul 2007, Linus Torvalds wrote:
> > 
> > Does something like this fix it?
> > 
> > Christoph, please go over this and see if there are other cases like that.
> 
> Actually, here's a better version, I think.
> 
> Andi, does this patch fix your problem?

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

Re: kmalloc zero size changes break i386

[Pekka J Enberg]

Hi Roland,

On Thu, 19 Jul 2007, Roland Dreier wrote:
> [ 1350.668590] Unable to handle kernel NULL pointer dereference at 0000000000000028 RIP:
> [ 1350.674068]  [<ffffffff8027b373>] __kmalloc+0x51/0xaf

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

Re: kmalloc zero size changes break i386

[Pekka J Enberg]

On Fri, 20 Jul 2007, Pekka J Enberg wrote:
> 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, of course, please check if a5c96d8a1c67f31ef48935a78da2d2076513842b 
fixes it.

				Pekka

[PATCH] Fix ZERO_OR_NULL_PTR(ZERO_SIZE_PTR)

[Roland Dreier]

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.

Signed-off-by: Roland Dreier <rolandd@cisco.com>
---
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(-)

diff --git a/include/linux/slab.h b/include/linux/slab.h
index 7d0ecc1..d859354 100644
--- a/include/linux/slab.h
+++ b/include/linux/slab.h
@@ -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)
 
 /*