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

[Marcelo Moreira <marcelomoreira1905@gmail.com>]

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`.

--
Cheers,
Marcelo Moreira