[PATCH v4 0/3] rust: revocable: documentation and refactorings

[PATCH v4 0/3] rust: revocable: documentation and refactorings

[Marcelo Moreira]

This series of patches brings documentation and refactorings to the `Revocable` type.

The main changes include:
- Clarifying the write invariant and updating associated safety comments for `Revocable<T>`.
- Refactoring `RevocableGuard` to be internally safe by holding a direct reference (`&'a T`), simplifying its usage and reducing unsafe code.
- Splitting the internal `revoke_internal` function into two distinct, explicit functions: `revoke()` (safe, synchronizing with RCU) and `revoke_nosync()` (unsafe, without RCU synchronization).

This is v4 of the patch series.

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/DA6XFLKUTP1P.RR6P5AK9PKIZ@kernel.org/T/#t

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

Signed-off-by: Marcelo Moreira <marcelomoreira1905@gmail.com>

---

Marcelo Moreira (3):
  rust: revocable: update write invariant and fix safety comments
  rust: revocable: simplify RevocableGuard for internal safety
  rust: revocable: split revoke_internal into revoke and revoke_nosync

 rust/kernel/revocable.rs | 84 +++++++++++++++++-----------------------
 1 file changed, 36 insertions(+), 48 deletions(-)

--- base-commit: 7a17bbc1d952057898cb0739e60665908fbb8c72

Cheers,
Marcelo Moreira
-- 
2.49.0

[PATCH v4 1/3] rust: revocable: update write invariant and fix safety comments

[Marcelo Moreira]

This commit clarifies the write invariant of the `Revocable` 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>
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 1e5a9d25c21b..d14f9052f1ac 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
@@ -176,9 +185,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,
+            // and because this `PinnedDrop` context (having `&mut Self`) guarantees exclusive access,
+            // ensuring no other thread can concurrently access or revoke `data`.
+            // This ensures `data` is valid for `drop_in_place`.
             unsafe { drop_in_place(p.data.get()) };
         }
     }
-- 
2.49.0

[PATCH v4 2/3] rust: revocable: simplify RevocableGuard for internal safety

[Marcelo Moreira]

This commit refactors `RevocableGuard` to hold a direct reference
(`&'a T`) instead of a raw pointer (`*const T`). This makes the guard
internally safe, reducing the need for `unsafe` blocks in its usage
and simplifying its implementation.

The `try_access` function is updated to leverage `try_access_with_guard`
and `map` to construct the `RevocableGuard` in a more idiomatic and safe
Rust way, avoiding manual pointer operations. The associated invariants
and `SAFETY` comments for `RevocableGuard` itself are removed as its
safety is now guaranteed by its type definition.

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

diff --git a/rust/kernel/revocable.rs b/rust/kernel/revocable.rs
index d14f9052f1ac..43cc9bdc94f4 100644
--- a/rust/kernel/revocable.rs
+++ b/rust/kernel/revocable.rs
@@ -105,13 +105,7 @@ pub fn new(data: impl PinInit<T>) -> impl PinInit<Self> {
     /// because another CPU may be waiting to complete the revocation of this object.
     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))
-        } else {
-            None
-        }
+        self.try_access_with_guard(&guard).map(|data| RevocableGuard::new(data, guard))
     }
 
     /// Tries to access the revocable wrapped object.
@@ -198,22 +192,16 @@ fn drop(self: Pin<&mut Self>) {
 ///
 /// CPUs may not sleep while holding on to [`RevocableGuard`] because it's in atomic context
 /// holding the RCU read-side lock.
-///
-/// # Invariants
-///
-/// The RCU read-side lock is held while the guard is alive.
 pub struct RevocableGuard<'a, T> {
-    data_ref: *const T,
+    data: &'a T,
     _rcu_guard: rcu::Guard,
-    _p: PhantomData<&'a ()>,
 }
 
-impl<T> RevocableGuard<'_, T> {
-    fn new(data_ref: *const T, rcu_guard: rcu::Guard) -> Self {
+impl<'a, T> RevocableGuard<'a, T> {
+    fn new(data: &'a T, rcu_guard: rcu::Guard) -> Self {
         Self {
-            data_ref,
+            data,
             _rcu_guard: rcu_guard,
-            _p: PhantomData,
         }
     }
 }
@@ -222,8 +210,6 @@ 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.
-        unsafe { &*self.data_ref }
+        self.data
     }
 }
-- 
2.49.0

[PATCH v4 3/3] rust: revocable: split revoke_internal into revoke and revoke_nosync

[Marcelo Moreira]

This commit refactors the revocation mechanism 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>
Signed-off-by: Marcelo Moreira <marcelomoreira1905@gmail.com>
---
 rust/kernel/revocable.rs | 38 +++++++++++++++-----------------------
 1 file changed, 15 insertions(+), 23 deletions(-)

diff --git a/rust/kernel/revocable.rs b/rust/kernel/revocable.rs
index 43cc9bdc94f4..daf22e3a7d20 100644
--- a/rust/kernel/revocable.rs
+++ b/rust/kernel/revocable.rs
@@ -126,22 +126,6 @@ pub fn try_access_with_guard<'a>(&'a self, _guard: &'a rcu::Guard) -> Option<&'a
         }
     }
 
-    /// # Safety
-    ///
-    /// Callers must ensure that there are no more concurrent users of the revocable object.
-    unsafe fn revoke_internal<const SYNC: bool>(&self) {
-        if self.is_available.swap(false, Ordering::Relaxed) {
-            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()) };
-        }
-    }
-
     /// Revokes access to and drops the wrapped object.
     ///
     /// Access to the object is revoked immediately to new callers of [`Revocable::try_access`],
