diff mbox

[v9,01/11] KEYS: Add a rwsem to each key type

Message ID 20161116223806.14601-2-mathew.j.martineau@linux.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Mat Martineau Nov. 16, 2016, 10:37 p.m. UTC
The key_types_sem read-write semaphore has been performing two jobs,
protecting the key_types_list and making sure individual key types are
not unregistered while they are in use. It is held for most of
key_create_or_update, which means a second key type cannot be looked up
by name while a key is being created or updated without re-acquiring the
semaphore and triggering a lockdep warning:

[  445.752215] =============================================
[  445.752946] [ INFO: possible recursive locking detected ]
[  445.753745] 4.8.0-rc1+ #34 Not tainted
[  445.754087] ---------------------------------------------
[  445.754540] test-key/948 is trying to acquire lock:
[  445.754950]  (key_types_sem){++++.+}, at: [<ffffffff903c4dcb>] key_type_lookup+0x1b/0x80
[  445.755854]
[  445.755854] but task is already holding lock:
[  445.756515]  (key_types_sem){++++.+}, at: [<ffffffff903c4dcb>] key_type_lookup+0x1b/0x80

By adding a rwsem to struct key_type, key_types_sem only needs to be
held when key_types_list is being accessed. key_type_put() is modified
to only release the given key_type (the key_type parameter to this
function was previously ignored).

Signed-off-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
---
 include/linux/key-type.h |  1 +
 security/keys/gc.c       |  2 +-
 security/keys/key.c      | 32 +++++++++++++++++++++++---------
 3 files changed, 25 insertions(+), 10 deletions(-)

Comments

David Howells Nov. 24, 2016, 5:38 p.m. UTC | #1
Mat Martineau <mathew.j.martineau@linux.intel.com> wrote:

> +static void add_special_key_type(struct key_type *ktype)
> +{
> +	init_rwsem(&ktype->sem);
> +	list_add_tail(&ktype->link, &key_types_list);
> +}
> +

This should probably be __init just in case the compiler decides not to inline
it.  I can add that.

David
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/include/linux/key-type.h b/include/linux/key-type.h
index eaee981..1f5b863 100644
--- a/include/linux/key-type.h
+++ b/include/linux/key-type.h
@@ -150,6 +150,7 @@  struct key_type {
 	/* internal fields */
 	struct list_head	link;		/* link in types list */
 	struct lock_class_key	lock_class;	/* key->sem lock class */
+	struct rw_semaphore	sem;		/* for safe key type removal */
 };
 
 extern struct key_type key_type_keyring;
diff --git a/security/keys/gc.c b/security/keys/gc.c
index addf060..467af2c 100644
--- a/security/keys/gc.c
+++ b/security/keys/gc.c
@@ -99,7 +99,7 @@  static void key_gc_timer_func(unsigned long data)
  * collect dead links and the third to clean up the dead keys.  We have to be
  * careful as there may already be a cycle in progress.
  *
- * The caller must be holding key_types_sem.
+ * The caller must be holding key_types_sem and ktype->sem.
  */
 void key_gc_keytype(struct key_type *ktype)
 {
diff --git a/security/keys/key.c b/security/keys/key.c
index 346fbf2..c3556e5 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -672,8 +672,8 @@  struct key *key_lookup(key_serial_t id)
 /*
  * Find and lock the specified key type against removal.
  *
- * We return with the sem read-locked if successful.  If the type wasn't
- * available -ENOKEY is returned instead.
+ * We return with the key type's sem read-locked if successful.  If the
+ * type wasn't available -ENOKEY is returned instead.
  */
 struct key_type *key_type_lookup(const char *type)
 {
@@ -684,14 +684,18 @@  struct key_type *key_type_lookup(const char *type)
 	/* look up the key type to see if it's one of the registered kernel
 	 * types */
 	list_for_each_entry(ktype, &key_types_list, link) {
-		if (strcmp(ktype->name, type) == 0)
+		/* ktype->sem is only write locked when unregistering, so
+		 * ignore a key type with a contended lock.
+		 */
+		if ((strcmp(ktype->name, type) == 0) &&
+		    down_read_trylock(&ktype->sem))
 			goto found_kernel_type;
 	}
 
-	up_read(&key_types_sem);
 	ktype = ERR_PTR(-ENOKEY);
 
 found_kernel_type:
+	up_read(&key_types_sem);
 	return ktype;
 }
 
@@ -720,7 +724,8 @@  EXPORT_SYMBOL_GPL(key_set_timeout);
  */
 void key_type_put(struct key_type *ktype)
 {
-	up_read(&key_types_sem);
+	if (ktype)
+		up_read(&ktype->sem);
 }
 
 /*
@@ -1102,6 +1107,7 @@  int register_key_type(struct key_type *ktype)
 	int ret;
 
 	memset(&ktype->lock_class, 0, sizeof(ktype->lock_class));
+	init_rwsem(&ktype->sem);
 
 	ret = -EEXIST;
 	down_write(&key_types_sem);
@@ -1135,14 +1141,22 @@  EXPORT_SYMBOL(register_key_type);
 void unregister_key_type(struct key_type *ktype)
 {
 	down_write(&key_types_sem);
+	down_write(&ktype->sem);
 	list_del_init(&ktype->link);
 	downgrade_write(&key_types_sem);
 	key_gc_keytype(ktype);
 	pr_notice("Key type %s unregistered\n", ktype->name);
+	up_write(&ktype->sem);
 	up_read(&key_types_sem);
 }
 EXPORT_SYMBOL(unregister_key_type);
 
+static void add_special_key_type(struct key_type *ktype)
+{
+	init_rwsem(&ktype->sem);
+	list_add_tail(&ktype->link, &key_types_list);
+}
+
 /*
  * Initialise the key management state.
  */
@@ -1153,10 +1167,10 @@  void __init key_init(void)
 			0, SLAB_HWCACHE_ALIGN|SLAB_PANIC, NULL);
 
 	/* add the special key types */
-	list_add_tail(&key_type_keyring.link, &key_types_list);
-	list_add_tail(&key_type_dead.link, &key_types_list);
-	list_add_tail(&key_type_user.link, &key_types_list);
-	list_add_tail(&key_type_logon.link, &key_types_list);
+	add_special_key_type(&key_type_keyring);
+	add_special_key_type(&key_type_dead);
+	add_special_key_type(&key_type_user);
+	add_special_key_type(&key_type_logon);
 
 	/* record the root user tracking */
 	rb_link_node(&root_key_user.node,