Re: [PATCH v4 1/3] rust: revocable: update write invariant and fix safety comments

[Marcelo Moreira <marcelomoreira1905@gmail.com>]

Em qui., 12 de jun. de 2025 às 06:02, Benno Lossin <lossin@kernel.org> escreveu:
>
> On Tue Jun 3, 2025 at 1:26 AM CEST, Marcelo Moreira wrote:
> > This commit clarifies the write invariant of the `Revocable` type and
> > updates associated `SAFETY` comments. The write invariant now precisely
> > states that `data` is valid for writes after `is_available` transitions
> > from true to false, provided no thread holding an RCU read-side lock
> > (acquired before the change) still has access to `data`.
> >
> > The `SAFETY` comment in `try_access_with_guard` is updated to reflect
> > this invariant, and the `PinnedDrop` `drop` implementation's `SAFETY`
> > comment is refined to clearly state the guarantees provided by the `&mut Self`
> > context regarding exclusive access and `data`'s validity for dropping.
> >
> > Reported-by: Benno Lossin <lossin@kernel.org>
> > Closes: https://github.com/Rust-for-Linux/linux/issues/1160
> > Suggested-by: Benno Lossin <lossin@kernel.org>
> > Suggested-by: Danilo Krummrich <dakr@kernel.org>
> > Signed-off-by: Marcelo Moreira <marcelomoreira1905@gmail.com>
>
> I would have done this change after the other two, since then you can
> change all the safety comments here, but if Danilo is fine with doing it
> this way, then you can leave it this way.
>

ok, I'll wait for Danilo's answer :)

> The changes are almost perfect now, just some small formatting changes
> below. With those fixed, you may add:
>
> Reviewed-by: Benno Lossin <lossin@kernel.org>

ok :)

>
> > ---
> >  rust/kernel/revocable.rs | 20 +++++++++++++++-----
> >  1 file changed, 15 insertions(+), 5 deletions(-)
> >
> > diff --git a/rust/kernel/revocable.rs b/rust/kernel/revocable.rs
> > index 1e5a9d25c21b..d14f9052f1ac 100644
> > --- a/rust/kernel/revocable.rs
> > +++ b/rust/kernel/revocable.rs
> > @@ -61,6 +61,15 @@
> >  /// v.revoke();
> >  /// assert_eq!(add_two(&v), None);
> >  /// ```
> > +///
> > +/// # Invariants
> > +///
> > +/// - `data` is valid for reads in two cases:
> > +///   - while `is_available` is true, or
> > +///   - while the RCU read-side lock is taken and it was acquired while `is_available` was `true`.
> > +/// - `data` is valid for writes when `is_available` was atomically changed from `true` to `false`
> > +///   and no thread that has access to `data` is holding an RCU read-side lock that was acquired prior to
> > +///   the change in `is_available`.
> >  #[pin_data(PinnedDrop)]
> >  pub struct Revocable<T> {
> >      is_available: AtomicBool,
> > @@ -115,8 +124,8 @@ pub fn try_access(&self) -> Option<RevocableGuard<'_, T>> {
> >      /// object.
> >      pub fn try_access_with_guard<'a>(&'a self, _guard: &'a rcu::Guard) -> Option<&'a T> {
> >          if self.is_available.load(Ordering::Relaxed) {
> > -            // SAFETY: 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.
> > +            // SAFETY: `Self::data` is valid for reads because of `Self`'s type invariants,
>
> s/Self::data/self.data/

ok

>
> > +            // as `Self::is_available` is true and `_guard` holds the RCU read-side lock
>
> s/Self::is_available/self.is_available/

ok

>
> Missing `.` at the end.

ok, and sorry for this hehe . . .

>
> >              Some(unsafe { &*self.data.get() })
> >          } else {
> >              None
> > @@ -176,9 +185,10 @@ fn drop(self: Pin<&mut Self>) {
> >          // SAFETY: We are not moving out of `p`, only dropping in place
> >          let p = unsafe { self.get_unchecked_mut() };
> >          if *p.is_available.get_mut() {
> > -            // SAFETY: We know `self.data` is valid because no other CPU has changed
> > -            // `is_available` to `false` yet, and no other CPU can do it anymore because this CPU
> > -            // holds the only reference (mutable) to `self` now.
> > +            // SAFETY: `Self::data` is valid for writes because of `Self`'s type invariants,
>
> s/Self::data/self.data/
>
> > +            // and because this `PinnedDrop` context (having `&mut Self`) guarantees exclusive access,
> > +            // ensuring no other thread can concurrently access or revoke `data`.
> > +            // This ensures `data` is valid for `drop_in_place`.
>
> This last sentence is not needed. Instead, we should mention that since
> this is a `drop` function, we're only called once, so it's fine to call
> drop (since that also is only allowed to be called once).
>

ok, I thought of something like:

// SAFETY: `self.data` is valid for writes because of `Self`'s type invariants.
// The `&mut Self` context guarantees exclusive access and a single
call to `drop`,
// making `self.data` valid for `drop_in_place`.


Thanks Benno! (^_^)b

Cheers,
Marcelo Moreira