On Mon, 06 Jul 2026 13:39:47 +0100
Gary Guo <[email protected]> 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/[email protected]/:
> >
> > 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
