[PATCH] keys: Discard key spinlock and use RCU for key payload

The attached patch changes the key implementation in a number of ways:

 (1) It removes the spinlock from the key structure.

 (2) The key flags are now accessed using atomic bitops instead of
     write-locking the key spinlock and using C bitwise operators.

     The three instantiation flags are dealt with with the construction
     semaphore held during the request_key/instantiate/negate sequence, thus
     rendering the spinlock superfluous.

     The key flags are also now bit numbers not bit masks.

 (3) The key payload is now accessed using RCU. This permits the recursive
     keyring search algorithm to be simplified greatly since no locks need be
     taken other than the usual RCU preemption disablement. Searching now does
     not require any locks or semaphores to be held; merely that the starting
     keyring be pinned.

 (4) The keyring payload now includes an RCU head so that it can be disposed
     of by call_rcu(). This requires that the payload be copied on unlink to
     prevent introducing races in copy-down vs search-up.

 (5) The user key payload is now a structure with the data following it. It
     includes an RCU head like the keyring payload and for the same reason. It
     also contains a data length because the data length in the key may be
     changed on another CPU whilst an RCU protected read is in progress on the
     payload. This would then see the supposed RCU payload and the on-key data
     length getting out of sync.

     I'm tempted to drop the key's datalen entirely, except that it's used in
     conjunction with quota management and so is a little tricky to get rid
     of.

 (6) Update the keys documentation.

Signed-Off-By: David Howells <dhowells@redhat.com>
Signed-off-by: Andrew Morton <akpm@osdl.org>
Signed-off-by: Linus Torvalds <torvalds@osdl.org>

1 files changed, 42 insertions, 52 deletions

diff --git a/security/keys/key.c b/security/keys/key.c
index 59402c843203..1fdfccb3fe43 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -294,7 +294,6 @@ struct key *key_alloc(struct key_type *type, const char *desc,
         }
 
         atomic_set(&key->usage, 1);
-        rwlock_init(&key->lock);
         init_rwsem(&key->sem);
         key->type = type;
         key->user = user;
