On Sat, 02 May 2026 18:55:56 +0100 Gary Guo <[email protected]> wrote:
> On Sat May 2, 2026 at 5:27 PM BST, Onur Özkan wrote: > > Add a Rust abstraction for sleepable RCU. > > > > 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. > > > > Signed-off-by: Onur Özkan <[email protected]> > > --- > > rust/kernel/sync.rs | 2 + > > rust/kernel/sync/srcu.rs | 152 +++++++++++++++++++++++++++++++++++++++ > > 2 files changed, 154 insertions(+) > > create mode 100644 rust/kernel/sync/srcu.rs > > > > diff --git a/rust/kernel/sync.rs b/rust/kernel/sync.rs > > index 993dbf2caa0e..0d6a5f1300c3 100644 > > --- a/rust/kernel/sync.rs > > +++ b/rust/kernel/sync.rs > > @@ -21,6 +21,7 @@ > > pub mod rcu; > > mod refcount; > > mod set_once; > > +pub mod srcu; > > > > pub use arc::{Arc, ArcBorrow, UniqueArc}; > > pub use completion::Completion; > > @@ -31,6 +32,7 @@ > > pub use locked_by::LockedBy; > > pub use refcount::Refcount; > > pub use set_once::SetOnce; > > +pub use srcu::Srcu; > > > > /// Represents a lockdep class. > > /// > > diff --git a/rust/kernel/sync/srcu.rs b/rust/kernel/sync/srcu.rs > > new file mode 100644 > > index 000000000000..7bd713e96375 > > --- /dev/null > > +++ b/rust/kernel/sync/srcu.rs > > @@ -0,0 +1,152 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > + > > +//! Sleepable read-copy update (SRCU) support. > > +//! > > +//! C header: [`include/linux/srcu.h`](srctree/include/linux/srcu.h) > > + > > +use crate::{ > > + bindings, > > + error::to_result, > > + prelude::*, > > + sync::LockClassKey, > > + types::{ > > + NotThreadSafe, > > + Opaque, // > > + }, > > +}; > > + > > +use pin_init::pin_data; > > + > > +/// Creates an [`Srcu`] initialiser with the given name and a > > newly-created lock class. > > +#[macro_export] > > #[doc(hidden)] > > Given this is a new macro, let's not encourage user from using it from crate > root. > > > +macro_rules! new_srcu { > > + ($($name:literal)?) => { > > + $crate::sync::Srcu::new($crate::optional_name!($($name)?), > > $crate::static_lock_class!()) > > + }; > > +} > > +pub use new_srcu; > > + > > +/// Sleepable read-copy update primitive. > > +/// > > +/// SRCU readers may sleep while holding the read-side guard. > > +/// > > +/// The destructor may sleep. > > +/// > > +/// # Invariants > > +/// > > +/// This represents a valid `struct srcu_struct` initialized by the C SRCU > > API > > +/// and it remains pinned and valid until the pinned destructor runs. > > +#[repr(transparent)] > > +#[pin_data(PinnedDrop)] > > +pub struct Srcu { > > + #[pin] > > + inner: Opaque<bindings::srcu_struct>, > > +} > > + > > +impl Srcu { > > + /// Creates a new SRCU instance. > > + #[inline] > > + pub fn new(name: &'static CStr, key: Pin<&'static LockClassKey>) -> > > impl PinInit<Self, Error> { > > + try_pin_init!(Self { > > + inner <- Opaque::try_ffi_init(|ptr: *mut > > bindings::srcu_struct| { > > + // SAFETY: `ptr` points to valid uninitialised memory for > > a `srcu_struct`. > > + to_result(unsafe { > > + bindings::init_srcu_struct_with_key(ptr, > > name.as_char_ptr(), key.as_ptr()) > > + }) > > + }), > > + }) > > + } > > + > > + /// Enters an SRCU read-side critical section. > > + /// > > + /// # Safety > > + /// > > + /// The returned [`Guard`] must not be leaked. Leaking it with > > [`core::mem::forget`] > > + /// leaves the SRCU read-side critical section active. > > I generally would prefer if we could use guard-like API instead of forcing a > callback. Me too and developers can still do that. I think the safety contract here is very simple to handle. It's essentially this: // SAFETY: Guard is not leaked. let _guard = unsafe { x.read_lock() }; To me it's very simple and straightforward for both the developer and the reviewer. It doesn't add any overhead to the implementation and it ensures that the developer (and later the reviewer) is aware of the potential issue. Of course, there's also the safe option if the developer is happy with closure-based API: x.with_read_lock(|_guard| { ... }); So it allows you to use the guard-based approach directly with the requirement of a safety comment and also provides a safe API for developers who don't want to deal with that. I am not sure if you fall into the third category, which is "I don't like writing safety comments and I don't like the closure-based approach" :) > > I suppose the only reason that this is unsafe is the "just leak it" condition > when cleaning up SRCU struct, which skips cleaning up delayed work, which > could > call into `process_srcu`, which accesses `srcu_struct`. This however is *not* > leaked, because it's controlled by the user. Only the auxillary data allocated > by SRCU is leaked. So UAF is going to happen. > > So in some aspect, the leak caused by `srcu_readers_active(ssp)` can cause > more > damage compared to just continuing cleanup despite active users? I think this > could be changed in one of these ways: > * Have SRCU allocate all memory instead, and user-side would just have a > `struct srcu_struct*`; then leaking would be safe. This is probably a bit > difficult to change because it affects many users. We could do the same on the Rust side only. Basically instead of embedding srcu_struct in Rust srcu, allocate it separately and store its pointer. Then, if cleanup hits the active reader case, we could leak that allocation so any remaining srcu work does not hit UAF. I was aware of this option but I would prefer to avoid it because it adds an extra allocation for every Rust srcu. > * Continue to flush delayed work and stop the timer, and then leak before the > actual kfree happens. Can you say more? I didn't understand this particular solution. > * Trigger a `BUG` when the leak condition is hit for Rust users. We need an atomic counter to detect the leak and I thought that would be too much overhead for this abstraction. Basically each lock and drop will call an atomic operation so. > * Declare the `WARN_ON` to be a sufficient protection and say this can be > considered safe. Kinda similar to the strategy we take to the > sleep-inside-atomic context issue. I think this is a rather weak precaution. > > > + #[inline] > > + pub unsafe fn read_lock(&self) -> Guard<'_> { > > + // SAFETY: By the type invariants, `self` contains a valid `struct > > srcu_struct`. > > + let idx = unsafe { bindings::srcu_read_lock(self.inner.get()) }; > > + > > + // INVARIANT: `idx` was returned by `srcu_read_lock()` for this > > `Srcu`. > > + Guard { > > + srcu: self, > > + idx, > > + _not_send: NotThreadSafe, > > + } > > + } > > + > > + /// Runs `f` in an SRCU read-side critical section. > > + #[inline] > > + pub fn with_read_lock<T>(&self, f: impl FnOnce(&Guard<'_>) -> T) -> T { > > + // SAFETY: The guard is owned within this function and is not > > leaked. > > + let guard = unsafe { self.read_lock() }; > > + > > + f(&guard) > > + } > > + > > + /// Waits until all pre-existing SRCU readers have completed. > > + #[inline] > > + pub fn synchronize(&self) { > > + // SAFETY: By the type invariants, `self` contains a valid `struct > > srcu_struct`. > > + unsafe { bindings::synchronize_srcu(self.inner.get()) }; > > + } > > + > > + /// Waits until all pre-existing SRCU readers have completed, > > expedited. > > + /// > > + /// This requests a lower-latency grace period than > > [`Srcu::synchronize`] typically > > + /// at the cost of higher system-wide overhead. Prefer > > [`Srcu::synchronize`] by default > > + /// and use this variant only when reducing reset or teardown latency > > is more important > > + /// than the extra cost. > > + #[inline] > > + pub fn synchronize_expedited(&self) { > > + // SAFETY: By the type invariants, `self` contains a valid `struct > > srcu_struct`. > > + unsafe { bindings::synchronize_srcu_expedited(self.inner.get()) }; > > + } > > +} > > + > > +#[pinned_drop] > > +impl PinnedDrop for Srcu { > > + fn drop(self: Pin<&mut Self>) { > > + let ptr = self.inner.get(); > > + > > This needs a comment on why this is invoked. Something like: > > // Ensure all SRCU callbacks have been finished before freeing. > > Best, > Gary > > > + // SAFETY: By the type invariants, `self` contains a valid and > > pinned `struct srcu_struct`. > > + unsafe { bindings::srcu_barrier(ptr) }; > > + // SAFETY: Same as above. > > + unsafe { bindings::cleanup_srcu_struct(ptr) }; > > + } > > +} > > + > > +// SAFETY: `srcu_struct` may be shared and used across threads. > > +unsafe impl Send for Srcu {} > > +// SAFETY: `srcu_struct` may be shared and used concurrently. > > +unsafe impl Sync for Srcu {}
