[PATCH v6 0/3] rust: revocable: Documentation and refactorings for Revocable type

[PATCH v6 0/3] rust: revocable: Documentation and refactorings for Revocable type

[Marcelo Moreira]

This patch series brings documentation and refactorings to the `Revocable` type, addressing recent feedback and improving clarity.

Changes include:

- Clarifying the write invariant and updating associated safety comments for `Revocable<T>`.
- Splitting the internal `revoke_internal` function into two distinct,
  explicit functions: `revoke()` (safe, synchronizing with RCU) and
  `revoke_nosync()` (unsafe, without RCU synchronization), now returning
  `bool` to indicate revocation status.
- Documenting `RevocableGuard`'s pointer validity invariant and refining
  its `Deref` safety comment.

---
Changelog:

Changes since v5:
- Reordered the patch series to apply documentation fixes before the
  refactoring, as suggested by Benno. The new order is:
  1. `rust: revocable: Clarify write invariant and update safety comments`
  2. `rust: revocable: Refactor revocation mechanism to remove generic revoke_internal`
  3. `rust: revocable: Document RevocableGuard invariants and refine Deref safety`
- Added a new patch, "rust: revocable: Document RevocableGuard invariants
  and refine Deref safety", which explicitly documents the validity
  invariant for `RevocableGuard`'s `data_ref` member and refines the
  associated `Deref` `SAFETY` comment, addressing specific maintainer feedback.
- Updated the `SAFETY` comment in `Deref` implementation of `RevocableGuard`
  to match common kernel patterns.

Link to v5: https://lore.kernel.org/rust-for-linux/DB3XFMG7M4SO.J6A2LVOAOJDX@kernel.org/T/#t

Changes since v4:
- Rebased the series onto the latest `rfl/rust-next` to integrate recent
  changes, specifically the `bool` return for `revoke()` and
  `revoke_nosync()`.
- Dropped the "rust: revocable: simplify RevocableGuard for internal
  safety" patch, as the approach of using a direct reference (`&'a T`)
  for `RevocableGuard` was found to be unsound due to Rust's aliasing
  rules and LLVM's `dereferencable` attribute guarantees, which require
  references to remain valid for the entire function call duration, even
  if the internal RCU guard is dropped earlier.
- Refined the `PinnedDrop::drop` `SAFETY` comment based on Benno's and
  Miguel's feedback, adopting a more concise and standard Kernel-style
  bullet point format.

Link to v4: https://lore.kernel.org/rust-for-linux/DAOMIWBZXFO9.U353H8NWTLC5@kernel.org/T/#u

Changes since v3:
- Refined the wording of the `Revocable<T>` invariants to be more
  precise about read and write validity conditions, specifically including
  RCU read-side lock acquisition timing for reads and RCU grace period
  for writes.
- Simplified the `try_access_with_guard` safety comment for better conciseness.
- Refactored `RevocableGuard` to use `&'a T` instead of `*const T`, removing its internal invariants and `unsafe` blocks.
- Simplified `Revocable::try_access` to leverage `try_access_with_guard` and `map`.
- Split `revoke_internal` into `revoke()` and `revoke_nosync()` functions, making synchronization behavior explicit.

Link to v3: https://lore.kernel.org/rust-for-linux/CAPZ3m_hTr7BN=zy10m8kWchYiJ04MXKuJAp9wt67Krqw6wH-JQ@mail.gmail.com/

Changes in v2:
- Refined the wording of the invariants in `Revocable<T>` to be
  more direct and address feedback regarding the phrase 'must occur'.
- Added '// INVARIANT:' comments in `try_access` and `try_access_with_guard` as suggested by reviewers.
- Added the missing invariant for `RevocableGuard<'_, T>` regarding the validity of `data_ref`.
- Updated the safety comment in the `Deref` implementation of `RevocableGuard` to refer to the new invariant.

Link to v2: https://lore.kernel.org/rust-for-linux/CAPZ3m_jw0LxK1MmseaamNYhj9VY8AXtJ0AOcYd9qcn=5wPE4eA@mail.gmail.com/T/#t