@@ -151,10 +135,12 @@ unsafe fn revoke_internal<const SYNC: bool>(&self) {
     ///
     /// Callers must ensure that there are no more concurrent users of the revocable object.
     pub unsafe fn revoke_nosync(&self) {
-        // 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>() }
+        if self.is_available.swap(false, Ordering::Relaxed) {
+            // 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()) };
+        }
     }
 
     /// Revokes access to and drops the wrapped object.
@@ -165,9 +151,15 @@ pub unsafe fn revoke_nosync(&self) {
     /// [`Revocable::try_access`] beforehand and still haven't dropped the returned guard), this
     /// function waits for the concurrent access to complete before dropping the wrapped object.
     pub fn revoke(&self) {
-        // SAFETY: By passing `true` we ask `revoke_internal` to wait for the grace period to
-        // finish.
-        unsafe { self.revoke_internal::<true>() }
+        if self.is_available.swap(false, Ordering::Relaxed) {
+            // 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()) };
+        }
     }
 }
 
-- 
2.49.0

Re: [PATCH v4 1/3] rust: revocable: update write invariant and fix safety comments

[Benno Lossin]

On Tue Jun 3, 2025 at 1:26 AM CEST, Marcelo Moreira wrote:
> This commit clarifies the write invariant of the `Revocable` 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>
> Signed-off-by: Marcelo Moreira <marcelomoreira1905@gmail.com>

I would have done this change after the other two, since then you can
change all the safety comments here, but if Danilo is fine with doing it
this way, then you can leave it this way.

The changes are almost perfect now, just some small formatting changes
below. With those fixed, you may add:

Reviewed-by: Benno Lossin <lossin@kernel.org>

> ---
>  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 1e5a9d25c21b..d14f9052f1ac 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,

s/Self::data/self.data/

> +            // as `Self::is_available` is true and `_guard` holds the RCU read-side lock

s/Self::is_available/self.is_available/

Missing `.` at the end.

>              Some(unsafe { &*self.data.get() })
>          } else {
>              None
> @@ -176,9 +185,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,

s/Self::data/self.data/

> +            // and because this `PinnedDrop` context (having `&mut Self`) guarantees exclusive access,
> +            // ensuring no other thread can concurrently access or revoke `data`.
> +            // This ensures `data` is valid for `drop_in_place`.

This last sentence is not needed. Instead, we should mention that since
this is a `drop` function, we're only called once, so it's fine to call
drop (since that also is only allowed to be called once).

---
Cheers,
Benno

>              unsafe { drop_in_place(p.data.get()) };
>          }
>      }

Re: [PATCH v4 2/3] rust: revocable: simplify RevocableGuard for internal safety

[Benno Lossin]

On Tue Jun 3, 2025 at 1:26 AM CEST, Marcelo Moreira wrote:
> This commit refactors `RevocableGuard` to hold a direct reference
> (`&'a T`) instead of a raw pointer (`*const T`). This makes the guard
> internally safe, reducing the need for `unsafe` blocks in its usage
> and simplifying its implementation.
>
> The `try_access` function is updated to leverage `try_access_with_guard`
> and `map` to construct the `RevocableGuard` in a more idiomatic and safe
> Rust way, avoiding manual pointer operations. The associated invariants
> and `SAFETY` comments for `RevocableGuard` itself are removed as its
> safety is now guaranteed by its type definition.
>
> Suggested-by: Benno Lossin <lossin@kernel.org>
> Suggested-by: Danilo Krummrich <dakr@kernel.org>
> Signed-off-by: Marcelo Moreira <marcelomoreira1905@gmail.com>
> ---
>  rust/kernel/revocable.rs | 26 ++++++--------------------
>  1 file changed, 6 insertions(+), 20 deletions(-)
>
> diff --git a/rust/kernel/revocable.rs b/rust/kernel/revocable.rs
> index d14f9052f1ac..43cc9bdc94f4 100644
> --- a/rust/kernel/revocable.rs
> +++ b/rust/kernel/revocable.rs
> @@ -105,13 +105,7 @@ pub fn new(data: impl PinInit<T>) -> impl PinInit<Self> {
>      /// because another CPU may be waiting to complete the revocation of this object.
>      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))
> -        } else {
> -            None
> -        }
> +        self.try_access_with_guard(&guard).map(|data| RevocableGuard::new(data, guard))

My intuition tells me that the borrow checker will complain here, have
you tested it?

---
Cheers,
Benno

>      }
>  
>      /// Tries to access the revocable wrapped object.

Re: [PATCH v4 3/3] rust: revocable: split revoke_internal into revoke and revoke_nosync

[Benno Lossin]

On Tue Jun 3, 2025 at 1:26 AM CEST, Marcelo Moreira wrote:
> This commit refactors the revocation mechanism 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>
> Signed-off-by: Marcelo Moreira <marcelomoreira1905@gmail.com>

One comment below, with that fixed:

Reviewed-by: Benno Lossin <lossin@kernel.org>

