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

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

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()) };
>          }
>      }