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

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

On Thu Jun 12, 2025 at 11:28 AM CEST, Alice Ryhl wrote:
> 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

That's a good point. I don't see a way to make this work then... That's
a bit sad :(

@Marcelo: sorry for the extra work.

---
Cheers,
Benno