Re: [PATCH v2] rust: doc: Clarify safety invariants for Revocable type

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

On Sat May 3, 2025 at 4:53 PM CEST, Marcelo Moreira wrote:
> The Revocable type in rust/kernel/revocable.rs lacked a comprehensive
> documentation of its safety invariants, specifically regarding the
> validity of the wrapped data and the necessity of holding the RCU
> read-side lock for access. This patch addresses this by:
>
> - Adding an '# Invariants' section to the documentation of `Revocable<T>`
>   clarifying that `data` is valid if and only if `is_available` is true,
>   and that access to `data` requires holding the RCU read-side lock.
> - Adding '// INVARIANT:' comments in `try_access` and `try_access_with_guard`
>   to explicitly refer to these invariants before accessing the underlying data.
> - Adding an '# Invariants' section to the documentation of `RevocableGuard<'_, T>`
>   documenting that the RCU read-side lock is held for the lifetime of the guard
>   and that `data_ref` points to valid data during this time.
> - Updating the safety comment in the `Deref` implementation of `RevocableGuard`
>   to explicitly mention the relevant invariants.
>
> Changes in v2:

The changelog should not be part of the commit message, instead place it
below the `---`, but before any file diff. It then will only appear in
the email and not the commit message.

Another thing: could you please CC Danilo Krummrich the next time you
send this patch? I think he should also take a look at this.

>
> - 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.
>
> Reported-by: Benno Lossin <benno.lossin@proton.me>
> Closes: https://github.com/Rust-for-Linux/linux/issues/1160
> Suggested-by: Miguel Ojeda <ojeda@kernel.org>
> Signed-off-by: Marcelo Moreira <marcelomoreira1905@gmail.com>
> ---

^^ right here would be the place for the changelog.

>  rust/kernel/revocable.rs | 29 ++++++++++++++++-------------
>  1 file changed, 16 insertions(+), 13 deletions(-)

It seems to me that you sent this patch on top of your previous one [1].
Normally, one doesn't do that and instead sends the patch based on a tag
(like `v6.15-rc4`) or the subsystems `-next` branch (so in our case
`rust-next`). So a new version should not rely on any previous one.

[1]: https://lore.kernel.org/all/20250501005726.744027-1-marcelomoreira1905@gmail.com

> diff --git a/rust/kernel/revocable.rs b/rust/kernel/revocable.rs
> index 2da3e9460c07..7ef2f34782b4 100644
> --- a/rust/kernel/revocable.rs
> +++ b/rust/kernel/revocable.rs
> @@ -64,12 +64,11 @@
>  ///
>  /// # Invariants
>  ///
> -/// - The wrapped object `data` is valid if and only if `is_available` is `true`.
> -/// - Access to `data` must occur only while holding the RCU read-side lock (e.g., via
> -///   [`Revocable::try_access`] or [`Revocable::try_access_with_guard`]).
> -/// - Once `is_available` is set to `false`, further access to `data` is disallowed,
> -///   and the object is dropped either after an RCU grace period (in [`revoke`]),
> -///   or immediately (in [`revoke_nosync`]).
> +/// - `data` is valid if and only if `is_available` is true.
> +/// - Access to `data` requires holding the RCU read-side lock.

I'm not sure what the correct wording here should be. The current
wording makes the `revoke_internal` function illegal, as it doesn't hold
the read-side lock, but still accesses `data`.

Maybe @Danilo can help here, but as I understand it, the value in `data`
is valid for as long as the rcu read-side lock is held *and* if
`is_available` was true at some point while holding the lock.

> +/// - Once is_available is set to false, further access to data is disallowed,
> +///   and the object is dropped either after an RCU grace period (in [revoke]),
> +///   or immediately (in [revoke_nosync]).

Several missing `

>  #[pin_data(PinnedDrop)]
>  pub struct Revocable<T> {
>      is_available: AtomicBool,
> @@ -106,8 +105,9 @@ pub fn new(data: impl PinInit<T>) -> impl PinInit<Self> {
>      pub fn try_access(&self) -> Option<RevocableGuard<'_, T>> {
>          let guard = rcu::read_lock();
>          if self.is_available.load(Ordering::Relaxed) {
> -            // Since `self.is_available` is true, data is initialised and has to remain valid
> -            // because the RCU read side lock prevents it from being dropped.
> +            // INVARIANT: Since `self.is_available` is true, `self.data` is valid. The
> +            // RCU read-side lock held by `guard` ensures that `self.data` remains valid for
> +            // the lifetime of the guard.
>              Some(RevocableGuard::new(self.data.get(), guard))
>          } else {
>              None
> @@ -124,8 +124,10 @@ 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:

Empty SAFETY comment?

> +            // INVARIANT: Since `self.is_available` is true, `self.data` is valid. The
> +            // RCU read-side lock held by `_guard` ensures that `self.data` remains valid for
> +            // the lifetime of the returned reference.
>              Some(unsafe { &*self.data.get() })
>          } else {
>              None
> @@ -200,7 +202,8 @@ fn drop(self: Pin<&mut Self>) {
>  ///
>  /// # Invariants
>  ///
> -/// The RCU read-side lock is held while the guard is alive.
> +/// The RCU read-side lock is held for the lifetime of this guard.
> +/// `data_ref` points to valid data for the lifetime of this guard.

Please use a bullet point list.

---
Cheers,
Benno

>  pub struct RevocableGuard<'a, T> {
>      data_ref: *const T,
>      _rcu_guard: rcu::Guard,
> @@ -221,8 +224,8 @@ impl<T> Deref for RevocableGuard<'_, T> {
>      type Target = T;
>  
>      fn deref(&self) -> &Self::Target {
> -        // SAFETY: By the type invariants, we hold the rcu read-side lock, so the object is
> -        // guaranteed to remain valid.
> +        // SAFETY: By the invariant of `Revocable`, `self.data_ref` is valid because the
> +        // RCU read-side lock is held for the lifetime of this guard.
>          unsafe { &*self.data_ref }
>      }
>  }