Broken "User namespaces: Only put the userns when we unhash the uid"

Broken "User namespaces: Only put the userns when we unhash the uid"

[Alexey Dobriyan]

The following commit breaks clone(CLONE_NEWUSER)
with an oops.

	__lock_acquire
	lock_acquire
	_spin_lock
	_atomic_dec_and_lock
	free_uid
	free_user_ns
	kref_put
	free_uid
	put_cred_rcu
	rcu_process_callbacks

commit fb5ae64fdde29236e1a15e0366946df7060f41f2
Author: Serge E. Hallyn <serue@us.ibm.com>
Date:   Fri Feb 13 14:04:21 2009 +0000

    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().
    
    Reported-and-tested-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
    Signed-off-by: Serge E. Hallyn <serue@us.ibm.com>
    Acked-by: David Howells <dhowells@redhat.com>
    Tested-by: Ingo Molnar <mingo@elte.hu>
    Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

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);
 }
 

Re: Broken "User namespaces: Only put the userns when we unhash the uid"

[David Howells]

Alexey Dobriyan <adobriyan@gmail.com> wrote:

> The following commit breaks clone(CLONE_NEWUSER)
> with an oops.

Can you give the actual text of the oops, please?

David

Re: Broken "User namespaces: Only put the userns when we unhash the uid"

[Alexey Dobriyan]

On Sun, Feb 22, 2009 at 12:37:59AM +0000, David Howells wrote:
> Alexey Dobriyan <adobriyan@gmail.com> wrote:
> 
> > The following commit breaks clone(CLONE_NEWUSER)
> > with an oops.
> 
> Can you give the actual text of the oops, please?

One needs UP kernel for this. The box is really dead afterwards.

=============================================
[ INFO: possible recursive locking detected ]
2.6.29-rc5-2ec77fc93ca8731368fbe8e71f805c0569d4bcee #4
---------------------------------------------
swapper/0 is trying to acquire lock:
 (uidhash_lock){.+..}, at: [<c02182c8>] _atomic_dec_and_lock+0x18/0x40

but task is already holding lock:
 (uidhash_lock){.+..}, at: [<c02182c8>] _atomic_dec_and_lock+0x18/0x40

other info that might help us debug this:
1 lock held by swapper/0:
 #0:  (uidhash_lock){.+..}, at: [<c02182c8>] _atomic_dec_and_lock+0x18/0x40

stack backtrace:
Pid: 0, comm: swapper Not tainted 2.6.29-rc5-2ec77fc93ca8731368fbe8e71f805c0569d4bcee #4
Call Trace:
 [<c013ecd5>] validate_chain+0xac5/0x1130
 [<c013e5e0>] ? validate_chain+0x3d0/0x1130
 [<c013e5e0>] ? validate_chain+0x3d0/0x1130
 [<c013f5b6>] __lock_acquire+0x276/0xa10
 [<c013f5b6>] ? __lock_acquire+0x276/0xa10
 [<c013fdac>] lock_acquire+0x5c/0x80
 [<c02182c8>] ? _atomic_dec_and_lock+0x18/0x40
 [<c03bc0ac>] _spin_lock+0x3c/0x50
 [<c02182c8>] ? _atomic_dec_and_lock+0x18/0x40
 [<c02182c8>] _atomic_dec_and_lock+0x18/0x40
 [<c0125887>] free_uid+0x27/0xc0
 [<c01459f0>] ? free_user_ns+0x0/0x20
 [<c0145a01>] free_user_ns+0x11/0x20
 [<c021a4cb>] kref_put+0x2b/0x80
 [<c02182c8>] ? _atomic_dec_and_lock+0x18/0x40
 [<c01258c0>] free_uid+0x60/0xc0
 [<c013492f>] put_cred_rcu+0x2f/0x60
 [<c03bc709>] ? _spin_unlock_irqrestore+0x39/0x70
 [<c0148982>] __rcu_process_callbacks+0xd2/0x230
 [<c0148af2>] rcu_process_callbacks+0x12/0x30
 [<c012058a>] __do_softirq+0x7a/0x130
 [<c0120695>] do_softirq+0x55/0x60
 [<c012098b>] irq_exit+0x7b/0x80
 [<c0104f0e>] do_IRQ+0x3e/0x80
 [<c013da7c>] ? trace_hardirqs_on_caller+0xfc/0x190
 [<c01037ec>] common_interrupt+0x2c/0x34
 [<c01087df>] ? mwait_idle+0x3f/0x60
 [<c0101bc5>] cpu_idle+0x45/0x80
 [<c03af9f1>] rest_init+0x61/0x70

Re: Broken "User namespaces: Only put the userns when we unhash the uid"

[David Howells]

So, free_uid() calls free_user_ns(), which calls free_uid()...  Obvious,
really.   I wonder if free_user_ns() should always be deferred irrespective of
whether CONFIG_USER_SCHED is set.

David