Re: [RFC PATCH v2 01/11] KEYS: Add key_gc_flush() for draining RCU callbacks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] KEYS: premature return in key_gc_flush() when key_gc_work queues RCU
--

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

KEYS: Add key_gc_flush() for draining RCU callbacks

This commit introduces key_gc_flush() to drain RCU callbacks and flush
key_gc_work until no further garbage collection passes remain queued. This
provides callers clearing an internal key cache before module unload with a
synchronous barrier for key destruction.

> diff --git a/security/keys/gc.c b/security/keys/gc.c
> index eda445f815d47..3cfbf00085b3d 100644
> --- a/security/keys/gc.c
> +++ b/security/keys/gc.c
> @@ -21,6 +21,20 @@ unsigned key_gc_delay = 5 * 60;
>  static void key_garbage_collector(struct work_struct *work);
>  DECLARE_WORK(key_gc_work, key_garbage_collector);
>  
> +/**
> + * key_gc_flush - Wait for pending key unlinks and garbage collection.
> + *
> + * Drain RCU callbacks from keyring edits and wait for the key garbage
> + * collector work item to finish reaping unreferenced keys.
> + */
> +void key_gc_flush(void)
> +{
> +	do {
> +		rcu_barrier();
> +	} while (flush_work(&key_gc_work));
> +}
> +EXPORT_SYMBOL_GPL(key_gc_flush);

[Severity: Medium]
Can key_gc_flush() return prematurely before keys invalidated by
key_invalidate() or key_remove_domain() are unlinked and reaped?

Unlike keyring_clear(), which queues assoc_array_rcu_cleanup() via call_rcu()
synchronously before returning, key_invalidate() schedules key_gc_work to
reap links first:

security/keys/key.c:key_invalidate() {
	if (!test_bit(KEY_FLAG_INVALIDATED, &key->flags)) {
		down_write_nested(&key->sem, 1);
		if (!test_and_set_bit(KEY_FLAG_INVALIDATED, &key->flags)) {
			notify_key(key, NOTIFY_KEY_INVALIDATED, 0);
			key_schedule_gc_links();
		}
		up_write(&key->sem);
	}
}

When key_garbage_collector() runs the link-reaping pass at found_keyring, it
calls keyring_gc() -> assoc_array_gc() -> assoc_array_apply_edit(), which
queues assoc_array_rcu_cleanup() via call_rcu() without setting
KEY_GC_REAP_AGAIN:

security/keys/gc.c:key_garbage_collector() {
	/* We found a keyring and we need to check the payload for links to
	 * dead or expired keys.  We don't flag another reap immediately as we
	 * have to wait for the old payload to be destroyed by RCU before we
	 * can reap the keys to which it refers.
	 */
found_keyring:
	spin_unlock(&key_serial_lock);
	keyring_gc(key, limit);
	goto maybe_resched;
}

Because key_gc_flush() calls rcu_barrier() before flush_work(&key_gc_work),
if key_invalidate() is followed by key_gc_flush(), the RCU callback is not
queued yet when the first rcu_barrier() starts. If key_gc_work runs
keyring_gc(), queues edit->rcu via call_rcu() after rcu_barrier() has already
passed that CPU (or returned), and finishes before flush_work(&key_gc_work)
is reached, flush_work(&key_gc_work) sees the work item is already idle and
returns false.

At that point the loop exits while assoc_array_rcu_cleanup() is still pending
in RCU, key_put() has not been called on the unlinked key, and
key_gc_unused_keys() has not reaped it.

Should flush_work(&key_gc_work) be called before rcu_barrier() in the loop
so that any pending link-reaping pass queues its RCU callbacks before
rcu_barrier() waits for them?

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