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

> 2. Or are you suggesting that the use of bullet points within the
> `SAFETY` is not preferred here?

No that is always preferred.

>> > +            Some(unsafe { RevocableGuard::new(self.data.get(), guard) })
>> >          } else {
>> >              None
>> >          }
>> > @@ -233,7 +236,7 @@ fn drop(self: Pin<&mut Self>) {
>> >  ///
>> >  /// # Invariants
>> >  ///
>> > -/// The RCU read-side lock is held while the guard is alive.
>> > +/// - `data_ref` is a valid pointer for as long as the RCU read-side lock is held.
>> >  pub struct RevocableGuard<'a, T> {
>> >      // This can't use the `&'a T` type because references that appear in function arguments must
>> >      // not become dangling during the execution of the function, which can happen if the
>> > @@ -245,7 +248,15 @@ pub struct RevocableGuard<'a, T> {
>> >  }
>> >
>> >  impl<T> RevocableGuard<'_, T> {
>> > -    fn new(data_ref: *const T, rcu_guard: rcu::Guard) -> Self {
>> > +    /// Creates a new `RevocableGuard`.
>> > +    ///
>> > +    /// # Safety
>> > +    ///
>> > +    /// Callers must ensure that `data_ref` is a valid pointer to a `T` object,
>> > +    /// and that it remains valid for as long as the returned `RevocableGuard` is alive.
>> > +    /// The RCU read-side lock must be held for the duration of the guard's lifetime,
>> > +    /// as indicated by `rcu_guard`.
>>
>> This last part shouldn't be needed, as the `rcu::Guard` already
>> guarantees it.
>
> Ok, I can remove just that part and keep the rest.

Thanks!

---
Cheers,
Benno