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

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

On Sat Jul 12, 2025 at 1:06 AM CEST, Marcelo Moreira wrote:
> Em ter., 8 de jul. de 2025 às 12:04, Benno Lossin <lossin@kernel.org> escreveu:
>> On Tue Jul 8, 2025 at 2:33 AM CEST, Marcelo Moreira wrote:
>> > Improves the `RevocableGuard` documentation by explicitly stating that
>> > its `data_ref` member is a valid pointer as an invariant. Additionally,
>> > the `Deref` implementation's `SAFETY` comment is refined to justify
>> > the `unsafe` dereference based on this new invariant and the
>> > `_rcu_guard` ensuring data accessibility.
>> >
>> > These changes address feedback regarding the clarity and completeness
>> > of `RevocableGuard`'s safety guarantees.
>> >
>> > Signed-off-by: Marcelo Moreira <marcelomoreira1905@gmail.com>
>> > ---
>> >  rust/kernel/revocable.rs | 7 ++++---
>> >  1 file changed, 4 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/rust/kernel/revocable.rs b/rust/kernel/revocable.rs
>> > index 6d8e9237dbdf..64fe44b4f5a3 100644
>> > --- a/rust/kernel/revocable.rs
>> > +++ b/rust/kernel/revocable.rs
>> > @@ -233,7 +233,8 @@ 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 to a `T` object for the entire lifetime of this guard.
>>
>> This should be:
>>
>>     /// - `data_ref` is a valid pointer for as long as the RCU read-side lock is held.
>>
>> > +/// - The RCU read-side lock is held while the guard is alive.
>>
>> And this invariant is unnecessary.
>>
>> >  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
>> > @@ -258,8 +259,8 @@ impl<T> Deref for RevocableGuard<'_, T> {
>> >      type Target = T;
>> >
>> >      fn deref(&self) -> &Self::Target {
>> > -        // SAFETY: By the type invariants, we hold the rcu read-side lock, so the object is
>> > -        // guaranteed to remain valid.
>> > +        // SAFETY: `self.data_ref` is valid for writes because of `Self`'s type invariants,
>> > +        // and `_rcu_guard` ensures the data's accessibility for the lifetime of this guard.
>>
>> This also needs to be adjusted.
>>
>> Also this patch should fix any invariant comments needed to construct
>> `RevocableGuard`.
>
> Regarding this point, "...fix any invariant comments needed to
> construct `RevocableGuard`."
>
> I looked at the function that constructs `RevocableGuard`
> (try_access->RevocableGuard::new) and I think it's correct.
>
> In `try_access`, the comment justifies the validity of self.data.get()
> by mentioning that `self.is_available` is true and that the RCU read
> lock prevents data from being dropped. This aligns well with what we
> already have in try_access_with_guard, for example (from patch 1/3).

The comment seems wrong to me, it isn't even an `INVARIANT` comment. It
also talks about RCU in the wrong way and doesn't mention the type
invariants of `Self`...

> Since there are no other code snippets that construct
> `RevocableGuard`, I thought I'd submit the patch with just the
> adjustments I made previously.

There also is the `new` function of `RevocableGuard` that doesn't use an
invariant comment. It also isn't unsafe despite taking a raw pointer...
So either we make it unsafe, add the correct safety requirements which
should just be the type invariants of the guard or we remove it, since
we only use it once.

---
Cheers,
Benno