@@ -308,7 +307,7 @@ struct key *key_alloc(struct key_type *type, const char *desc,
         key->payload.data = NULL;
 
         if (!not_in_quota)
-                key->flags |= KEY_FLAG_IN_QUOTA;
+                key->flags |= 1 << KEY_FLAG_IN_QUOTA;
 
         memset(&key->type_data, 0, sizeof(key->type_data));
 
@@ -359,7 +358,7 @@ int key_payload_reserve(struct key *key, size_t datalen)
         key_check(key);
 
         /* contemplate the quota adjustment */
-        if (delta != 0 && key->flags & KEY_FLAG_IN_QUOTA) {
+        if (delta != 0 && test_bit(KEY_FLAG_IN_QUOTA, &key->flags)) {
                 spin_lock(&key->user->lock);
 
                 if (delta > 0 &&
@@ -405,23 +404,17 @@ static int __key_instantiate_and_link(struct key *key,
         down_write(&key_construction_sem);
 
         /* can't instantiate twice */
-        if (!(key->flags & KEY_FLAG_INSTANTIATED)) {
+        if (!test_bit(KEY_FLAG_INSTANTIATED, &key->flags)) {
                 /* instantiate the key */
                 ret = key->type->instantiate(key, data, datalen);
 
                 if (ret == 0) {
                         /* mark the key as being instantiated */
-                        write_lock(&key->lock);
-
                         atomic_inc(&key->user->nikeys);
-                        key->flags |= KEY_FLAG_INSTANTIATED;
+                        set_bit(KEY_FLAG_INSTANTIATED, &key->flags);
 
-                        if (key->flags & KEY_FLAG_USER_CONSTRUCT) {
-                                key->flags &= ~KEY_FLAG_USER_CONSTRUCT;
+                        if (test_and_clear_bit(KEY_FLAG_USER_CONSTRUCT, &key->flags))
                                 awaken = 1;
-                        }
-
-                        write_unlock(&key->lock);
 
                         /* and link it into the destination keyring */
                         if (keyring)
@@ -486,21 +479,17 @@ int key_negate_and_link(struct key *key,
         down_write(&key_construction_sem);
 
         /* can't instantiate twice */
-        if (!(key->flags & KEY_FLAG_INSTANTIATED)) {
+        if (!test_bit(KEY_FLAG_INSTANTIATED, &key->flags)) {
                 /* mark the key as being negatively instantiated */
-                write_lock(&key->lock);
-
                 atomic_inc(&key->user->nikeys);
-                key->flags |= KEY_FLAG_INSTANTIATED | KEY_FLAG_NEGATIVE;
+                set_bit(KEY_FLAG_NEGATIVE, &key->flags);
+                set_bit(KEY_FLAG_INSTANTIATED, &key->flags);
                 now = current_kernel_time();
                 key->expiry = now.tv_sec + timeout;
 
-                if (key->flags & KEY_FLAG_USER_CONSTRUCT) {
-                        key->flags &= ~KEY_FLAG_USER_CONSTRUCT;
+                if (test_and_clear_bit(KEY_FLAG_USER_CONSTRUCT, &key->flags))
                         awaken = 1;
-                }
 
-                write_unlock(&key->lock);
                 ret = 0;
 
                 /* and link it into the destination keyring */
@@ -553,8 +542,10 @@ static void key_cleanup(void *data)
         rb_erase(&key->serial_node, &key_serial_tree);
         spin_unlock(&key_serial_lock);
 
+        key_check(key);
+
         /* deal with the user's key tracking and quota */
-        if (key->flags & KEY_FLAG_IN_QUOTA) {
+        if (test_bit(KEY_FLAG_IN_QUOTA, &key->flags)) {
                 spin_lock(&key->user->lock);
                 key->user->qnkeys--;
                 key->user->qnbytes -= key->quotalen;
@@ -562,7 +553,7 @@ static void key_cleanup(void *data)
         }
 
         atomic_dec(&key->user->nkeys);
-        if (key->flags & KEY_FLAG_INSTANTIATED)
+        if (test_bit(KEY_FLAG_INSTANTIATED, &key->flags))
                 atomic_dec(&key->user->nikeys);
 
         key_user_put(key->user);
@@ -631,9 +622,9 @@ struct key *key_lookup(key_serial_t id)
         goto error;
 
  found:
-        /* pretent doesn't exist if it's dead */
+        /* pretend it doesn't exist if it's dead */
         if (atomic_read(&key->usage) == 0 ||
-            (key->flags & KEY_FLAG_DEAD) ||
+            test_bit(KEY_FLAG_DEAD, &key->flags) ||
             key->type == &key_type_dead)
                 goto not_found;
 
@@ -708,12 +699,9 @@ static inline struct key *__key_update(struct key *key, const void *payload,
 
         ret = key->type->update(key, payload, plen);
 
-        if (ret == 0) {
+        if (ret == 0)
                 /* updating a negative key instantiates it */
-                write_lock(&key->lock);
-                key->flags &= ~KEY_FLAG_NEGATIVE;
-                write_unlock(&key->lock);
-        }
+                clear_bit(KEY_FLAG_NEGATIVE, &key->flags);
 
         up_write(&key->sem);
 
@@ -841,12 +829,9 @@ int key_update(struct key *key, const void *payload, size_t plen)
                 down_write(&key->sem);
                 ret = key->type->update(key, payload, plen);
 
-                if (ret == 0) {
+                if (ret == 0)
                         /* updating a negative key instantiates it */
-                        write_lock(&key->lock);
-                        key->flags &= ~KEY_FLAG_NEGATIVE;
-                        write_unlock(&key->lock);
-                }
+                        clear_bit(KEY_FLAG_NEGATIVE, &key->flags);
 
                 up_write(&key->sem);
         }
@@ -892,10 +877,7 @@ struct key *key_duplicate(struct key *source, const char *desc)
                 goto error2;
 
         atomic_inc(&key->user->nikeys);
-
-        write_lock(&key->lock);
-        key->flags |= KEY_FLAG_INSTANTIATED;
-        write_unlock(&key->lock);
+        set_bit(KEY_FLAG_INSTANTIATED, &key->flags);
 
  error_k:
         up_read(&key_types_sem);
@@ -922,9 +904,7 @@ void key_revoke(struct key *key)
         /* make sure no one's trying to change or use the key when we mark
          * it */
         down_write(&key->sem);
-        write_lock(&key->lock);
-        key->flags |= KEY_FLAG_REVOKED;
-        write_unlock(&key->lock);
+        set_bit(KEY_FLAG_REVOKED, &key->flags);
         up_write(&key->sem);
 
 } /* end key_revoke() */
@@ -975,24 +955,33 @@ void unregister_key_type(struct key_type *ktype)
         /* withdraw the key type */
         list_del_init(&ktype->link);
 
-        /* need to withdraw all keys of this type */
+        /* mark all the keys of this type dead */
         spin_lock(&key_serial_lock);
 
         for (_n = rb_first(&key_serial_tree); _n; _n = rb_next(_n)) {
                 key = rb_entry(_n, struct key, serial_node);
 
-                if (key->type != ktype)
-                        continue;
+                if (key->type == ktype)
+                        key->type = &key_type_dead;
+        }
+
+        spin_unlock(&key_serial_lock);
+
+        /* make sure everyone revalidates their keys */
+        synchronize_kernel();
+
+        /* we should now be able to destroy the payloads of all the keys of
+         * this type with impunity */
+        spin_lock(&key_serial_lock);
 
-                write_lock(&key->lock);
-                key->type = &key_type_dead;
-                write_unlock(&key->lock);
+        for (_n = rb_first(&key_serial_tree); _n; _n = rb_next(_n)) {
+                key = rb_entry(_n, struct key, serial_node);
 
-                /* there shouldn't be anyone looking at the description or
-                 * payload now */
-                if (ktype->destroy)
-                        ktype->destroy(key);
-                memset(&key->payload, 0xbd, sizeof(key->payload));
+                if (key->type == ktype) {
+                        if (ktype->destroy)
+                                ktype->destroy(key);
+                        memset(&key->payload, 0xbd, sizeof(key->payload));
+                }
         }
 
         spin_unlock(&key_serial_lock);
@@ -1037,4 +1026,5 @@ void __init key_init(void)
 
         /* link the two root keyrings together */
         key_link(&root_session_keyring, &root_user_keyring);
+
 } /* end key_init() */