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

[Marcelo Moreira <marcelomoreira1905@gmail.com>]

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?

-- 
Cheers,
Marcelo Moreira