[RFC PATCH 0/4] KEYS: sanitize the garbage collector behavior

[RFC PATCH 0/4] KEYS: sanitize the garbage collector behavior

[Jarkko Sakkinen]

1. Remove key_type_dead and replace it with the regular module semantics.
2. Replace variable size workqueue items with fixed-size 64 key chunks.

The first change has user space visible effect: rmmod semantics will change
from always succeeding into returning -EBUSY when keys or restrictions have
the LKM pinned.

Jarkko Sakkinen (4):
  KEYS: trusted: Disable sysfs bind attributes for TEE driver
  KEYS: Pin the backing module for struct key_type
  KEYS: Remove key_type_dead
  KEYS: key_garbage_collector(): process in chunks

 include/linux/key-type.h                 |   6 +-
 security/keys/gc.c                       | 249 ++++++++---------------
 security/keys/internal.h                 |   7 +-
 security/keys/key.c                      |  46 +++--
 security/keys/keyring.c                  |  84 +++-----
 security/keys/trusted-keys/trusted_tee.c |   1 +
 6 files changed, 148 insertions(+), 245 deletions(-)

--
2.47.3

[RFC PATCH 1/4] KEYS: trusted: Disable sysfs bind attributes for TEE driver

[Jarkko Sakkinen]

trusted_key_exit() calls unregister_key_type() from the TEE client
driver .remove() callback. Suppress sysfs bind and unbind attributes on
trusted_key_driver so userspace cannot unbind the driver and unregister
key_type_trusted while the trusted module remains loaded.

Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
---
 security/keys/trusted-keys/trusted_tee.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/security/keys/trusted-keys/trusted_tee.c b/security/keys/trusted-keys/trusted_tee.c
index 6e465c8bef5e..5d1288189e5c 100644
--- a/security/keys/trusted-keys/trusted_tee.c
+++ b/security/keys/trusted-keys/trusted_tee.c
@@ -264,6 +264,7 @@ static struct tee_client_driver trusted_key_driver = {
 	.id_table	= trusted_key_id_table,
 	.driver		= {
 		.name		= DRIVER_NAME,
+		.suppress_bind_attrs = true,
 	},
 };
 
-- 
2.47.3

[RFC PATCH 2/4] KEYS: Pin the backing module for struct key_type

[Jarkko Sakkinen]

Add the owner field (populated with THIS_MODULE) to struct key_type.

Pin the module in key_alloc() and keyring_restrict(), and release it in
key_gc_unused_keys() and keyring_destroy(). These functions are the entry
points for keys and restrictions.

Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
---
 include/linux/key-type.h |  6 +++++-
 security/keys/gc.c       |  3 +++
 security/keys/key.c      | 33 ++++++++++++++++++++++-----------
 security/keys/keyring.c  | 32 +++++++++++++++++++++++++-------
 4 files changed, 55 insertions(+), 19 deletions(-)

diff --git a/include/linux/key-type.h b/include/linux/key-type.h
index c4f329b6ae3f..9ecec31ad571 100644
--- a/include/linux/key-type.h
+++ b/include/linux/key-type.h
@@ -10,9 +10,11 @@
 
 #include <linux/key.h>
 #include <linux/errno.h>
+#include <linux/init.h>
 
 #ifdef CONFIG_KEYS
 
+struct module;
 struct kernel_pkey_query;
 struct kernel_pkey_params;
 
@@ -161,6 +163,7 @@ struct key_type {
 				     const void *in, const void *in2);
 
 	/* internal fields */
+	struct module		*owner;
 	atomic64_t		index_id;	/* Opaque keyring index ID */
 	struct list_head	link;		/* link in types list */
 	struct lock_class_key	lock_class;	/* key->sem lock class */
@@ -168,7 +171,8 @@ struct key_type {
 
 extern struct key_type key_type_keyring;
 
-extern int register_key_type(struct key_type *ktype);
+int __register_key_type(struct key_type *ktype, struct module *owner);
+#define register_key_type(ktype) __register_key_type(ktype, THIS_MODULE)
 extern void unregister_key_type(struct key_type *ktype);
 
 extern int key_payload_reserve(struct key *key, size_t datalen);
diff --git a/security/keys/gc.c b/security/keys/gc.c
index eda445f815d4..df5c614e4d1d 100644
--- a/security/keys/gc.c
+++ b/security/keys/gc.c
@@ -6,6 +6,7 @@
  */
 
 #include <linux/slab.h>
+#include <linux/module.h>
 #include <linux/security.h>
 #include <keys/keyring-type.h>
 #include "internal.h"
@@ -137,6 +138,7 @@ static noinline void key_gc_unused_keys(struct list_head *keys)
 	while (!list_empty(keys)) {
 		struct key *key =
 			list_entry(keys->next, struct key, graveyard_link);
+		struct module *owner = key->type->owner;
 		short state = key->state;
 
 		list_del(&key->graveyard_link);
@@ -165,6 +167,7 @@ static noinline void key_gc_unused_keys(struct list_head *keys)
 
 		memzero_explicit(key, sizeof(*key));
 		kmem_cache_free(key_jar, key);
+		module_put(owner);
 	}
 }
 
diff --git a/security/keys/key.c b/security/keys/key.c
index 3f1f0fc13710..5ba8810f903d 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -7,6 +7,7 @@
 
 #include <linux/export.h>
 #include <linux/init.h>
+#include <linux/module.h>
 #include <linux/poison.h>
 #include <linux/sched.h>
 #include <linux/slab.h>
