rust: add SRCU abstraction

Restructure the SRCU initialization functions so it always follows one direction:

    init_srcu_struct() -> __init_srcu_struct() -> lockdep or generic

This uses the same wrapper style as mutex. It avoids the old confusing style where init_srcu_struct() and __init_srcu_struct() called each other in different configs. It also helps Rust side to have simpler helper for SRCU initialization.

Signed-off-by Onur Özkan <[email protected]>

---
 include/linux/srcu.h  | 29 ++++++++++++++++++++---------
 kernel/rcu/srcutiny.c | 12 ++++++------
 kernel/rcu/srcutree.c | 11 ++++++-----
 3 files changed, 32 insertions(+), 20 deletions(-)

Add helper wrappers for SRCU functions that are exposed to Rust through generated bindings.

Signed-off-by Onur Özkan <[email protected]>

---
 rust/helpers/helpers.c |  1 +
 rust/helpers/srcu.c    | 30 ++++++++++++++++++++++++++++++
 2 files changed, 31 insertions(+)
 create mode 100644 rust/helpers/srcu.c

rust/kernel/sync/srcu.rs new file +21 -0

+#[pinned_drop]
+impl PinnedDrop for Srcu {
+    fn drop(self: Pin<&mut Self>) {
+        let ptr = self.inner.get();
+
+        if crate::warn_on!(
+            // SAFETY: By the type invariants, `self` contains a valid and pinned `struct srcu_struct`
+            // and `srcu_readers_active()` only checks the active reader count.
+            unsafe { bindings::srcu_readers_active(ptr) }
+        ) {
+            // `cleanup_srcu_struct()` may return early if there are still active readers.
+            // This should only happen if a guard was leaked with `mem::forget`, which is
+            // "WRONG" code and may cause a UAF because Rust will free the `srcu_struct`
+            // while it is still referenced from the C side (e.g. by `call_srcu()` callbacks).
+            //
+            // Another consequence of leaking guards is that `call_srcu()` callbacks will
+            // never run because the grace period can never complete due to permanently
+            // active readers (i.e. leaked guards).
+            //
+            // If this ever happens, that means the guard was leaked by mistake and the
+            // caller must fix the bug. Sleeping here is intentional and less harmful

Quoted 21 lines · Onur Özkan

rust/kernel/sync/srcu.rs new file +21 -0

+#[pinned_drop]
+impl PinnedDrop for Srcu {
+    fn drop(self: Pin<&mut Self>) {
+        let ptr = self.inner.get();
+
+        if crate::warn_on!(
+            // SAFETY: By the type invariants, `self` contains a valid and pinned `struct srcu_struct`
+            // and `srcu_readers_active()` only checks the active reader count.
+            unsafe { bindings::srcu_readers_active(ptr) }
+        ) {
+            // `cleanup_srcu_struct()` may return early if there are still active readers.
+            // This should only happen if a guard was leaked with `mem::forget`, which is
+            // "WRONG" code and may cause a UAF because Rust will free the `srcu_struct`
+            // while it is still referenced from the C side (e.g. by `call_srcu()` callbacks).
+            //
+            // Another consequence of leaking guards is that `call_srcu()` callbacks will
+            // never run because the grace period can never complete due to permanently
+            // active readers (i.e. leaked guards).
+            //
+            // If this ever happens, that means the guard was leaked by mistake and the
+            // caller must fix the bug. Sleeping here is intentional and less harmful

rust/kernel/sync/srcu.rs line 131

Quoted 10 lines

rust/kernel/sync/srcu.rs new file +10 -0

+            // than risking a UAF.
+            //
+            // SAFETY: By the type invariants, `self` contains a valid and pinned
+            // `struct srcu_struct`.
+            unsafe { bindings::synchronize_srcu(ptr) };
+        }
+
+        // Ensure all SRCU callbacks have been finished before freeing.
+        // SAFETY: By the type invariants, `self` contains a valid and pinned `struct srcu_struct`.
+        unsafe { bindings::srcu_barrier(ptr) };

Hmm. It's not entirely clear to me that synchronize_srcu() is needed here. If there are calls to srcu_read_lock() that do not have a matching unlock due to use of mem::forget(), then either there is a pending call_srcu() callback in the queue, in which case srcu_barrier() already sleeps forever, or there are no such callbacks in which case I don't think this actually leads to UAF.

Thoughts?

But after taking another look, I am not even sure if this is needed. A quick glance of the code it appears that __srcu_read_unlock doesn't do anything apart from adjusting the counter, and the SRCU grace period and thus the timers won't actually start unless there's a pending grace period, which won't start unless there's a call_srcu or sychronize_srcu. And we *know* that none of them would happen, as the lifetime guarantees that nothing accesses the Srcu struct when drop starts, and inside drop we have already invoked srcu_barrier().

So I think, even if we hit the "Just leak it" scenario, we can still safely deallocate the backing storage of srcu_struct and nothing should break?

Quoted 40 lines · Alice Ryhl

Quoted 21 lines · Onur Özkan

rust/kernel/sync/srcu.rs new file +21 -0

+#[pinned_drop]
+impl PinnedDrop for Srcu {
+    fn drop(self: Pin<&mut Self>) {
+        let ptr = self.inner.get();
+
+        if crate::warn_on!(
+            // SAFETY: By the type invariants, `self` contains a valid and pinned `struct srcu_struct`
+            // and `srcu_readers_active()` only checks the active reader count.
+            unsafe { bindings::srcu_readers_active(ptr) }
+        ) {
+            // `cleanup_srcu_struct()` may return early if there are still active readers.
+            // This should only happen if a guard was leaked with `mem::forget`, which is
+            // "WRONG" code and may cause a UAF because Rust will free the `srcu_struct`
+            // while it is still referenced from the C side (e.g. by `call_srcu()` callbacks).
+            //
+            // Another consequence of leaking guards is that `call_srcu()` callbacks will
+            // never run because the grace period can never complete due to permanently
+            // active readers (i.e. leaked guards).
+            //
+            // If this ever happens, that means the guard was leaked by mistake and the
+            // caller must fix the bug. Sleeping here is intentional and less harmful

rust/kernel/sync/srcu.rs line 131

Quoted 10 lines

rust/kernel/sync/srcu.rs new file +10 -0

+            // than risking a UAF.
+            //
+            // SAFETY: By the type invariants, `self` contains a valid and pinned
+            // `struct srcu_struct`.
+            unsafe { bindings::synchronize_srcu(ptr) };
+        }
+
+        // Ensure all SRCU callbacks have been finished before freeing.
+        // SAFETY: By the type invariants, `self` contains a valid and pinned `struct srcu_struct`.
+        unsafe { bindings::srcu_barrier(ptr) };

Hmm. It's not entirely clear to me that synchronize_srcu() is needed here. If there are calls to srcu_read_lock() that do not have a matching unlock due to use of mem::forget(), then either there is a pending call_srcu() callback in the queue, in which case srcu_barrier() already sleeps forever, or there are no such callbacks in which case I don't think this actually leads to UAF.

Quoted 1 line

Thoughts?

If srcu_readers_active returns true, then cleanup_srcu_struct will hit one of the "leak it" code path, and the question is whether that is okay.

My analysis in https://lore.kernel.org/all/[email protected]/:

Quoted 9 lines

But after taking another look, I am not even sure if this is needed. A quick glance of the code it appears that __srcu_read_unlock doesn't do anything apart from adjusting the counter, and the SRCU grace period and thus the timers won't actually start unless there's a pending grace period, which won't start unless there's a call_srcu or sychronize_srcu. And we *know* that none of them would happen, as the lifetime guarantees that nothing accesses the Srcu struct when drop starts, and inside drop we have already invoked srcu_barrier().

Quoted 2 lines

So I think, even if we hit the "Just leak it" scenario, we can still safely deallocate the backing storage of srcu_struct and nothing should break?

So I _think_ it is not needed, but this is quite heavily related to internals of SRCU implementation.

Best,
Gary

Add a Rust abstraction for sleepable RCU (SRCU), backed by C srcu_struct. Provide FFI helpers and a safe wrapper with a guard-based API for read-side critical sections.

Cleanup is handled via PinnedDrop. It first checks for active read-side sections and emits a warning if any guards were leaked. In that case, it waits in synchronize_srcu() rather than risking a UAF by freeing the srcu_struct that is still reachable from the C side. It then uses srcu_barrier() to drain pending callbacks before finally calling cleanup_srcu_struct().

Signed-off-by Onur Özkan <[email protected]>

---
 rust/kernel/sync.rs      |   2 +
 rust/kernel/sync/srcu.rs | 171 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 173 insertions(+)
 create mode 100644 rust/kernel/sync/srcu.rs

The immediate motivation is the Tyr reset infrastructure [1] which needs to serialize reset sensitive hardware access against reset and teardown paths. That reset series started to require many independent dependencies so this SRCU support is split out as a standalone Rust API to keep the reset series focused on the reset logic and easier to review, rebase and land.

Changes since v9:

- SRCU initialization restructuring is now a separate patch (the first one in this series).
- Comment fix on C initialization functions.

Changes since v8:

- Refactor srcu init handling to provide better init API (motivated by the "mutex.h" initializers.
- Use warn_on! instead of pr_warn! on leaked guard detection.

Changes since v7:

- Moved synchronize_srcu() call inside "if srcu_readers_active()" condition in "impl PinnedDrop for Srcu".
- Improved comments on the synchronize_srcu() call in "impl PinnedDrop for Srcu".

Changes since v6:

- Removed "CONFIG_DEBUG_LOCK_ALLOC" condition from "rust/helpers/srcu.c" and created a simple wrapper inside "include/linux/srcu.h" for it.

Changes since v5:

- Created separate srcu_readers_active() variants for "srcutiny.h" and "srcutree.h".

Changes since v4:

- Exposed srcu_readers_active from C side and wired it to the Rust helpers.
- Used srcu_readers_active() in SRCU drop and logged with pr_warn if there are leaked guards during the drop.

Changes since v3 (which are for Sashiko notes [2]):

- Added rust helpers for srcu_barrier() and synchronize_srcu_expedited() so the abstraction builds with CONFIG_TINY_SRCU, where these are static inline functions.
- Added missing INVARIANT comment in Srcu::new() about why the type invariants hold after successful initialization.

Changes since v2:

- Removed closure-based API.
- Added #[doc(hidden)] on new_srcu macro.
- Added #[must_use..] on srcu::Guard.
- Improved the clean-up path (PinnedDrop implementation) which eventually made read_lock safe with leaked guards.

Changes since v1:

- Made the owned SRCU read-side guard API unsafe and added a safe closure based helper for callers that do not need to keep the guard. This is to avoid UB on the C side cleanup_srcu_struct where the SRCU struct is freed while there are still active guards, which can happen if the caller leaks the guard e.g., with mem::forget().
- Improved doc comments.

v1: https://lore.kernel.org/all/[email protected]
v2: https://lore.kernel.org/all/[email protected]
v3: https://lore.kernel.org/all/[email protected]
v4: https://lore.kernel.org/all/[email protected]
v5: https://lore.kernel.org/all/[email protected]
v6: https://lore.kernel.org/all/[email protected]
v7: https://lore.kernel.org/all/[email protected]
v8: https://lore.kernel.org/all/[email protected]
v9: https://lore.kernel.org/all/[email protected]

[1]: https://lore.kernel.org/all/[email protected]
[2]: https://sashiko.dev/#/patchset/[email protected]?part=2

Onur Özkan (5):
srcu: make init_srcu_struct() consistently wrap __init_srcu_struct()
rust: helpers: add SRCU helpers
srcu: expose srcu_readers_active()
rust: sync: add SRCU abstraction
MAINTAINERS: add Rust SRCU files to SRCU entry

 MAINTAINERS              |   3 +
 include/linux/srcu.h     |  29 ++++---
 include/linux/srcutiny.h |  13 +++
 include/linux/srcutree.h |  24 ++++++
 kernel/rcu/srcutiny.c    |  14 ++--
 kernel/rcu/srcutree.c    |  36 ++-------
 rust/helpers/helpers.c   |   1 +
 rust/helpers/srcu.c      |  35 ++++++++
 rust/kernel/sync.rs      |   2 +
 rust/kernel/sync/srcu.rs | 171 +++++++++++++++++++++++++++++++++++++++
 10 files changed, 282 insertions(+), 46 deletions(-)
 create mode 100644 rust/helpers/srcu.c
 create mode 100644 rust/kernel/sync/srcu.rs

Signature · 1 line

2.51.2

The immediate motivation is the Tyr reset infrastructure [1] which needs to serialize reset sensitive hardware access against reset and teardown paths. That reset series started to require many independent dependencies so this SRCU support is split out as a standalone Rust API to keep the reset series focused on the reset logic and easier to review, rebase and land.

The immediate motivation is the Tyr reset infrastructure [1] which needs to serialize reset sensitive hardware access against reset and teardown paths. That reset series started to require many independent dependencies so this SRCU support is split out as a standalone Rust API to keep the reset series focused on the reset logic and easier to review, rebase and land.

Quoted 7 lines · Onur Özkan

The immediate motivation is the Tyr reset infrastructure [1] which needs to serialize reset sensitive hardware access against reset and teardown paths. That reset series started to require many independent dependencies so this SRCU support is split out as a standalone Rust API to keep the reset series focused on the reset logic and easier to review, rebase and land.

[...]

Thank you all!

Reviewed-by Boqun Feng <[email protected]>

Paul, per our previous discussion, I think this series should into rcu tree, since it changes a significant part of SRCU C code. Thanks!