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.
Best,
Gary