@@ -234,19 +235,24 @@ struct key *key_alloc(struct key_type *type, const char *desc,
 
 	key = ERR_PTR(-EINVAL);
 	if (!desc || !*desc)
-		goto error;
+		goto out;
+
+	if (!try_module_get(type->owner)) {
+		key = ERR_PTR(-ENOENT);
+		goto out;
+	}
 
 	if (type->vet_description) {
 		ret = type->vet_description(desc);
 		if (ret < 0) {
 			key = ERR_PTR(ret);
-			goto error;
+			goto out_put_module;
 		}
 	}
 
 	desclen = strlen(desc);
 	if (desclen >= KEY_MAX_DESC_SIZE)
-		goto error;
+		goto out_put_module;
 	quotalen = desclen + 1 + type->def_datalen;
 
 	/* get hold of the key tracking for this user */
@@ -323,8 +329,6 @@ struct key *key_alloc(struct key_type *type, const char *desc,
 	refcount_inc(&key->domain_tag->usage);
 	atomic_inc(&user->nkeys);
 	key_alloc_serial(key);
-
-error:
 	return key;
 
 security_error:
@@ -338,7 +342,7 @@ struct key *key_alloc(struct key_type *type, const char *desc,
 	}
 	key_user_put(user);
 	key = ERR_PTR(ret);
-	goto error;
+	goto out_put_module;
 
 no_memory_3:
 	kmem_cache_free(key_jar, key);
@@ -352,13 +356,18 @@ struct key *key_alloc(struct key_type *type, const char *desc,
 	key_user_put(user);
 no_memory_1:
 	key = ERR_PTR(-ENOMEM);
-	goto error;
+	goto out_put_module;
 
 no_quota:
 	spin_unlock_irqrestore(&user->lock, irqflags);
 	key_user_put(user);
 	key = ERR_PTR(-EDQUOT);
-	goto error;
+
+out_put_module:
+	module_put(type->owner);
+
+out:
+	return key;
 }
 EXPORT_SYMBOL(key_alloc);
 
@@ -1231,18 +1240,20 @@ int generic_key_instantiate(struct key *key, struct key_preparsed_payload *prep)
 EXPORT_SYMBOL(generic_key_instantiate);
 
 /**
- * register_key_type - Register a type of key.
+ * __register_key_type - Register a type of key.
  * @ktype: The new key type.
+ * @owner: Module owning the key type.
  *
  * Register a new key type.
  *
  * Returns 0 on success or -EEXIST if a type of this name already exists.
  */
-int register_key_type(struct key_type *ktype)
+int __register_key_type(struct key_type *ktype, struct module *owner)
 {
 	struct key_type *p;
 	int ret;
 
+	ktype->owner = owner;
 	memset(&ktype->lock_class, 0, sizeof(ktype->lock_class));
 
 	ret = -EEXIST;
@@ -1264,7 +1275,7 @@ int register_key_type(struct key_type *ktype)
 	up_write(&key_types_sem);
 	return ret;
 }
-EXPORT_SYMBOL(register_key_type);
+EXPORT_SYMBOL(__register_key_type);
 
 /**
  * unregister_key_type - Unregister a type of key.
diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index 5281da9d5bf7..2061fc2022a5 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -7,6 +7,7 @@
 
 #include <linux/export.h>
 #include <linux/init.h>
+#include <linux/module.h>
 #include <linux/sched.h>
 #include <linux/slab.h>
 #include <linux/security.h>
@@ -436,6 +437,9 @@ static void keyring_destroy(struct key *keyring)
 	if (keyring->restrict_link) {
 		struct key_restriction *keyres = keyring->restrict_link;
 
+		if (keyres->keytype)
+			module_put(keyres->keytype->owner);
+
 		key_put(keyres->key);
 		kfree(keyres);
 	}
@@ -1062,7 +1066,7 @@ int keyring_restrict(key_ref_t keyring_ref, const char *type,
 
 		if (!restrict_type->lookup_restriction) {
 			ret = -ENOENT;
-			goto error;
+			goto out;
 		}
 
 		restrict_link = restrict_type->lookup_restriction(restriction);
@@ -1070,7 +1074,13 @@ int keyring_restrict(key_ref_t keyring_ref, const char *type,
 
 	if (IS_ERR(restrict_link)) {
 		ret = PTR_ERR(restrict_link);
-		goto error;
+		goto out;
+	}
+
+	if (restrict_link->keytype &&
+	    !try_module_get(restrict_link->keytype->owner)) {
+		ret = -ENOENT;
+		goto out_free_restriction;
 	}
 
 	down_write(&keyring->sem);
@@ -1088,12 +1098,20 @@ int keyring_restrict(key_ref_t keyring_ref, const char *type,
 	up_write(&keyring_serialise_restrict_sem);
 	up_write(&keyring->sem);
 
-	if (ret < 0) {
-		key_put(restrict_link->key);
-		kfree(restrict_link);
-	}
+	if (ret < 0)
+		goto out_put_module;
 
-error:
+	goto out;
+
+out_put_module:
+	if (restrict_link->keytype)
+		module_put(restrict_link->keytype->owner);
+
+out_free_restriction:
+	key_put(restrict_link->key);
+	kfree(restrict_link);
+
+out:
 	if (restrict_type)
 		key_type_put(restrict_type);
 
-- 
2.47.3

[RFC PATCH 3/4] KEYS: Remove key_type_dead

[Jarkko Sakkinen]

unregister_key_type() no longer needs to retype live keys as keys and
restrictions pin the module while they reference it.

Remove key_type_dead and associated code from the keyring GC.

Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
---
 security/keys/gc.c       | 136 ++-------------------------------------
 security/keys/internal.h |   4 --
 security/keys/key.c      |   9 +--
 security/keys/keyring.c  |  50 --------------
 4 files changed, 9 insertions(+), 190 deletions(-)

diff --git a/security/keys/gc.c b/security/keys/gc.c
index df5c614e4d1d..876c30c019ad 100644
--- a/security/keys/gc.c
+++ b/security/keys/gc.c
@@ -29,22 +29,8 @@ static void key_gc_timer_func(struct timer_list *);
 static DEFINE_TIMER(key_gc_timer, key_gc_timer_func);
 
 static time64_t key_gc_next_run = TIME64_MAX;
-static struct key_type *key_gc_dead_keytype;
-
 static unsigned long key_gc_flags;
 #define KEY_GC_KEY_EXPIRED	0	/* A key expired and needs unlinking */
-#define KEY_GC_REAP_KEYTYPE	1	/* A keytype is being unregistered */
-#define KEY_GC_REAPING_KEYTYPE	2	/* Cleared when keytype reaped */
-
-
-/*
- * Any key whose type gets unregistered will be re-typed to this if it can't be
- * immediately unlinked.
- */
-struct key_type key_type_dead = {
-	.name = ".dead",
-};
-
 /*
  * Schedule a garbage collection run.
  * - time precision isn't particularly important
@@ -56,7 +42,7 @@ void key_schedule_gc(time64_t gc_at)
 
 	kenter("%lld", gc_at - now);
 
-	if (gc_at <= now || test_bit(KEY_GC_REAP_KEYTYPE, &key_gc_flags)) {
+	if (gc_at <= now) {
 		kdebug("IMMEDIATE");
 		schedule_work(&key_gc_work);
 	} else if (gc_at < key_gc_next_run) {
@@ -100,36 +86,6 @@ static void key_gc_timer_func(struct timer_list *unused)
 	key_schedule_gc_links();
 }
 
-/*
- * Reap keys of dead type.
- *
- * We use three flags to make sure we see three complete cycles of the garbage
- * collector: the first to mark keys of that type as being dead, the second to
- * 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.
- */
-void key_gc_keytype(struct key_type *ktype)
-{
-	kenter("%s", ktype->name);
-
-	key_gc_dead_keytype = ktype;
-	set_bit(KEY_GC_REAPING_KEYTYPE, &key_gc_flags);
-	smp_mb();
-	set_bit(KEY_GC_REAP_KEYTYPE, &key_gc_flags);
-
-	kdebug("schedule");
-	schedule_work(&key_gc_work);
-
-	kdebug("sleep");
-	wait_on_bit(&key_gc_flags, KEY_GC_REAPING_KEYTYPE,
-		    TASK_UNINTERRUPTIBLE);
-
-	key_gc_dead_keytype = NULL;
-	kleave("");
-}
-
 /*
  * Garbage collect a list of unreferenced, detached keys
  */
@@ -181,30 +137,20 @@ static noinline void key_gc_unused_keys(struct list_head *keys)
 static void key_garbage_collector(struct work_struct *work)
 {
 	static LIST_HEAD(graveyard);
-	static u8 gc_state;		/* Internal persistent state */
+	u8 gc_state = 0;
 #define KEY_GC_REAP_AGAIN	0x01	/* - Need another cycle */
 #define KEY_GC_REAPING_LINKS	0x02	/* - We need to reap links */
-#define KEY_GC_REAPING_DEAD_1	0x10	/* - We need to mark dead keys */
-#define KEY_GC_REAPING_DEAD_2	0x20	/* - We need to reap dead key links */
-#define KEY_GC_REAPING_DEAD_3	0x40	/* - We need to reap dead keys */
-#define KEY_GC_FOUND_DEAD_KEY	0x80	/* - We found at least one dead key */
 
 	struct rb_node *cursor;
 	struct key *key;
 	time64_t new_timer, limit, expiry;
 
-	kenter("[%lx,%x]", key_gc_flags, gc_state);
+	kenter("[%lx]", key_gc_flags);
 
 	limit = ktime_get_real_seconds();
 
-	/* Work out what we're going to be doing in this pass */
-	gc_state &= KEY_GC_REAPING_DEAD_1 | KEY_GC_REAPING_DEAD_2;
-	gc_state <<= 1;
 	if (test_and_clear_bit(KEY_GC_KEY_EXPIRED, &key_gc_flags))
 		gc_state |= KEY_GC_REAPING_LINKS;
-
-	if (test_and_clear_bit(KEY_GC_REAP_KEYTYPE, &key_gc_flags))
-		gc_state |= KEY_GC_REAPING_DEAD_1;
 	kdebug("new pass %x", gc_state);
 
 	new_timer = TIME64_MAX;
@@ -226,18 +172,6 @@ static void key_garbage_collector(struct work_struct *work)
 			goto found_unreferenced_key;
 		}
 
-		if (unlikely(gc_state & KEY_GC_REAPING_DEAD_1)) {
-			if (key->type == key_gc_dead_keytype) {
-				gc_state |= KEY_GC_FOUND_DEAD_KEY;
-				set_bit(KEY_FLAG_DEAD, &key->flags);
-				key->perm = 0;
-				goto skip_dead_key;
-			} else if (key->type == &key_type_keyring &&
-				   key->restrict_link) {
-				goto found_restricted_keyring;
-			}
-		}
-
 		expiry = key->expiry;
 		if (expiry != TIME64_MAX) {
 			if (!(key->type->flags & KEY_TYPE_INSTANT_REAP))
@@ -249,21 +183,10 @@ static void key_garbage_collector(struct work_struct *work)
 			}
 		}
 
-		if (unlikely(gc_state & KEY_GC_REAPING_DEAD_2))
-			if (key->type == key_gc_dead_keytype)
-				gc_state |= KEY_GC_FOUND_DEAD_KEY;
-
-		if ((gc_state & KEY_GC_REAPING_LINKS) ||
-		    unlikely(gc_state & KEY_GC_REAPING_DEAD_2)) {
-			if (key->type == &key_type_keyring)
-				goto found_keyring;
-		}
-
-		if (unlikely(gc_state & KEY_GC_REAPING_DEAD_3))
-			if (key->type == key_gc_dead_keytype)
-				goto destroy_dead_key;
+		if ((gc_state & KEY_GC_REAPING_LINKS) &&
+		    key->type == &key_type_keyring)
+			goto found_keyring;
 
-	skip_dead_key:
 		if (spin_is_contended(&key_serial_lock) || need_resched())
 			goto contended;
 	}
