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

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

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@garyguo.net/:
>
> 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.

Best,
Gary