Re: [PATCH v2] rust: doc: Clarify safety invariants for Revocable type

[Marcelo Moreira <marcelomoreira1905@gmail.com>]

Hello guys!

Thank you for the continued feedback =)

Based on your point, I'm revising the `# Invariants` section for `Revocable<T>`
to be more precise about when access to `data` is valid. I'm
considering the following wording:

- data is valid if and only if is_available is true.
- Access to data is valid while the RCU read-side lock is held, ensuring no
concurrent revocation, or within the specific scope of the
revoke_internal function
where the revocation logic guarantees exclusive access before dropping data.
 - Once is_available is set to false, further access to data outside
of the revocation
process is disallowed, and the object is dropped either after an RCU
grace period
(in [revoke]), or immediately (in [revoke_nosync]).

I've attempted to clarify that the RCU read-side lock protects against
concurrent
revocation during normal access, but that `revoke_internal` has its own safety
guarantees that allow access without the lock in that specific context. I
'd appreciate your feedback on this revised wording to ensure it accurately
reflects the intended behavior and safety invariants.

Thank you for your patience and guidance. I will send the next version
of the patch soon.

Best regards,
Marcelo Moreira


Em sex., 9 de mai. de 2025 às 07:10, Benno Lossin <lossin@kernel.org> escreveu:
>
> On Sat May 3, 2025 at 4:53 PM CEST, Marcelo Moreira wrote:
> > The Revocable type in rust/kernel/revocable.rs lacked a comprehensive
> > documentation of its safety invariants, specifically regarding the
> > validity of the wrapped data and the necessity of holding the RCU
> > read-side lock for access. This patch addresses this by:
> >
> > - Adding an '# Invariants' section to the documentation of `Revocable<T>`
> >   clarifying that `data` is valid if and only if `is_available` is true,
> >   and that access to `data` requires holding the RCU read-side lock.
> > - Adding '// INVARIANT:' comments in `try_access` and `try_access_with_guard`
> >   to explicitly refer to these invariants before accessing the underlying data.
> > - Adding an '# Invariants' section to the documentation of `RevocableGuard<'_, T>`
> >   documenting that the RCU read-side lock is held for the lifetime of the guard
> >   and that `data_ref` points to valid data during this time.
> > - Updating the safety comment in the `Deref` implementation of `RevocableGuard`
> >   to explicitly mention the relevant invariants.
> >
> > Changes in v2:
>
> The changelog should not be part of the commit message, instead place it
> below the `---`, but before any file diff. It then will only appear in
> the email and not the commit message.
>
> Another thing: could you please CC Danilo Krummrich the next time you
> send this patch? I think he should also take a look at this.
>
> >
> > - Refined the wording of the invariants in `Revocable<T>` to be more direct
> >   and address feedback regarding the phrase 'must occur'.
> > - Added '// INVARIANT:' comments in `try_access` and `try_access_with_guard`
> >   as suggested by reviewers.
> > - Added the missing invariant for `RevocableGuard<'_, T>` regarding the
> >   validity of `data_ref`.
> > - Updated the safety comment in the `Deref` implementation of `RevocableGuard`
> >   to refer to the new invariant.
> >
> > Reported-by: Benno Lossin <benno.lossin@proton.me>
> > Closes: https://github.com/Rust-for-Linux/linux/issues/1160
> > Suggested-by: Miguel Ojeda <ojeda@kernel.org>
> > Signed-off-by: Marcelo Moreira <marcelomoreira1905@gmail.com>
> > ---
>
> ^^ right here would be the place for the changelog.
>
> >  rust/kernel/revocable.rs | 29 ++++++++++++++++-------------
> >  1 file changed, 16 insertions(+), 13 deletions(-)
>
> It seems to me that you sent this patch on top of your previous one [1].
> Normally, one doesn't do that and instead sends the patch based on a tag
> (like `v6.15-rc4`) or the subsystems `-next` branch (so in our case
> `rust-next`). So a new version should not rely on any previous one.
>
> [1]: https://lore.kernel.org/all/20250501005726.744027-1-marcelomoreira1905@gmail.com
>
> > diff --git a/rust/kernel/revocable.rs b/rust/kernel/revocable.rs
> > index 2da3e9460c07..7ef2f34782b4 100644
> > --- a/rust/kernel/revocable.rs
> > +++ b/rust/kernel/revocable.rs
> > @@ -64,12 +64,11 @@
> >  ///
> >  /// # Invariants
> >  ///
> > -/// - The wrapped object `data` is valid if and only if `is_available` is `true`.
> > -/// - Access to `data` must occur only while holding the RCU read-side lock (e.g., via
> > -///   [`Revocable::try_access`] or [`Revocable::try_access_with_guard`]).
> > -/// - Once `is_available` is set to `false`, further access to `data` is disallowed,
> > -///   and the object is dropped either after an RCU grace period (in [`revoke`]),
> > -///   or immediately (in [`revoke_nosync`]).
> > +/// - `data` is valid if and only if `is_available` is true.
> > +/// - Access to `data` requires holding the RCU read-side lock.
>
> I'm not sure what the correct wording here should be. The current
> wording makes the `revoke_internal` function illegal, as it doesn't hold
> the read-side lock, but still accesses `data`.
>
> Maybe @Danilo can help here, but as I understand it, the value in `data`
> is valid for as long as the rcu read-side lock is held *and* if
> `is_available` was true at some point while holding the lock.
>
> > +/// - Once is_available is set to false, further access to data is disallowed,
> > +///   and the object is dropped either after an RCU grace period (in [revoke]),
> > +///   or immediately (in [revoke_nosync]).
>
> Several missing `
>
> >  #[pin_data(PinnedDrop)]
> >  pub struct Revocable<T> {
> >      is_available: AtomicBool,
> > @@ -106,8 +105,9 @@ pub fn new(data: impl PinInit<T>) -> impl PinInit<Self> {
> >      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.
> > +            // INVARIANT: Since `self.is_available` is true, `self.data` is valid. The
> > +            // RCU read-side lock held by `guard` ensures that `self.data` remains valid for
> > +            // the lifetime of the guard.
> >              Some(RevocableGuard::new(self.data.get(), guard))
> >          } else {
> >              None
> > @@ -124,8 +124,10 @@ 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:
>
> Empty SAFETY comment?
>
> > +            // INVARIANT: Since `self.is_available` is true, `self.data` is valid. The
> > +            // RCU read-side lock held by `_guard` ensures that `self.data` remains valid for
> > +            // the lifetime of the returned reference.
> >              Some(unsafe { &*self.data.get() })
> >          } else {
> >              None
> > @@ -200,7 +202,8 @@ fn drop(self: Pin<&mut Self>) {
> >  ///
> >  /// # Invariants
> >  ///
> > -/// The RCU read-side lock is held while the guard is alive.
> > +/// The RCU read-side lock is held for the lifetime of this guard.
> > +/// `data_ref` points to valid data for the lifetime of this guard.
>
> Please use a bullet point list.
>
> ---
> Cheers,
> Benno
>
> >  pub struct RevocableGuard<'a, T> {
> >      data_ref: *const T,
> >      _rcu_guard: rcu::Guard,
> > @@ -221,8 +224,8 @@ impl<T> Deref for RevocableGuard<'_, T> {
> >      type Target = T;
> >
> >      fn deref(&self) -> &Self::Target {
> > -        // SAFETY: By the type invariants, we hold the rcu read-side lock, so the object is
> > -        // guaranteed to remain valid.
> > +        // SAFETY: By the invariant of `Revocable`, `self.data_ref` is valid because the
> > +        // RCU read-side lock is held for the lifetime of this guard.
> >          unsafe { &*self.data_ref }
> >      }
> >  }
>