Re: namespaces?: bug at mm/slub.c:2750

Re: namespaces?: bug at mm/slub.c:2750

[Andrew Morton]

(cc's added)

On Wed, 11 Feb 2009 08:55:08 +0100 Vegard Nossum <vegard.nossum-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> On Sat, Feb 7, 2009 at 1:15 AM, Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org> wrote:
> > On Fri, 6 Feb 2009 12:35:56 +0100
> > Eric Sesterhenn <snakebyte-Mmb7MZpHnFY@public.gmane.org> wrote:
> >>
> >> with todays -git i get the following bug when i reboot the machine
> >>
> >> [   94.369135] ------------[ cut here ]------------
> >> [   94.369344] kernel BUG at mm/slub.c:2750!
> 
> [...]
> 
> > Well that's ugly.  We seem to have passed a non-slab address into
> > kfree().
> >
> > void kfree(const void *x)
> > {
> >        struct page *page;
> >        void *object = (void *)x;
> >
> >        if (unlikely(ZERO_OR_NULL_PTR(x)))
> >                return;
> >
> >        page = virt_to_head_page(x);
> >        if (unlikely(!PageSlab(page))) {
> >                BUG_ON(!PageCompound(page));
> >
> >
> > doing BUG_ON(!PageCompound) is a rather odd way of reporting that.
> >
> 
> This might be pointing out the obvious, but page allocator
> pass-through means that we can't really tell slab pages from page
> allocator pages. But since pass-through uses GFP_COMP, we know it'd be
> a bug to pass a non-compound page into this function.
> 
> > I'm unsure what could have caused this.  Could you have a play around
> > please?  Set all the memory debug options, try using slab instead of
> > slub, etc?
> 
> I think it is trying to free the init_user_ns (because of a
> refcounting bug). I am able to reproduce it with a clean stack trace:
> 
> [  648.864710] ------------[ cut here ]------------
> [  648.865554] kernel BUG at mm/slub.c:2750!
> [  648.865554] invalid opcode: 0000 [#1] SMP DEBUG_PAGEALLOC
> [  648.865554] last sysfs file: /sys/devices/pnp0/00:0d/id
> [  648.865554] CPU 1
> [  648.865554] Modules linked in:
> [  648.865554] Pid: 917, comm: udevd Not tainted 2.6.29-rc3 #223
> [  648.865554] RIP: 0010:[<ffffffff810b99c9>]  [<ffffffff810b99c9>]
> kfree+0x29/0x87
> [  648.865554] RSP: 0018:ffff88003f187e28  EFLAGS: 00010246
> [  648.865554] RAX: 0100000000000400 RBX: ffffffff8171fe00 RCX: 0000000000000086
> [  648.865554] RDX: ffffe20000050ec8 RSI: 0000000000000085 RDI: ffffe20000050ec8
> [  648.865554] RBP: ffff88003f187e38 R08: 0000000000000000 R09: ffffffff81819cb0
> [  648.865554] R10: 0000000000000002 R11: ffff88003f187e98 R12: ffffffff81072144
> [  648.865554] R13: 00000000000000cf R14: ffffffff818b13e0 R15: 000000000000000a
> [  648.865554] FS:  0000000000000000(0000) GS:ffff88003f156f80(0063)
> knlGS:00000000f7fac700
> [  648.865554] CS:  0010 DS: 002b ES: 002b CR0: 000000008005003b
> [  648.865554] CR2: 0000000009b2d630 CR3: 000000003e92c000 CR4: 00000000000006a0
> [  648.865554] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> [  648.865554] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
> [  648.865554] Process udevd (pid: 917, threadinfo ffff88003e458000,
> task ffff88003f1b8e80)
> [  648.865554] Stack:
> [  648.865554]  ffffffff8171fe00 ffffffff81072144 ffff88003f187e58
> ffffffff81072169
> [  648.865554]  ffffffff818b13e0 ffffffff8171fe00 ffff88003f187e78
> ffffffff811b2f68
> [  648.865554]  ffff88003dd4c500 0000000000000286 ffff88003f187e98
> ffffffff81046712
> [  648.865554] Call Trace:
> [  648.865554]  <IRQ> <0> [<ffffffff81072144>] ? free_user_ns+0x0/0x29
> [  648.865554]  [<ffffffff81072169>] free_user_ns+0x25/0x29
> [  648.865554]  [<ffffffff811b2f68>] kref_put+0x66/0x72
> [  648.865554]  [<ffffffff81046712>] free_uid+0x4f/0x90
> [  648.865554]  [<ffffffff810555b8>] put_cred_rcu+0xa5/0xb9
> [  648.865554]  [<ffffffff8107ea86>] __rcu_process_callbacks+0x1f4/0x263
> [  648.865554]  [<ffffffff8107eb2c>] rcu_process_callbacks+0x37/0x5d
> [  648.865554]  [<ffffffff81041bd3>] __do_softirq+0x88/0x149
> [  648.865554]  [<ffffffff8100d53c>] call_softirq+0x1c/0x28
> [  648.865554]  [<ffffffff8100e4e9>] do_softirq+0x39/0x7b
> [  648.865554]  [<ffffffff81041946>] irq_exit+0x44/0x7e
> [  648.865554]  [<ffffffff814c3d8f>] smp_apic_timer_interrupt+0x98/0xb1
> [  648.865554]  [<ffffffff8100cf73>] apic_timer_interrupt+0x13/0x20
> [  648.865554]  <EOI> <0>Code: c9 c3 55 48 89 e5 41 54 53 0f 1f 44 00
> 00 48 83 ff 10 48 89 fb 7
> 6 6d e8 1d ed ff ff 48 89 c7 48 8b 00 84 c0 78 10 f6 c4 60 75 04 <0f>
> 0b eb fe e8 f4 cd fd ff e
> b 4e 48 8b 4d 08 4c 8b 4f 10 9c 41
> [  648.865554] RIP  [<ffffffff810b99c9>] kfree+0x29/0x87
> [  648.865554]  RSP <ffff88003f187e28>
> [  649.131912] ---[ end trace a48f49f69d2bcb3e ]---
> [  649.136840] Kernel panic - not syncing: Fatal exception in interrupt
> [  649.143558] Rebooting in 10 seconds..
> 
> And if you look closely in the stack dump, you will find init_user_ns:
> 
> ffffffff8171fe00 D init_user_ns
> 
> In case you missed it, KOSAKI Motohiro posted a similar stack-trace
> (but not the same BUG) in this thread:
> http://lkml.org/lkml/2009/2/10/7
> 

Both traces include the newly-added put_cred_rcu().  Suspicious.

Re: namespaces?: bug at mm/slub.c:2750

[Vegard Nossum]

On Wed, Feb 11, 2009 at 9:07 AM, Andrew Morton
<akpm@linux-foundation.org> wrote:
>> In case you missed it, KOSAKI Motohiro posted a similar stack-trace
>> (but not the same BUG) in this thread:
>> http://lkml.org/lkml/2009/2/10/7
>>
>
> Both traces include the newly-added put_cred_rcu().  Suspicious.
>

I have this test case which triggers it regularly after some minutes:

#include <sys/wait.h>
#include <sys/types.h>

#include <errno.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>

int main(int argc, char* argv[])
{
        unsigned int i;

        if (fork() == 0) {
                while (1)
                        system("echo -n .");
        }

        for (i = 0; i < 2; ++i) {
                if (fork() == 0) {
                        while (1) {
                                setreuid(0, 0xcafeba);
                                setreuid(0xcafeba, 0);

                                setreuid(0, 0xcafebb);
                                setreuid(0xcafebb, 0);
                        }

                        exit(EXIT_SUCCESS);
                }
        }

        while (!(wait(NULL) == -1 && errno == ECHILD))
                ;

        return 0;
}

It seems to be the combination of exec() and setreuid(), but I
couldn't get it to work with just exec() instead of system(). It is
possible that CONFIG_USER_SCHED must be =y for this to work. It can
probably be simplified too...


Vegard

-- 
"The animistic metaphor of the bug that maliciously sneaked in while
the programmer was not looking is intellectually dishonest as it
disguises that the error is the programmer's own creation."
	-- E. W. Dijkstra, EWD1036

Re: namespaces?: bug at mm/slub.c:2750

[Serge E. Hallyn]

Quoting Vegard Nossum (vegard.nossum@gmail.com):
> On Wed, Feb 11, 2009 at 9:07 AM, Andrew Morton
> <akpm@linux-foundation.org> wrote:
> >> In case you missed it, KOSAKI Motohiro posted a similar stack-trace
> >> (but not the same BUG) in this thread:
> >> http://lkml.org/lkml/2009/2/10/7
> >>
> >
> > Both traces include the newly-added put_cred_rcu().  Suspicious.
> >
> 
> I have this test case which triggers it regularly after some minutes:
> 
> #include <sys/wait.h>
> #include <sys/types.h>
> 
> #include <errno.h>
> #include <stdio.h>
> #include <stdlib.h>
> #include <string.h>
> #include <unistd.h>
> 
> int main(int argc, char* argv[])
> {
>         unsigned int i;
> 
>         if (fork() == 0) {
>                 while (1)
>                         system("echo -n .");
>         }
> 
>         for (i = 0; i < 2; ++i) {
>                 if (fork() == 0) {
>                         while (1) {
>                                 setreuid(0, 0xcafeba);
>                                 setreuid(0xcafeba, 0);
> 
>                                 setreuid(0, 0xcafebb);
>                                 setreuid(0xcafebb, 0);
>                         }
> 
>                         exit(EXIT_SUCCESS);
>                 }
>         }
> 
>         while (!(wait(NULL) == -1 && errno == ECHILD))
>                 ;
> 
>         return 0;
> }
> 
> It seems to be the combination of exec() and setreuid(), but I
> couldn't get it to work with just exec() instead of system(). It is
> possible that CONFIG_USER_SCHED must be =y for this to work. It can
> probably be simplified too...
> 
> 
> Vegard

I haven't yet been able to reproduce it, but I'm wondering whether
something like the following is needed.

Note that free_user() still seems wrong there, as it won't call
uid_hash_remove(up) for a user which is not inthe init_user_ns at
all, right?


From 2bc9da9728e0a4242bd8b1362d34e8d425be1b66 Mon Sep 17 00:00:00 2001
From: Serge E. Hallyn <serue@us.ibm.com>
Date: Wed, 11 Feb 2009 11:29:52 -0500
Subject: [PATCH 1/1] user namespaces: only put the userns when we unhash the uid

uids in namespaces other than init don't get a sysfs entry.

For those in the init namespace, while we're waiting to remove
the sysfs entry for the uid the uid is still hashed, and
alloc_uid() may re-grab that uid without getting a new
reference to the user_ns, which we've already put in free_user
before scheduling remove_user_sysfs_dir().

Signed-off-by: Serge E. Hallyn <serue@us.ibm.com>
---
 kernel/user.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/kernel/user.c b/kernel/user.c
index 477b666..bb401a5 100644
--- a/kernel/user.c
+++ b/kernel/user.c
@@ -71,6 +71,7 @@ static void uid_hash_insert(struct user_struct *up, struct hlist_head *hashent)
 
 static void uid_hash_remove(struct user_struct *up)
 {
+	put_user_ns(up->user_ns);
 	hlist_del_init(&up->uidhash_node);
 }
 
@@ -334,7 +335,6 @@ static void free_user(struct user_struct *up, unsigned long flags)
 	atomic_inc(&up->__count);
 	spin_unlock_irqrestore(&uidhash_lock, flags);
 
-	put_user_ns(up->user_ns);
 	INIT_WORK(&up->work, remove_user_sysfs_dir);
 	schedule_work(&up->work);
 }
@@ -357,7 +357,6 @@ static void free_user(struct user_struct *up, unsigned long flags)
 	sched_destroy_user(up);
 	key_put(up->uid_keyring);
 	key_put(up->session_keyring);
-	put_user_ns(up->user_ns);
 	kmem_cache_free(uid_cachep, up);
 }
 
-- 
1.6.1

Re: namespaces?: bug at mm/slub.c:2750

[David Howells]

Serge E. Hallyn <serue@us.ibm.com> wrote:

>  static void uid_hash_remove(struct user_struct *up)
>  {
> +	put_user_ns(up->user_ns);
>  	hlist_del_init(&up->uidhash_node);
>  }

Don't you need to do the hlist_del_init() first?  Otherwise, mightn't the
put_user_ns() cause the namespace to be freed before hlist_del_init() removes
the user_struct from it?

David

Re: namespaces?: bug at mm/slub.c:2750

[Serge E. Hallyn]

Quoting David Howells (dhowells-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org):
> Serge E. Hallyn <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org> wrote:
> 
> >  static void uid_hash_remove(struct user_struct *up)
> >  {
> > +	put_user_ns(up->user_ns);
> >  	hlist_del_init(&up->uidhash_node);
> >  }
> 
> Don't you need to do the hlist_del_init() first?  Otherwise, mightn't the
> put_user_ns() cause the namespace to be freed before hlist_del_init() removes
> the user_struct from it?

It's called under uidhash_lock spinlock so should be ok, but in
principle you're right so it's probably a good idea.

The main point is that without this patch, put_user_ns is done before
the hlist_del_init and *not* atomically under uidhash_lock.

thanks,
-serge

Re: namespaces?: bug at mm/slub.c:2750

[Vegard Nossum]

On Wed, Feb 11, 2009 at 6:24 PM, Serge E. Hallyn <serue@us.ibm.com> wrote:
> Quoting David Howells (dhowells@redhat.com):
>> Serge E. Hallyn <serue@us.ibm.com> wrote:
>>
>> >  static void uid_hash_remove(struct user_struct *up)
>> >  {
>> > +   put_user_ns(up->user_ns);
>> >     hlist_del_init(&up->uidhash_node);
>> >  }
>>
>> Don't you need to do the hlist_del_init() first?  Otherwise, mightn't the
>> put_user_ns() cause the namespace to be freed before hlist_del_init() removes
>> the user_struct from it?
>
> It's called under uidhash_lock spinlock so should be ok, but in
> principle you're right so it's probably a good idea.
>
> The main point is that without this patch, put_user_ns is done before
> the hlist_del_init and *not* atomically under uidhash_lock.

Congrats, your (unmodified) patch made it through the first 20 minutes
of testing! :-D

(In comparison, the unpatched kernel would usually crash after ~3 minutes)

I wonder why you couldn't reproduce it, though.

KOSAKI Motohiro: You might want to see if this patch helps too. It is
here: http://lkml.org/lkml/2009/2/11/251


Vegard

-- 
"The animistic metaphor of the bug that maliciously sneaked in while
the programmer was not looking is intellectually dishonest as it
disguises that the error is the programmer's own creation."
	-- E. W. Dijkstra, EWD1036

Re: namespaces?: bug at mm/slub.c:2750

[David Howells]

Serge E. Hallyn <serue@us.ibm.com> wrote:

> It's called under uidhash_lock spinlock so should be ok, but in
> principle you're right so it's probably a good idea.

The lock is nothing to do with it.  put_user_ns() may call kfree() on the
user_namespace, but the user_struct given to uid_hash_remove() may still be
attached to it.

David

Re: namespaces?: bug at mm/slub.c:2750

[Serge E. Hallyn]

Quoting David Howells (dhowells@redhat.com):
> Serge E. Hallyn <serue@us.ibm.com> wrote:
> 
> > It's called under uidhash_lock spinlock so should be ok, but in
> > principle you're right so it's probably a good idea.
> 
> The lock is nothing to do with it.  put_user_ns() may call kfree() on the
> user_namespace, but the user_struct given to uid_hash_remove() may still be
> attached to it.

Yes, but noone will pull the user_struct off the list without
taking the lock.

what am I missing?

Anyway, I do like swapping the lines (as below) better.

-serge


From 8b83d11023c1064e99bffae3c2a05580b915de60 Mon Sep 17 00:00:00 2001
From: Serge E. Hallyn <serue@us.ibm.com>
Date: Wed, 11 Feb 2009 11:29:52 -0500
Subject: [PATCH 1/1] user namespaces: only put the userns when we unhash the uid

uids in namespaces other than init don't get a sysfs entry.

For those in the init namespace, while we're waiting to remove
the sysfs entry for the uid the uid is still hashed, and
alloc_uid() may re-grab that uid without getting a new
reference to the user_ns, which we've already put in free_user
before scheduling remove_user_sysfs_dir().

Signed-off-by: Serge E. Hallyn <serue@us.ibm.com>
---
 kernel/user.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/kernel/user.c b/kernel/user.c
index 477b666..3551ac7 100644
--- a/kernel/user.c
+++ b/kernel/user.c
@@ -72,6 +72,7 @@ static void uid_hash_insert(struct user_struct *up, struct hlist_head *hashent)
 static void uid_hash_remove(struct user_struct *up)
 {
 	hlist_del_init(&up->uidhash_node);
+	put_user_ns(up->user_ns);
 }
 
 static struct user_struct *uid_hash_find(uid_t uid, struct hlist_head *hashent)
@@ -334,7 +335,6 @@ static void free_user(struct user_struct *up, unsigned long flags)
 	atomic_inc(&up->__count);
 	spin_unlock_irqrestore(&uidhash_lock, flags);
 
-	put_user_ns(up->user_ns);
 	INIT_WORK(&up->work, remove_user_sysfs_dir);
 	schedule_work(&up->work);
 }
@@ -357,7 +357,6 @@ static void free_user(struct user_struct *up, unsigned long flags)
 	sched_destroy_user(up);
 	key_put(up->uid_keyring);
 	key_put(up->session_keyring);
-	put_user_ns(up->user_ns);
 	kmem_cache_free(uid_cachep, up);
 }
 
