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

Marcelo Moreira
2025-06-12T18:53:11Z

Em qui., 12 de jun. de 2025 às 06:52, Benno Lossin <lossin@kernel.org> escreveu:
>
> 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 :(

Thanks for the feedback. I didn't see this in my testing. Sorry.

I'm going to revert `RevocableGuard` back to the original design. Thanks! :)

>
> @Marcelo: sorry for the extra work.

don't worry! I'm learning =D

Just one thing, I wanted to ask for your opinion on another change in
the patch, specifically in the `try_access` function. I refactored the
logic of an `if/else` block to use
`self.try_access_with_guard(&guard).map(|data|
RevocableGuard::new(data, guard))`.

This change uses `Option::map`, the logic is: "if
`try_access_with_guard` succeeds, transform its result into a
`RevocableGuard` using the given `rcu::Guard`; otherwise, propagate
`None`." As I understand it, this transformation maintains the
original semantics and security.

Do you think keeping this specific change (using `.map()`) is a good
decision for clarity and style, or should I revert it to the explicit
`if/else` block as well?

Thanks Alice :)
Thanks Benno :)

Cheers,
Marcelo Moreira