lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <32c1e996-ac34-496f-933e-a266b487da1a@samsung.com>
Date: Mon, 7 Apr 2025 12:25:11 +0200
From: Marek Szyprowski <m.szyprowski@...sung.com>
To: Jarkko Sakkinen <jarkko@...nel.org>, keyrings@...r.kernel.org
Cc: Jarkko Sakkinen <jarkko.sakkinen@...nsys.com>, stable@...r.kernel.org,
	David Howells <dhowells@...hat.com>, Lukas Wunner <lukas@...ner.de>, Ignat
	Korchagin <ignat@...udflare.com>, Herbert Xu <herbert@...dor.apana.org.au>,
	"David S. Miller" <davem@...emloft.net>, Peter Huewe <peterhuewe@....de>,
	Jason Gunthorpe <jgg@...pe.ca>, Paul Moore <paul@...l-moore.com>, James
	Morris <jmorris@...ei.org>, "Serge E. Hallyn" <serge@...lyn.com>, James
	Bottomley <James.Bottomley@...senPartnership.com>, Mimi Zohar
	<zohar@...ux.ibm.com>, linux-crypto@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-integrity@...r.kernel.org,
	linux-security-module@...r.kernel.org
Subject: Re: [PATCH v7] KEYS: Add a list for unreferenced keys

On 07.04.2025 04:39, Jarkko Sakkinen wrote:
> From: Jarkko Sakkinen <jarkko.sakkinen@...nsys.com>
>
> Add an isolated list of unreferenced keys to be queued for deletion, and
> try to pin the keys in the garbage collector before processing anything.
> Skip unpinnable keys.
>
> Use this list for blocking the reaping process during the teardown:
>
> 1. First off, the keys added to `keys_graveyard` are snapshotted, and the
>     list is flushed. This the very last step in `key_put()`.
> 2. `key_put()` reaches zero. This will mark key as busy for the garbage
>     collector.
> 3. `key_garbage_collector()` will try to increase refcount, which won't go
>     above zero. Whenever this happens, the key will be skipped.
>
> Cc: stable@...r.kernel.org # v6.1+
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@...nsys.com>

This patch landed in today's linux-next as commit b0d023797e3e ("keys: 
Add a list for unreferenced keys"). In my tests I found that it triggers 
the following lockdep issue:

================================
WARNING: inconsistent lock state
6.15.0-rc1-next-20250407 #15630 Not tainted
--------------------------------
inconsistent {SOFTIRQ-ON-W} -> {IN-SOFTIRQ-W} usage.
ksoftirqd/3/32 [HC0[0]:SC1[1]:HE1:SE0] takes:
c13fdd68 (key_serial_lock){+.?.}-{2:2}, at: key_put+0x74/0x128
{SOFTIRQ-ON-W} state was registered at:
   lock_acquire+0x134/0x384
   _raw_spin_lock+0x38/0x48
   key_alloc+0x2fc/0x4d8
   keyring_alloc+0x40/0x90
   system_trusted_keyring_init+0x50/0x7c
   do_one_initcall+0x68/0x314
   kernel_init_freeable+0x1c0/0x224
   kernel_init+0x1c/0x12c
   ret_from_fork+0x14/0x28
irq event stamp: 234
hardirqs last  enabled at (234): [<c0cb7060>] 
_raw_spin_unlock_irqrestore+0x5c/0x60
hardirqs last disabled at (233): [<c0cb6dd0>] 
_raw_spin_lock_irqsave+0x64/0x68
softirqs last  enabled at (42): [<c013bcd8>] handle_softirqs+0x328/0x520
softirqs last disabled at (47): [<c013bf10>] run_ksoftirqd+0x40/0x68

other info that might help us debug this:
  Possible unsafe locking scenario:

        CPU0
        ----
   lock(key_serial_lock);
   <Interrupt>
     lock(key_serial_lock);

  *** DEADLOCK ***

1 lock held by ksoftirqd/3/32:
  #0: c137c040 (rcu_callback){....}-{0:0}, at: rcu_core+0x4d0/0x14a4

stack backtrace:
CPU: 3 UID: 0 PID: 32 Comm: ksoftirqd/3 Not tainted 
6.15.0-rc1-next-20250407 #15630 PREEMPT
Hardware name: Samsung Exynos (Flattened Device Tree)
Call trace:
  unwind_backtrace from show_stack+0x10/0x14
  show_stack from dump_stack_lvl+0x68/0x88
  dump_stack_lvl from print_usage_bug.part.0+0x24c/0x270
  print_usage_bug.part.0 from mark_lock.part.0+0xc20/0x129c
  mark_lock.part.0 from __lock_acquire+0xafc/0x2970
  __lock_acquire from lock_acquire+0x134/0x384
  lock_acquire from _raw_spin_lock+0x38/0x48
  _raw_spin_lock from key_put+0x74/0x128
  key_put from put_cred_rcu+0x20/0xd0
  put_cred_rcu from rcu_core+0x478/0x14a4
  rcu_core from handle_softirqs+0x130/0x520
  handle_softirqs from run_ksoftirqd+0x40/0x68
  run_ksoftirqd from smpboot_thread_fn+0x17c/0x330
  smpboot_thread_fn from kthread+0x138/0x25c
  kthread from ret_from_fork+0x14/0x28
Exception stack(0xf090dfb0 to 0xf090dff8)
...

To fix this issue I had to change all calls around key_serial_lock and 
key_graveyard_lock spinlocks with the irqsave/irqrestore variants (in 
security/keys/key.c and security/keys/gc.c), but I'm not sure if this is 
desired solution.