> ---
>  rust/kernel/revocable.rs | 38 +++++++++++++++-----------------------
>  1 file changed, 15 insertions(+), 23 deletions(-)
>
> diff --git a/rust/kernel/revocable.rs b/rust/kernel/revocable.rs
> index 43cc9bdc94f4..daf22e3a7d20 100644
> --- a/rust/kernel/revocable.rs
> +++ b/rust/kernel/revocable.rs
> @@ -126,22 +126,6 @@ pub fn try_access_with_guard<'a>(&'a self, _guard: &'a rcu::Guard) -> Option<&'a
>          }
>      }
>  
> -    /// # Safety
> -    ///
> -    /// Callers must ensure that there are no more concurrent users of the revocable object.
> -    unsafe fn revoke_internal<const SYNC: bool>(&self) {
> -        if self.is_available.swap(false, Ordering::Relaxed) {
> -            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()) };
> -        }
> -    }
> -
>      /// Revokes access to and drops the wrapped object.
>      ///
>      /// Access to the object is revoked immediately to new callers of [`Revocable::try_access`],
> @@ -151,10 +135,12 @@ unsafe fn revoke_internal<const SYNC: bool>(&self) {
>      ///
>      /// Callers must ensure that there are no more concurrent users of the revocable object.
>      pub unsafe fn revoke_nosync(&self) {
> -        // 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>() }
> +        if self.is_available.swap(false, Ordering::Relaxed) {
> +            // 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

Please also use `self.data`/`self.is_available` here (& below) instead
of `Self::`.

---
Cheers,
Benno

> +            // requirements of this function, no thread is accessing `data` anymore.
> +            unsafe { drop_in_place(self.data.get()) };
> +        }
>      }
>  
>      /// Revokes access to and drops the wrapped object.
> @@ -165,9 +151,15 @@ pub unsafe fn revoke_nosync(&self) {
>      /// [`Revocable::try_access`] beforehand and still haven't dropped the returned guard), this
>      /// function waits for the concurrent access to complete before dropping the wrapped object.
>      pub fn revoke(&self) {
> -        // SAFETY: By passing `true` we ask `revoke_internal` to wait for the grace period to
> -        // finish.
> -        unsafe { self.revoke_internal::<true>() }
> +        if self.is_available.swap(false, Ordering::Relaxed) {
> +            // 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()) };
> +        }
>      }
>  }
>  

Re: [PATCH v4 2/3] rust: revocable: simplify RevocableGuard for internal safety

[Alice Ryhl]

On Mon, Jun 02, 2025 at 08:26:23PM -0300, Marcelo Moreira wrote:
> This commit refactors `RevocableGuard` to hold a direct reference
> (`&'a T`) instead of a raw pointer (`*const T`). This makes the guard
> internally safe, reducing the need for `unsafe` blocks in its usage
> and simplifying its implementation.
> 
> The `try_access` function is updated to leverage `try_access_with_guard`
> and `map` to construct the `RevocableGuard` in a more idiomatic and safe
> Rust way, avoiding manual pointer operations. The associated invariants
> and `SAFETY` comments for `RevocableGuard` itself are removed as its
> safety is now guaranteed by its type definition.
> 
> Suggested-by: Benno Lossin <lossin@kernel.org>
> Suggested-by: Danilo Krummrich <dakr@kernel.org>
> Signed-off-by: Marcelo Moreira <marcelomoreira1905@gmail.com>
> ---
>  rust/kernel/revocable.rs | 26 ++++++--------------------
>  1 file changed, 6 insertions(+), 20 deletions(-)
> 
> diff --git a/rust/kernel/revocable.rs b/rust/kernel/revocable.rs
> index d14f9052f1ac..43cc9bdc94f4 100644
> --- a/rust/kernel/revocable.rs
> +++ b/rust/kernel/revocable.rs
> @@ -105,13 +105,7 @@ pub fn new(data: impl PinInit<T>) -> impl PinInit<Self> {
>      /// because another CPU may be waiting to complete the revocation of this object.
>      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))
> -        } else {
> -            None
> -        }
> +        self.try_access_with_guard(&guard).map(|data| RevocableGuard::new(data, guard))
>      }
>  
>      /// Tries to access the revocable wrapped object.
> @@ -198,22 +192,16 @@ fn drop(self: Pin<&mut Self>) {
>  ///
>  /// CPUs may not sleep while holding on to [`RevocableGuard`] because it's in atomic context
>  /// holding the RCU read-side lock.
> -///
> -/// # Invariants
> -///
> -/// The RCU read-side lock is held while the guard is alive.
>  pub struct RevocableGuard<'a, T> {
> -    data_ref: *const T,
> +    data: &'a T,
>      _rcu_guard: rcu::Guard,
> -    _p: PhantomData<&'a ()>,
>  }

I don't think this change is valid. Consider this code:

fn takes_guard(arg: RevocableGuard<'_, i32>) {
    drop(arg);
    // rcu guard is dropped, so `arg.data` may become dangling now
}

This violates the requirement that references that appear in function
arguments are valid for the entire function call, see:
https://perso.crans.org/vanille/treebor/protectors.html

Or the LLVM perspective: When Rust sees a reference in a function
argument, it adds the LLVM attribute dereferencable to it, which implies
that the pointer must be valid for *the entire function call*. If the
memory becomes dangling after the rcu guard is dropped, then this is
violated and the compiler could perform optimizations that are not
correct.

Alice

Re: [PATCH v4 2/3] rust: revocable: simplify RevocableGuard for internal safety

[Benno Lossin]

