Re: [PATCH]2.6 test1 mm2 user.c race (?)

Re: [PATCH]2.6 test1 mm2 user.c race (?)

[Andrew Morton]

<ffrederick@prov-liege.be> wrote:
>
> +	spin_lock(&uidhash_lock);
>  	uid_hash_insert(&root_user, uidhashentry(0));
> +	spin_unlock(&uidhash_lock);	

This code runs within an initcall, so it is very unlikely that anything
will race with us here.

But SMP is up, and this code gets dropped out of memory later (the
out-of-line spinlock code doesn't get dropped though).

So yes, I'd prefer that the locking be there, if only for documentary
purposes.  A /* comment */ which explains why the locking was omitted would
also be suitabe.

Re: [PATCH]2.6 test1 mm2 user.c race (?)

[Bill Davidsen]

On Mon, 28 Jul 2003, Andrew Morton wrote:

> <ffrederick@prov-liege.be> wrote:
> >
> > +	spin_lock(&uidhash_lock);
> >  	uid_hash_insert(&root_user, uidhashentry(0));
> > +	spin_unlock(&uidhash_lock);	
> 
> This code runs within an initcall, so it is very unlikely that anything
> will race with us here.
> 
> But SMP is up, and this code gets dropped out of memory later (the
> out-of-line spinlock code doesn't get dropped though).
> 
> So yes, I'd prefer that the locking be there, if only for documentary
> purposes.  A /* comment */ which explains why the locking was omitted would
> also be suitabe.

I like the locking better than the comment, I trust the analysis today,
but with SMP and preempt, the lock protects the future (and you may be
missing something even today).

-- 
bill davidsen <davidsen@tmr.com>
  CTO, TMR Associates, Inc
Doing interesting things with little computers since 1979.

[PATCH]2.6 test1 mm2 user.c race (?)

[ffrederick]

Andrew,

        Trivial patch against possible smp race (?) in user.c
or do we have bkl above ?

Regards,
Fabian

diff -Naur orig/kernel/user.c edited/kernel/user.c
--- orig/kernel/user.c	2003-07-14 05:32:42.000000000 +0200
+++ edited/kernel/user.c	2003-07-28 13:44:55.000000000 +0200
@@ -146,8 +146,11 @@
 	for(n = 0; n < UIDHASH_SZ; ++n)
 		INIT_LIST_HEAD(uidhash_table + n);
 
-	/* Insert the root user immediately - init already runs with this */
+	/* Insert the root user immediately (init already runs as root) */
+	spin_lock(&uidhash_lock);
 	uid_hash_insert(&root_user, uidhashentry(0));
+	spin_unlock(&uidhash_lock);	
+
 	return 0;
 }
 


___________________________________