[PATCH] rust: revocable: clarify invariants in safety documentation

[PATCH] rust: revocable: clarify invariants in safety documentation

[Marcelo Moreira]

Clarify the safety invariants for `Revocable<T>`, documenting that `data`
is only valid if `is_available` is `true`, and that access requires
holding the RCU read-side lock.

This helps developers understand when it is safe to access the wrapped
data and the role of RCU in ensuring memory safety.

This is part of issue #1160 ("correct safety comments and documentation
in revocable.rs"), reported by Miguel Ojeda and Benno Lossin.

Reported-by: Miguel Ojeda <ojeda@kernel.org>
Closes: https://github.com/Rust-for-Linux/linux/issues/1160
Reported-by: Benno Lossin <benno.lossin@proton.me>
Closes: https://github.com/Rust-for-Linux/linux/issues/1160
Tested-by: Marcelo Moreira <marcelomoreira1905@gmail.com>
Signed-off-by: Marcelo Moreira <marcelomoreira1905@gmail.com>
---
 rust/kernel/revocable.rs | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/rust/kernel/revocable.rs b/rust/kernel/revocable.rs
index 1e5a9d25c21b..2da3e9460c07 100644
--- a/rust/kernel/revocable.rs
+++ b/rust/kernel/revocable.rs
@@ -61,6 +61,15 @@
 /// v.revoke();
 /// assert_eq!(add_two(&v), None);
 /// ```
+///
+/// # 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`]).
 #[pin_data(PinnedDrop)]
 pub struct Revocable<T> {
     is_available: AtomicBool,
-- 
2.49.0

Re: [PATCH] rust: revocable: clarify invariants in safety documentation

[Miguel Ojeda]

On Thu, May 1, 2025 at 2:57 AM Marcelo Moreira
<marcelomoreira1905@gmail.com> wrote:
>
> This is part of issue #1160 ("correct safety comments and documentation
> in revocable.rs"), reported by Miguel Ojeda and Benno Lossin.
>
> Reported-by: Miguel Ojeda <ojeda@kernel.org>
> Closes: https://github.com/Rust-for-Linux/linux/issues/1160

I think it was just Benno. :)

> Tested-by: Marcelo Moreira <marcelomoreira1905@gmail.com>

Normally one is supposed to test a patch before submitting it, i.e. it
is implicit.

> +/// # Invariants

We also place `// INVARIANT: ...` comments in the places that need to
ensure the type invariants hold, e.g. in constructors.

Given the issue, I think Benno may have also wanted that you then
refer to the type invariants to justify some of the comments, e.g. in
the "Since `self.is_available` is true, ..." ones.

> +/// - Access to `data` must occur only while holding the RCU read-side lock (e.g., vi

The "must occur" wording seems a bit strange for a type invariant, but
maybe Benno had something in mind here?

Thanks for the patch!

Cheers,
Miguel

Re: [PATCH] rust: revocable: clarify invariants in safety documentation

[Benno Lossin]

On Thu May 1, 2025 at 12:18 PM CEST, Miguel Ojeda wrote:
> On Thu, May 1, 2025 at 2:57 AM Marcelo Moreira
> <marcelomoreira1905@gmail.com> wrote:
>>
>> This is part of issue #1160 ("correct safety comments and documentation
>> in revocable.rs"), reported by Miguel Ojeda and Benno Lossin.
>>
>> Reported-by: Miguel Ojeda <ojeda@kernel.org>
>> Closes: https://github.com/Rust-for-Linux/linux/issues/1160
>
> I think it was just Benno. :)
>
>> Tested-by: Marcelo Moreira <marcelomoreira1905@gmail.com>
>
> Normally one is supposed to test a patch before submitting it, i.e. it
> is implicit.
>
>> +/// # Invariants
>
> We also place `// INVARIANT: ...` comments in the places that need to
> ensure the type invariants hold, e.g. in constructors.
>
> Given the issue, I think Benno may have also wanted that you then
> refer to the type invariants to justify some of the comments, e.g. in
> the "Since `self.is_available` is true, ..." ones.

Indeed, we also need the comments that establish/make use of the
invariant. For example in the `try_access_with_guard` function.

There also is a missing invariant on `RevocableGuard` that the
`data_ref` pointer is valid.

>> +/// - Access to `data` must occur only while holding the RCU read-side lock (e.g., vi
>
> The "must occur" wording seems a bit strange for a type invariant, but
> maybe Benno had something in mind here?

Yeah, I don't think that using RCU is even a hard requirement. One could
also call `revoke_nosync`, since one knows the revocable is only used by
a single kernel thread. I also don't think that it's needed for memory
correctness, but I could be wrong there.

The most important invariant is that `data` is valid while
`is_available` is true. That's what's needed by the different functions
accessing it.

---
Cheers,
Benno

Re: [PATCH] rust: revocable: clarify invariants in safety documentation

[Marcelo Moreira]

Hey Miguel, hey Benno,

Thanks a lot for the feedback! =)

I now get what you meant about the invariants: it's not just about
documenting them at the top, but also about adding // INVARIANT: ...
comments in the code where they actually matter — like in the
constructors and in try_access_with_guard.

Benno, thanks for pointing out the missing invariant in
RevocableGuard. I’ll add a comment explaining that data_ref is only
valid when is_available is true.

And regarding RCU, your comments made things clearer. Instead of
saying it's a hard requirement, I’ll reword it to focus on the
is_available check as the actual safety condition, and just mention
RCU as a typical way to ensure safety — but not the only one.

I’ll get v2 ready with those improvements and send it soon.

Cheers,
Marcelo M.


Em sex., 2 de mai. de 2025 às 04:33, Benno Lossin <lossin@kernel.org> escreveu:
>
> On Thu May 1, 2025 at 12:18 PM CEST, Miguel Ojeda wrote:
> > On Thu, May 1, 2025 at 2:57 AM Marcelo Moreira
> > <marcelomoreira1905@gmail.com> wrote:
> >>
> >> This is part of issue #1160 ("correct safety comments and documentation
> >> in revocable.rs"), reported by Miguel Ojeda and Benno Lossin.
> >>
> >> Reported-by: Miguel Ojeda <ojeda@kernel.org>
> >> Closes: https://github.com/Rust-for-Linux/linux/issues/1160
> >
> > I think it was just Benno. :)
> >
> >> Tested-by: Marcelo Moreira <marcelomoreira1905@gmail.com>
> >
> > Normally one is supposed to test a patch before submitting it, i.e. it
> > is implicit.
> >
> >> +/// # Invariants
> >
> > We also place `// INVARIANT: ...` comments in the places that need to
> > ensure the type invariants hold, e.g. in constructors.
> >
> > Given the issue, I think Benno may have also wanted that you then
> > refer to the type invariants to justify some of the comments, e.g. in
> > the "Since `self.is_available` is true, ..." ones.
>
> Indeed, we also need the comments that establish/make use of the
> invariant. For example in the `try_access_with_guard` function.
>
> There also is a missing invariant on `RevocableGuard` that the
> `data_ref` pointer is valid.
>
> >> +/// - Access to `data` must occur only while holding the RCU read-side lock (e.g., vi
> >
> > The "must occur" wording seems a bit strange for a type invariant, but
> > maybe Benno had something in mind here?
>
> Yeah, I don't think that using RCU is even a hard requirement. One could
> also call `revoke_nosync`, since one knows the revocable is only used by
> a single kernel thread. I also don't think that it's needed for memory
> correctness, but I could be wrong there.
>
> The most important invariant is that `data` is valid while
> `is_available` is true. That's what's needed by the different functions
> accessing it.
>
> ---
> Cheers,
> Benno