Re: [PATCH v7] KEYS: key_garbage_collector(): process in chunks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] KEYS: key_put() sets KEY_GC_KEY_EXPIRED via key_schedule_gc(0)
--

commit 46ff4f9d21b46f71b2432aa577bc3594d1df5791
Author: Jarkko Sakkinen <jarkko@kernel.org>

KEYS: key_garbage_collector(): process in chunks

This commit updates key_garbage_collector() and key_gc_unused_keys() to scan
the key serial tree and destroy unused keys in bounded chunks of 64 keys. It
also introduces key_schedule_gc_work() and KEY_GC_REAP_REQUESTED to preserve
garbage collection requests across chunked workqueue continuations.

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 eda445f815d47..ea2efde5f33fd 100644
> --- a/security/keys/gc.c
> +++ b/security/keys/gc.c

[ ... ]

> @@ -44,6 +56,21 @@ struct key_type key_type_dead = {
>  	.name = ".dead",
>  };
>  
> +/*
> + * 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
> @@ -57,7 +84,8 @@ void key_schedule_gc(time64_t gc_at)
>  
>  	if (gc_at <= now || test_bit(KEY_GC_REAP_KEYTYPE, &key_gc_flags)) {
>  		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) {

[ ... ]

> diff --git a/security/keys/key.c b/security/keys/key.c
> index a438c4508595d..ab3d3a70705af 100644
> --- a/security/keys/key.c
> +++ b/security/keys/key.c
> @@ -661,7 +661,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(0);
>  		}
>  	}
>  }

[Severity: Medium]
When key_put() drops the final reference on a key
(refcount_dec_and_test(&key->usage)), calling key_schedule_gc(0) causes
gc_at <= now to evaluate as true and calls key_schedule_gc_work(true), which
sets KEY_GC_KEY_EXPIRED in key_gc_flags:

key_put()
  key_schedule_gc(0)
    key_schedule_gc_work(true)
      set_mask_bits(&key_gc_flags, 0,
                    BIT(KEY_GC_REAP_REQUESTED) | BIT(KEY_GC_KEY_EXPIRED))

In key_garbage_collector(), KEY_GC_KEY_EXPIRED sets KEY_GC_REAPING_LINKS in
gc->state:

	if (test_and_clear_bit(KEY_GC_KEY_EXPIRED, &key_gc_flags))
		gc->state |= KEY_GC_REAPING_LINKS;
	...
		if ((gc->state & KEY_GC_REAPING_LINKS) ||
		    unlikely(gc->state & KEY_GC_REAPING_DEAD_2)) {
			if (key->type == &key_type_keyring)
				goto found_keyring;
		}

Because every keyring link holds a reference on key->usage, a key reaching a
usage count of zero has no remaining links in any keyring and only needs to be
removed from key_serial_tree and freed via the graveyard list. With
KEY_GC_KEY_EXPIRED set on every final key_put(), key_garbage_collector() will
instead drop key_serial_lock on every keyring in key_serial_tree and run
keyring_gc() across all keyrings in the system whenever any unreferenced key
is freed. In addition, if key_gc_unused_keys() frees a keyring that held links
to other keys, keyring_destroy() will call key_put() and set
KEY_GC_KEY_EXPIRED again for the next pass.

Similarly, in key_schedule_gc(), if gc_at > now while KEY_GC_REAP_KEYTYPE is
set, key_schedule_gc_work(true) will set KEY_GC_KEY_EXPIRED even though the
expiration time has not been reached.

Should key_put() call key_schedule_gc_work(false) directly instead of
key_schedule_gc(0), and should key_schedule_gc() pass gc_at <= now to
key_schedule_gc_work()?

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