rust: add SRCU abstraction

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

rust/kernel/sync.rs +2 -0

index 993dbf2caa0e..0d6a5f1300c3 100644
@@ -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.
 ///

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

new file mode 100644
index 000000000000..7bd713e96375
@@ -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,

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

+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())
+                })

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

+    #[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) {

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

+        // 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 {}

Quoted 50 lines · Onur Özkan

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

rust/kernel/sync.rs +2 -0

index 993dbf2caa0e..0d6a5f1300c3 100644
@@ -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.
 ///

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

new file mode 100644
index 000000000000..7bd713e96375
@@ -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,

rust/kernel/sync/srcu.rs line 21

Quoted 10 lines

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

+    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.

Quoted 39 lines

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

+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.

rust/kernel/sync/srcu.rs line 65

Quoted 5 lines

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

+    ///
+    /// # 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.

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.

* Continue to flush delayed work and stop the timer, and then leak before the actual kfree happens.

* Trigger a BUG when the leak condition is hit for Rust users.

* 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.

Quoted 38 lines

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

+    #[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) {

rust/kernel/sync/srcu.rs line 112

Quoted 10 lines

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

+        // 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

Quoted 11 lines

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

+        // 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 {}

Quoted 32 lines

Quoted 32 lines

(quoted diff) +32 -0

+/// 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.

Quoted 8 lines

Review

Quoted 5 lines

(quoted diff) +5 -0

+    ///
+    /// # 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" :)

Quoted 8 lines

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.

Quoted 6 lines

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.

Quoted 2 lines

* 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.

Quoted 2 lines

* 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.

Quoted 4 lines

* 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.

Quoted 58 lines · Onur Özkan

Quoted 32 lines

Quoted 32 lines

(quoted diff) +32 -0

+/// 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.

Quoted 8 lines

Review

Quoted 5 lines

(quoted diff) +5 -0

+    ///
+    /// # 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| {
		...
	});

Quoted 5 lines

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" :)

We have been avoiding using callback-based API if there's an alternatively way to achieve this. There has been quite a very precedents with this:

- spin_lock_irqsave requires taking and releasing in correct order, which is easy to solve with a callback approach. The same logic reasoning can be used to provide an unsafe API + safe callback API, but Boqun & Lyude reworked the spinlock IRQ design so we don't need that anymore.

- Task::current API could easily be replaced callback-based approach, but we used a macro to achieve without unsafe.

The API here is not inherently impossible to use guard API. It's not safe today because a very specific detail. The callback-API is the "path of least resistence" approach, but it's not the optimal one.

Quoted 21 lines

Quoted 8 lines

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.

Quoted 6 lines

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.

Quoted 4 lines

Quoted 2 lines

* 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.

I was thinking that doing this _might_ be sufficient. I have to admit that I've not very familar with the internal implementation of SRCU to make it an assertion though.

kernel/rcu/srcutree.c +0 -2

index 0d01cd8c4b4a..5d75a4dbb6e5 100644
@@ -717,8 +717,6 @@ void cleanup_srcu_struct(struct srcu_struct *ssp)
 	raw_spin_unlock_irq_rcu_node(ssp->srcu_sup);
 	if (WARN_ON(!delay))
 		return; /* Just leak it! */
-	if (WARN_ON(srcu_readers_active(ssp)))
-		return; /* Just leak it! */
 	/* Wait for irq_work to finish first as it may queue a new work. */
 	irq_work_sync(&sup->irq_work);
 	flush_delayed_work(&sup->work);

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 3 lines

Quoted 2 lines

* Trigger a BUG when the leak condition is hit for Rust users.

Quoted 3 lines

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.

You could just check if srcu_sup is NULL after calling cleanup_srcu_struct.

Best,
Gary

Quoted 7 lines

Quoted 4 lines

* 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.

Quoted 138 lines · On Sun, 03 May 2026 20:25:03 +0100 Gary Guo <[email protected]> wrote:

Quoted 58 lines · Onur Özkan

Quoted 32 lines

Quoted 32 lines

(quoted diff) +32 -0

+/// 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.

Quoted 8 lines

Review

Quoted 5 lines

(quoted diff) +5 -0

+    ///
+    /// # 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| {
		...
	});

Quoted 5 lines

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" :)

We have been avoiding using callback-based API if there's an alternatively way to achieve this. There has been quite a very precedents with this:

- spin_lock_irqsave requires taking and releasing in correct order, which is easy to solve with a callback approach. The same logic reasoning can be used to provide an unsafe API + safe callback API, but Boqun & Lyude reworked the spinlock IRQ design so we don't need that anymore.

- Task::current API could easily be replaced callback-based approach, but we used a macro to achieve without unsafe.

The API here is not inherently impossible to use guard API. It's not safe today because a very specific detail. The callback-API is the "path of least resistence" approach, but it's not the optimal one.

Quoted 21 lines

Quoted 8 lines

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.

Quoted 6 lines

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.

Quoted 4 lines

Quoted 2 lines

* 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.

I was thinking that doing this _might_ be sufficient. I have to admit that I've not very familar with the internal implementation of SRCU to make it an assertion though.

kernel/rcu/srcutree.c +0 -2

index 0d01cd8c4b4a..5d75a4dbb6e5 100644
@@ -717,8 +717,6 @@ void cleanup_srcu_struct(struct srcu_struct *ssp)
 	raw_spin_unlock_irq_rcu_node(ssp->srcu_sup);
 	if (WARN_ON(!delay))
 		return; /* Just leak it! */
-	if (WARN_ON(srcu_readers_active(ssp)))
-		return; /* Just leak it! */
 	/* Wait for irq_work to finish first as it may queue a new work. */
 	irq_work_sync(&sup->irq_work);
 	flush_delayed_work(&sup->work);

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 3 lines

Quoted 2 lines

* Trigger a BUG when the leak condition is hit for Rust users.

Quoted 17 lines

Quoted 3 lines

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.

You could just check if srcu_sup is NULL after calling cleanup_srcu_struct.

Best,
Gary

Quoted 7 lines

Quoted 4 lines

* 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.

Alright, here is the alternative approach I just figured and I think this makes the most sense compared to the ones we discussed in this series:

	#[pinned_drop]
	impl PinnedDrop for Srcu {
		fn drop(self: Pin<&mut Self>) {
			let ptr = self.inner.get();

			// `cleanup_srcu_struct()` may return early if readers are still active. Because `Srcu`
			// owns the embedded `srcu_struct`, returning from `drop` in that state could free memory
			// that is still referenced by the C side.
			//
			// Wait for all readers to complete first. If any `Guard` was leaked, `synchronize_srcu()`
			// will sleep forever.
			//
			// SAFETY: By the type invariants, `self` contains a valid and pinned `struct srcu_struct`.
			unsafe { bindings::synchronize_srcu(ptr) };

			// ...

(the code snippet above is copied from [1])

As explained in the doc comments, we avoid the potential UAFs by sleeping forever which is considered non-unsafe, right? Alice said during today's call that "sleeping is not good, but it is not unsafe". If that's the case, I think this fixes the overall problem and we can make read_unlock safe again.

What do you think? I will hold the next version until we reach a common point.

[1]: https://github.com/onur-ozkan/linux/commit/28d9739f03

- Onur