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

Alice Ryhl
2025-06-12T09:28:31Z

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