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

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

On Thu Jul 24, 2025 at 1:49 AM CEST, Marcelo Moreira wrote:
> Em qua., 23 de jul. de 2025 às 11:22, Benno Lossin <lossin@kernel.org> escreveu:
>>
>> 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`.
>
> good! but how about we simplify it:
> // SAFETY: `self.data` is valid for reads because of `Self`'s type invariants:
> // `self.is_available` is true and the RCU read-side lock is held by `guard`.

Sounds good!

---
Cheers,
Benno