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 [off-list ref]
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.
quoted hunk ↗ jump to hunk
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