Re: [PATCH v6] 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: missed KEY_GC_KEY_EXPIRED for keys expiring during chunked GC scan
--

commit 8a5b903eb11c1b0cb7aae033aae44253bd9ca4d8
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 to prevent workqueue lockups when
there are a large number of keys.

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

[ ... ]

> @@ -44,6 +56,20 @@ struct key_type key_type_dead = {
>  	.name = ".dead",
>  };
>  
> +/*
> + * Schedule a new GC pass or re-schedule an ongoing GC pass.
> + */
> +static void key_schedule_gc_work(void)
> +{
> +	/*
> +	 * Order caller updates before setting KEY_GC_REAP_REQUESTED, given that
> +	 * set_bit() is unordered.
> +	 */
> +	smp_mb__before_atomic();
> +	set_bit(KEY_GC_REAP_REQUESTED, &key_gc_flags);
> +	schedule_work(&key_gc_work.work);
> +}
> +
>  /*
>   * Schedule a garbage collection run.
>   * - time precision isn't particularly important
> @@ -57,7 +83,7 @@ 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);
> +		key_schedule_gc_work();
>  	} else if (gc_at < key_gc_next_run) {
>  		kdebug("DEFERRED");
>  		key_gc_next_run = gc_at;

[ ... ]

> @@ -178,54 +206,64 @@ 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 */
> +	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 */
>  #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;
> +	unsigned int chunk = KEY_GC_CHUNK_SIZE;
>  	struct key *key;
> -	time64_t new_timer, limit, expiry;
> +	time64_t expiry;
>  
> -	kenter("[%lx,%x]", key_gc_flags, gc_state);
> +	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();
>  
>  	/* 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;
> +	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;
> +		gc->state |= KEY_GC_REAPING_LINKS;

[ ... ]

> @@ -239,24 +277,24 @@ 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;
>  			}
>  		}

[ ... ]

> @@ -269,7 +307,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;
> @@ -281,12 +323,12 @@ 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);
>  	}

[Severity: Medium]
Can an expired key be left linked in its keyring if wall-clock time
reaches or passes gc->end while a multi-chunk scan is in flight?

At the start of a pass in key_garbage_collector(), gc->start is sampled
once via ktime_get_real_seconds() and persisted across all chunk
continuations. When scanning a key whose effective reap time (expiry) is
greater than gc->start, keyring_gc(key, gc->start) does not unlink the key
during the current pass, and its expiration is recorded in gc->end so that
key_schedule_gc(gc->end) can arm the timer once the scan completes.

Because scanning is now split into chunks of KEY_GC_CHUNK_SIZE across
multiple schedule_work(&key_gc_work.work) continuations, wall-clock time
can advance to or past gc->end before the final chunk finishes and calls
key_schedule_gc(gc->end).

When key_schedule_gc() sees gc_at <= now (or KEY_GC_REAP_KEYTYPE is set),
it takes the immediate branch:

    key_garbage_collector()
      key_schedule_gc(gc->end)
        key_schedule_gc_work()
          set_bit(KEY_GC_REAP_REQUESTED, &key_gc_flags);
          schedule_work(&key_gc_work.work);

Unlike the deferred branch (which arms key_gc_timer -> key_gc_timer_func()
-> key_schedule_gc_links() and sets KEY_GC_KEY_EXPIRED),
key_schedule_gc_work() only sets KEY_GC_REAP_REQUESTED without setting
KEY_GC_KEY_EXPIRED.

When the next pass starts, gc->start is updated to the new wall-clock time
(now >= expiry) and KEY_GC_REAPING_LINKS is not set in gc->state. In that
pass, keyring_gc() is not called because KEY_GC_REAPING_LINKS is clear, and
expiry > gc->start is now false for the expired key, so the key is neither
unlinked from its keyring nor scheduled for timer reaping until an
unrelated event sets KEY_GC_KEY_EXPIRED.

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