Message-Id: <DB29YAYDK6YW.1NF5I2WSI1BPR@kernel.org>
Date: Thu, 03 Jul 2025 10:24:50 +0200
From: "Benno Lossin" <lossin@...nel.org>
To: "Marcelo Moreira" <marcelomoreira1905@...il.com>,
 <rust-for-linux@...r.kernel.org>
Cc: <linux-kernel@...r.kernel.org>, <dakr@...nel.org>, <ojeda@...nel.org>,
 <skhan@...uxfoundation.org>,
 <linux-kernel-mentees@...ts.linuxfoundation.org>,
 <~lkcamp/patches@...ts.sr.ht>
Subject: Re: [PATCH v5 0/2] rust: revocable: documentation and refactorings

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

Powered by blists - more mailing lists