@@ -289,8 +212,7 @@ static void key_garbage_collector(struct work_struct *work)
 		key_schedule_gc(new_timer);
 	}
 
-	if (unlikely(gc_state & KEY_GC_REAPING_DEAD_2) ||
-	    !list_empty(&graveyard)) {
+	if (!list_empty(&graveyard)) {
 		/* Make sure that all pending keyring payload destructions are
 		 * fulfilled and that people aren't now looking at dead or
 		 * dying keys that they don't have a reference upon or a link
@@ -298,32 +220,10 @@ static void key_garbage_collector(struct work_struct *work)
 		 */
 		kdebug("gc sync");
 		synchronize_rcu();
-	}
-
-	if (!list_empty(&graveyard)) {
 		kdebug("gc keys");
 		key_gc_unused_keys(&graveyard);
 	}
 
-	if (unlikely(gc_state & (KEY_GC_REAPING_DEAD_1 |
-				 KEY_GC_REAPING_DEAD_2))) {
-		if (!(gc_state & KEY_GC_FOUND_DEAD_KEY)) {
-			/* No remaining dead keys: short circuit the remaining
-			 * keytype reap cycles.
-			 */
-			kdebug("dead short");
-			gc_state &= ~(KEY_GC_REAPING_DEAD_1 | KEY_GC_REAPING_DEAD_2);
-			gc_state |= KEY_GC_REAPING_DEAD_3;
-		} else {
-			gc_state |= KEY_GC_REAP_AGAIN;
-		}
-	}
-
-	if (unlikely(gc_state & KEY_GC_REAPING_DEAD_3)) {
-		kdebug("dead wake");
-		clear_and_wake_up_bit(KEY_GC_REAPING_KEYTYPE, &key_gc_flags);
-	}
-
 	if (gc_state & KEY_GC_REAP_AGAIN)
 		schedule_work(&key_gc_work);
 	kleave(" [end %x]", gc_state);
@@ -341,14 +241,6 @@ static void key_garbage_collector(struct work_struct *work)
 	gc_state |= KEY_GC_REAP_AGAIN;
 	goto maybe_resched;
 
-	/* We found a restricted keyring and need to update the restriction if
-	 * it is associated with the dead key type.
-	 */
-found_restricted_keyring:
-	spin_unlock(&key_serial_lock);
-	keyring_restriction_gc(key, key_gc_dead_keytype);
-	goto maybe_resched;
-
 	/* We found a keyring and we need to check the payload for links to
 	 * dead or expired keys.  We don't flag another reap immediately as we
 	 * have to wait for the old payload to be destroyed by RCU before we
@@ -358,18 +250,4 @@ static void key_garbage_collector(struct work_struct *work)
 	spin_unlock(&key_serial_lock);
 	keyring_gc(key, limit);
 	goto maybe_resched;
-
-	/* We found a dead key that is still referenced.  Reset its type and
-	 * destroy its payload with its semaphore held.
-	 */
-destroy_dead_key:
-	spin_unlock(&key_serial_lock);
-	kdebug("destroy key %d", key->serial);
-	down_write(&key->sem);
-	key->type = &key_type_dead;
-	if (key_gc_dead_keytype->destroy)
-		key_gc_dead_keytype->destroy(key);
-	memset(&key->payload, KEY_DESTROY, sizeof(key->payload));
-	up_write(&key->sem);
-	goto maybe_resched;
 }
