linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v5 0/2] rust: revocable: documentation and refactorings
@ 2025-06-26 16:59 Marcelo Moreira
  2025-06-26 16:59 ` [PATCH v5 1/2] rust: revocable: Refactor revocation mechanism to remove generic revoke_internal Marcelo Moreira
                   ` (3 more replies)
  0 siblings, 4 replies; 10+ messages in thread
From: Marcelo Moreira @ 2025-06-26 16:59 UTC (permalink / raw)
  To: rust-for-linux
  Cc: linux-kernel, lossin, dakr, ojeda, skhan, linux-kernel-mentees,
	~lkcamp/patches

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

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.

Marcelo Moreira (2):
  rust: revocable: Refactor revocation mechanism to remove generic
    revoke_internal
  rust: revocable: Clarify write invariant and update safety comments

Changelog
---------

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 Lossin's and Miguel Ojeda's feedback, adopting a more concise and standard Kernel-style bullet point format.
- Corrected a duplicated line in the commit message of the second patch.

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/DAOMIWBZXFO9.U353H8NWTLC5@kernel.org/T/#u

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

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

base-commit: 0303584766b7bdb6564c7e8f13e0b59b6ef44984

-- 
2.50.0

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v5 1/2] rust: revocable: Refactor revocation mechanism to remove generic revoke_internal
  2025-06-26 16:59 [PATCH v5 0/2] rust: revocable: documentation and refactorings Marcelo Moreira
@ 2025-06-26 16:59 ` Marcelo Moreira
  2025-07-03  8:24   ` Benno Lossin
  2025-06-26 16:59 ` [PATCH v5 2/2] rust: revocable: Clarify write invariant and update safety comments Marcelo Moreira
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 10+ messages in thread
From: Marcelo Moreira @ 2025-06-26 16:59 UTC (permalink / raw)
  To: rust-for-linux
  Cc: linux-kernel, lossin, dakr, ojeda, skhan, linux-kernel-mentees,
	~lkcamp/patches

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>
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 06a3cdfce344..f10ce5c1ed77 100644
--- a/rust/kernel/revocable.rs
+++ b/rust/kernel/revocable.rs
@@ -151,26 +151,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`],
@@ -183,10 +163,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.
@@ -200,9 +185,18 @@ pub unsafe fn revoke_nosync(&self) -> bool {
     /// Returns `true` if `&self` has been revoked with this call, `false` if it was revoked
     /// already.
     pub fn revoke(&self) -> bool {
-        // SAFETY: By passing `true` we ask `revoke_internal` to wait for the grace period to
-        // finish.
-        unsafe { self.revoke_internal::<true>() }
+        let revoke = self.is_available.swap(false, Ordering::Relaxed);
+
+        if revoke {
+            // SAFETY: Just an FFI call, there are no further requirements.
+            unsafe { bindings::synchronize_rcu() };
+
+            // SAFETY: `self.data` is valid for writes because of `Self`'s type invariants,
+            // as `self.is_available` is false due to the atomic swap, and `synchronize_rcu`
+            // ensures all prior RCU read-side critical sections have completed.
+            unsafe { drop_in_place(self.data.get()) };
+        }
+        revoke
     }
 }
 
-- 
2.50.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v5 2/2] rust: revocable: Clarify write invariant and update safety comments
  2025-06-26 16:59 [PATCH v5 0/2] rust: revocable: documentation and refactorings Marcelo Moreira
  2025-06-26 16:59 ` [PATCH v5 1/2] rust: revocable: Refactor revocation mechanism to remove generic revoke_internal Marcelo Moreira
@ 2025-06-26 16:59 ` Marcelo Moreira
  2025-07-01 11:27 ` [PATCH v5 0/2] rust: revocable: documentation and refactorings Alice Ryhl
  2025-07-03  8:24 ` Benno Lossin
  3 siblings, 0 replies; 10+ messages in thread
From: Marcelo Moreira @ 2025-06-26 16:59 UTC (permalink / raw)
  To: rust-for-linux
  Cc: linux-kernel, lossin, dakr, ojeda, skhan, linux-kernel-mentees,
	~lkcamp/patches

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 f10ce5c1ed77..88976c62e1ef 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
@@ -208,9 +217,10 @@ fn drop(self: Pin<&mut Self>) {
         // SAFETY: We are not moving out of `p`, only dropping in place
         let p = unsafe { self.get_unchecked_mut() };
         if *p.is_available.get_mut() {
-            // SAFETY: We know `self.data` is valid because no other CPU has changed
-            // `is_available` to `false` yet, and no other CPU can do it anymore because this CPU
-            // holds the only reference (mutable) to `self` now.
+            // SAFETY:
+            // - `self.data` is valid for writes because of `Self`'s type invariants:
+            //   `&mut Self` guarantees exclusive access, so no other thread can concurrently access `data`.
+            // - this function is a drop function, thus this code is at most executed once.
             unsafe { drop_in_place(p.data.get()) };
         }
     }