Marcelo Moreira (3):
  rust: revocable: Clarify write invariant and update safety comments
  rust: revocable: Refactor revocation mechanism to remove generic
    revoke_internal
  rust: revocable: Document RevocableGuard invariants and refine Deref
    safety

 rust/kernel/revocable.rs | 75 +++++++++++++++++++++-------------------
 1 file changed, 40 insertions(+), 35 deletions(-)

-- 
2.50.0

[PATCH v6 1/3] rust: revocable: Clarify write invariant and update safety comments

[Marcelo Moreira]

Clarifies the write invariant of the `Revocabl` type and
updates associated `SAFETY` comments. The write invariant now precisely
states that `data` is valid for writes after `is_available` transitions
from true to false, provided no thread holding an RCU read-side lock
(acquired before the change) still has access to `data`.

The `SAFETY` comment in `try_access_with_guard` is updated to reflect
this invariant, and the `PinnedDrop` `drop` implementation's `SAFETY`
comment is refined to clearly state the guarantees provided by the `&mut Self`
context regarding exclusive access and `data`'s validity for dropping.

Reported-by: Benno Lossin <lossin@kernel.org>
Closes: https://github.com/Rust-for-Linux/linux/issues/1160
Suggested-by: Benno Lossin <lossin@kernel.org>
Suggested-by: Danilo Krummrich <dakr@kernel.org>
Reviewed-by: Benno Lossin <lossin@kernel.org>
Reviewed-by: Danilo Krummrich <dakr@kernel.org>
Signed-off-by: Marcelo Moreira <marcelomoreira1905@gmail.com>
---
 rust/kernel/revocable.rs | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/rust/kernel/revocable.rs b/rust/kernel/revocable.rs
index 1cd4511f0260..2dfee25240a0 100644
--- a/rust/kernel/revocable.rs
+++ b/rust/kernel/revocable.rs
@@ -61,6 +61,15 @@
 /// v.revoke();
 /// assert_eq!(add_two(&v), None);
 /// ```
