Re: [PATCH v7 3/3] rust: revocable: Document RevocableGuard invariants/safety and refine Deref safety

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

On Mon Jul 21, 2025 at 3:01 AM CEST, Marcelo Moreira wrote:
> Refinements include:
> - `RevocableGuard`'s invariants are updated to precisely state that
>   `data_ref` is valid as long as the RCU read-side lock is held.
> - The `RevocableGuard::new` constructor is made `unsafe`, explicitly
>   requiring callers to guarantee the validity of the raw pointer and
>   RCU read-side lock lifetime.
> - A new `SAFETY` comment is added to `Revocable::try_access` to
>   justify the `unsafe` call to `RevocableGuard::new`, detailing how
>   `Self`'s type invariants and the active RCU read-side lock ensure data
>   validity for reads.
> - The `Deref` implementation's `SAFETY` comment for `RevocableGuard`
>   is refined.
>
> Signed-off-by: Marcelo Moreira <marcelomoreira1905@gmail.com>
> ---
>  rust/kernel/revocable.rs | 25 ++++++++++++++++++-------
>  1 file changed, 18 insertions(+), 7 deletions(-)
>
> diff --git a/rust/kernel/revocable.rs b/rust/kernel/revocable.rs
> index 6d8e9237dbdf..0048de23ab44 100644
> --- a/rust/kernel/revocable.rs
> +++ b/rust/kernel/revocable.rs
> @@ -106,9 +106,12 @@ 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.
> -            Some(RevocableGuard::new(self.data.get(), guard))
> +            // SAFETY:
> +            // - `self.data` is valid for reads because of `Self`'s type invariants:
> +            //   `self.is_available` is true.
> +            // - The RCU read-side lock is active via `guard`, preventing `self.data`
> +            //   from being dropped and ensuring its validity for the guard's lifetime.

This shouldn't be needed.

> +            Some(unsafe { RevocableGuard::new(self.data.get(), guard) })
>          } else {
>              None
>          }
> @@ -233,7 +236,7 @@ fn drop(self: Pin<&mut Self>) {
>  ///
>  /// # Invariants
>  ///
> -/// The RCU read-side lock is held while the guard is alive.
> +/// - `data_ref` is a valid pointer for as long as the RCU read-side lock is held.
>  pub struct RevocableGuard<'a, T> {
>      // This can't use the `&'a T` type because references that appear in function arguments must
>      // not become dangling during the execution of the function, which can happen if the
> @@ -245,7 +248,15 @@ pub struct RevocableGuard<'a, T> {
>  }
>  
>  impl<T> RevocableGuard<'_, T> {
> -    fn new(data_ref: *const T, rcu_guard: rcu::Guard) -> Self {
> +    /// Creates a new `RevocableGuard`.
> +    ///
> +    /// # Safety
> +    ///
> +    /// Callers must ensure that `data_ref` is a valid pointer to a `T` object,
> +    /// and that it remains valid for as long as the returned `RevocableGuard` is alive.
> +    /// The RCU read-side lock must be held for the duration of the guard's lifetime,
> +    /// as indicated by `rcu_guard`.

This last part shouldn't be needed, as the `rcu::Guard` already
guarantees it.

---
Cheers,
Benno

> +    unsafe fn new(data_ref: *const T, rcu_guard: rcu::Guard) -> Self {
>          Self {
>              data_ref,
>              _rcu_guard: rcu_guard,
> @@ -258,8 +269,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: `self.data_ref` is valid because of `Self`'s type invariants,
> +        // and the active RCU read-side lock held via `_rcu_guard`, ensuring the data's accessibility.
>          unsafe { &*self.data_ref }
>      }
>  }