-- 
2.50.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH v5 0/2] rust: revocable: documentation and refactorings
  2025-06-26 16:59 [PATCH v5 0/2] rust: revocable: documentation and refactorings Marcelo Moreira
  2025-06-26 16:59 ` [PATCH v5 1/2] rust: revocable: Refactor revocation mechanism to remove generic revoke_internal Marcelo Moreira
  2025-06-26 16:59 ` [PATCH v5 2/2] rust: revocable: Clarify write invariant and update safety comments Marcelo Moreira
@ 2025-07-01 11:27 ` Alice Ryhl
  2025-07-01 11:40   ` Danilo Krummrich
  2025-07-03  8:24 ` Benno Lossin
  3 siblings, 1 reply; 10+ messages in thread
From: Alice Ryhl @ 2025-07-01 11:27 UTC (permalink / raw)
  To: Marcelo Moreira, Danilo Krummrich
  Cc: rust-for-linux, linux-kernel, lossin, ojeda, skhan,
	linux-kernel-mentees, ~lkcamp/patches

On Thu, Jun 26, 2025 at 6:59 PM Marcelo Moreira
<marcelomoreira1905@gmail.com> wrote:
>
> This patch series brings documentation and refactorings to the `Revocable` type.
>
> 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.
>
> Marcelo Moreira (2):
>   rust: revocable: Refactor revocation mechanism to remove generic
>     revoke_internal
>   rust: revocable: Clarify write invariant and update safety comments

Danilo, did you have Revocable / Devres changes that conflict with this?

Alice

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v5 0/2] rust: revocable: documentation and refactorings
  2025-07-01 11:27 ` [PATCH v5 0/2] rust: revocable: documentation and refactorings Alice Ryhl
@ 2025-07-01 11:40   ` Danilo Krummrich
  2025-07-01 12:40     ` Marcelo Moreira
  0 siblings, 1 reply; 10+ messages in thread
From: Danilo Krummrich @ 2025-07-01 11:40 UTC (permalink / raw)
  To: Alice Ryhl
  Cc: Marcelo Moreira, rust-for-linux, linux-kernel, lossin, ojeda,
	skhan, linux-kernel-mentees, ~lkcamp/patches

On Tue, Jul 01, 2025 at 01:27:17PM +0200, Alice Ryhl wrote:
> On Thu, Jun 26, 2025 at 6:59 PM Marcelo Moreira
> <marcelomoreira1905@gmail.com> wrote:
> >
> > This patch series brings documentation and refactorings to the `Revocable` type.
> >
> > 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.
> >
> > Marcelo Moreira (2):
> >   rust: revocable: Refactor revocation mechanism to remove generic
> >     revoke_internal
> >   rust: revocable: Clarify write invariant and update safety comments
> 
> Danilo, did you have Revocable / Devres changes that conflict with this?

Yes, but I sent them to Linus for -rc3 already. Given that rust-next is based on
-rc3, we should be good. There shouldn't be any further conflicts.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v5 0/2] rust: revocable: documentation and refactorings
  2025-07-01 11:40   ` Danilo Krummrich
@ 2025-07-01 12:40     ` Marcelo Moreira
  0 siblings, 0 replies; 10+ messages in thread
From: Marcelo Moreira @ 2025-07-01 12:40 UTC (permalink / raw)
  To: Danilo Krummrich
  Cc: Alice Ryhl, rust-for-linux, linux-kernel, lossin, ojeda, skhan,
	linux-kernel-mentees, ~lkcamp/patches

Em ter., 1 de jul. de 2025 às 08:40, Danilo Krummrich
<dakr@kernel.org> escreveu:
>
> On Tue, Jul 01, 2025 at 01:27:17PM +0200, Alice Ryhl wrote:
> > On Thu, Jun 26, 2025 at 6:59 PM Marcelo Moreira
> > <marcelomoreira1905@gmail.com> wrote:
> > >
> > > This patch series brings documentation and refactorings to the `Revocable` type.
> > >
> > > 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.
> > >
> > > Marcelo Moreira (2):
> > >   rust: revocable: Refactor revocation mechanism to remove generic
> > >     revoke_internal
> > >   rust: revocable: Clarify write invariant and update safety comments
> >
> > Danilo, did you have Revocable / Devres changes that conflict with this?
>
> Yes, but I sent them to Linus for -rc3 already. Given that rust-next is based on
> -rc3, we should be good. There shouldn't be any further conflicts.

Hi guys!

I resolved the conflicts. I hope it is the way you expected.