+///
+/// # Invariants
+///
+/// - `data` is valid for reads in two cases:
+///   - while `is_available` is true, or
+///   - while the RCU read-side lock is taken and it was acquired while `is_available` was `true`.
+/// - `data` is valid for writes when `is_available` was atomically changed from `true` to `false`
+///   and no thread that has access to `data` is holding an RCU read-side lock that was acquired
+///   prior to the change in `is_available`.
 #[pin_data(PinnedDrop)]
 pub struct Revocable<T> {
     is_available: AtomicBool,
@@ -115,8 +124,8 @@ pub fn try_access(&self) -> Option<RevocableGuard<'_, T>> {
     /// object.
     pub fn try_access_with_guard<'a>(&'a self, _guard: &'a rcu::Guard) -> Option<&'a T> {
         if self.is_available.load(Ordering::Relaxed) {
-            // SAFETY: 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.
+            // SAFETY: `self.data` is valid for reads because of `Self`'s type invariants,
+            // as `self.is_available` is true and `_guard` holds the RCU read-side lock.
             Some(unsafe { &*self.data.get() })
         } else {
             None
@@ -214,9 +223,10 @@ fn drop(self: Pin<&mut Self>) {
         // SAFETY: We are not moving out of `p`, only dropping in place
         let p = unsafe { self.get_unchecked_mut() };
         if *p.is_available.get_mut() {
-            // SAFETY: We know `self.data` is valid because no other CPU has changed
-            // `is_available` to `false` yet, and no other CPU can do it anymore because this CPU
-            // holds the only reference (mutable) to `self` now.
+            // SAFETY:
+            // - `self.data` is valid for writes because of `Self`'s type invariants:
+            //   `&mut Self` guarantees exclusive access, so no other thread can concurrently access `data`.
+            // - this function is a drop function, thus this code is at most executed once.
             unsafe { drop_in_place(p.data.get()) };
         }
     }
-- 
2.50.0

[PATCH v6 2/3] rust: revocable: Refactor revocation mechanism to remove generic revoke_internal

[Marcelo Moreira]

The revocation mechanism is refactored by removing the generic
`revoke_internal` function. Its logic is now directly integrated into
two distinct public functions: `revoke()` and `revoke_nosync()`.

`revoke_nosync()` is an `unsafe` function that requires the caller to
guarantee no concurrent users, thus avoiding an RCU grace period.
`revoke()` is a safe function that internally waits for the RCU grace
period to ensure all concurrent accesses have completed before dropping
the wrapped object.

This change improves API clarity and simplifies associated `SAFETY`
comments by making the synchronization behavior explicit in the function
signatures.

Suggested-by: Benno Lossin <lossin@kernel.org>
Suggested-by: Danilo Krummrich <dakr@kernel.org>
Reviewed-by: Benno Lossin <lossin@kernel.org>
Signed-off-by: Marcelo Moreira <marcelomoreira1905@gmail.com>
---
 rust/kernel/revocable.rs | 48 ++++++++++++++++++----------------------
 1 file changed, 21 insertions(+), 27 deletions(-)

diff --git a/rust/kernel/revocable.rs b/rust/kernel/revocable.rs
index 2dfee25240a0..6d8e9237dbdf 100644
--- a/rust/kernel/revocable.rs
+++ b/rust/kernel/revocable.rs
@@ -160,26 +160,6 @@ pub unsafe fn access(&self) -> &T {
         unsafe { &*self.data.get() }
     }
 
-    /// # Safety
-    ///
-    /// Callers must ensure that there are no more concurrent users of the revocable object.
-    unsafe fn revoke_internal<const SYNC: bool>(&self) -> bool {
-        let revoke = self.is_available.swap(false, Ordering::Relaxed);
-
-        if revoke {
-            if SYNC {
-                // SAFETY: Just an FFI call, there are no further requirements.
-                unsafe { bindings::synchronize_rcu() };
-            }
-
-            // SAFETY: We know `self.data` is valid because only one CPU can succeed the
-            // `compare_exchange` above that takes `is_available` from `true` to `false`.
-            unsafe { drop_in_place(self.data.get()) };
-        }
-
-        revoke
-    }
-
     /// Revokes access to and drops the wrapped object.
     ///
     /// Access to the object is revoked immediately to new callers of [`Revocable::try_access`],
@@ -192,10 +172,15 @@ unsafe fn revoke_internal<const SYNC: bool>(&self) -> bool {
     ///
     /// Callers must ensure that there are no more concurrent users of the revocable object.
     pub unsafe fn revoke_nosync(&self) -> bool {
-        // SAFETY: By the safety requirement of this function, the caller ensures that nobody is
-        // accessing the data anymore and hence we don't have to wait for the grace period to
-        // finish.
-        unsafe { self.revoke_internal::<false>() }
+        let revoke = self.is_available.swap(false, Ordering::Relaxed);
+
+        if revoke {
+            // SAFETY: `self.data` is valid for writes because of `Self`'s type invariants,
+            // as `self.is_available` is false due to the atomic swap, and by the safety
+            // requirements of this function, no thread is accessing `data` anymore.
+            unsafe { drop_in_place(self.data.get()) };
+        }
+        revoke
     }
 
     /// Revokes access to and drops the wrapped object.
@@ -209,9 +194,18 @@ pub unsafe fn revoke_nosync(&self) -> bool {
     /// Returns `true` if `&self` has been revoked with this call, `false` if it was revoked
     /// already.
     pub fn revoke(&self) -> bool {
-        // SAFETY: By passing `true` we ask `revoke_internal` to wait for the grace period to
-        // finish.
-        unsafe { self.revoke_internal::<true>() }
+        let revoke = self.is_available.swap(false, Ordering::Relaxed);
+
+        if revoke {
+            // SAFETY: Just an FFI call, there are no further requirements.
+            unsafe { bindings::synchronize_rcu() };
+
+            // SAFETY: `self.data` is valid for writes because of `Self`'s type invariants,
+            // as `self.is_available` is false due to the atomic swap, and `synchronize_rcu`
+            // ensures all prior RCU read-side critical sections have completed.
+            unsafe { drop_in_place(self.data.get()) };
+        }
+        revoke
     }
 }
 
-- 
2.50.0

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

[Marcelo Moreira]

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.
+/// - The RCU read-side lock is held while the guard is alive.
 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.
         unsafe { &*self.data_ref }
     }
 }
-- 
2.50.0

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

[Benno Lossin]

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

---
Cheers,
Benno

>          unsafe { &*self.data_ref }
>      }
>  }

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

