Re: [PATCH v10 4/5] rust: sync: add SRCU abstraction

[Onur Özkan]

On Mon, 06 Jul 2026 13:39:47 +0100
Gary Guo <gary@xxxxxxxxxxx> wrote:

> On Mon Jul 6, 2026 at 12:55 PM BST, Alice Ryhl wrote:
> > On Sat, Jun 13, 2026 at 09:40:10AM +0300, Onur Özkan wrote:
> >> +#[pinned_drop]
> >> +impl PinnedDrop for Srcu {
> >> +    fn drop(self: Pin<&mut Self>) {
> >> +        let ptr = self.inner.get();
> >> +
> >> +        if crate::warn_on!(
> >> +            // SAFETY: By the type invariants, `self` contains a valid and pinned `struct srcu_struct`
> >> +            // and `srcu_readers_active()` only checks the active reader count.
> >> +            unsafe { bindings::srcu_readers_active(ptr) }
> >> +        ) {
> >> +            // `cleanup_srcu_struct()` may return early if there are still active readers.
> >> +            // This should only happen if a guard was leaked with `mem::forget`, which is
> >> +            // "WRONG" code and may cause a UAF because Rust will free the `srcu_struct`
> >> +            // while it is still referenced from the C side (e.g. by `call_srcu()` callbacks).
> >> +            //
> >> +            // Another consequence of leaking guards is that `call_srcu()` callbacks will
> >> +            // never run because the grace period can never complete due to permanently
> >> +            // active readers (i.e. leaked guards).
> >> +            //
> >> +            // If this ever happens, that means the guard was leaked by mistake and the
> >> +            // caller must fix the bug. Sleeping here is intentional and less harmful
> >> +            // than risking a UAF.
> >> +            //
> >> +            // SAFETY: By the type invariants, `self` contains a valid and pinned
> >> +            // `struct srcu_struct`.
> >> +            unsafe { bindings::synchronize_srcu(ptr) };
> >> +        }
> >> +
> >> +        // Ensure all SRCU callbacks have been finished before freeing.
> >> +        // SAFETY: By the type invariants, `self` contains a valid and pinned `struct srcu_struct`.
> >> +        unsafe { bindings::srcu_barrier(ptr) };
> >
> > Hmm. It's not entirely clear to me that synchronize_srcu() is needed
> > here. If there are calls to srcu_read_lock() that do not have a matching
> > unlock due to use of mem::forget(), then either there is a pending
> > call_srcu() callback in the queue, in which case srcu_barrier() already
> > sleeps forever, or there are no such callbacks in which case I don't
> > think this actually leads to UAF.
> >
> > Thoughts?
> 
> If srcu_readers_active returns true, then `cleanup_srcu_struct` will hit one of
> the "leak it" code path, and the question is whether that is okay.
> 
> My analysis in https://lore.kernel.org/all/DI9ADEUBBUD2.22OR0R12PKVQL@xxxxxxxxxxx/:
> >
> > 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?
> 
> So I _think_ it is not needed, but this is quite heavily related to internals of
> SRCU implementation.

So when cleanup_srcu_struct() hits "leak it" path, I wanted to make sure nothing
can access to that freed srcu_struct and the easiest and safest approach for it
is calling synchronize_srcu().

Callbacks are just a detail that can become problem after this leak, so relying
on srcu_barrier() to handle the "leak it" case doesn't sound like a good
solution to me. It does not make the active reader case safe by itself and
honestly we don't really lose anything with calling synchronize_srcu() as an
additional safety guard, so I just kept it that way..

Thanks,
Onur

> 
> Best,
> Gary