-- 
Cheers,
Marcelo Moreira

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v5 1/2] rust: revocable: Refactor revocation mechanism to remove generic revoke_internal
  2025-06-26 16:59 ` [PATCH v5 1/2] rust: revocable: Refactor revocation mechanism to remove generic revoke_internal Marcelo Moreira
@ 2025-07-03  8:24   ` Benno Lossin
  0 siblings, 0 replies; 10+ messages in thread
From: Benno Lossin @ 2025-07-03  8:24 UTC (permalink / raw)
  To: Marcelo Moreira, rust-for-linux
  Cc: linux-kernel, dakr, ojeda, skhan, linux-kernel-mentees,
	~lkcamp/patches

On Thu Jun 26, 2025 at 6:59 PM CEST, Marcelo Moreira wrote:
> 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>
> Signed-off-by: Marcelo Moreira <marcelomoreira1905@gmail.com>

With the patch order changed:

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

---
Cheer,
Benno

> ---
>  rust/kernel/revocable.rs | 48 ++++++++++++++++++----------------------
>  1 file changed, 21 insertions(+), 27 deletions(-)

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v5 0/2] rust: revocable: documentation and refactorings
  2025-06-26 16:59 [PATCH v5 0/2] rust: revocable: documentation and refactorings Marcelo Moreira
                   ` (2 preceding siblings ...)
  2025-07-01 11:27 ` [PATCH v5 0/2] rust: revocable: documentation and refactorings Alice Ryhl
@ 2025-07-03  8:24 ` Benno Lossin
  2025-07-05  5:09   ` Marcelo Moreira
  3 siblings, 1 reply; 10+ messages in thread
From: Benno Lossin @ 2025-07-03  8:24 UTC (permalink / raw)
  To: Marcelo Moreira, rust-for-linux
  Cc: linux-kernel, dakr, ojeda, skhan, linux-kernel-mentees,
	~lkcamp/patches

On Thu Jun 26, 2025 at 6:59 PM CEST, Marcelo Moreira wrote:
> This patch series brings documentation and refactorings to the `Revocable` type.
>
> 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.

Could you wrap your text to a more readable column? Thanks!

>
> Marcelo Moreira (2):
>   rust: revocable: Refactor revocation mechanism to remove generic
>     revoke_internal
>   rust: revocable: Clarify write invariant and update safety comments
>
> Changelog
> ---------
>
> 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 Lossin's and Miguel Ojeda's feedback, adopting a more concise and standard Kernel-style bullet point format.
> - Corrected a duplicated line in the commit message of the second patch.

Now since we had to drop the `RevocableGuard` change, its safety
invariant & comment in `deref` is insufficient. It shouldn't have the
invariant that the rcu lock is held (since it owns an `rcu::Guard`, that
already is guaranteed), but instead it should require that the
`data_ref` pointer is valid. That invariant is then used by the safety
comment in `deref` to justify dereferencing the pointer.

Also, I think it's better to reorder the patches again (since the
current first one relies on changes from the second one), the first one
should be the change to the invariants section of `Revocable` (so
currently the second patch). Then the second and third patches can be
the removal of `revoke_internal` and the `RevocableGuard` safety
documentation fix.

---
Cheers,
Benno

> 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/DAOMIWBZXFO9.U353H8NWTLC5@kernel.org/T/#u
>
> 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
>
>  rust/kernel/revocable.rs | 68 +++++++++++++++++++++-------------------
>  1 file changed, 36 insertions(+), 32 deletions(-)
>
> base-commit: 0303584766b7bdb6564c7e8f13e0b59b6ef44984


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v5 0/2] rust: revocable: documentation and refactorings
  2025-07-03  8:24 ` Benno Lossin
@ 2025-07-05  5:09   ` Marcelo Moreira
  2025-07-05  7:01     ` Benno Lossin
  0 siblings, 1 reply; 10+ messages in thread
From: Marcelo Moreira @ 2025-07-05  5:09 UTC (permalink / raw)
  To: Benno Lossin
  Cc: rust-for-linux, linux-kernel, dakr, ojeda, skhan,
	linux-kernel-mentees, ~lkcamp/patches

Em qui., 3 de jul. de 2025 às 05:24, Benno Lossin <lossin@kernel.org> escreveu:
>
> On Thu Jun 26, 2025 at 6:59 PM CEST, Marcelo Moreira wrote:
> > This patch series brings documentation and refactorings to the `Revocable` type.
> >
> > 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.
>
> Could you wrap your text to a more readable column? Thanks!

Sure! Thanks!

>
> >
> > Marcelo Moreira (2):
> >   rust: revocable: Refactor revocation mechanism to remove generic
> >     revoke_internal
> >   rust: revocable: Clarify write invariant and update safety comments
> >
> > Changelog
> > ---------
> >
> > 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 Lossin's and Miguel Ojeda's feedback, adopting a more concise and standard Kernel-style bullet point format.
> > - Corrected a duplicated line in the commit message of the second patch.
>
> Now since we had to drop the `RevocableGuard` change, its safety
> invariant & comment in `deref` is insufficient. It shouldn't have the
> invariant that the rcu lock is held (since it owns an `rcu::Guard`, that
> already is guaranteed), but instead it should require that the
> `data_ref` pointer is valid. That invariant is then used by the safety
> comment in `deref` to justify dereferencing the pointer.
>
> Also, I think it's better to reorder the patches again (since the
> current first one relies on changes from the second one), the first one
> should be the change to the invariants section of `Revocable` (so
> currently the second patch). Then the second and third patches can be
> the removal of `revoke_internal` and the `RevocableGuard` safety
> documentation fix.

All right Benno, I'll prepare the comment for `RevocableGuard` and send v6.

The order now is:
1- Documentation for invariant and updates associated `SAFETY` comments
2- Remove `revoke_internal` (Refactoring)
3- `RevocableGuard` safety documentation fix.

Thanks! :)

--
Cheers,
Marcelo Moreira

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v5 0/2] rust: revocable: documentation and refactorings
  2025-07-05  5:09   ` Marcelo Moreira
