On Sun May 3, 2026 at 4:39 AM BST, Onur Özkan wrote:
>> > +/// Sleepable read-copy update primitive.
>> > +///
>> > +/// SRCU readers may sleep while holding the read-side guard.
>> > +///
>> > +/// The destructor may sleep.
>> > +///
>> > +/// # Invariants
>> > +///
>> > +/// This represents a valid `struct srcu_struct` initialized by the C
>> > SRCU API
>> > +/// and it remains pinned and valid until the pinned destructor runs.
>> > +#[repr(transparent)]
>> > +#[pin_data(PinnedDrop)]
>> > +pub struct Srcu {
>> > + #[pin]
>> > + inner: Opaque<bindings::srcu_struct>,
>> > +}
>> > +
>> > +impl Srcu {
>> > + /// Creates a new SRCU instance.
>> > + #[inline]
>> > + pub fn new(name: &'static CStr, key: Pin<&'static LockClassKey>) ->
>> > impl PinInit<Self, Error> {
>> > + try_pin_init!(Self {
>> > + inner <- Opaque::try_ffi_init(|ptr: *mut
>> > bindings::srcu_struct| {
>> > + // SAFETY: `ptr` points to valid uninitialised memory for
>> > a `srcu_struct`.
>> > + to_result(unsafe {
>> > + bindings::init_srcu_struct_with_key(ptr,
>> > name.as_char_ptr(), key.as_ptr())
>> > + })
>> > + }),
>> > + })
>> > + }
>> > +
>> > + /// Enters an SRCU read-side critical section.
>> > + ///
>> > + /// # Safety
>> > + ///
>> > + /// The returned [`Guard`] must not be leaked. Leaking it with
>> > [`core::mem::forget`]
>> > + /// leaves the SRCU read-side critical section active.
>>
>> I generally would prefer if we could use guard-like API instead of forcing a
>> callback.
>
> Me too and developers can still do that. I think the safety contract here is
> very simple to handle. It's essentially this:
>
> // SAFETY: Guard is not leaked.
> let _guard = unsafe { x.read_lock() };
>
> To me it's very simple and straightforward for both the developer and the
> reviewer. It doesn't add any overhead to the implementation and it ensures
> that the developer (and later the reviewer) is aware of the potential issue.
>
> Of course, there's also the safe option if the developer is happy with
> closure-based API:
>
> x.with_read_lock(|_guard| {
> ...
> });
>
> So it allows you to use the guard-based approach directly with the requirement
> of a safety comment and also provides a safe API for developers who don't want
> to deal with that. I am not sure if you fall into the third category, which is
> "I don't like writing safety comments and I don't like the closure-based
> approach" :)
We have been avoiding using callback-based API if there's an alternatively way
to achieve this. There has been quite a very precedents with this:
- spin_lock_irqsave requires taking and releasing in correct order, which is
easy to solve with a callback approach. The same logic reasoning can be used
to provide an unsafe API + safe callback API, but Boqun & Lyude reworked the
spinlock IRQ design so we don't need that anymore.
- `Task::current` API could easily be replaced callback-based approach, but we
used a macro to achieve without unsafe.
The API here is not inherently impossible to use guard API. It's not safe today
because a very specific detail. The callback-API is the "path of least
resistence" approach, but it's not the optimal one.
>
>>
>> I suppose the only reason that this is unsafe is the "just leak it" condition
>> when cleaning up SRCU struct, which skips cleaning up delayed work, which
>> could
>> call into `process_srcu`, which accesses `srcu_struct`. This however is *not*
>> leaked, because it's controlled by the user. Only the auxillary data
>> allocated
>> by SRCU is leaked. So UAF is going to happen.
>>
>> So in some aspect, the leak caused by `srcu_readers_active(ssp)` can cause
>> more
>> damage compared to just continuing cleanup despite active users? I think this
>> could be changed in one of these ways:
>> * Have SRCU allocate all memory instead, and user-side would just have a
>> `struct srcu_struct*`; then leaking would be safe. This is probably a bit
>> difficult to change because it affects many users.
>
> We could do the same on the Rust side only. Basically instead of embedding
> srcu_struct in Rust srcu, allocate it separately and store its pointer. Then,
> if cleanup hits the active reader case, we could leak that allocation so any
> remaining srcu work does not hit UAF. I was aware of this option but I would
> prefer to avoid it because it adds an extra allocation for every Rust srcu.
>
>> * Continue to flush delayed work and stop the timer, and then leak before the
>> actual kfree happens.
>
> Can you say more? I didn't understand this particular solution.
I was thinking that doing this _might_ be sufficient. I have to admit that I've
not very familar with the internal implementation of SRCU to make it an
assertion though.
diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
index 0d01cd8c4b4a..5d75a4dbb6e5 100644
--- a/kernel/rcu/srcutree.c
+++ b/kernel/rcu/srcutree.c
@@ -717,8 +717,6 @@ void cleanup_srcu_struct(struct srcu_struct *ssp)
raw_spin_unlock_irq_rcu_node(ssp->srcu_sup);
if (WARN_ON(!delay))
return; /* Just leak it! */
- if (WARN_ON(srcu_readers_active(ssp)))
- return; /* Just leak it! */
/* Wait for irq_work to finish first as it may queue a new work. */
irq_work_sync(&sup->irq_work);
flush_delayed_work(&sup->work);
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?
>
>> * Trigger a `BUG` when the leak condition is hit for Rust users.
>
> We need an atomic counter to detect the leak and I thought that would be too
> much overhead for this abstraction. Basically each lock and drop will call an
> atomic operation so.
You could just check if srcu_sup is NULL after calling `cleanup_srcu_struct`.
Best,
Gary
>
>> * Declare the `WARN_ON` to be a sufficient protection and say this can be
>> considered safe. Kinda similar to the strategy we take to the
>> sleep-inside-atomic context issue.
>
> I think this is a rather weak precaution.
>
