diff mbox series

[RFC,v2] KEYS: Add a list for unreferenced keys

Message ID 20250330111649.13547-1-jarkko@kernel.org (mailing list archive)
State New
Headers show
Series [RFC,v2] KEYS: Add a list for unreferenced keys | expand

Commit Message

Jarkko Sakkinen March 30, 2025, 11:16 a.m. UTC 
Add an isolated list for unreferenced keys. This splits key deletion as
separate phase, after the key reaper. This makes the whole process more
rigid, as these two distinct tasks don't intervene each other.

Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
---
v2:
- Rename key_gc_unused_keys as key_gc_graveyard, and re-document the
  function.
---
 include/linux/key.h      |  1 -
 security/keys/gc.c       | 27 +++++++--------------------
 security/keys/internal.h |  1 +
 security/keys/key.c      |  7 +++++--
 4 files changed, 13 insertions(+), 23 deletions(-)

Comments

Jarkko Sakkinen March 30, 2025, 11:22 a.m. UTC | #1 
On Sun, Mar 30, 2025 at 02:16:49PM +0300, Jarkko Sakkinen wrote:
> diff --git a/security/keys/key.c b/security/keys/key.c
> index 7198cd2ac3a3..b34b4cba6ce7 100644
> --- a/security/keys/key.c
> +++ b/security/keys/key.c
> @@ -22,6 +22,7 @@ DEFINE_SPINLOCK(key_serial_lock);
>  
>  struct rb_root	key_user_tree; /* tree of quota records indexed by UID */
>  DEFINE_SPINLOCK(key_user_lock);
> +LIST_HEAD(key_graveyard);
>  
>  unsigned int key_quota_root_maxkeys = 1000000;	/* root's key count quota */
>  unsigned int key_quota_root_maxbytes = 25000000; /* root's key space quota */
> @@ -658,8 +659,10 @@ void key_put(struct key *key)
>  				key->user->qnbytes -= key->quotalen;
>  				spin_unlock_irqrestore(&key->user->lock, flags);
>  			}
> -			smp_mb(); /* key->user before FINAL_PUT set. */
> -			set_bit(KEY_FLAG_FINAL_PUT, &key->flags);
> +			spin_lock(&key_serial_lock);

	kdebug("unrefd key %d", key->serial);

Now this message gets spuriously deleted so maybe better just carry it
(just noticed)?

> +			rb_erase(&key->serial_node, &key_serial_tree);
> +			list_add_tail(&key->graveyard_link, &key_graveyard);
> +			spin_unlock(&key_serial_lock);
>  			schedule_work(&key_gc_work);
>  		}
>  	}
> -- 
> 2.39.5
> 
> 

BR, Jarkko
Jarkko Sakkinen March 31, 2025, 5:48 p.m. UTC | #2 
On Sun, Mar 30, 2025 at 02:16:49PM +0300, Jarkko Sakkinen wrote:
> Add an isolated list for unreferenced keys. This splits key deletion as
> separate phase, after the key reaper. This makes the whole process more
> rigid, as these two distinct tasks don't intervene each other.
> 
> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> ---
> v2:
> - Rename key_gc_unused_keys as key_gc_graveyard, and re-document the
>   function.
> ---
>  include/linux/key.h      |  1 -
>  security/keys/gc.c       | 27 +++++++--------------------
>  security/keys/internal.h |  1 +
>  security/keys/key.c      |  7 +++++--
>  4 files changed, 13 insertions(+), 23 deletions(-)

kernel-test-bot reported:

https://lore.kernel.org/oe-lkp/202503312252.bef52733-lkp@intel.com/

I.e., v3 is coming.

BR, Jarkko
diff mbox series

Patch