@ 2025-07-05  7:01     ` Benno Lossin
  0 siblings, 0 replies; 10+ messages in thread
From: Benno Lossin @ 2025-07-05  7:01 UTC (permalink / raw)
  To: Marcelo Moreira
  Cc: rust-for-linux, linux-kernel, dakr, ojeda, skhan,
	linux-kernel-mentees, ~lkcamp/patches

On Sat Jul 5, 2025 at 7:09 AM CEST, Marcelo Moreira wrote:
> Em qui., 3 de jul. de 2025 às 05:24, Benno Lossin <lossin@kernel.org> escreveu:
>>
>> On Thu Jun 26, 2025 at 6:59 PM CEST, Marcelo Moreira wrote:
>> > This patch series brings documentation and refactorings to the `Revocable` type.
>> >
>> > 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.
>>
>> Could you wrap your text to a more readable column? Thanks!
>
> Sure! Thanks!
>
>>
>> >
>> > Marcelo Moreira (2):
>> >   rust: revocable: Refactor revocation mechanism to remove generic
>> >     revoke_internal
>> >   rust: revocable: Clarify write invariant and update safety comments
>> >
>> > Changelog
>> > ---------
>> >
>> > 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 Lossin's and Miguel Ojeda's feedback, adopting a more concise and standard Kernel-style bullet point format.
>> > - Corrected a duplicated line in the commit message of the second patch.
>>
>> Now since we had to drop the `RevocableGuard` change, its safety
>> invariant & comment in `deref` is insufficient. It shouldn't have the
>> invariant that the rcu lock is held (since it owns an `rcu::Guard`, that
>> already is guaranteed), but instead it should require that the
>> `data_ref` pointer is valid. That invariant is then used by the safety
>> comment in `deref` to justify dereferencing the pointer.
>>
>> Also, I think it's better to reorder the patches again (since the
>> current first one relies on changes from the second one), the first one
>> should be the change to the invariants section of `Revocable` (so
>> currently the second patch). Then the second and third patches can be
>> the removal of `revoke_internal` and the `RevocableGuard` safety
>> documentation fix.
>
> All right Benno, I'll prepare the comment for `RevocableGuard` and send v6.
>
> The order now is:
> 1- Documentation for invariant and updates associated `SAFETY` comments
> 2- Remove `revoke_internal` (Refactoring)
> 3- `RevocableGuard` safety documentation fix.

Sounds good!

---
Cheers,
Benno

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2025-07-05  7:01 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-06-26 16:59 [PATCH v5 0/2] rust: revocable: documentation and refactorings Marcelo Moreira
2025-06-26 16:59 ` [PATCH v5 1/2] rust: revocable: Refactor revocation mechanism to remove generic revoke_internal Marcelo Moreira
2025-07-03  8:24   ` Benno Lossin
2025-06-26 16:59 ` [PATCH v5 2/2] rust: revocable: Clarify write invariant and update safety comments Marcelo Moreira
2025-07-01 11:27 ` [PATCH v5 0/2] rust: revocable: documentation and refactorings Alice Ryhl
2025-07-01 11:40   ` Danilo Krummrich
2025-07-01 12:40     ` Marcelo Moreira
2025-07-03  8:24 ` Benno Lossin
2025-07-05  5:09   ` Marcelo Moreira
2025-07-05  7:01     ` Benno Lossin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).