[PATCH v3] rust: revocable: Correct safety comments and add invariants

Marcelo Moreira
2025-05-26T02:01:33Z

Introducing a comprehensive `# Invariants` section for the `Revocable<T>`
type. This new documentation details the validity conditions of the `data`
field concerning `is_available` and RCU read-side locks.

The safety comments in `Revocable::try_access`,
`Revocable::try_access_with_guard`, and the `PinnedDrop` implementation
for `Revocable<T>` have been updated to explicitly reference these newly
defined invariants, ensuring that the justification for unsafe operations
is clear and directly tied to the type's guaranteed properties.

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>
---
Changes in v3:
- Refined the wording of the `Revocable<T>` invariants to be more precise
about read and write validity conditions, specifically including RCU
read-side lock acquisition timing for reads and RCU grace period for writes.
- Simplified the `try_access_with_guard` safety comment for better conciseness.
- Removed changes related to `RevocableGuard` invariants and its `Deref`
implementation, as these are planned for a separate, future patch.
- Reverted `revoke_internal` changes (like `else` block for `revoke_nosync`
and `swap` correction) to be part of a separate refactoring patch.
---
rust/kernel/revocable.rs | 25 ++++++++++++++++++-------
1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/rust/kernel/revocable.rs b/rust/kernel/revocable.rs
index 1e5a9d25c21b..fed860ad2e60 100644
--- a/rust/kernel/revocable.rs
+++ b/rust/kernel/revocable.rs
@@ -61,6 +61,14 @@
/// 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 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,
@@ -97,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: `self.data` is valid for reads because `self.is_available` is true,
+ // and the RCU read-side lock held by `guard` ensures this condition is maintained
+ // during access.
Some(RevocableGuard::new(self.data.get(), guard))
} else {
None
@@ -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 as `is_available` is true and `_guard`
+ // holds the RCU read-side lock, adhering to `Revocable<T>`'s invariants.
Some(unsafe { &*self.data.get() })
} else {
None
@@ -176,9 +185,11 @@ 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.
+ // INVARIANT: `is_available` is true, so `data` is valid for reads.
+ // SAFETY: `self.data` is valid for writes because `is_available` is true, and
+ // 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`.
unsafe { drop_in_place(p.data.get()) };
}
}
--
2.49.0