Re: [PATCH v2 2/3] rust: sync: add SRCU abstraction

["Onur Özkan" <work@onurozkan.dev>]

On Sat, 02 May 2026 18:55:56 +0100
Gary Guo <gary@garyguo.net> 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 <work@onurozkan.dev>
> > ---
> >  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 {}