On Thu Jun 12, 2025 at 11:28 AM CEST, Alice Ryhl wrote:
> On Mon, Jun 02, 2025 at 08:26:23PM -0300, Marcelo Moreira wrote:
>> This commit refactors `RevocableGuard` to hold a direct reference
>> (`&'a T`) instead of a raw pointer (`*const T`). This makes the guard
>> internally safe, reducing the need for `unsafe` blocks in its usage
>> and simplifying its implementation.
>> 
>> The `try_access` function is updated to leverage `try_access_with_guard`
>> and `map` to construct the `RevocableGuard` in a more idiomatic and safe
>> Rust way, avoiding manual pointer operations. The associated invariants
>> and `SAFETY` comments for `RevocableGuard` itself are removed as its
>> safety is now guaranteed by its type definition.
>> 
>> Suggested-by: Benno Lossin <lossin@kernel.org>
>> Suggested-by: Danilo Krummrich <dakr@kernel.org>
>> Signed-off-by: Marcelo Moreira <marcelomoreira1905@gmail.com>
>> ---
>>  rust/kernel/revocable.rs | 26 ++++++--------------------
>>  1 file changed, 6 insertions(+), 20 deletions(-)
>> 
>> diff --git a/rust/kernel/revocable.rs b/rust/kernel/revocable.rs
>> index d14f9052f1ac..43cc9bdc94f4 100644
>> --- a/rust/kernel/revocable.rs
>> +++ b/rust/kernel/revocable.rs
>> @@ -105,13 +105,7 @@ pub fn new(data: impl PinInit<T>) -> impl PinInit<Self> {
>>      /// because another CPU may be waiting to complete the revocation of this object.
>>      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))
>> -        } else {
>> -            None
>> -        }
>> +        self.try_access_with_guard(&guard).map(|data| RevocableGuard::new(data, guard))
>>      }
>>  
>>      /// Tries to access the revocable wrapped object.
>> @@ -198,22 +192,16 @@ fn drop(self: Pin<&mut Self>) {
>>  ///
>>  /// CPUs may not sleep while holding on to [`RevocableGuard`] because it's in atomic context
>>  /// holding the RCU read-side lock.
>> -///
>> -/// # Invariants
>> -///
>> -/// The RCU read-side lock is held while the guard is alive.
>>  pub struct RevocableGuard<'a, T> {
>> -    data_ref: *const T,
>> +    data: &'a T,
>>      _rcu_guard: rcu::Guard,
>> -    _p: PhantomData<&'a ()>,
>>  }
>
> I don't think this change is valid. Consider this code:
>
> fn takes_guard(arg: RevocableGuard<'_, i32>) {
>     drop(arg);
>     // rcu guard is dropped, so `arg.data` may become dangling now
> }
>
> This violates the requirement that references that appear in function
> arguments are valid for the entire function call, see:
> https://perso.crans.org/vanille/treebor/protectors.html

That's a good point. I don't see a way to make this work then... That's
a bit sad :(

@Marcelo: sorry for the extra work.

---
Cheers,
Benno

Re: [PATCH v4 2/3] rust: revocable: simplify RevocableGuard for internal safety

[Marcelo Moreira]

Em qui., 12 de jun. de 2025 às 06:52, Benno Lossin <lossin@kernel.org> escreveu:
>
> On Thu Jun 12, 2025 at 11:28 AM CEST, Alice Ryhl wrote:
> > On Mon, Jun 02, 2025 at 08:26:23PM -0300, Marcelo Moreira wrote:
> >> This commit refactors `RevocableGuard` to hold a direct reference
> >> (`&'a T`) instead of a raw pointer (`*const T`). This makes the guard
> >> internally safe, reducing the need for `unsafe` blocks in its usage
> >> and simplifying its implementation.
> >>
> >> The `try_access` function is updated to leverage `try_access_with_guard`
> >> and `map` to construct the `RevocableGuard` in a more idiomatic and safe
> >> Rust way, avoiding manual pointer operations. The associated invariants
> >> and `SAFETY` comments for `RevocableGuard` itself are removed as its
> >> safety is now guaranteed by its type definition.
> >>
> >> Suggested-by: Benno Lossin <lossin@kernel.org>
> >> Suggested-by: Danilo Krummrich <dakr@kernel.org>
> >> Signed-off-by: Marcelo Moreira <marcelomoreira1905@gmail.com>
> >> ---
> >>  rust/kernel/revocable.rs | 26 ++++++--------------------
> >>  1 file changed, 6 insertions(+), 20 deletions(-)
> >>
> >> diff --git a/rust/kernel/revocable.rs b/rust/kernel/revocable.rs
> >> index d14f9052f1ac..43cc9bdc94f4 100644
> >> --- a/rust/kernel/revocable.rs
> >> +++ b/rust/kernel/revocable.rs
> >> @@ -105,13 +105,7 @@ pub fn new(data: impl PinInit<T>) -> impl PinInit<Self> {
> >>      /// because another CPU may be waiting to complete the revocation of this object.
> >>      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))
> >> -        } else {
> >> -            None
> >> -        }
> >> +        self.try_access_with_guard(&guard).map(|data| RevocableGuard::new(data, guard))
> >>      }
> >>
> >>      /// Tries to access the revocable wrapped object.
> >> @@ -198,22 +192,16 @@ fn drop(self: Pin<&mut Self>) {
> >>  ///
> >>  /// CPUs may not sleep while holding on to [`RevocableGuard`] because it's in atomic context
> >>  /// holding the RCU read-side lock.
> >> -///
> >> -/// # Invariants
> >> -///
> >> -/// The RCU read-side lock is held while the guard is alive.
> >>  pub struct RevocableGuard<'a, T> {
> >> -    data_ref: *const T,
> >> +    data: &'a T,
> >>      _rcu_guard: rcu::Guard,
> >> -    _p: PhantomData<&'a ()>,
> >>  }
> >
> > I don't think this change is valid. Consider this code:
> >
> > fn takes_guard(arg: RevocableGuard<'_, i32>) {
> >     drop(arg);
> >     // rcu guard is dropped, so `arg.data` may become dangling now
> > }
> >
> > This violates the requirement that references that appear in function
> > arguments are valid for the entire function call, see:
> > https://perso.crans.org/vanille/treebor/protectors.html
>
> That's a good point. I don't see a way to make this work then... That's
> a bit sad :(

Thanks for the feedback. I didn't see this in my testing. Sorry.

I'm going to revert `RevocableGuard` back to the original design. Thanks! :)

>
> @Marcelo: sorry for the extra work.

don't worry! I'm learning =D

Just one thing, I wanted to ask for your opinion on another change in
the patch, specifically in the `try_access` function. I refactored the
logic of an `if/else` block to use
`self.try_access_with_guard(&guard).map(|data|
RevocableGuard::new(data, guard))`.

This change uses `Option::map`, the logic is: "if
`try_access_with_guard` succeeds, transform its result into a
`RevocableGuard` using the given `rcu::Guard`; otherwise, propagate
`None`." As I understand it, this transformation maintains the
original semantics and security.

Do you think keeping this specific change (using `.map()`) is a good
decision for clarity and style, or should I revert it to the explicit
`if/else` block as well?

Thanks Alice :)
Thanks Benno :)

