Re: [RFC] install_session_keyring

[Suzanne Wood]

  > From David Howells Mon Apr  3 01:46:08 2006

  > Suzanne Wood <suzannew@xxxxxxxxxx> wrote:

  > > In a study of the control flow graph dumps to check that 
  > > an rcu_assign_pointer() with a given argument type has 
  > > preceded a call to rcu_dereference(), I've come across 
  > > install_session_keyring() of security/keys/process_keys.c.  
  > > We note that although no rcu_read_lock() is in place 
  > > locally or in the function's kernel callers, siglock 
  > > likely addresses that.  While the rcu_dereference() would 
  > > indicate a desire for 'old' to persist, synchronize_rcu() 
  > > is called prior to key_put(old) which "disposes of 
  > > reference to a key."  The order of events with a use of 
  > > the copy of the pointer following synchronize_rcu() is 
  > > what I question.

  > Are you simply suggesting that the rcu_dereference() in:

  > 	/* install the keyring */
  >        	spin_lock_irqsave(&tsk->sighand->siglock, flags);
  >        	old = rcu_dereference(tsk->signal->session_keyring);
  >        	rcu_assign_pointer(tsk->signal->session_keyring, keyring);
  >        	spin_unlock_irqrestore(&tsk->sighand->siglock, flags);

  > is unnecessary?

Since RCU is applicable in read intensive environments and I
look for rcu_dereference() on the read side to be paired with
rcu_assign_pointer() on the update side, I wonder if key_put()
might be redundant or risky after synchronize_rcu(), because 
key_put() opens with if(key) and employs atomic_dec_and_test(&key->usage)) 
before schedule_work(&key_cleanup_task).  If the memory referenced 
by old has already been reclaimed which appears is made possible by 
synchronize_rcu(), it seems that there is a contention here.

Yes, so I guess you mean to get rid of 'old' altogether and the three
lines that refer to it.  But I think the original intent was to have
the former session keyring persist to some extent.
Thanks.
Suzanne 

  > If so, I think you are right since the pointer is only changed with the
  > siglock held[*], and so modify/modify conflict isn't a problem and doesn't
  > need memory barriers.

  > [*] Apart from during the exit() cleanup, when I don't think this should be a
  > problem anyway.

  > 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/