-- 
1.6.1

Re: namespaces?: bug at mm/slub.c:2750

[David Howells]

Serge E. Hallyn <serue@us.ibm.com> wrote:

> Yes, but noone will pull the user_struct off the list without
> taking the lock.
> 
> what am I missing?

I believe that the hash link (uidhash_node) in the user_struct that is passed
to uid_hash_remove() points to, and is pointed to by the user_namespace to
which the user_struct belongs.

In which case calling put_user_ns() may kfree the head pointer of the list
_before_ hlist_del_init() is invoked - in which case hlist_del_init() will act
upon freed memory.

At least, I think it works like this.


Anyway, I have no objection to your new patch.

Acked-by: David Howells <dhowells@redhat.com>

Re: namespaces?: bug at mm/slub.c:2750

[Serge E. Hallyn]

Quoting David Howells (dhowells@redhat.com):
> Serge E. Hallyn <serue@us.ibm.com> wrote:
> 
> > Yes, but noone will pull the user_struct off the list without
> > taking the lock.
> > 
> > what am I missing?
> 
> I believe that the hash link (uidhash_node) in the user_struct that is passed
> to uid_hash_remove() points to, and is pointed to by the user_namespace to
> which the user_struct belongs.
> 
> In which case calling put_user_ns() may kfree the head pointer of the list
> _before_ hlist_del_init() is invoked - in which case hlist_del_init() will act
> upon freed memory.
> 
> At least, I think it works like this.

Yikes, you're right.  I was thinking there was on hash table with
the key calculated from ns+uid, but instead each ns has its own
hash table keyed on uid.

> Anyway, I have no objection to your new patch.
> 
> Acked-by: David Howells <dhowells@redhat.com>

thanks,
-serge