[PATCH v7 0/3] rust: revocable: Documentation and safety refinements

[PATCH v7 0/3] rust: revocable: Documentation and safety refinements

[Marcelo Moreira]

This patch series brings documentation and safety refinements to the
`Revocable` type, addressing recent feedback and improving clarity of
`unsafe` operations.

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 invariants, making its
  constructor `unsafe`, and refining its `Deref` and `try_access` safety comments.

---
Changelog:

Changes since v6:
- Patch 3 (`rust: revocable: Document RevocableGuard invariants/safety and refine Deref safety`) was significantly refined:
  - `RevocableGuard`'s invariants were updated to precisely state that `data_ref` is valid as long as the RCU read-side lock is held,
    and the redundant RCU invariant was removed.
  - The `RevocableGuard::new` constructor was made `unsafe`, and a detailed `# Safety` comment was added specifying caller responsibilities.
  - The comment in `Revocable::try_access` was changed to a `SAFETY` block, justifying the `unsafe` call to `RevocableGuard::new`
    by detailing `Self`'s type invariants (`is_available` being true) and the active RCU read-side lock.
  - The `Deref` implementation's `SAFETY` comment was refined.

Link to v6: https://lore.kernel.org/rust-for-linux/20250708003428.76783-1-marcelomoreira1905@gmail.com/T/#t

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 `Revocable<T>` invariants 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/safety and refine Deref
    safety

rust/kernel/revocable.rs | 93 ++++++++++++++++++++++++++++++++++++++++++++++++++++++---------------------------------------
1 file changed, 54 insertions(+), 39 deletions(-)

[PATCH v7 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.1

[PATCH v7 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.1

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

[Marcelo Moreira]

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.
+            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`.
+    unsafe fn new(data_ref: *const T, rcu_guard: rcu::Guard) -> Self {
         Self {
             data_ref,
             _rcu_guard: rcu_guard,
@@ -258,8 +269,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 because of `Self`'s type invariants,
+        // and the active RCU read-side lock held via `_rcu_guard`, ensuring the data's accessibility.
         unsafe { &*self.data_ref }
     }
 }
-- 
2.50.1

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

[Benno Lossin]

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.

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

---
Cheers,
Benno

> +    unsafe fn new(data_ref: *const T, rcu_guard: rcu::Guard) -> Self {
>          Self {
>              data_ref,
>              _rcu_guard: rcu_guard,
> @@ -258,8 +269,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 because of `Self`'s type invariants,
> +        // and the active RCU read-side lock held via `_rcu_guard`, ensuring the data's accessibility.
>          unsafe { &*self.data_ref }
>      }
>  }

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

[Marcelo Moreira]

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?
2. Or are you suggesting that the use of bullet points within the
`SAFETY` is not preferred here?


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

-- 
Cheers,
Marcelo Moreira

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

[Benno Lossin]

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

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

[Marcelo Moreira]

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

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

[Benno Lossin]

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

---
Cheers,
Benno

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

[Marcelo Moreira]

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

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

[Benno Lossin]

On Thu Jul 24, 2025 at 1:49 AM CEST, Marcelo Moreira wrote:
> 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`.

Sounds good!

---
Cheers,
Benno