> ---
> v7:
> - Fixed multiple definitions (from rebasing, sorry).
> v6:
> - Rebase went wrong in v5.
> v5:
> - Rebased on top of v6.15-rc
> - Updated commit message to explain how spin lock and refcount
>    isolate the time window in key_put().
> v4:
> - Pin the key while processing key type teardown. Skip dead keys.
> - Revert key_gc_graveyard back key_gc_unused_keys.
> - Rewrote the commit message.
> - "unsigned long flags" declaration somehow did make to the previous
>    patch (sorry).
> v3:
> - Using spin_lock() fails since key_put() is executed inside IRQs.
>    Using spin_lock_irqsave() would neither work given the lock is
>    acquired for /proc/keys. Therefore, separate the lock for
>    graveyard and key_graveyard before reaping key_serial_tree.
> v2:
> - Rename key_gc_unused_keys as key_gc_graveyard, and re-document the
>    function.
> ---
>   include/linux/key.h      |  7 ++-----
>   security/keys/gc.c       | 40 ++++++++++++++++++++++++----------------
>   security/keys/internal.h |  5 +++++
>   security/keys/key.c      |  7 +++++--
>   4 files changed, 36 insertions(+), 23 deletions(-)
>
> diff --git a/include/linux/key.h b/include/linux/key.h
> index ba05de8579ec..c50659184bdf 100644
> --- a/include/linux/key.h
> +++ b/include/linux/key.h
> @@ -195,10 +195,8 @@ enum key_state {
>   struct key {
>   	refcount_t		usage;		/* number of references */
>   	key_serial_t		serial;		/* key serial number */
> -	union {
> -		struct list_head graveyard_link;
> -		struct rb_node	serial_node;
> -	};
> +	struct list_head	graveyard_link; /* key->usage == 0 */
> +	struct rb_node		serial_node;
>   #ifdef CONFIG_KEY_NOTIFICATIONS
>   	struct watch_list	*watchers;	/* Entities watching this key for changes */
>   #endif
> @@ -236,7 +234,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..0a3beb68633c 100644
> --- a/security/keys/gc.c
> +++ b/security/keys/gc.c
> @@ -189,6 +189,7 @@ static void key_garbage_collector(struct work_struct *work)
>   	struct rb_node *cursor;
>   	struct key *key;
>   	time64_t new_timer, limit, expiry;
> +	unsigned long flags;
>   
>   	kenter("[%lx,%x]", key_gc_flags, gc_state);
>   
> @@ -206,21 +207,35 @@ static void key_garbage_collector(struct work_struct *work)
>   
>   	new_timer = TIME64_MAX;
>   
> +	spin_lock_irqsave(&key_graveyard_lock, flags);
> +	list_splice_init(&key_graveyard, &graveyard);
> +	spin_unlock_irqrestore(&key_graveyard_lock, flags);
> +
> +	list_for_each_entry(key, &graveyard, graveyard_link) {
> +		spin_lock(&key_serial_lock);
> +		kdebug("unrefd key %d", key->serial);
> +		rb_erase(&key->serial_node, &key_serial_tree);
> +		spin_unlock(&key_serial_lock);
> +	}
> +
>   	/* 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);
> +	key = NULL;
>   	cursor = rb_first(&key_serial_tree);
>   
>   continue_scanning:
> +	key_put(key);
>   	while (cursor) {
>   		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;
> +		/* key_get(), unless zero: */
> +		if (!refcount_inc_not_zero(&key->usage)) {
> +			key = NULL;
> +			gc_state |= KEY_GC_REAP_AGAIN;
> +			goto skip_dead_key;
>   		}
>   
>   		if (unlikely(gc_state & KEY_GC_REAPING_DEAD_1)) {
> @@ -274,6 +289,7 @@ static void key_garbage_collector(struct work_struct *work)
>   		spin_lock(&key_serial_lock);
>   		goto continue_scanning;
>   	}
> +	key_put(key);
>   
>   	/* We've completed the pass.  Set the timer if we need to and queue a
>   	 * new cycle if necessary.  We keep executing cycles until we find one
> @@ -286,6 +302,10 @@ static void key_garbage_collector(struct work_struct *work)
>   		key_schedule_gc(new_timer);
>   	}
>   
> +	spin_lock(&key_graveyard_lock);
> +	list_splice_init(&key_graveyard, &graveyard);
> +	spin_unlock(&key_graveyard_lock);
> +
>   	if (unlikely(gc_state & KEY_GC_REAPING_DEAD_2) ||
>   	    !list_empty(&graveyard)) {
>   		/* Make sure that all pending keyring payload destructions are
> @@ -328,18 +348,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..4e3d9b322390 100644
> --- a/security/keys/internal.h
> +++ b/security/keys/internal.h
> @@ -63,9 +63,14 @@ struct key_user {
>   	int			qnbytes;	/* number of bytes allocated to this user */
>   };
>   
> +extern struct list_head key_graveyard;
> +extern spinlock_t key_graveyard_lock;
> +
>   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 spinlock_t	key_graveyard_lock;
>   
>   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..7511f2017b6b 100644
> --- a/security/keys/key.c
> +++ b/security/keys/key.c
> @@ -22,6 +22,8 @@ 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);
> +DEFINE_SPINLOCK(key_graveyard_lock);
>   
>   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 +660,9 @@ 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_irqsave(&key_graveyard_lock, flags);
> +			list_add_tail(&key->graveyard_link, &key_graveyard);
> +			spin_unlock_irqrestore(&key_graveyard_lock, flags);
>   			schedule_work(&key_gc_work);
>   		}
>   	}

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