On Wed, Mar 23, 2005 at 08:55:46PM +0000, David Howells wrote: > > The attached patch uses RCU to manage the session keyring pointer in struct > signal_struct. This means that searching need not disable interrupts and get a > the sighand spinlock to access this pointer. Furthermore, by judicious use of > rcu_read_(un)lock(), this patch also avoids the need to take and put refcounts > on the session keyring itself, thus saving on even more atomic ops. > > Signed-Off-By: David Howells <[EMAIL PROTECTED]>
This looks quite good! A few questions interspersed below. Thanx, Paul PS. Sorry to be so slow in getting to this one! > --- > warthog>diffstat -p1 keys-rcu-session-2612rc1mm1.diff > security/keys/process_keys.c | 42 > +++++++++++++++++++++--------------------- > security/keys/request_key.c | 7 +++---- > 2 files changed, 24 insertions(+), 25 deletions(-) > > diff -uNrp linux-2.6.12-rc1-mm1-keys-umhelper/security/keys/process_keys.c > linux-2.6.12-rc1-mm1-keys-rcu-session/security/keys/process_keys.c > --- linux-2.6.12-rc1-mm1-keys-umhelper/security/keys/process_keys.c > 2005-03-23 17:22:46.000000000 +0000 > +++ linux-2.6.12-rc1-mm1-keys-rcu-session/security/keys/process_keys.c > 2005-03-23 18:27:12.055768099 +0000 > @@ -1,6 +1,6 @@ > /* process_keys.c: management of a process's keyrings > * > - * Copyright (C) 2004 Red Hat, Inc. All Rights Reserved. > + * Copyright (C) 2004-5 Red Hat, Inc. All Rights Reserved. > * Written by David Howells ([EMAIL PROTECTED]) > * > * This program is free software; you can redistribute it and/or > @@ -181,7 +181,7 @@ static int install_process_keyring(struc > goto error; > } > > - /* attach or swap keyrings */ > + /* attach keyring */ > spin_lock_irqsave(&tsk->sighand->siglock, flags); > if (!tsk->signal->process_keyring) { > tsk->signal->process_keyring = keyring; > @@ -227,12 +227,14 @@ static int install_session_keyring(struc > > /* install the keyring */ > 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.) > + rcu_assign_pointer(tsk->signal->session_keyring, keyring); > spin_unlock_irqrestore(&tsk->sighand->siglock, flags); > > ret = 0; > > + /* we're using RCU on the pointer */ > + synchronize_kernel(); This would want to become synchronize_rcu(). > key_put(old); > error: > return ret; > @@ -245,8 +247,6 @@ static int install_session_keyring(struc > */ > int copy_thread_group_keys(struct task_struct *tsk) > { > - unsigned long flags; > - > key_check(current->thread_group->session_keyring); > key_check(current->thread_group->process_keyring); > > @@ -254,10 +254,10 @@ int copy_thread_group_keys(struct task_s > tsk->signal->process_keyring = NULL; > > /* same session keyring */ > - spin_lock_irqsave(¤t->sighand->siglock, flags); > + rcu_read_lock(); > tsk->signal->session_keyring = > - key_get(current->signal->session_keyring); > - spin_unlock_irqrestore(¤t->sighand->siglock, flags); > + key_get(rcu_dereference(current->signal->session_keyring)); > + rcu_read_unlock(); > > return 0; > > @@ -381,8 +381,7 @@ struct key *search_process_keyrings_aux( > key_match_func_t match) > { > struct task_struct *tsk = current; > - unsigned long flags; > - struct key *key, *ret, *err, *tmp; > + struct key *key, *ret, *err; > > /* we want to return -EAGAIN or -ENOKEY if any of the keyrings were > * searchable, but we failed to find a key or we found a negative key; > @@ -436,17 +435,18 @@ struct key *search_process_keyrings_aux( > } > > /* search the session keyring last */ > - spin_lock_irqsave(&tsk->sighand->siglock, flags); > - > - tmp = tsk->signal->session_keyring; > - if (!tmp) > - tmp = tsk->user->session_keyring; > - atomic_inc(&tmp->usage); > - > - spin_unlock_irqrestore(&tsk->sighand->siglock, flags); > + 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. > + } > > - key = keyring_search_aux(tmp, type, description, match); > - key_put(tmp); > if (!IS_ERR(key)) > goto found; > > diff -uNrp linux-2.6.12-rc1-mm1-keys-umhelper/security/keys/request_key.c > linux-2.6.12-rc1-mm1-keys-rcu-session/security/keys/request_key.c > --- linux-2.6.12-rc1-mm1-keys-umhelper/security/keys/request_key.c > 2005-03-23 17:35:16.000000000 +0000 > +++ linux-2.6.12-rc1-mm1-keys-rcu-session/security/keys/request_key.c > 2005-03-23 18:14:13.908029567 +0000 > @@ -175,13 +175,12 @@ static struct key *__request_key_constru > key->expiry = now.tv_sec + key_negative_timeout; > > if (current->signal->session_keyring) { > - unsigned long flags; > struct key *keyring; > > - spin_lock_irqsave(¤t->sighand->siglock, flags); > - keyring = current->signal->session_keyring; > + rcu_read_lock(); > + keyring = rcu_dereference(current->signal->session_keyring); > atomic_inc(&keyring->usage); > - spin_unlock_irqrestore(¤t->sighand->siglock, flags); > + rcu_read_unlock(); > > key_link(keyring, key); > key_put(keyring); > - > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to [EMAIL PROTECTED] > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ > > - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/