Re: [PATCH v7 3/4] rust: sync: add SRCU abstraction

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

On Fri May 29, 2026 at 1:29 PM BST, Onur Özkan wrote:
> On Fri, 29 May 2026 13:07:18 +0100
> Gary Guo <gary@garyguo.net> wrote:
>
>> On Fri May 29, 2026 at 7:57 AM BST, Onur Özkan wrote:
>> >> >> > +#[pinned_drop]
>> >> >> > +impl PinnedDrop for Srcu {
>> >> >> > +    fn drop(self: Pin<&mut Self>) {
>> >> >> > +        let ptr = self.inner.get();
>> >> >> > +
>> >> >> > +        // SAFETY: By the type invariants, `self` contains a valid and pinned `struct srcu_struct`
>> >> >> > +        // and `srcu_readers_active()` only checks the active reader count.
>> >> >> > +        if unsafe { bindings::srcu_readers_active(ptr) } {
>> >> >> > +            crate::pr_warn!(
>> >> >> > +                "Leaked `Guard` detected while dropping SRCU; drop will block forever.\n"
>> >> >> > +            );
>> >> 
>> >> I think this could be a `warn_on` similar to how cleanup_srcu_struct handle the
>> >> condition.
>> >
>> > We also call cleanup_srcu_struct below. The idea was to provide additional
>> > information, we don't need to call warn_on twice.
>> 
>> If the code blocks on `synchronize_srcu` then there's no call to
>> `cleanup_srcu_struct`.
>
>
> Ah right. I can do that in this case but honestly it's still more informative
> with the current way. It explicitly tells you what the problem is.

While the error message itself is not that informative (there's some potential
to improve this, see
https://lore.kernel.org/all/DI1IQE7MDV4O.5B2DVIXMX2OT@garyguo.net/), the stack
trace produced by a `warn_on` would have all the information and is more useful
to troubleshoot than a `pr_warn` which doesn't tell you which `Srcu` being
dropped is causing the issue.

>> >> >
>> >> > Actually, now I am now thinking about whether we can come up with a better
>> >> > approach when we detect leaked guards. Initially I came up with the
>> >> > synchronize_srcu() solution because it would handle leaked guards automatically
>> >> > without requiring any additional checks. But now that we can actually detect
>> >> > whether guards are leaked the question becomes:
>> >> >
>> >> > 	"Is there a better option than effectively sleeping forever when leaked
>> >> > 	 guards are detected?"
>> >> >
>> >> > I have no plans for tomorrow other than finalizing this series including the
>> >> > question above.
>> >> 
>> >> The best solution is to proceed cleanups anyway, given Rust rules ensure that
>> >> these are actual leaks and not just srcu read-side critical section that failed
>> >> to synchronize with the destruction of SRCU.
>> >> 
>> >> This obviously require changes to the SRCU code though.
>> >
>> >
>> > The issue is difficult to fix purely from the C side. Once drop returns Rust
>> > is free to destroy srcu_struct. If srcu still has pending callback associated
>> > with that srcu_struct, for example from a future call_srcu() wrapper then
>> > returning from drop while readers are active can turn into a UAF. There is also
>> > no way to handle callbacks in a reasonable way in cleanup logic while there are
>> > active readers.
>> 
>> Callbacks should be flushed during the drop due to srcu_barrier. Am I missing
>> something?
>
> No. Callbacks can only be invoked once the grace period has completed [1], which
> can never happen while there is an active reader.
>
> [1]: https://elixir.bootlin.com/linux/v7.1-rc5/source/kernel/rcu/srcutree.c#L1452-L1454

Well, then srcu_barrier will not return. When `srcu_barrier` returns all
in-flight SRCU callbacks must have been executed.

Best,
Gary

>
>> 
>> I'm pretty sure that, if we disregard potential misuses from C side, removing
>> all "leak it" paths would be fine and won't leak to UAF if all users are from
>> Rust side.
>> 
>> To be very clear, I am not advocating to actually implement this way. I agree
>> with your conclusion below that this is broken code and a warning + blocking is
>> good enough. This is really just my thoughts on your "is there a better option"
>> question, and I think it's better in ideal world, but I think blocking is a
>> good pragmatic choice.
>
> I see. Maybe I should have phrased the question like "Is there a better option
> with similar complexity" to be more clear.