Re: [PATCH v7 3/3] rust: revocable: Document RevocableGuard invariants/safety and refine Deref safety

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

On Tue Jul 22, 2025 at 11:23 PM CEST, Marcelo Moreira wrote:
> Em ter., 22 de jul. de 2025 às 07:51, Benno Lossin <lossin@kernel.org> escreveu:
>> On Tue Jul 22, 2025 at 1:01 AM CEST, Marcelo Moreira wrote:
>> > Em seg., 21 de jul. de 2025 às 11:21, Benno Lossin <lossin@kernel.org> escreveu:
>> >> On Mon Jul 21, 2025 at 3:01 AM CEST, Marcelo Moreira wrote:
>> >> > Refinements include:
>> >> > - `RevocableGuard`'s invariants are updated to precisely state that
>> >> >   `data_ref` is valid as long as the RCU read-side lock is held.
>> >> > - The `RevocableGuard::new` constructor is made `unsafe`, explicitly
>> >> >   requiring callers to guarantee the validity of the raw pointer and
>> >> >   RCU read-side lock lifetime.
>> >> > - A new `SAFETY` comment is added to `Revocable::try_access` to
>> >> >   justify the `unsafe` call to `RevocableGuard::new`, detailing how
>> >> >   `Self`'s type invariants and the active RCU read-side lock ensure data
>> >> >   validity for reads.
>> >> > - The `Deref` implementation's `SAFETY` comment for `RevocableGuard`
>> >> >   is refined.
>> >> >
>> >> > Signed-off-by: Marcelo Moreira <marcelomoreira1905@gmail.com>
>> >> > ---
>> >> >  rust/kernel/revocable.rs | 25 ++++++++++++++++++-------
>> >> >  1 file changed, 18 insertions(+), 7 deletions(-)
>> >> >
>> >> > diff --git a/rust/kernel/revocable.rs b/rust/kernel/revocable.rs
>> >> > index 6d8e9237dbdf..0048de23ab44 100644
>> >> > --- a/rust/kernel/revocable.rs
>> >> > +++ b/rust/kernel/revocable.rs
>> >> > @@ -106,9 +106,12 @@ pub fn new(data: impl PinInit<T>) -> impl PinInit<Self> {
>> >> >      pub fn try_access(&self) -> Option<RevocableGuard<'_, T>> {
>> >> >          let guard = rcu::read_lock();
>> >> >          if self.is_available.load(Ordering::Relaxed) {
>> >> > -            // Since `self.is_available` is true, data is initialised and has to remain valid
>> >> > -            // because the RCU read side lock prevents it from being dropped.
>> >> > -            Some(RevocableGuard::new(self.data.get(), guard))
>> >> > +            // SAFETY:
>> >> > +            // - `self.data` is valid for reads because of `Self`'s type invariants:
>> >> > +            //   `self.is_available` is true.
>> >> > +            // - The RCU read-side lock is active via `guard`, preventing `self.data`
>> >> > +            //   from being dropped and ensuring its validity for the guard's lifetime.
>> >>
>> >> This shouldn't be needed.
>> >
>> > hmm, about what exactly?
>> >
>> > Are you suggesting to:
>> > 1. Simplify the content of the `SAFETY`, is it too verbose?
>>
>> It's not too verbose. The requirement of holding the RCU read-side lock
>> is not a *safety requirement*. It's already guaranteed by the existence
>> of the `rcu::Guard` instance, so we don't need to concern ourselves with
>> it in the safety requirements.
>>
>> Essentially you're just stating a tautology in the safety comment like
>> saying "2 + 2 = 4".
>
> Thanks for showing me that Benno.
> So we can keep it like this:
> // SAFETY: `self.data` is valid for reads because of `Self`'s type invariants:
> // `self.is_available` is true.
>
> Sounds good?

Ah sorry, I didn't take a good look at the non-RCU justification. I
liked the phrasing in the non-safety comment:

    // Since `self.is_available` is true, data is initialised and has to remain valid
    // because the RCU read side lock prevents it from being dropped.

So how about we combine that with your current version:

    // SAFETY: `self.data` is valid for reads for as long as the RCU read-side lock is held because of
    // `Self`'s type invariants: `self.is_available` is true and the RCU read-side lock is held by
    // `guard`.

---
Cheers,
Benno