diff --git a/security/keys/internal.h b/security/keys/internal.h
index b7b622bc36a1..005d609ae2df 100644
--- a/security/keys/internal.h
+++ b/security/keys/internal.h
@@ -38,7 +38,6 @@ struct iovec;
 	no_printk(KERN_DEBUG FMT"\n", ##__VA_ARGS__)
 #endif
 
-extern struct key_type key_type_dead;
 extern struct key_type key_type_user;
 extern struct key_type key_type_logon;
 
@@ -165,12 +164,9 @@ extern void key_change_session_keyring(struct callback_head *twork);
 extern struct work_struct key_gc_work;
 extern unsigned key_gc_delay;
 extern void keyring_gc(struct key *keyring, time64_t limit);
-extern void keyring_restriction_gc(struct key *keyring,
-				   struct key_type *dead_type);
 void key_set_expiry(struct key *key, time64_t expiry);
 extern void key_schedule_gc(time64_t gc_at);
 extern void key_schedule_gc_links(void);
-extern void key_gc_keytype(struct key_type *ktype);
 
 extern int key_task_permission(const key_ref_t key_ref,
 			       const struct cred *cred,
diff --git a/security/keys/key.c b/security/keys/key.c
index 5ba8810f903d..f396e56d640f 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -1281,18 +1281,14 @@ EXPORT_SYMBOL(__register_key_type);
  * unregister_key_type - Unregister a type of key.
  * @ktype: The key type.
  *
- * Unregister a key type and mark all the extant keys of this type as dead.
- * Those keys of this type are then destroyed to get rid of their payloads and
- * they and their links will be garbage collected as soon as possible.
+ * Unregister a key type.
  */
 void unregister_key_type(struct key_type *ktype)
 {
 	down_write(&key_types_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_read(&key_types_sem);
+	up_write(&key_types_sem);
 }
 EXPORT_SYMBOL(unregister_key_type);
 
@@ -1307,7 +1303,6 @@ void __init key_init(void)
 
 	/* 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);
 
diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index 2061fc2022a5..94a886a84bd6 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -1787,53 +1787,3 @@ void keyring_gc(struct key *keyring, time64_t limit)
 	up_write(&keyring->sem);
 	kleave(" [gc]");
 }
-
-/*
- * Garbage collect restriction pointers from a keyring.
- *
- * Keyring restrictions are associated with a key type, and must be cleaned
- * up if the key type is unregistered. The restriction is altered to always
- * reject additional keys so a keyring cannot be opened up by unregistering
- * a key type.
- *
- * Not called with any keyring locks held. The keyring's key struct will not
- * be deallocated under us as only our caller may deallocate it.
- *
- * The caller is required to hold key_types_sem and dead_type->sem. This is
- * fulfilled by key_gc_keytype() holding the locks on behalf of
- * key_garbage_collector(), which it invokes on a workqueue.
- */
-void keyring_restriction_gc(struct key *keyring, struct key_type *dead_type)
-{
-	struct key_restriction *keyres;
-
-	kenter("%x{%s}", keyring->serial, keyring->description ?: "");
-
-	/*
-	 * keyring->restrict_link is only assigned at key allocation time
-	 * or with the key type locked, so the only values that could be
-	 * concurrently assigned to keyring->restrict_link are for key
-	 * types other than dead_type. Given this, it's ok to check
-	 * the key type before acquiring keyring->sem.
-	 */
-	if (!dead_type || !keyring->restrict_link ||
-	    keyring->restrict_link->keytype != dead_type) {
-		kleave(" [no restriction gc]");
-		return;
-	}
-
-	/* Lock the keyring to ensure that a link is not in progress */
-	down_write(&keyring->sem);
-
-	keyres = keyring->restrict_link;
-
-	keyres->check = restrict_link_reject;
-
-	key_put(keyres->key);
-	keyres->key = NULL;
-	keyres->keytype = NULL;
-
-	up_write(&keyring->sem);
-
-	kleave(" [restriction gc]");
-}
-- 
2.47.3

[RFC PATCH 4/4] KEYS: key_garbage_collector(): process in chunks

[Jarkko Sakkinen]

key_garbage_collector() has a design that is not resistant enough for
workqueue lockups. The duration scales up relative to the number of items
in the serial tree, which causes a reoccuring theme of being guilty of
causing workqueue lockups.

Resolve the problem by processing in chunks:

1. Declare a static variable key_gc_work, which owns the garbage collection
   in-flight state, and key_schedule_gc_work() for re-scheduling the work.
2. Define KEY_GC_CHUNK_SIZE, set to 64 keys, as the bound for the number
   of items scanned and destroyed.
3. Define KEY_GC_DESTROYING to mark resume to destruction without repeating
   the RCU grace period.
4. Define KEY_GC_REAP_REQUESTED for preserving full-pass requests across
   the continuations.

keygcbench results without the patch:

unused:   min=413.359 avg=1817.240 max=3221.120 var=1970880.024
expired:  min=0.002 avg=1360.124 max=2481.990 var=1055093.994
combined: min=0.002 avg=1542.970 max=3221.120 var=1471557.422

keygcbench results with the patch:

unused:   min=0.005 avg=0.126 max=54.173 var=0.946
expired:  min=0.002 avg=0.210 max=216.470 var=7.894
combined: min=0.002 avg=0.158 max=216.470 var=3.627

These results translate to:

- >99.9% drop in the combined variance.
- 3.0% increase in the total task time.

The value 64 for KEY_GC_CHUNK_SIZE was cherry-picked from the set {1, 2,
..., 1024} by running the benchmark for each chunk size three times.

At the far left variance is 97% lower but at the same time total time
grows by 133%, when compared to the selected chunk size. The size was
picked based on getting a reasonable variance with a minimal throughput
decline.

Fixes: 0c061b5707ab ("KEYS: Correctly destroy key payloads when their keytype is removed")
Link: https://git.kernel.org/pub/scm/linux/kernel/git/jarkko/linux-tpmdd-test.git/tree/package/keygcbench
Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
---
 security/keys/gc.c       | 132 ++++++++++++++++++++++++++-------------
 security/keys/internal.h |   3 +-
 security/keys/key.c      |   4 +-
 security/keys/keyring.c  |   2 +-
 4 files changed, 94 insertions(+), 47 deletions(-)

diff --git a/security/keys/gc.c b/security/keys/gc.c
index 876c30c019ad..fc1e5e39b828 100644
--- a/security/keys/gc.c
+++ b/security/keys/gc.c
@@ -11,16 +11,27 @@
 #include <keys/keyring-type.h>
 #include "internal.h"
 
-/*
- * Delay between key revocation/expiry in seconds
- */
-unsigned key_gc_delay = 5 * 60;
+#define KEY_GC_CHUNK_SIZE 64
 
 /*
  * Reaper for unused keys.
  */
 static void key_garbage_collector(struct work_struct *work);
-DECLARE_WORK(key_gc_work, key_garbage_collector);
+
+static struct {
+	time64_t start;			/* expiry range start */
+	time64_t end;			/* expiry range end */
+	struct rb_node *cursor;		/* serial tree position */
+	u8 state;			/* state flags */
+	struct work_struct work;	/* work item */
+} key_gc_work = {
+	.work = __WORK_INITIALIZER(key_gc_work.work, key_garbage_collector),
+};
+
+/*
+ * Delay between key revocation/expiry in seconds
+ */
+unsigned int key_gc_delay = 5 * 60;
 
 /*
  * Reaper for links from keyrings to dead keys.
@@ -31,6 +42,23 @@ static DEFINE_TIMER(key_gc_timer, key_gc_timer_func);
 static time64_t key_gc_next_run = TIME64_MAX;
 static unsigned long key_gc_flags;
 #define KEY_GC_KEY_EXPIRED	0	/* A key expired and needs unlinking */
+#define KEY_GC_REAP_REQUESTED	1	/* Another complete pass was requested */
+
+/*
+ * Schedule a new GC pass or re-schedule an ongoing GC pass.
+ */
+void key_schedule_gc_work(bool expired)
+{
+	unsigned long flags = BIT(KEY_GC_REAP_REQUESTED);
+
+	if (expired)
+		flags |= BIT(KEY_GC_KEY_EXPIRED);
+
+	/* Publish all request flags together, after the caller's updates. */
+	set_mask_bits(&key_gc_flags, 0, flags);
+	schedule_work(&key_gc_work.work);
+}
+
 /*
  * Schedule a garbage collection run.
  * - time precision isn't particularly important
@@ -44,7 +72,8 @@ void key_schedule_gc(time64_t gc_at)
 
 	if (gc_at <= now) {
 		kdebug("IMMEDIATE");
-		schedule_work(&key_gc_work);
+		/* Catch expirations that became due during the scan. */
+		key_schedule_gc_work(true);
 	} else if (gc_at < key_gc_next_run) {
 		kdebug("DEFERRED");
 		key_gc_next_run = gc_at;
@@ -66,15 +95,6 @@ void key_set_expiry(struct key *key, time64_t expiry)
 	}
 }
 
-/*
- * Schedule a dead links collection run.
- */
-void key_schedule_gc_links(void)
-{
-	set_bit(KEY_GC_KEY_EXPIRED, &key_gc_flags);
-	schedule_work(&key_gc_work);
-}
-
 /*
  * Some key's cleanup time was met after it expired, so we need to get the
  * reaper to go through a cycle finding expired keys.
@@ -83,7 +103,7 @@ static void key_gc_timer_func(struct timer_list *unused)
 {
 	kenter("");
 	key_gc_next_run = TIME64_MAX;
-	key_schedule_gc_links();
+	key_schedule_gc_work(true);
 }
 
 /*
@@ -91,7 +111,9 @@ static void key_gc_timer_func(struct timer_list *unused)
  */
 static noinline void key_gc_unused_keys(struct list_head *keys)
 {
-	while (!list_empty(keys)) {
+	unsigned int chunk = KEY_GC_CHUNK_SIZE;
+
+	while (!list_empty(keys) && chunk--) {
 		struct key *key =
 			list_entry(keys->next, struct key, graveyard_link);
 		struct module *owner = key->type->owner;
@@ -137,35 +159,45 @@ static noinline void key_gc_unused_keys(struct list_head *keys)
 static void key_garbage_collector(struct work_struct *work)
 {
 	static LIST_HEAD(graveyard);
-	u8 gc_state = 0;
+	typeof(key_gc_work) *gc = &key_gc_work;
 #define KEY_GC_REAP_AGAIN	0x01	/* - Need another cycle */
 #define KEY_GC_REAPING_LINKS	0x02	/* - We need to reap links */
+#define KEY_GC_DESTROYING	0x04	/* - We need to destroy keys */
 
-	struct rb_node *cursor;
+	unsigned int chunk = KEY_GC_CHUNK_SIZE;
 	struct key *key;
-	time64_t new_timer, limit, expiry;
+	time64_t expiry;
 
-	kenter("[%lx]", key_gc_flags);
+	kenter("[%lx,%x]", key_gc_flags, gc->state);
 
-	limit = ktime_get_real_seconds();
+	if (gc->state & KEY_GC_DESTROYING)
+		goto destroy_keys;
+	if (gc->cursor) {
+		spin_lock(&key_serial_lock);
+		goto continue_scanning;
+	}
 
+	test_and_clear_bit(KEY_GC_REAP_REQUESTED, &key_gc_flags);
+	gc->start = ktime_get_real_seconds();
+	gc->state = 0;
 	if (test_and_clear_bit(KEY_GC_KEY_EXPIRED, &key_gc_flags))
-		gc_state |= KEY_GC_REAPING_LINKS;
-	kdebug("new pass %x", gc_state);
+		gc->state |= KEY_GC_REAPING_LINKS;
+	kdebug("new pass %x", gc->state);
 
-	new_timer = TIME64_MAX;
+	gc->end = TIME64_MAX;
 
 	/* As only this function is permitted to remove things from the key
 	 * serial tree, if cursor is non-NULL then it will always point to a
 	 * valid node in the tree - even if lock got dropped.
 	 */
 	spin_lock(&key_serial_lock);
-	cursor = rb_first(&key_serial_tree);
+	gc->cursor = rb_first(&key_serial_tree);
 
 continue_scanning:
-	while (cursor) {
-		key = rb_entry(cursor, struct key, serial_node);
-		cursor = rb_next(cursor);
+	while (gc->cursor && chunk) {
+		key = rb_entry(gc->cursor, struct key, serial_node);
+		gc->cursor = rb_next(gc->cursor);
+		chunk--;
 
 		if (!test_bit_acquire(KEY_FLAG_USER_ALIVE, &key->flags)) {
 			/* Clobber key->user after final put seen. */
@@ -176,14 +208,14 @@ static void key_garbage_collector(struct work_struct *work)
 		if (expiry != TIME64_MAX) {
 			if (!(key->type->flags & KEY_TYPE_INSTANT_REAP))
 				expiry += key_gc_delay;
-			if (expiry > limit && expiry < new_timer) {
+			if (expiry > gc->start && expiry < gc->end) {
 				kdebug("will expire %x in %lld",
-				       key_serial(key), key->expiry - limit);
-				new_timer = key->expiry;
+				       key_serial(key), key->expiry - gc->start);
+				gc->end = key->expiry;
 			}
 		}
 
-		if ((gc_state & KEY_GC_REAPING_LINKS) &&
+		if ((gc->state & KEY_GC_REAPING_LINKS) &&
 		    key->type == &key_type_keyring)
 			goto found_keyring;
 
@@ -195,7 +227,11 @@ static void key_garbage_collector(struct work_struct *work)
 	spin_unlock(&key_serial_lock);
 
 maybe_resched:
-	if (cursor) {
+	if (gc->cursor) {
+		if (!chunk) {
+			schedule_work(&key_gc_work.work);
+			return;
+		}
 		cond_resched();
 		spin_lock(&key_serial_lock);
 		goto continue_scanning;
@@ -207,9 +243,9 @@ static void key_garbage_collector(struct work_struct *work)
 	 */
 	kdebug("pass complete");
 
-	if (new_timer != TIME64_MAX) {
-		new_timer += key_gc_delay;
-		key_schedule_gc(new_timer);
+	if (gc->end != TIME64_MAX) {
+		gc->end += key_gc_delay;
+		key_schedule_gc(gc->end);
 	}
 
 	if (!list_empty(&graveyard)) {
@@ -220,13 +256,25 @@ static void key_garbage_collector(struct work_struct *work)
 		 */
 		kdebug("gc sync");
 		synchronize_rcu();
+	}
+
+	gc->state |= KEY_GC_DESTROYING;
+
+destroy_keys:
+	if (!list_empty(&graveyard)) {
 		kdebug("gc keys");
 		key_gc_unused_keys(&graveyard);
+		if (!list_empty(&graveyard)) {
+			schedule_work(&key_gc_work.work);
+			return;
+		}
 	}
+	gc->state &= ~KEY_GC_DESTROYING;
 
-	if (gc_state & KEY_GC_REAP_AGAIN)
-		schedule_work(&key_gc_work);
-	kleave(" [end %x]", gc_state);
+	if ((gc->state & KEY_GC_REAP_AGAIN) ||
+	    test_bit(KEY_GC_REAP_REQUESTED, &key_gc_flags))
+		schedule_work(&key_gc_work.work);
+	kleave(" [end %x]", gc->state);
 	return;
 
 	/* We found an unreferenced key - once we've removed it from the tree,
@@ -238,7 +286,7 @@ static void key_garbage_collector(struct work_struct *work)
 	spin_unlock(&key_serial_lock);
 
 	list_add_tail(&key->graveyard_link, &graveyard);
-	gc_state |= KEY_GC_REAP_AGAIN;
+	gc->state |= KEY_GC_REAP_AGAIN;
 	goto maybe_resched;
 
 	/* We found a keyring and we need to check the payload for links to
@@ -248,6 +296,6 @@ static void key_garbage_collector(struct work_struct *work)
 	 */
 found_keyring:
 	spin_unlock(&key_serial_lock);
-	keyring_gc(key, limit);
+	keyring_gc(key, gc->start);
 	goto maybe_resched;
 }
diff --git a/security/keys/internal.h b/security/keys/internal.h
index 005d609ae2df..976f16baf632 100644
--- a/security/keys/internal.h
+++ b/security/keys/internal.h
@@ -161,12 +161,11 @@ extern bool lookup_user_key_possessed(const struct key *key,
 extern long join_session_keyring(const char *name);
 extern void key_change_session_keyring(struct callback_head *twork);
 
-extern struct work_struct key_gc_work;
 extern unsigned key_gc_delay;
 extern void keyring_gc(struct key *keyring, time64_t limit);
 void key_set_expiry(struct key *key, time64_t expiry);
 extern void key_schedule_gc(time64_t gc_at);
-extern void key_schedule_gc_links(void);
+void key_schedule_gc_work(bool expired);
 
 extern int key_task_permission(const key_ref_t key_ref,
 			       const struct cred *cred,
diff --git a/security/keys/key.c b/security/keys/key.c
index f396e56d640f..e6569f89cca9 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -679,7 +679,7 @@ void key_put(struct key *key)
 			}
 			/* Mark key as safe for GC after key->user done. */
 			clear_bit_unlock(KEY_FLAG_USER_ALIVE, &key->flags);
-			schedule_work(&key_gc_work);
+			key_schedule_gc_work(false);
 		}
 	}
 }
@@ -1200,7 +1200,7 @@ void key_invalidate(struct key *key)
 		down_write_nested(&key->sem, 1);
 		if (!test_and_set_bit(KEY_FLAG_INVALIDATED, &key->flags)) {
 			notify_key(key, NOTIFY_KEY_INVALIDATED, 0);
-			key_schedule_gc_links();
+			key_schedule_gc_work(true);
 		}
 		up_write(&key->sem);
 	}
diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index 94a886a84bd6..b2ae7a65b186 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -266,7 +266,7 @@ void key_remove_domain(struct key_tag *domain_tag)
 {
 	domain_tag->removed = true;
 	if (!key_put_tag(domain_tag))
-		key_schedule_gc_links();
+		key_schedule_gc_work(true);
 }
 
 /*
-- 
2.47.3

Re: [RFC PATCH 0/4] KEYS: sanitize the garbage collector behavior

[Jarkko Sakkinen]

On Mon, Nov 02, 2026 at 02:15:11PM +0200, Jarkko Sakkinen wrote:
> 1. Remove key_type_dead and replace it with the regular module semantics.
> 2. Replace variable size workqueue items with fixed-size 64 key chunks.
> 
> The first change has user space visible effect: rmmod semantics will change
> from always succeeding into returning -EBUSY when keys or restrictions have
> the LKM pinned.
> 
> Jarkko Sakkinen (4):
>   KEYS: trusted: Disable sysfs bind attributes for TEE driver
>   KEYS: Pin the backing module for struct key_type
>   KEYS: Remove key_type_dead
>   KEYS: key_garbage_collector(): process in chunks
> 
>  include/linux/key-type.h                 |   6 +-
>  security/keys/gc.c                       | 249 ++++++++---------------
>  security/keys/internal.h                 |   7 +-
>  security/keys/key.c                      |  46 +++--
>  security/keys/keyring.c                  |  84 +++-----
>  security/keys/trusted-keys/trusted_tee.c |   1 +
>  6 files changed, 148 insertions(+), 245 deletions(-)
> 
> --
> 2.47.3
> 

cc: keyring

Br, Jarkko

Re: [RFC PATCH 3/4] KEYS: Remove key_type_dead

[Jarkko Sakkinen]

On Mon, Nov 02, 2026 at 02:15:14PM +0200, Jarkko Sakkinen wrote:
> unregister_key_type() no longer needs to retype live keys as keys and
> restrictions pin the module while they reference it.
> 
> Remove key_type_dead and associated code from the keyring GC.
> 
> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>

This was not too hard as I had just paid tons of time implementing
and benchmarking the chunk processing code, and I think these
features supplement quite nicely each other.

Br, Jarkko

Re: [RFC PATCH 2/4] KEYS: Pin the backing module for struct key_type

[Jarkko Sakkinen]

On Mon, Nov 02, 2026 at 02:15:13PM +0200, Jarkko Sakkinen wrote:
> Add the owner field (populated with THIS_MODULE) to struct key_type.
> 
> Pin the module in key_alloc() and keyring_restrict(), and release it in
> key_gc_unused_keys() and keyring_destroy(). These functions are the entry
> points for keys and restrictions.
> 
> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>

Missing suggested-by from Eric.

Br, Jarkko

Re: [RFC PATCH 1/4] KEYS: trusted: Disable sysfs bind attributes for TEE driver

[sashiko-bot]

> trusted_key_exit() calls unregister_key_type() from the TEE client
> driver .remove() callback. Suppress sysfs bind and unbind attributes on
> trusted_key_driver so userspace cannot unbind the driver and unregister
> key_type_trusted while the trusted module remains loaded.
> 
> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102121518.1197453-1-jarkko@kernel.org?part=1

Re: [RFC PATCH 4/4] KEYS: key_garbage_collector(): process in chunks

[Jarkko Sakkinen]

On Mon, Nov 02, 2026 at 02:15:15PM +0200, Jarkko Sakkinen wrote:
> key_garbage_collector() has a design that is not resistant enough for
> workqueue lockups. The duration scales up relative to the number of items
> in the serial tree, which causes a reoccuring theme of being guilty of
> causing workqueue lockups.
> 
> Resolve the problem by processing in chunks:
> 
> 1. Declare a static variable key_gc_work, which owns the garbage collection
>    in-flight state, and key_schedule_gc_work() for re-scheduling the work.
> 2. Define KEY_GC_CHUNK_SIZE, set to 64 keys, as the bound for the number
>    of items scanned and destroyed.
> 3. Define KEY_GC_DESTROYING to mark resume to destruction without repeating
>    the RCU grace period.
> 4. Define KEY_GC_REAP_REQUESTED for preserving full-pass requests across
>    the continuations.
> 
> keygcbench results without the patch:
> 
> unused:   min=413.359 avg=1817.240 max=3221.120 var=1970880.024
> expired:  min=0.002 avg=1360.124 max=2481.990 var=1055093.994
> combined: min=0.002 avg=1542.970 max=3221.120 var=1471557.422
> 
> keygcbench results with the patch:
> 
> unused:   min=0.005 avg=0.126 max=54.173 var=0.946
> expired:  min=0.002 avg=0.210 max=216.470 var=7.894
> combined: min=0.002 avg=0.158 max=216.470 var=3.627
> 
> These results translate to:
> 
> - >99.9% drop in the combined variance.
> - 3.0% increase in the total task time.
> 
> The value 64 for KEY_GC_CHUNK_SIZE was cherry-picked from the set {1, 2,
> ..., 1024} by running the benchmark for each chunk size three times.
> 
> At the far left variance is 97% lower but at the same time total time
> grows by 133%, when compared to the selected chunk size. The size was
> picked based on getting a reasonable variance with a minimal throughput
> decline.
> 
> Fixes: 0c061b5707ab ("KEYS: Correctly destroy key payloads when their keytype is removed")
> Link: https://git.kernel.org/pub/scm/linux/kernel/git/jarkko/linux-tpmdd-test.git/tree/package/keygcbench
> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
 
I left out syzkaller tags (and do so also in the foreseeable future) as
they seem to influence kind of feedback to appear that can only
interfere the topic.

Br, Jarkko

Re: [RFC PATCH 2/4] KEYS: Pin the backing module for struct key_type

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] KEYS: self-pinning module reference cycle in key_alloc()
--

commit 8793dbc22dd9f6067e0bfcfa9c4b1b437edcf052
Author: Jarkko Sakkinen <jarkko@kernel.org>

KEYS: Pin the backing module for struct key_type

This commit adds an owner field populated with THIS_MODULE to struct key_type
when registering a key type. It pins the module in key_alloc() and
keyring_restrict(), releasing the reference in key_gc_unused_keys() and
keyring_destroy().

> diff --git a/security/keys/key.c b/security/keys/key.c
> index e88548f1a7040..9f1f08fcbe863 100644
> --- a/security/keys/key.c
> +++ b/security/keys/key.c
> @@ -7,6 +7,7 @@
>  
>  #include <linux/export.h>
>  #include <linux/init.h>
> +#include <linux/module.h>
>  #include <linux/poison.h>
>  #include <linux/sched.h>
>  #include <linux/slab.h>
> @@ -234,19 +235,24 @@ struct key *key_alloc(struct key_type *type, const char *desc,
>  
>  	key = ERR_PTR(-EINVAL);
>  	if (!desc || !*desc)
> -		goto error;
> +		goto out;
> +
> +	if (!try_module_get(type->owner)) {
> +		key = ERR_PTR(-ENOENT);
> +		goto out;
> +	}

[Severity: High]
Does pinning type->owner in key_alloc() until key_gc_unused_keys() create a
self-pinning reference cycle for modular key types that cache keys in their
own internal keyrings?

With register_key_type() setting ktype->owner to THIS_MODULE:

include/linux/key-type.h:
#define register_key_type(ktype) __register_key_type(ktype, THIS_MODULE)

every allocated key of that type holds a module reference on its owning
module until the key's reference count drops to zero and it is garbage
collected in key_gc_unused_keys().

However, several loadable modules allocate an internal keyring during module
initialization, link keys of their own key_type into that keyring during
normal operation, and only revoke and release that keyring in their module
exit path:

drivers/nvme/common/keyring.c:nvme_keyring_exit() {
	unregister_key_type(&nvme_tls_psk_key_type);
	key_revoke(nvme_keyring);
	key_put(nvme_keyring);
}

fs/nfs/nfs4idmap.c:nfs_idmap_quit() {
	key_revoke(id_resolver_cache->thread_keyring);
	unregister_key_type(&key_type_id_resolver);
	unregister_key_type(&key_type_id_resolver_legacy);
	put_cred(id_resolver_cache);
}

fs/smb/client/cifsacl.c:exit_cifs_idmap() {
	key_revoke(root_cred->thread_keyring);
	unregister_key_type(&cifs_idmap_key_type);
	put_cred(root_cred);
	cifs_dbg(FYI, "Unregistered %s key type\n", cifs_idmap_key_type.name);
}

net/dns_resolver/dns_key.c:exit_dns_resolver() {
	key_revoke(dns_resolver_cache->thread_keyring);
	unregister_key_type(&key_type_dns_resolver);
	put_cred(dns_resolver_cache);
}

Once any key is linked into one of these internal module keyrings (or added
by userspace via add_key() to a session or user keyring), the key holds a
reference on the module while the module's keyring holds a reference on the
key. Won't this prevent the module reference count from ever reaching zero,
making it impossible to unload these modules unless all cached keys expire or
are manually evicted first?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102121518.1197453-1-jarkko@kernel.org?part=2

Re: [RFC PATCH 4/4] KEYS: key_garbage_collector(): process in chunks

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] keys: non-atomic flag clearing in key_garbage_collector() drops link reaping
--

commit 9857f1d86bdced127ae471468f5b25b61aa04d0f
Author: Jarkko Sakkinen <jarkko@kernel.org>

KEYS: key_garbage_collector(): process in chunks

This commit updates key_garbage_collector() to scan the serial tree and
destroy unused keys in chunks of 64 items, tracking in-flight state across
workqueue continuations to prevent workqueue lockups.

Link: https://git.kernel.org/pub/scm/linux/kernel/git/jarkko/linux-tpmdd-test.git/tree/package/keygcbench

> diff --git a/security/keys/gc.c b/security/keys/gc.c
> index 876c30c019ad4..fc1e5e39b828c 100644
> --- a/security/keys/gc.c
> +++ b/security/keys/gc.c

[ ... ]

> @@ -31,6 +42,23 @@ static DEFINE_TIMER(key_gc_timer, key_gc_timer_func);
>  static time64_t key_gc_next_run = TIME64_MAX;
>  static unsigned long key_gc_flags;
>  #define KEY_GC_KEY_EXPIRED	0	/* A key expired and needs unlinking */
> +#define KEY_GC_REAP_REQUESTED	1	/* Another complete pass was requested */
> +
> +/*
> + * Schedule a new GC pass or re-schedule an ongoing GC pass.
> +*/
> +void key_schedule_gc_work(bool expired)
> +{
> +	unsigned long flags = BIT(KEY_GC_REAP_REQUESTED);
> +
> +	if (expired)
> +		flags |= BIT(KEY_GC_KEY_EXPIRED);
> +
> +	/* Publish all request flags together, after the caller's updates. */
> +	set_mask_bits(&key_gc_flags, 0, flags);
> +	schedule_work(&key_gc_work.work);
> +}

[ ... ]

> @@ -137,35 +159,45 @@ static noinline void key_gc_unused_keys(struct list_head *keys)
>  static void key_garbage_collector(struct work_struct *work)
>  {
>  	static LIST_HEAD(graveyard);
> -	u8 gc_state = 0;
> +	typeof(key_gc_work) *gc = &key_gc_work;
>  #define KEY_GC_REAP_AGAIN	0x01	/* - Need another cycle */
>  #define KEY_GC_REAPING_LINKS	0x02	/* - We need to reap links */
> +#define KEY_GC_DESTROYING	0x04	/* - We need to destroy keys */
>  
> -	struct rb_node *cursor;
> +	unsigned int chunk = KEY_GC_CHUNK_SIZE;
>  	struct key *key;
> -	time64_t new_timer, limit, expiry;
> +	time64_t expiry;
>  
> -	kenter("[%lx]", key_gc_flags);
> +	kenter("[%lx,%x]", key_gc_flags, gc->state);
>  
> -	limit = ktime_get_real_seconds();
> +	if (gc->state & KEY_GC_DESTROYING)
> +		goto destroy_keys;
> +	if (gc->cursor) {
> +		spin_lock(&key_serial_lock);
> +		goto continue_scanning;
> +	}
>  
> +	test_and_clear_bit(KEY_GC_REAP_REQUESTED, &key_gc_flags);
> +	gc->start = ktime_get_real_seconds();
> +	gc->state = 0;
>  	if (test_and_clear_bit(KEY_GC_KEY_EXPIRED, &key_gc_flags))
> -		gc_state |= KEY_GC_REAPING_LINKS;
> -	kdebug("new pass %x", gc_state);
> +		gc->state |= KEY_GC_REAPING_LINKS;

[Severity: Medium]
Can clearing KEY_GC_REAP_REQUESTED and KEY_GC_KEY_EXPIRED in two separate
operations on opposite sides of ktime_get_real_seconds() cause a link-reaping
request to be dropped?

In key_schedule_gc_work(true), called from key_gc_timer_func(),
key_schedule_gc(), key_invalidate(), and key_remove_domain(), both
KEY_GC_REAP_REQUESTED and KEY_GC_KEY_EXPIRED are published together via
set_mask_bits().

However, if another CPU calls key_schedule_gc_work(true) after
key_garbage_collector() executes:

    test_and_clear_bit(KEY_GC_REAP_REQUESTED, &key_gc_flags);
    gc->start = ktime_get_real_seconds();

(for example, right after the clock ticks to a new second, or after a new key
is invalidated or a domain is removed) and before:

    if (test_and_clear_bit(KEY_GC_KEY_EXPIRED, &key_gc_flags))

then the current pass consumes KEY_GC_KEY_EXPIRED while leaving
KEY_GC_REAP_REQUESTED set.

The current pass will run keyring_gc(key, gc->start) with the older gc->start
timestamp (or after already having scanned past a keyring if this were not
the start of the pass, or before the new expiry second is reached in
key_is_dead()), and when the follow-up pass triggered by
KEY_GC_REAP_REQUESTED runs, KEY_GC_KEY_EXPIRED is already clear so
KEY_GC_REAPING_LINKS will not be set.

Should key_gc_flags be consumed atomically with xchg(&key_gc_flags, 0) before
sampling gc->start = ktime_get_real_seconds()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102121518.1197453-1-jarkko@kernel.org?part=4

Re: [RFC PATCH 3/4] KEYS: Remove key_type_dead

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] KEYS: unregister_key_type() leaves live keys active for runtime callers
- [High] KEYS: UAF in unregister_key_type() when module_init fails after registration
--

commit 5a5ac1aac59bbfa4e21a7a0f1fe3391d7d6ec2c7
Author: Jarkko Sakkinen <jarkko@kernel.org>

KEYS: Remove key_type_dead

This commit removes key_type_dead, key_gc_keytype(), and
keyring_restriction_gc(), assuming that pinning the key type's owning module
while keys or restrictions exist prevents unregister_key_type() from being
called while keys of that type are alive.

> diff --git a/security/keys/key.c b/security/keys/key.c
> index 9f1f08fcbe863..c0efb87b6705d 100644
> --- a/security/keys/key.c
> +++ b/security/keys/key.c
> @@ -1279,18 +1279,14 @@ EXPORT_SYMBOL(__register_key_type);
>   * unregister_key_type - Unregister a type of key.
>   * @ktype: The key type.
>   *
> - * Unregister a key type and mark all the extant keys of this type as dead.
> - * Those keys of this type are then destroyed to get rid of their payloads and
> - * they and their links will be garbage collected as soon as possible.
> + * Unregister a key type.
>   */
>  void unregister_key_type(struct key_type *ktype)
>  {
>  	down_write(&key_types_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_read(&key_types_sem);
> +	up_write(&key_types_sem);
>  }

[Severity: High]
What happens when unregister_key_type() is called at runtime from a device
driver removal callback or error recovery path rather than module unload?

Pinning ktype->owner in key_alloc() and keyring_restrict() prevents
delete_module() from running module_exit, but it does not stop runtime calls
to unregister_key_type() while the module remains loaded or when the key type
is built-in.

For example, in security/keys/trusted-keys/trusted_tee.c, when the parent
driver unbinds and optee_remove_common() -> optee_unregister_devices() ->
device_unregister() invokes trusted_key_remove(), it unregisters
key_type_trusted and immediately frees pvt_data.ctx:

security/keys/trusted-keys/trusted_tee.c:trusted_key_remove() {
	unregister_key_type(&key_type_trusted);
	tee_client_close_session(pvt_data.ctx, pvt_data.session_id);
	tee_client_close_context(pvt_data.ctx);
}

Without key_gc_keytype() marking extant keys dead, unlinking them, destroying
their payloads via trusted_destroy(), and retyping them to key_type_dead,
existing trusted keys remain active. If userspace subsequently updates or
instantiates one of those keys via keyctl_update_key() or
keyctl_instantiate_key(), trusted_update() or trusted_instantiate() calls into
trusted_tee_seal(), trusted_tee_unseal(), or trusted_tee_get_random() and
dereferences the freed pvt_data.ctx and pvt_data.dev:

security/keys/trusted-keys/trusted_tee.c:trusted_tee_seal() {
	reg_shm = tee_shm_register_kernel_buf(pvt_data.ctx, p->key,
					      sizeof(p->key) + sizeof(p->blob));
	if (IS_ERR(reg_shm)) {
		dev_err(pvt_data.dev, "shm register failed\n");
		return PTR_ERR(reg_shm);
	}
}

Similarly, in arch/s390/kernel/cert_store.c, where key_type_cert_store_key is
built-in (owner is null), a runtime certificate store refresh via
create_cs_keyring() -> cleanup_cs_keys() relies on unregister_key_type() to
force-reap and destroy extant cert_store_key keys when per-key invalidation
fails before re-registering key_type_cert_store_key:

arch/s390/kernel/cert_store.c:cleanup_cs_keys() {
	if (invalidate_keyring_keys(cs_keyring))
		unregister_key_type(&key_type_cert_store_key);
}

In both paths, if register_key_type() is later called again on the same static
struct key_type (in create_cs_keyring() or trusted_key_probe()) while keys
from the previous registration are still alive, __register_key_type() will
also zero ktype->lock_class while live keys still have key->sem registered
with lockdep using &ktype->lock_class:

security/keys/key.c:__register_key_type() {
	ktype->owner = owner;
	memset(&ktype->lock_class, 0, sizeof(ktype->lock_class));
}

Doesn't unregister_key_type() still need to reap and retype extant keys so
runtime callers cannot leave live keys referencing freed driver resources or
active lockdep classes?

[Severity: High]
Could removing key_gc_keytype() and keyring_restriction_gc() from
unregister_key_type() also cause a use-after-free when a module's init
function fails after calling register_key_type()?

Several loadable modules register key types before performing subsequent
fallible initialization steps and call unregister_key_type() on their init
error unwind path, such as af_rxrpc_init() in net/rxrpc/af_rxrpc.c,
turris_signing_key_init() in drivers/platform/cznic/turris-signing-key.c,
init_nfs_fs() in fs/nfs/inode.c, nfs_idmap_init() in fs/nfs/nfs4idmap.c,
init_cifs() in fs/smb/client/cifsfs.c, and init_ceph_lib() in
net/ceph/ceph_common.c:

drivers/platform/cznic/turris-signing-key.c:turris_signing_key_init() {
	if (IS_ERR(turris_signing_keyring)) {
		pr_err("Cannot allocate Turris keyring\n");

		unregister_key_type(&turris_signing_key_type);

		return PTR_ERR(turris_signing_keyring);
	}
}

While mod->init() is running in do_init_module() in kernel/module/main.c,
module_is_live(mod) is true, so try_module_get(type->owner) in key_alloc() and
keyring_restrict() succeeds as soon as register_key_type() publishes the key
type. If a concurrent task allocates a key or restricts a keyring with that
key type and a later step in mod->init() fails, the error path calls
unregister_key_type() and do_init_module() unconditionally frees the module
regardless of its reference count:

kernel/module/main.c:do_init_module() {
fail:
	/* Try to protect us from buggy refcounters. */
	mod->state = MODULE_STATE_GOING;
	synchronize_rcu();
	module_put(mod);
	blocking_notifier_call_chain(&module_notify_list,
				     MODULE_STATE_GOING, mod);
	klp_module_going(mod);
	ftrace_release_mod(mod);
	free_module(mod);
}

Previously, key_gc_keytype() synchronously destroyed key payloads, retyped
those keys to key_type_dead, and cleared keyring restrictions via
keyring_restriction_gc() before unregister_key_type() returned. Without that
cleanup, won't surviving keys and restrictions retain dangling pointers into
freed module memory when later accessed or garbage-collected in
key_gc_unused_keys() in security/keys/gc.c and keyring_destroy() in
security/keys/keyring.c:

security/keys/gc.c:key_gc_unused_keys() {
		struct key *key =
			list_entry(keys->next, struct key, graveyard_link);
		struct module *owner = key->type->owner;
		short state = key->state;
		...
		if (state == KEY_IS_POSITIVE && key->type->destroy)
			key->type->destroy(key);
		...
		module_put(owner);
}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102121518.1197453-1-jarkko@kernel.org?part=3

Re: [RFC PATCH 2/4] KEYS: Pin the backing module for struct key_type

[Jarkko Sakkinen]

On Mon, Nov 02, 2026 at 12:32:55PM +0000, sashiko-bot@kernel.org wrote:
> drivers/nvme/common/keyring.c:nvme_keyring_exit() {
> 	unregister_key_type(&nvme_tls_psk_key_type);
> 	key_revoke(nvme_keyring);
> 	key_put(nvme_keyring);
> }
> 
> fs/nfs/nfs4idmap.c:nfs_idmap_quit() {
> 	key_revoke(id_resolver_cache->thread_keyring);
> 	unregister_key_type(&key_type_id_resolver);
> 	unregister_key_type(&key_type_id_resolver_legacy);
> 	put_cred(id_resolver_cache);
> }
> 
> fs/smb/client/cifsacl.c:exit_cifs_idmap() {
> 	key_revoke(root_cred->thread_keyring);
> 	unregister_key_type(&cifs_idmap_key_type);
> 	put_cred(root_cred);
> 	cifs_dbg(FYI, "Unregistered %s key type\n", cifs_idmap_key_type.name);
> }
> 
> net/dns_resolver/dns_key.c:exit_dns_resolver() {
> 	key_revoke(dns_resolver_cache->thread_keyring);
> 	unregister_key_type(&key_type_dns_resolver);
> 	put_cred(dns_resolver_cache);
> }

I did this on purpose to check which sites Sashiko detects so this was
expected outcome.

I'll address all the Sashiko issues and send v2 so that they are out
of the way.

Br, Jarkko

Re: [RFC PATCH 4/4] KEYS: key_garbage_collector(): process in chunks

[Jarkko Sakkinen]

On Mon, Nov 02, 2026 at 12:32:57PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] keys: non-atomic flag clearing in key_garbage_collector() drops link reaping
> --
> 
> commit 9857f1d86bdced127ae471468f5b25b61aa04d0f
> Author: Jarkko Sakkinen <jarkko@kernel.org>
> 
> KEYS: key_garbage_collector(): process in chunks
> 
> This commit updates key_garbage_collector() to scan the serial tree and
> destroy unused keys in chunks of 64 items, tracking in-flight state across
> workqueue continuations to prevent workqueue lockups.
> 
> Link: https://git.kernel.org/pub/scm/linux/kernel/git/jarkko/linux-tpmdd-test.git/tree/package/keygcbench

I'm addressing all the Sashiko issues also for this.

Br, Jarkko