Cheers,
Marcelo Moreira

Re: [PATCH v4 1/3] rust: revocable: update write invariant and fix safety comments

[Marcelo Moreira]

Em qui., 12 de jun. de 2025 às 06:02, Benno Lossin <lossin@kernel.org> escreveu:
>
> On Tue Jun 3, 2025 at 1:26 AM CEST, Marcelo Moreira wrote:
> > This commit clarifies the write invariant of the `Revocable` 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>
> > Signed-off-by: Marcelo Moreira <marcelomoreira1905@gmail.com>
>
> I would have done this change after the other two, since then you can
> change all the safety comments here, but if Danilo is fine with doing it
> this way, then you can leave it this way.
>

ok, I'll wait for Danilo's answer :)

> The changes are almost perfect now, just some small formatting changes
> below. With those fixed, you may add:
>
> Reviewed-by: Benno Lossin <lossin@kernel.org>

ok :)

>
> > ---
> >  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 1e5a9d25c21b..d14f9052f1ac 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,
>
> s/Self::data/self.data/

ok

>
> > +            // as `Self::is_available` is true and `_guard` holds the RCU read-side lock
>
> s/Self::is_available/self.is_available/

ok

>
> Missing `.` at the end.

ok, and sorry for this hehe . . .

>
> >              Some(unsafe { &*self.data.get() })
> >          } else {
> >              None
> > @@ -176,9 +185,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,
>
> s/Self::data/self.data/
>
> > +            // and because this `PinnedDrop` context (having `&mut Self`) guarantees exclusive access,
> > +            // ensuring no other thread can concurrently access or revoke `data`.
> > +            // This ensures `data` is valid for `drop_in_place`.
>
> This last sentence is not needed. Instead, we should mention that since
> this is a `drop` function, we're only called once, so it's fine to call
> drop (since that also is only allowed to be called once).
>

ok, I thought of something like:

// SAFETY: `self.data` is valid for writes because of `Self`'s type invariants.
// The `&mut Self` context guarantees exclusive access and a single
call to `drop`,
// making `self.data` valid for `drop_in_place`.


Thanks Benno! (^_^)b

Cheers,
Marcelo Moreira

Re: [PATCH v4 3/3] rust: revocable: split revoke_internal into revoke and revoke_nosync

[Marcelo Moreira]

Em qui., 12 de jun. de 2025 às 06:07, Benno Lossin <lossin@kernel.org> escreveu:
>
> On Tue Jun 3, 2025 at 1:26 AM CEST, Marcelo Moreira wrote:
> > This commit refactors the revocation mechanism 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>
> > Signed-off-by: Marcelo Moreira <marcelomoreira1905@gmail.com>
>
> One comment below, with that fixed:
>
> Reviewed-by: Benno Lossin <lossin@kernel.org>
>
> > ---
> >  rust/kernel/revocable.rs | 38 +++++++++++++++-----------------------
> >  1 file changed, 15 insertions(+), 23 deletions(-)
> >
> > diff --git a/rust/kernel/revocable.rs b/rust/kernel/revocable.rs
> > index 43cc9bdc94f4..daf22e3a7d20 100644
> > --- a/rust/kernel/revocable.rs
> > +++ b/rust/kernel/revocable.rs
> > @@ -126,22 +126,6 @@ pub fn try_access_with_guard<'a>(&'a self, _guard: &'a rcu::Guard) -> Option<&'a
> >          }
> >      }
> >
> > -    /// # Safety
> > -    ///
> > -    /// Callers must ensure that there are no more concurrent users of the revocable object.
> > -    unsafe fn revoke_internal<const SYNC: bool>(&self) {
> > -        if self.is_available.swap(false, Ordering::Relaxed) {
> > -            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()) };
> > -        }
> > -    }
> > -
> >      /// Revokes access to and drops the wrapped object.
> >      ///
> >      /// Access to the object is revoked immediately to new callers of [`Revocable::try_access`],
> > @@ -151,10 +135,12 @@ unsafe fn revoke_internal<const SYNC: bool>(&self) {
> >      ///
> >      /// Callers must ensure that there are no more concurrent users of the revocable object.
> >      pub unsafe fn revoke_nosync(&self) {
> > -        // 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>() }
> > +        if self.is_available.swap(false, Ordering::Relaxed) {
> > +            // 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
>
> Please also use `self.data`/`self.is_available` here (& below) instead
> of `Self::`.

ok, Thanks! :D

Cheers,
Marcelo Moreira

Re: [PATCH v4 1/3] rust: revocable: update write invariant and fix safety comments

[Danilo Krummrich]

On Thu, Jun 12, 2025 at 11:02:21AM +0200, Benno Lossin wrote:
> On Tue Jun 3, 2025 at 1:26 AM CEST, Marcelo Moreira wrote:
> > This commit clarifies the write invariant of the `Revocable` 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>
> > Signed-off-by: Marcelo Moreira <marcelomoreira1905@gmail.com>
> 
> I would have done this change after the other two, since then you can
> change all the safety comments here, but if Danilo is fine with doing it
> this way, then you can leave it this way.

I agree, it's probably better to have this patch last.

> The changes are almost perfect now, just some small formatting changes
> below. With those fixed, you may add:

and:

Reviewed-by: Danilo Krummrich <dakr@kernel.org>

> Reviewed-by: Benno Lossin <lossin@kernel.org>

Re: [PATCH v4 3/3] rust: revocable: split revoke_internal into revoke and revoke_nosync

[Danilo Krummrich]

On Thu, Jun 12, 2025 at 11:06:56AM +0200, Benno Lossin wrote:
> On Tue Jun 3, 2025 at 1:26 AM CEST, Marcelo Moreira wrote:
> > This commit refactors the revocation mechanism 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>
> > Signed-off-by: Marcelo Moreira <marcelomoreira1905@gmail.com>
> 
> One comment below, with that fixed:
> 
> Reviewed-by: Benno Lossin <lossin@kernel.org>

Reviewed-by: Danilo Krummrich <dakr@kernel.org>

Re: [PATCH v4 2/3] rust: revocable: simplify RevocableGuard for internal safety

[Danilo Krummrich]

On Thu, Jun 12, 2025 at 09:28:26AM +0000, Alice Ryhl wrote:
> I don't think this change is valid. Consider this code:
> 
> fn takes_guard(arg: RevocableGuard<'_, i32>) {
>     drop(arg);
>     // rcu guard is dropped, so `arg.data` may become dangling now
> }
> 
> This violates the requirement that references that appear in function
> arguments are valid for the entire function call, see:
> https://perso.crans.org/vanille/treebor/protectors.html
> 
> Or the LLVM perspective: When Rust sees a reference in a function
> argument, it adds the LLVM attribute dereferencable to it, which implies
> that the pointer must be valid for *the entire function call*. If the
> memory becomes dangling after the rcu guard is dropped, then this is
> violated and the compiler could perform optimizations that are not
> correct.

Interesting, I wasn't aware of that. I wonder, why can't the compiler catch this
and throw an error?

Re: [PATCH v4 2/3] rust: revocable: simplify RevocableGuard for internal safety

[Benno Lossin]

On Fri Jun 13, 2025 at 4:11 PM CEST, Danilo Krummrich wrote:
> On Thu, Jun 12, 2025 at 09:28:26AM +0000, Alice Ryhl wrote:
>> I don't think this change is valid. Consider this code:
>> 
>> fn takes_guard(arg: RevocableGuard<'_, i32>) {
>>     drop(arg);
>>     // rcu guard is dropped, so `arg.data` may become dangling now
>> }
>> 
>> This violates the requirement that references that appear in function
>> arguments are valid for the entire function call, see:
>> https://perso.crans.org/vanille/treebor/protectors.html
>> 
>> Or the LLVM perspective: When Rust sees a reference in a function
>> argument, it adds the LLVM attribute dereferencable to it, which implies
>> that the pointer must be valid for *the entire function call*. If the
>> memory becomes dangling after the rcu guard is dropped, then this is
>> violated and the compiler could perform optimizations that are not
>> correct.
>
> Interesting, I wasn't aware of that. I wonder, why can't the compiler catch this
> and throw an error?

Because the compiler doesn't know that the reference's validity is tied
to the rcu guard existing.

---
Cheers,
Benno

Re: [PATCH v4 2/3] rust: revocable: simplify RevocableGuard for internal safety

[Benno Lossin]

On Thu Jun 12, 2025 at 8:52 PM CEST, Marcelo Moreira wrote:
> Just one thing, I wanted to ask for your opinion on another change in
> the patch, specifically in the `try_access` function. I refactored the
> logic of an `if/else` block to use
> `self.try_access_with_guard(&guard).map(|data|
> RevocableGuard::new(data, guard))`.

I don't think it compiles, since `try_access_with_guard` borrows the
guard and thus you can't give it to `RevocableGuard::new`.

---
Cheers,
Benno

Re: [PATCH v4 1/3] rust: revocable: update write invariant and fix safety comments

[Benno Lossin]

On Thu Jun 12, 2025 at 9:22 PM CEST, Marcelo Moreira wrote:
> Em qui., 12 de jun. de 2025 às 06:02, Benno Lossin <lossin@kernel.org> escreveu:
>> On Tue Jun 3, 2025 at 1:26 AM CEST, Marcelo Moreira wrote:
>> > +            // and because this `PinnedDrop` context (having `&mut Self`) guarantees exclusive access,
>> > +            // ensuring no other thread can concurrently access or revoke `data`.
>> > +            // This ensures `data` is valid for `drop_in_place`.
>>
>> This last sentence is not needed. Instead, we should mention that since
>> this is a `drop` function, we're only called once, so it's fine to call
>> drop (since that also is only allowed to be called once).
>>
>
> ok, I thought of something like:
>
> // SAFETY: `self.data` is valid for writes because of `Self`'s type invariants.
> // The `&mut Self` context guarantees exclusive access and a single
> call to `drop`,

Let's mention that no thread can access `data` *because* of the
exclusive reference.

> // making `self.data` valid for `drop_in_place`.

The part about drop should be separate. We usually use bullet points for
that.

---
Cheers,
Benno

Re: [PATCH v4 1/3] rust: revocable: update write invariant and fix safety comments

[Marcelo Moreira]

Em sáb., 14 de jun. de 2025 às 15:05, Benno Lossin <lossin@kernel.org> escreveu:
>
> On Thu Jun 12, 2025 at 9:22 PM CEST, Marcelo Moreira wrote:
> > Em qui., 12 de jun. de 2025 às 06:02, Benno Lossin <lossin@kernel.org> escreveu:
> >> On Tue Jun 3, 2025 at 1:26 AM CEST, Marcelo Moreira wrote:
> >> > +            // and because this `PinnedDrop` context (having `&mut Self`) guarantees exclusive access,
> >> > +            // ensuring no other thread can concurrently access or revoke `data`.
> >> > +            // This ensures `data` is valid for `drop_in_place`.
> >>
> >> This last sentence is not needed. Instead, we should mention that since
> >> this is a `drop` function, we're only called once, so it's fine to call
> >> drop (since that also is only allowed to be called once).
> >>
> >
> > ok, I thought of something like:
> >
> > // SAFETY: `self.data` is valid for writes because of `Self`'s type invariants.
> > // The `&mut Self` context guarantees exclusive access and a single
> > call to `drop`,
>
> Let's mention that no thread can access `data` *because* of the
> exclusive reference.
>
> > // making `self.data` valid for `drop_in_place`.
>
> The part about drop should be separate. We usually use bullet points for
> that.
>

Hmm, ok.
I think I understand now.

what about:

// SAFETY: `self.data` is valid for writes because of `Self`'s type invariants.
// The `&mut Self` context guarantees exclusive access, meaning no other
// thread can concurrently access `data`.
//
// - `drop_in_place` is valid to call because `drop` is only called once.
unsafe { drop_in_place(p.data.get()) };

I'm not sure if the line break in the comment is okay...

Re: [PATCH v4 1/3] rust: revocable: update write invariant and fix safety comments

[Miguel Ojeda]

On Sun, Jun 15, 2025 at 1:12 AM Marcelo Moreira
<marcelomoreira1905@gmail.com> wrote:
>
> what about:
>
> // SAFETY: `self.data` is valid for writes because of `Self`'s type invariants.
> // The `&mut Self` context guarantees exclusive access, meaning no other
> // thread can concurrently access `data`.
> //
> // - `drop_in_place` is valid to call because `drop` is only called once.
> unsafe { drop_in_place(p.data.get()) };
>
> I'm not sure if the line break in the comment is okay...

Benno likely meant to use bullet points for each one, rather than a
bullet point after a paragraph.

If you do e.g.

    git grep -A5 '// SAFETY:$' -- rust/kernel

you will find examples of that.

Cheers,
Miguel

Re: [PATCH v4 1/3] rust: revocable: update write invariant and fix safety comments

[Marcelo Moreira]

Em dom., 15 de jun. de 2025 às 05:38, Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> escreveu:
>
> On Sun, Jun 15, 2025 at 1:12 AM Marcelo Moreira
> <marcelomoreira1905@gmail.com> wrote:
> >
> > what about:
> >
> > // SAFETY: `self.data` is valid for writes because of `Self`'s type invariants.
> > // The `&mut Self` context guarantees exclusive access, meaning no other
> > // thread can concurrently access `data`.
> > //
> > // - `drop_in_place` is valid to call because `drop` is only called once.
> > unsafe { drop_in_place(p.data.get()) };
> >
> > I'm not sure if the line break in the comment is okay...
>
> Benno likely meant to use bullet points for each one, rather than a
> bullet point after a paragraph.
>
> If you do e.g.
>
>     git grep -A5 '// SAFETY:$' -- rust/kernel
>
> you will find examples of that.

Cool! Thanks for the command Miguel =D it helped a lot.

I'm ready to send like this:

// SAFETY:
// - `self.data` is valid for writes because of `Self`'s type invariants.
// - The `&mut Self` context guarantees exclusive access, meaning no other
//    thread can concurrently access `data`.
// - `drop_in_place` is valid to call because `drop` is only called once.
unsafe { drop_in_place(p.data.get()) };

-- 
Cheers,
Marcelo Moreira

Re: [PATCH v4 1/3] rust: revocable: update write invariant and fix safety comments

[Benno Lossin]

On Mon Jun 16, 2025 at 2:36 AM CEST, Marcelo Moreira wrote:
> Em dom., 15 de jun. de 2025 às 05:38, Miguel Ojeda
> <miguel.ojeda.sandonis@gmail.com> escreveu:
>>
>> On Sun, Jun 15, 2025 at 1:12 AM Marcelo Moreira
>> <marcelomoreira1905@gmail.com> wrote:
>> >
>> > what about:
>> >
>> > // SAFETY: `self.data` is valid for writes because of `Self`'s type invariants.
>> > // The `&mut Self` context guarantees exclusive access, meaning no other
>> > // thread can concurrently access `data`.
>> > //
>> > // - `drop_in_place` is valid to call because `drop` is only called once.
>> > unsafe { drop_in_place(p.data.get()) };
>> >
>> > I'm not sure if the line break in the comment is okay...
>>
>> Benno likely meant to use bullet points for each one, rather than a
>> bullet point after a paragraph.
>>
>> If you do e.g.
>>
>>     git grep -A5 '// SAFETY:$' -- rust/kernel
>>
>> you will find examples of that.
>
> Cool! Thanks for the command Miguel =D it helped a lot.
>
> I'm ready to send like this:
>
> // SAFETY:
> // - `self.data` is valid for writes because of `Self`'s type invariants.
> // - The `&mut Self` context guarantees exclusive access, meaning no other
> //    thread can concurrently access `data`.
       ^ spurious space here,

This argument is actually an extension of the previous one.

> // - `drop_in_place` is valid to call because `drop` is only called once.

This phrasing feels a bit weird to me, but I can't put my finger on what
exactly...

> unsafe { drop_in_place(p.data.get()) };

How about:

// - `self.data` is valid for writes because of `Self`'s type invariants:
//   `&mut Self` guarantees exclusive access, thus no other thread can concurrently access `data`.
// - this function is a drop function, thus this code is at most executed once.

---
Cheers,
Benno

Re: [PATCH v4 0/3] rust: revocable: documentation and refactorings

[Danilo Krummrich]

(Cc: +Greg, +Rafael)

On Mon, Jun 02, 2025 at 08:26:21PM -0300, Marcelo Moreira wrote:
> This series of patches brings documentation and refactorings to the `Revocable` type.

@Miguel: This series will have conflicts with [1]. Given that it does not
provide any functional changes, do you want me to route it through the
driver-core tree to avoid conflicts, once ready?

(Will have to backmerge -fixes anyways for other patches.)

[1] https://git.kernel.org/pub/scm/linux/kernel/git/driver-core/driver-core.git/commit/?h=driver-core-linus&id=4b76fafb20dd4a2becb94949d78e86bc88006509

Re: [PATCH v4 0/3] rust: revocable: documentation and refactorings

[Miguel Ojeda]

On Mon, Jun 16, 2025 at 12:27 PM Danilo Krummrich <dakr@kernel.org> wrote:
>
> @Miguel: This series will have conflicts with [1]. Given that it does not
> provide any functional changes, do you want me to route it through the
> driver-core tree to avoid conflicts, once ready?
>
> (Will have to backmerge -fixes anyways for other patches.)

I think the conflict is not too bad, right? But if you need it this
cycle anyway for something else, sure. Let's see the next version...

Cheers,
Miguel

Re: [PATCH v4 1/3] rust: revocable: update write invariant and fix safety comments

[Marcelo Moreira]

Em seg., 16 de jun. de 2025 às 04:15, Benno Lossin <lossin@kernel.org> escreveu:
>
> On Mon Jun 16, 2025 at 2:36 AM CEST, Marcelo Moreira wrote:
> > Em dom., 15 de jun. de 2025 às 05:38, Miguel Ojeda
> > <miguel.ojeda.sandonis@gmail.com> escreveu:
> >>
> >> On Sun, Jun 15, 2025 at 1:12 AM Marcelo Moreira
> >> <marcelomoreira1905@gmail.com> wrote:
> >> >
> >> > what about:
> >> >
> >> > // SAFETY: `self.data` is valid for writes because of `Self`'s type invariants.
> >> > // The `&mut Self` context guarantees exclusive access, meaning no other
> >> > // thread can concurrently access `data`.
> >> > //
> >> > // - `drop_in_place` is valid to call because `drop` is only called once.
> >> > unsafe { drop_in_place(p.data.get()) };
> >> >
> >> > I'm not sure if the line break in the comment is okay...
> >>
> >> Benno likely meant to use bullet points for each one, rather than a
> >> bullet point after a paragraph.
> >>
> >> If you do e.g.
> >>
> >>     git grep -A5 '// SAFETY:$' -- rust/kernel
> >>
> >> you will find examples of that.
> >
> > Cool! Thanks for the command Miguel =D it helped a lot.
> >
> > I'm ready to send like this:
> >
> > // SAFETY:
> > // - `self.data` is valid for writes because of `Self`'s type invariants.
> > // - The `&mut Self` context guarantees exclusive access, meaning no other
> > //    thread can concurrently access `data`.
>        ^ spurious space here,
>
> This argument is actually an extension of the previous one.
>
> > // - `drop_in_place` is valid to call because `drop` is only called once.
>
> This phrasing feels a bit weird to me, but I can't put my finger on what
> exactly...
>
> > unsafe { drop_in_place(p.data.get()) };
>
> How about:
>
> // - `self.data` is valid for writes because of `Self`'s type invariants:
> //   `&mut Self` guarantees exclusive access, thus no other thread can concurrently access `data`.
> // - this function is a drop function, thus this code is at most executed once.

Ok! =)

I thought about using "so" instead of the first "thus" so I wouldn't
have to repeat the use of "thus" so many times.

Final version:
// - `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.

Thanks! =)
--
Cheers,
Marcelo Moreira

Re: [PATCH v4 1/3] rust: revocable: update write invariant and fix safety comments

[Benno Lossin]

On Tue Jun 17, 2025 at 4:49 AM CEST, Marcelo Moreira wrote:
> Em seg., 16 de jun. de 2025 às 04:15, Benno Lossin <lossin@kernel.org> escreveu:
>> How about:
>>
>> // - `self.data` is valid for writes because of `Self`'s type invariants:
>> //   `&mut Self` guarantees exclusive access, thus no other thread can concurrently access `data`.
>> // - this function is a drop function, thus this code is at most executed once.
>
> Ok! =)
>
> I thought about using "so" instead of the first "thus" so I wouldn't
> have to repeat the use of "thus" so many times.

