Re: [RFC] install_session_keyring

[David Howells <dhowells@redhat.com>]

Suzanne Wood <suzannew@cs.pdx.edu> wrote:

> 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(),

No, it's not redundant, and neither is it risky, at least, not as far as I can
tell from the RCU docs.

> because key_put() opens with if(key)

That's just to permit key_put(NULL) to avoid going splat.

> and employs atomic_dec_and_test(&key->usage)) before
> schedule_work(&key_cleanup_task).

How else does the cleaner-upper know to dispose of the key? And which key?

The cleaner scans the entire key tree and weeds out any key that is dead.

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

It will not be reclaimed until after its refcount is zero, and once that
happens, there can't be any other valid links to it.

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

Sorry, I'm not sure what you mean.

I can't get rid of "old". I have to exchange the new pointer for the old
pointer and then release the old object, otherwise there's a resource leak.

However, I think the code should probably be:

	spin_lock_irqsave(&tsk->sighand->siglock, flags);
	old = tsk->signal->session_keyring;
	rcu_assign_pointer(tsk->signal->session_keyring, keyring);
	spin_unlock_irqrestore(&tsk->sighand->siglock, flags);

The code then calls synchronize_rcu() to make sure no one can then follow that
pointer and then releases the reference on it.  Anyone who wants the object to
persist beyond the rcu_read_lock()'d region must have incremented the refcount
whilst inside that region.  But once the synchronize_rcu() completes, it must
be safe for me to release the reference.

It's possible, if not probable, that a memory barrier is required before the
atomic_dec_and_test() in key_put().

I don't think one is required after an atomic_inc() between rcu_read_lock()
and rcu_read_unlock() because I think those need to imply LOCK/UNLOCK barrier
semantics.  Maybe Paul McKenney thinks otherwise, though I think I convinced
him to agree with me.

By the way, the use of schedule_work() to dispose of keys is so that the key
destroyer can be relatively slow; it also keeps the latency of key_get() as
minimal as possible in the slow case, but that's a lesser consideration.  It
also makes the locking simpler.  Note that there is no intention for the key
to persist any great length of time after its usage count reaches zero.

Note also that the replacement of the session keyring does not suggest that
the session keyring will most likely be destroyed; in fact the likelyhood is
that it won't.  It's a _session_ keyring, and is most probably going to be
shared by quite a few processes.


So, to sum up, I think there's three potential problems with my code:

 (1) key_put() probably needs smp_mb__before_atomic_dec().

 (2) key_get() may need smp_mb__after_atomic_inc() inside of rcu_read_lock()'d
     sections, but I don't think that's necessary as the mere fact it's inside
     a locked section should obviate that need.

 (3) rcu_dereference() is a waste of processing time inside the spinlocks in
     install_session_keyring().  It should be a direct dereference.  The
     semantics of spin_unlock() should obviate the need for the
     smp_read_barrier_depends().


But thanks for pointing out the potential problems in my code.  That's much
appreciated.  The kernel needs a good memory barrier audit.

David
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/