[Marcelo Moreira]

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.

Ok, I will keep only this comment. Thanks!


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

oh, of course, &T is an immutable reference, mb.

What about:
// SAFETY: `self.data_ref` is valid because of `Self`'s type invariants
// and the active RCU read-side lock held via `_rcu_guard`, ensuring
the data's accessibility.

-- 
Cheers,
Marcelo Moreira

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

[Benno Lossin]

On Wed Jul 9, 2025 at 12:24 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:
>> >  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.
>
> oh, of course, &T is an immutable reference, mb.
>
> What about:
> // SAFETY: `self.data_ref` is valid because of `Self`'s type invariants
> // and the active RCU read-side lock held via `_rcu_guard`, ensuring
> the data's accessibility.

Sounds good.

---
Cheers,
Benno

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

[Marcelo Moreira]

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

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

--
Cheers,
Marcelo Moreira

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

[Benno Lossin]

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

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

[Marcelo Moreira]

Em sáb., 12 de jul. de 2025 às 05:26, Benno Lossin <lossin@kernel.org> escreveu:
>
> 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.

Even if it's only called once, I think it's better to make `new`
unsafe because it can be used in the future, and keeping it unsafe
gives us the advantage of maintainability.

Understood, Benno. I'll change `new` to be unsafe, change
`RevocableGuard::new` to an unsafe block, and adjust the `SAFEFY`
comment.
ok?

-- 
Cheers,
Marcelo Moreira

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

[Benno Lossin]

On Sat Jul 12, 2025 at 8:49 PM CEST, Marcelo Moreira wrote:
> Em sáb., 12 de jul. de 2025 às 05:26, Benno Lossin <lossin@kernel.org> escreveu:
>>
>> 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.
>
> Even if it's only called once, I think it's better to make `new`
> unsafe because it can be used in the future, and keeping it unsafe
> gives us the advantage of maintainability.

Eh I'm not so sure that there is the need to construct it in more places
than one. And if we ever need it, we could add it back in. But we can go
either way. Maybe Danilo has a preference?

> Understood, Benno. I'll change `new` to be unsafe, change
> `RevocableGuard::new` to an unsafe block, and adjust the `SAFEFY`
> comment.
> ok?

Sounds good, you also need to add an invariant comment inside of
`RevocableGuard::new`.

---
Cheers,
Benno

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

[Marcelo Moreira]

Em sáb., 12 de jul. de 2025 às 16:56, Benno Lossin <lossin@kernel.org> escreveu:
>
> On Sat Jul 12, 2025 at 8:49 PM CEST, Marcelo Moreira wrote:
> > Em sáb., 12 de jul. de 2025 às 05:26, Benno Lossin <lossin@kernel.org> escreveu:
> >>
> >> 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.
> >
> > Even if it's only called once, I think it's better to make `new`
> > unsafe because it can be used in the future, and keeping it unsafe
> > gives us the advantage of maintainability.
>
> Eh I'm not so sure that there is the need to construct it in more places
> than one. And if we ever need it, we could add it back in. But we can go
> either way. Maybe Danilo has a preference?
>
> > Understood, Benno. I'll change `new` to be unsafe, change
> > `RevocableGuard::new` to an unsafe block, and adjust the `SAFEFY`
> > comment.
> > ok?
>
> Sounds good, you also need to add an invariant comment inside of
> `RevocableGuard::new`.

Ok, Thanks Benno! :)

-- 
Cheers,
Marcelo Moreira