Re: [PATCH v2] rust: doc: Clarify safety invariants for Revocable type

["Benno Lossin" <lossin@kernel.org>]

On Mon May 19, 2025 at 11:55 AM CEST, Danilo Krummrich wrote:
> On Mon, May 19, 2025 at 11:18:42AM +0200, Benno Lossin wrote:
>> On Mon May 19, 2025 at 10:50 AM CEST, Danilo Krummrich wrote:
>> > On Sat, May 17, 2025 at 09:09:02PM +0200, Benno Lossin wrote:
>> >> On Sat May 17, 2025 at 11:54 AM CEST, Danilo Krummrich wrote:
>> >> > On Fri, May 09, 2025 at 12:10:08PM +0200, Benno Lossin wrote:
>> >> >> On Sat May 3, 2025 at 4:53 PM CEST, Marcelo Moreira wrote:
>> >> >> >  /// # Invariants
>> >> >> >  ///
>> >> >> > -/// - The wrapped object `data` is valid if and only if `is_available` is `true`.
>> >> >> > -/// - Access to `data` must occur only while holding the RCU read-side lock (e.g., via
>> >> >> > -///   [`Revocable::try_access`] or [`Revocable::try_access_with_guard`]).
>> >> >> > -/// - Once `is_available` is set to `false`, further access to `data` is disallowed,
>> >> >> > -///   and the object is dropped either after an RCU grace period (in [`revoke`]),
>> >> >> > -///   or immediately (in [`revoke_nosync`]).
>> >> >> > +/// - `data` is valid if and only if `is_available` is true.
>> >> >> > +/// - Access to `data` requires holding the RCU read-side lock.
>> >> >> 
>> >> >> I'm not sure what the correct wording here should be. The current
>> >> >> wording makes the `revoke_internal` function illegal, as it doesn't hold
>> >> >> the read-side lock, but still accesses `data`.
>> >> >> 
>> >> >> Maybe @Danilo can help here, but as I understand it, the value in `data`
>> >> >> is valid for as long as the rcu read-side lock is held *and* if
>> >> >> `is_available` was true at some point while holding the lock.
>> >> >
>> >> > IMHO, the RCU read lock is *not* a requirement, it's (for some methods) the
>> >> > justification for how it is ensured that the `is_available` atomic cannot be
>> >> > altered during the usage of `data`. So, it shouldn't be part of the type
>> >> > invariants.
>> >> 
>> >> But the `is_available` atomic *can* be altered during the usage of
>> >> `data`. And in that case it isn't clear to me how you still allow usage
>> >> without relying on rcu.
>> >
>> > The only real rule is that the data is only valid as long as `is_available` is
>> > true.
>> >
>> > Some functions - the safe ones, i.e. try_access(), try_access_with_guard(),
>> > revoke() - ensure this by using RCU.
>> >
>> > Some other functions though - the unsafe ones, i.e. access() and revoke_nosync()
>> > - leave it to the caller to ensure this, because sometimes the caller (such as
>> > Devres) can give this gurarantee through other circumstances.
>> >
>> > So, the internal lock (RCU or any other kind of lock) isn't really part of the
>> > type invariant, it's just one tool to uphold the type invariant. Another such
>> > tool used by Revocable is `unsafe`, where we just require the caller to uphold
>> > this invariant.
>> 
>> I get what you're trying to say, but the statement above about `data`
>> being valid while `is_available` is `true` is not correct.
>
> Why not? Please show me a case where `is_available` is false, but I can still
> technically access data (without violating a safety requirement).

    let r: Arc<Revocable<i32>> = ...;
    let guard = r.try_access().unwrap(); // nobody else is holding a reference, so this can't fail

    let r2 = r.clone();

    // I know we don't have threads, but I don't want to have to look up
    // how to use the workqueue or something else...
    thread::spawn(move || {
        r2.revoke();
    });

    for _ in 0..10_000_000 {
        // do some non-sleeping work that takes a while
    }

    // now the thread above has executed `self.is_available.swap(false, Ordering::Relaxed)`
    // in `revoke_internal` and is waiting for the `synchronize_rcu` call to return.
    // but we can still access `guard`:

    pr_info!("{}", &*guard);

>> When using
>> RCU, you rely on the other functions like `revoke` to wait for the RCU
>> grace period to end. At the moment nothing prevents them from not doing
>> so, which means some safety documentation is wrong.
>
> So, you're saying the problem is that with only the invariant above, revoke()
> could be implemented without synchronize_rcu()?

Indeed.

> If so, why does not need to be covered by a type invariant? The implementation
> uses a lock to uphold its type invariant, if it doesn't it simply is a bug, no?

I don't follow this sentence. `try_access` is relying on RCU and thus
anyone else accessing the value concurrently needs to respect that.

