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

["Gary Guo" <gary@garyguo.net>]

On Sun May 3, 2026 at 4:39 AM BST, Onur Özkan wrote:
>> > +/// 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" :)

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.

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

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.

diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
index 0d01cd8c4b4a..5d75a4dbb6e5 100644
--- a/kernel/rcu/srcutree.c
+++ b/kernel/rcu/srcutree.c
@@ -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?

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

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

Best,
Gary

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