Re: [RFC PATCH 4/4] 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: 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