Re: [PATCH 2/3] Keys: Use RCU to manage session keyring pointer

[Paul E. McKenney]

On Tue, Apr 12, 2005 at 10:11:50AM +0100, David Howells wrote:
> Paul E. McKenney <paulmck@xxxxxxxxxx> wrote:
> 
> > >  	spin_lock_irqsave(&tsk->sighand->siglock, flags);
> > > -	old = tsk->signal->session_keyring;
> > > -	tsk->signal->session_keyring = keyring;
> > > +	old = rcu_dereference(tsk->signal->session_keyring);
> > 
> > I don't understand why rcu_dereference() is needed in this case.
> > Since we are holding the lock, it should not be possible for
> > this to change, right?  Or am I missing something?  (Quite possible,
> > am not all that familiar with this code.)
> 
> Erm... you're right. I stuck the rcu_dereference() in then added the locks
> back in when I realised I still needed them.
> 
> > > +	synchronize_kernel();
> > 
> > This would want to become synchronize_rcu().
> 
> I think the deprecation happened since I wrote my patch.

Yes, sorry, I should have made it clear that this was a change that
affected your code rather than an error on your part.

> > > +	if (tsk->signal->session_keyring) {
> > > +		rcu_read_lock();
> > > +		key = keyring_search_aux(
> > > +			rcu_dereference(tsk->signal->session_keyring),
> > > +			type, description, match);
> > > +		rcu_read_unlock();
> > > +	}
> > > +	else {
> > > +		key = keyring_search_aux(tsk->user->session_keyring,
> > > +					 type, description, match);
> > 
> > This one is constant, right?  If not, I don't understand the locking design.
> 
> Which one? tsk->user->session_keyring is, tsk->signal->session_keyring is not.

Good, that matches the code!

							Thanx, Paul

> Thanks for the review.
> 
> David
> 
-
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/