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

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

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.

> 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?

> One valid way for the caller would be to wrap it into an RCU read side critical
> section and check `is_available`. However, depending on the context, there are
> also other justifications, e.g. [2].

For the justification in [2], you need a type invariant.

---
Cheers,
Benno

> [1] https://gitlab.freedesktop.org/drm/nova/-/blob/nova-next/rust/kernel/revocable.rs?ref_type=heads#L148
> [2] https://gitlab.freedesktop.org/drm/nova/-/blob/nova-next/rust/kernel/devres.rs?ref_type=heads#L221
>
>> > +/// - 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]).
>
> Same here, RCU isn't a relevant factor for the type invariant IMHO. It's just
> how part of the implementation guarantees to up-hold the invariant.