Re: Oops on 2.6.18

From: Linus Torvalds
Date: Sat Nov 04 2006 - 13:14:46 EST




On Tue, 31 Oct 2006, Lukasz Trabinski wrote:
>
> oceanic:~$ uname -a
> Linux oceanic.wsisiz.edu.pl 2.6.18-oceanic #1 SMP Thu Sep 28 22:26:52 CEST
> 2006 x86_64 x86_64 x86_64 GNU/Linux
>
> filesystem with ext3/quota/acl
>
> Oct 31 13:30:09 oceanic kernel: Unable to handle kernel paging request at 0000000000100108

Ok, this should be fixed with the commit I just checked in and pushed
out.. That said, I don't think you'd be likely to ever be able to
reproduce it - it looks like a very unlikely race. I think the race
condition has been there for a long time, and googling doesn't actually
show anybody else seemingly having ever triggered it.

But in case you care, this is what I committed.

Linus

---
commit 45c18b0bb579b5c1b89f8c99f1b6ffa4c586ba08
Author: Linus Torvalds <torvalds@xxxxxxxxxxx>
Date: Sat Nov 4 10:06:02 2006 -0800

Fix unlikely (but possible) race condition on task->user access

There's a possible race condition when doing a "switch_uid()" from one
user to another, which could race with another thread doing a signal
allocation and looking at the old thread ->user pointer as it is freed.

This explains an oops reported by Lukasz Trabinski:
http://permalink.gmane.org/gmane.linux.kernel/462241

We fix this by delaying the (reference-counted) freeing of the user
structure until the thread signal handler lock has been released, so
that we know that the signal allocation has either seen the new value or
has properly incremented the reference count of the old one.

Race identified by Oleg Nesterov.

Cc: Lukasz Trabinski <lukasz@xxxxxxxxxxxxx>
Cc: Oleg Nesterov <oleg@xxxxxxxxxx>
Cc: Andrew Morton <akpm@xxxxxxxx>
Cc: Ingo Molnar <mingo@xxxxxxx>
Signed-off-by: Linus Torvalds <torvalds@xxxxxxxx>

diff --git a/kernel/user.c b/kernel/user.c
index 6408c04..220e586 100644
--- a/kernel/user.c
+++ b/kernel/user.c
@@ -187,6 +187,17 @@ void switch_uid(struct user_struct *new_
atomic_dec(&old_user->processes);
switch_uid_keyring(new_user);
current->user = new_user;
+
+ /*
+ * We need to synchronize with __sigqueue_alloc()
+ * doing a get_uid(p->user).. If that saw the old
+ * user value, we need to wait until it has exited
+ * its critical region before we can free the old
+ * structure.
+ */
+ smp_mb();
+ spin_unlock_wait(&current->sighand->siglock);
+
free_uid(old_user);
suid_keys(current);
}
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/