>> Currently `RevocableGuard` also mentions RCU in its type invariants, is
>> that correct?
>
> Yes, we either access the data through the Guard (which is an RCU guard, but
> could also be any other lock guard) or with unsafe accessors directly.
>
>> >> > For instance, we also have the Revocalbe::access() [1], which is an unsafe
>> >> > direct accessor for `data`. It has the following safety requirement:
>> >> >
>> >> > 	 "The caller must ensure this [`Revocable`] instance hasn't been revoked
>> >> > 	 and won't be revoked as long as the returned `&T` lives."
>> >> >
>> >> > Which is equal to the caller must ensure that `is_available` is true, and won't
>> >> > be altered to false as long as the returned reference lives.
>> >> 
>> >> Sure. We could add that it remains valid while `is_available` is true,
>> >> but when it changes, the data is still valid until the end of the rcu
>> >> grace period?
>> >
>> > Well, yes. But that would be true for every other lock as well. Let's say we'd
>> > protect is_available with a mutex, the data would still be guaranteed to be
>> > valid until the mutex is released. But for the reasons given above I don't think
>> > the "protection method" is part of the invariant, so we should just say:
>> > "remains valid at least as long as `is_available` is true".
>> 
>> No, in the mutex case, the `revoke` function could be implemented like
>> this:
>>     
>>     struct Revocable<T> {
>>         data: Mutex<Option<T>>,
>>     }
>> 
>>     fn revoke(&self) {
>>         self.mutex.lock().take();
>>     }
>> 
>> Now, anyone accessing first has to check if the value still exists and
>> while eg `try_access` is holding a `MutexGuard`, the data won't ever be
>> revoked.
>> 
>> Now this is not a feasible implementation due to its performance &
>> access pattern, but we could use the same design if we were to use a
>> standard read-write lock (which would probably still be worse than RCU).
>> 
>> But since `try_access` is using `RCU` under the hood, anyone revoking
>> **needs** to wait for the grace period to end after setting
>> `is_available` to false. That's just how life is and so we need to
>> document that in a type invariant (otherwise how is `try_access` going
>> to know that what it does is correct?). So if you want `try_access` to
>> be safe, the functions that don't respect the grace period need to
>> mention RCU or somehow prevent users from accessing `try_access`.
>> `revoke_nosync` at the moment does this with the safety requirement
>> "Callers need to ensure that there are no more concurrent users of the
>> revocable object." which is pretty vague to me.
>> 
>> I'd probably be able to help more with the safety comments if you can
>> give me the different use-cases of this function, at the moment nothing
>> calls that function.
>
> While we were talking past each other a bit with this,

Yeah I thought that as well. But I feel like we're getting closer to
mutual understanding :)

> I guess I get your point
> now, i.e. with a Mutex for instance, revoke() couldn't get it wrong, since
> there is no way to "bypass" the mutex, but with RCU, there is a away to bypass
> the lock by just not calling synchronize_rcu().

Exactly.

This difficulty makes a lot of sense, since we don't have a proper RCU
primitive yet (which has several reasons, one of them being that RCU
just has so many different applications). I guess `Revocable` is one
such use-case for RCU (but also should support circumventing it when
otherwise deemed correct).

> So, you want a type invariant that guarantees the call to synchronize_rcu() to
> justify the try_access() variants, correct?

I want a type invariant that allows `try_access` to give a valid pointer
to a valid `T` to the guard. (By the way, why doesn't the guard store a
`&T` instead of a raw pointer?)

> However, this invariant does not need to be fulfilled for access() and

Where is `access()` defined?

> revoke_nosync(), because it would circumvent their purpose, i.e. cases where an
> abstraction can prove that there can't be a concurrent user of the data or a
> concurrent user revoking the data respectively.

Yes. How about something like "`data` is valid while `is_available` is
true. It also is valid if the RCU read-side lock is being held and it
was taken while `is_available` was true."?

That should also cover the "nobody else is accessing this" case.

> An example of revoke_nosync() is the original Devres implementation [1].
> However, this was re-worked with [2] to use a different logic that doesn't need
> revoke_nosync() anymore.
>
> Actually, "doesn't need revoke_nosync()" isn't exactly true. We would still
> benefit from revoke_nosync(), but the implementation in [2] triggers the devres
> C callback from drop() and the devres C callback calls revoke().
>
> If we'd had a way to know that the devres C callback has been triggered due
> drop(), we could use revoke_nosync() in this case as an optimization.

Yeah that sounds like a plausible option. Given that, I think the
following kind of function could be useful on `Revocable`: a safe
`revoke_` function that takes `&mut self` and thus doesn't need to use
RCU (since we have a unique mutable reference, only we have access).

Do you have any other uses of `revoke_nosync` that do not have
(potential) access to `&mut Revocable`?

---
Cheers,
Benno

>
> [1] commit 76c01ded724b ("rust: add devres abstraction")
> [2] commit 8ff656643d30 ("rust: devres: remove action in `Devres::drop`")