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

[Marcelo Moreira <marcelomoreira1905@gmail.com>]

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