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

Benno Lossin
2025-06-12T09:52:35Z

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