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

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

On Mon, 11 May 2026 20:11:26 +0300
Onur Özkan <work@onurozkan.dev> wrote:

> On Sun, 03 May 2026 20:25:03 +0100
> Gary Guo <gary@garyguo.net> wrote:
> 
> > 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.
> > >
> > 
> 
> 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.

I will be less available in the coming week and since there has been no response
to my proposal here, I included it in v3 and sent it [1].

[1]: https://lore.kernel.org/all/20260522054228.114814-1-work@onurozkan.dev

Regards,
Onur

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