I personally don't care about repeating those "filler" words in safety
documentation. The clearer it is the better :)

> Final version:
> // - `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.

That is fine too.

---
Cheers,
Benno

Re: [PATCH v4 1/3] rust: revocable: update write invariant and fix safety comments

[Marcelo Moreira]

Em ter., 17 de jun. de 2025 às 04:18, Benno Lossin <lossin@kernel.org> escreveu:
>
> On Tue Jun 17, 2025 at 4:49 AM CEST, Marcelo Moreira wrote:
> > Em seg., 16 de jun. de 2025 às 04:15, Benno Lossin <lossin@kernel.org> escreveu:
> >> How about:
> >>
> >> // - `self.data` is valid for writes because of `Self`'s type invariants:
> >> //   `&mut Self` guarantees exclusive access, thus no other thread can concurrently access `data`.
> >> // - this function is a drop function, thus this code is at most executed once.
> >
> > Ok! =)
> >
> > I thought about using "so" instead of the first "thus" so I wouldn't
> > have to repeat the use of "thus" so many times.
>
> I personally don't care about repeating those "filler" words in safety
> documentation. The clearer it is the better :)
>

Ok, thanks Benno!

> > Final version:
> > // - `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.
>
> That is fine too.

Thanks!

Guys, sorry for the delay in replying. I traveled to my parents' house
to visit them :)

I'm sending v5 now!

---
Cheers,
Marcelo Moreira