diff --git a/include/linux/key.h b/include/linux/key.h
index ba05de8579ec..074dca3222b9 100644
--- a/include/linux/key.h
+++ b/include/linux/key.h
@@ -236,7 +236,6 @@  struct key {
 #define KEY_FLAG_ROOT_CAN_INVAL	7	/* set if key can be invalidated by root without permission */
 #define KEY_FLAG_KEEP		8	/* set if key should not be removed */
 #define KEY_FLAG_UID_KEYRING	9	/* set if key is a user or user session keyring */
-#define KEY_FLAG_FINAL_PUT	10	/* set if final put has happened on key */
 
 	/* the key type and key description string
 	 * - the desc is used to match a key against search criteria
diff --git a/security/keys/gc.c b/security/keys/gc.c
index f27223ea4578..ffd456b6967d 100644
--- a/security/keys/gc.c
+++ b/security/keys/gc.c
@@ -130,9 +130,9 @@  void key_gc_keytype(struct key_type *ktype)
 }
 
 /*
- * Garbage collect a list of unreferenced, detached keys
+ * Takes ownership of the given list, and deinitializes and destroys the keys.
  */
-static noinline void key_gc_unused_keys(struct list_head *keys)
+static noinline void key_gc_graveyard(struct list_head *keys)
 {
 	while (!list_empty(keys)) {
 		struct key *key =
@@ -218,11 +218,6 @@  static void key_garbage_collector(struct work_struct *work)
 		key = rb_entry(cursor, struct key, serial_node);
 		cursor = rb_next(cursor);
 
-		if (test_bit(KEY_FLAG_FINAL_PUT, &key->flags)) {
-			smp_mb(); /* Clobber key->user after FINAL_PUT seen. */
-			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;
@@ -286,6 +281,10 @@  static void key_garbage_collector(struct work_struct *work)
 		key_schedule_gc(new_timer);
 	}
 
+	spin_lock(&key_serial_lock);
+	list_splice_init(&key_graveyard, &graveyard);
+	spin_unlock(&key_serial_lock);
+
 	if (unlikely(gc_state & KEY_GC_REAPING_DEAD_2) ||
 	    !list_empty(&graveyard)) {
 		/* Make sure that all pending keyring payload destructions are
@@ -299,7 +298,7 @@  static void key_garbage_collector(struct work_struct *work)
 
 	if (!list_empty(&graveyard)) {
 		kdebug("gc keys");
-		key_gc_unused_keys(&graveyard);
+		key_gc_graveyard(&graveyard);
 	}
 
 	if (unlikely(gc_state & (KEY_GC_REAPING_DEAD_1 |
@@ -328,18 +327,6 @@  static void key_garbage_collector(struct work_struct *work)
 	kleave(" [end %x]", gc_state);
 	return;
 
-	/* We found an unreferenced key - once we've removed it from the tree,
-	 * we can safely drop the lock.
-	 */
-found_unreferenced_key:
-	kdebug("unrefd key %d", key->serial);
-	rb_erase(&key->serial_node, &key_serial_tree);
-	spin_unlock(&key_serial_lock);
-
-	list_add_tail(&key->graveyard_link, &graveyard);
-	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.
 	 */
diff --git a/security/keys/internal.h b/security/keys/internal.h
index 2cffa6dc8255..c1b6f0b5817c 100644
--- a/security/keys/internal.h
+++ b/security/keys/internal.h
@@ -66,6 +66,7 @@  struct key_user {
 extern struct rb_root	key_user_tree;
 extern spinlock_t	key_user_lock;
 extern struct key_user	root_key_user;
+extern struct list_head key_graveyard;
 
 extern struct key_user *key_user_lookup(kuid_t uid);
 extern void key_user_put(struct key_user *user);
diff --git a/security/keys/key.c b/security/keys/key.c
index 7198cd2ac3a3..b34b4cba6ce7 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -22,6 +22,7 @@  DEFINE_SPINLOCK(key_serial_lock);
 
 struct rb_root	key_user_tree; /* tree of quota records indexed by UID */
 DEFINE_SPINLOCK(key_user_lock);
+LIST_HEAD(key_graveyard);
 
 unsigned int key_quota_root_maxkeys = 1000000;	/* root's key count quota */
 unsigned int key_quota_root_maxbytes = 25000000; /* root's key space quota */
@@ -658,8 +659,10 @@  void key_put(struct key *key)
 				key->user->qnbytes -= key->quotalen;
 				spin_unlock_irqrestore(&key->user->lock, flags);
 			}
-			smp_mb(); /* key->user before FINAL_PUT set. */
-			set_bit(KEY_FLAG_FINAL_PUT, &key->flags);
+			spin_lock(&key_serial_lock);
+			rb_erase(&key->serial_node, &key_serial_tree);
+			list_add_tail(&key->graveyard_link, &key_graveyard);
+			spin_unlock(&key_serial_lock);
 			schedule_work(&key_gc_work);
 		}
 	}