[PATCH v2 4/4] rust: workqueue: document safety of Pin<KBox<T>> work item impls

[Cian McGuire]

The `WorkItemPointer` and `RawWorkItem` impls for `Pin<KBox<T>>` had
their SAFETY comments left as "TODO.". Both traits are already
implemented for `Arc<T>` earlier in this file with real justifications;
these two impls follow the same shape, adapted for `KBox`.

`WorkItemPointer`'s contract requires that `__enqueue` use a
`work_struct` initialized with this impl's `run` as the function
pointer. This holds regardless of which smart pointer type is involved:
the only safe ways to produce that `work_struct` are `Work::new` and
`DelayedWork::new` (reached via `HasDelayedWork`'s `raw_as_work`), and
both are generic over any `T: WorkItem<ID>` and always bake in
`T::Pointer::run`, so for a `T` bound by `WorkItem<ID, Pointer = Self>`
either one bakes in exactly this impl's `run`. The argument is identical
to the existing `Arc<T>` comment and does not depend on anything
Arc-specific.

`RawWorkItem`'s contract requires that pointers passed to the
`queue_work_on` closure remain valid for the duration `__enqueue`
guarantees. This one is not a direct copy of the `Arc<T>` case, because
the two `__enqueue` implementations differ: `__enqueue` here leaks the
box via `KBox::into_raw` before calling the closure, so the pointer is
valid for the call. If the closure returns true, the pointer remains
valid until reclaimed in `WorkItemPointer::run` via a matching
`KBox::from_raw`. If the closure returns false, `__enqueue`'s own safety
requirement only permits that when the `work_struct` is already
enqueued elsewhere -- but `self` is an exclusively-owned `Pin<KBox<T>>`,
and ownership is only ever given up by enqueuing it, so it cannot
already be enqueued while this call still holds it. That branch is
therefore genuinely unreachable, matching the `unreachable_unchecked`
call.

Suggested-by: Miguel Ojeda <ojeda@xxxxxxxxxx>
Link: https://github.com/Rust-for-Linux/linux/issues/351
Signed-off-by: Cian McGuire <cianmcguire167@xxxxxxxxx>
---
v2:
- The `WorkItemPointer` comment claimed `Work::new` was the only safe
  way to construct the backing `work_struct`; it now also credits
  `DelayedWork::new` (via `HasDelayedWork`'s `raw_as_work`) as a second
  valid path, and updates the closing sentence to match.

 rust/kernel/workqueue.rs | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/rust/kernel/workqueue.rs b/rust/kernel/workqueue.rs
index 7e253b6f299c..2a6bf0b9b83c 100644
--- a/rust/kernel/workqueue.rs
+++ b/rust/kernel/workqueue.rs
@@ -881,7 +881,16 @@ unsafe impl<T, const ID: u64> RawDelayedWorkItem<ID> for Arc<T>
 {
 }
 
-// SAFETY: TODO.
+// SAFETY: The `__enqueue` implementation in `RawWorkItem` uses a `work_struct` initialized
+// with the `run` method of this trait as the function pointer because:
+//   - `__enqueue` gets the `work_struct` from the `Work` field, using `T::raw_get_work`.
+//   - The only safe ways to produce that `work_struct` are `Work::new` and `DelayedWork::new`
+//     (via `HasDelayedWork`'s `raw_as_work`), and both pass `T::Pointer::run` to
+//     `init_work_with_key`.
+//   - Finally `Work` and `RawWorkItem` guarantee that the correct `Work` field will be used
+//     because of the ID const generic bound. This makes sure that `T::raw_get_work` uses the
+//     correct offset for the `Work` field, and the matching constructor picks the correct
+//     implementation of `WorkItemPointer` for `Pin<KBox<T>>`.
 unsafe impl<T, const ID: u64> WorkItemPointer<ID> for Pin<KBox<T>>
 where
     T: WorkItem<ID, Pointer = Self>,
@@ -901,7 +910,14 @@ unsafe impl<T, const ID: u64> WorkItemPointer<ID> for Pin<KBox<T>>
     }
 }
 
-// SAFETY: TODO.
+// SAFETY: `__enqueue` leaks the box via `KBox::into_raw` before calling `queue_work_on`, so
+// the pointer is valid for the duration of that call. If the closure returns true, the
+// pointer remains valid until reclaimed in `WorkItemPointer::run`, which reconstructs the box
+// with a matching `KBox::from_raw`. If the closure returns false, `__enqueue`'s own safety
+// requirement only permits that when the `work_struct` is already in a workqueue -- but
+// `self` is an exclusively-owned `Pin<KBox<T>>`, and ownership is only ever given up by
+// enqueuing it, so it cannot already be enqueued elsewhere while we still hold it here. That
+// branch is therefore unreachable, matching the `unreachable_unchecked` call.
 unsafe impl<T, const ID: u64> RawWorkItem<ID> for Pin<KBox<T>>
 where
     T: WorkItem<ID, Pointer = Self>,
-- 
2.55.0