[PATCH] rust: list: document safety discharge and add offset_of! check

[Yilin Chen]

Replace the remaining "// Safety: TODO" in the Rust intrusive list
implementation with explanations.

Also, the `impl_has_list_links_self_ptr!` macro does not perform
the field-path validation already used by `impl_has_list_links!`.
As a result, `addr_of_mut!` also accepts paths that require
implicit dereferencing.

Use offset_of! in an unreachable branch to ensure that the path consists
only of actual nested fields. The value is not evaluated, so the check
has no runtime cost.

Link: https://rust-for-linux.zulipchat.com/#narrow/channel/288089-General/topic/Should.20.60impl_has_list_links_self_ptr.21.60.20add.20offset_of.21.20macro.3F/with/624615061

Signed-off-by: Yilin Chen <1479826151@xxxxxx>
---

I don't know whether it is designed for allowing that format. But I think
it is better to restrict the field path to embedded in Self, so I added
offset_of! check to ensure that.

I opened a topic in Zulip to discuss this but nobody has responded yet.
The link is in the commit message.

Looking for feedback from the maintainers about whether this is
a good idea. Thank you for your feedback.

 rust/kernel/list.rs                    | 10 ++++++-
 rust/kernel/list/impl_list_item_mod.rs | 36 ++++++++++++++++++++------
 2 files changed, 37 insertions(+), 9 deletions(-)

diff --git a/rust/kernel/list.rs b/rust/kernel/list.rs
index 0f367264ee2e..28e109750968 100644
--- a/rust/kernel/list.rs
+++ b/rust/kernel/list.rs
@@ -599,7 +599,15 @@ pub fn pop_front(&mut self) -> Option<ListArc<T, ID>> {
     ///
     /// `item` must not be in a different linked list (with the same id).
     pub unsafe fn remove(&mut self, item: &T) -> Option<ListArc<T, ID>> {
-        // SAFETY: TODO.
+        // SAFETY:
+        // - `item` is a valid reference to a value of type `T`, satisfying the requirement of
+        //   `ListItem::view_links`.
+        // - If the item is not in a list, `ListItem::view_links` guarantees that the
+        //   returned pointer points at a read-only `ListLinks` with two null pointers,
+        //   which is dereferenceable.
+        // - If the item is in a list, `ListItem::view_links` guarantees that the
+        //   returned pointer is the same as the most recent `ListItem::prepare_to_insert`,
+        //   which is dereferenceable.
         let mut item = unsafe { ListLinks::fields(T::view_links(item)) };
         // SAFETY: The user provided a reference, and reference are never dangling.
         //
diff --git a/rust/kernel/list/impl_list_item_mod.rs b/rust/kernel/list/impl_list_item_mod.rs
index 5a3eac9f3cf0..87ce54d9215b 100644
--- a/rust/kernel/list/impl_list_item_mod.rs
+++ b/rust/kernel/list/impl_list_item_mod.rs
@@ -82,16 +82,26 @@ pub unsafe trait HasSelfPtr<T: ?Sized, const ID: u64 = 0>
        for $self:ty
        { self$(.$field:ident)* }
     )*) => {$(
-        // SAFETY: The implementation of `raw_get_list_links` only compiles if the field has the
-        // right type.
+        // SAFETY: The implementation of `raw_get_list_links` returns the `inner` field of the
+        // `ListLinksSelfPtr<$item_type, ID>` selected by the field path.
         unsafe impl$(<$($generics)*>)? $crate::list::HasSelfPtr<$item_type $(, $id)?> for $self {}
 
-        // SAFETY: TODO.
+        // SAFETY: The implementation of `raw_get_list_links` only compiles if the field has type
+        // `ListLinksSelfPtr<$item_type, ID>`. Since `ListLinksSelfPtr` is `repr(C)` and `inner` is
+        // its first field, a pointer to the former is also a pointer to the latter.
         unsafe impl$(<$($generics)*>)? $crate::list::HasListLinks$(<$id>)? for $self {
             #[inline]
             unsafe fn raw_get_list_links(ptr: *mut Self) -> *mut $crate::list::ListLinks$(<$id>)? {
+                // Statically ensure that `$(.field)*` doesn't follow any pointers.
+                //
+                // Cannot be `const` because `$self` may contain generics and E0401 says constants
+                // "can't use {`Self`,generic parameters} from outer item".
+                if false { let _: usize = ::core::mem::offset_of!(Self, $($field).*); }
+
                 let ptr: *mut $crate::list::ListLinksSelfPtr<$item_type $(, $id)?> =
-                    // SAFETY: The caller promises that the pointer is not dangling.
+                    // SAFETY: The caller promises that the pointer points at a valid `Self`. We
+                    // know that this expression doesn't follow any pointers, as the `offset_of!`
+                    // invocation above would otherwise not compile.
                     unsafe { ::core::ptr::addr_of_mut!((*ptr)$(.$field)*) };
                 ptr.cast()
             }
@@ -274,14 +284,18 @@ unsafe fn prepare_to_insert(me: *const Self) -> *mut $crate::list::ListLinks<$nu
                 // SAFETY: The caller promises that `me` points at a valid value of type `Self`.
                 let links_field = unsafe { <Self as $crate::list::ListItem<$num>>::view_links(me) };
 
-                // SAFETY: TODO.
+                // SAFETY: `links_field` was returned by `view_links`, which forwards the pointer
+                // returned by `HasListLinks::raw_get_list_links`. `HasSelfPtr` guarantees that
+                // this `ListLinks` is the `inner` field of a `ListLinksSelfPtr<Self, $num>`.
+                // Therefore, the resulting container pointer is in bounds of the same allocation.
                 let container = unsafe {
                     $crate::container_of!(
                         links_field, $crate::list::ListLinksSelfPtr<Self, $num>, inner
                     )
                 };
 
-                // SAFETY: By the same reasoning above, `links_field` is a valid pointer.
+                // SAFETY: By the reasoning above, `container` points at a valid
+                // `ListLinksSelfPtr<Self, $num>`.
                 let self_ptr = unsafe {
                     $crate::list::ListLinksSelfPtr::raw_get_self_ptr(container)
                 };
@@ -326,14 +340,20 @@ unsafe fn view_links(me: *const Self) -> *mut $crate::list::ListLinks<$num> {
             //   `ListArc` containing `Self` until the next call to `post_remove`. The value cannot
             //   be destroyed while a `ListArc` reference exists.
             unsafe fn view_value(links_field: *mut $crate::list::ListLinks<$num>) -> *const Self {
-                // SAFETY: TODO.
+                // SAFETY: The caller guarantees that `links_field` either originated from the
+                // most recent `prepare_to_insert` or was returned by a subsequent `view_links`,
+                // and that `post_remove` has not been called since. In either case, the pointer
+                // originated from `HasListLinks::raw_get_list_links`, and `HasSelfPtr` guarantees
+                // that this `ListLinks` is the `inner` field of a `ListLinksSelfPtr<Self, $num>`.
+                // Therefore, the resulting container pointer is in bounds of the same allocation.
                 let container = unsafe {
                     $crate::container_of!(
                         links_field, $crate::list::ListLinksSelfPtr<Self, $num>, inner
                     )
                 };
 
-                // SAFETY: By the same reasoning above, `links_field` is a valid pointer.
+                // SAFETY: By the reasoning above, `container` points at a valid
+                // `ListLinksSelfPtr<Self, $num>`.
                 let self_ptr = unsafe {
                     $crate::list::ListLinksSelfPtr::raw_get_self_ptr(container)
                 };
-- 
2.25.1