Re: [PATCH v4 2/3] rust: revocable: simplify RevocableGuard for internal safety

[Alice Ryhl <aliceryhl@google.com>]

On Mon, Jun 02, 2025 at 08:26:23PM -0300, Marcelo Moreira wrote:
> This commit refactors `RevocableGuard` to hold a direct reference
> (`&'a T`) instead of a raw pointer (`*const T`). This makes the guard
> internally safe, reducing the need for `unsafe` blocks in its usage
> and simplifying its implementation.
> 
> The `try_access` function is updated to leverage `try_access_with_guard`
> and `map` to construct the `RevocableGuard` in a more idiomatic and safe
> Rust way, avoiding manual pointer operations. The associated invariants
> and `SAFETY` comments for `RevocableGuard` itself are removed as its
> safety is now guaranteed by its type definition.
> 
> 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 | 26 ++++++--------------------
>  1 file changed, 6 insertions(+), 20 deletions(-)
> 
> diff --git a/rust/kernel/revocable.rs b/rust/kernel/revocable.rs
> index d14f9052f1ac..43cc9bdc94f4 100644
> --- a/rust/kernel/revocable.rs
> +++ b/rust/kernel/revocable.rs
> @@ -105,13 +105,7 @@ pub fn new(data: impl PinInit<T>) -> impl PinInit<Self> {
>      /// because another CPU may be waiting to complete the revocation of this object.
>      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))
> -        } else {
> -            None
> -        }
> +        self.try_access_with_guard(&guard).map(|data| RevocableGuard::new(data, guard))
>      }
>  
>      /// Tries to access the revocable wrapped object.
> @@ -198,22 +192,16 @@ fn drop(self: Pin<&mut Self>) {
>  ///
>  /// CPUs may not sleep while holding on to [`RevocableGuard`] because it's in atomic context
>  /// holding the RCU read-side lock.
> -///
> -/// # Invariants
> -///
> -/// The RCU read-side lock is held while the guard is alive.
>  pub struct RevocableGuard<'a, T> {
> -    data_ref: *const T,
> +    data: &'a T,
>      _rcu_guard: rcu::Guard,
> -    _p: PhantomData<&'a ()>,
>  }

I don't think this change is valid. Consider this code:

fn takes_guard(arg: RevocableGuard<'_, i32>) {
    drop(arg);
    // rcu guard is dropped, so `arg.data` may become dangling now
}

This violates the requirement that references that appear in function
arguments are valid for the entire function call, see:
https://perso.crans.org/vanille/treebor/protectors.html

Or the LLVM perspective: When Rust sees a reference in a function
argument, it adds the LLVM attribute dereferencable to it, which implies
that the pointer must be valid for *the entire function call*. If the
memory becomes dangling after the rcu guard is dropped, then this is
violated and the compiler could perform optimizations that are not
correct.

Alice