rust: list: Add unsafe for container_of

impl_list_item_mod.rs calls container_of() without unsafe blocks at a couple of places. Since container_of() is an unsafe macro / function, the blocks are strictly necessary.

The problem was so far not visible because the "unsafe-op-in-unsafe-fn" check is a linter rather than a compiler check. Rust suppresses lint checks triggered inside of a macro from another crate. Thus, the error becomes only visible once someone from without the core crate tries to use linked lists:

error[E0133]: call to unsafe function core::ptr::mut_ptr::<impl *mut T>::byte_sub is unsafe and requires unsafe block
--> rust/kernel/lib.rs:252:29

    |
252 |           let container_ptr = field_ptr.byte_sub(offset).cast::<$Container>();
    |                               ^^^^^^^^^^^^^^^^^^^^^^^^^^ call to unsafe function
    |
   ::: rust/kernel/drm/jq.rs:98:1
    |
98  | / impl_list_item! {
99  | |     impl ListItem<0> for BasicItem { using ListLinks { self.links }; }
100 | | }
    | |_- in this macro invocation
    |
note: an unsafe function restricts its caller, but its body is safe by default
   --> rust/kernel/list/impl_list_item_mod.rs:216:13
    |
216 |               unsafe fn view_value(me: *mut $crate::list::ListLinks<$num>) -> *const Self {
    |               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
   ::: rust/kernel/drm/jq.rs:98:1
    |
98  | / impl_list_item! {
99  | |     impl ListItem<0> for BasicItem { using ListLinks { self.links }; }
100 | | }
    | |_- in this macro invocation

= note: requested on the command line with -D unsafe-op-in-unsafe-fn = note: this error originates in the macro $crate::container_of which comes from the expansion of the macro impl_list_item

Add unsafe blocks to container_of to fix the issue.

---
Changes in v4:
- Add unsafe comments. (Miguel)

Changes in v3 (was accidentally called "v2" before):
- Tidy up commit message and provide exact reason for the error.
- Add Reviewed-bys.

---
 rust/kernel/list/impl_list_item_mod.rs | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

index 202bc6f97c13..9f9c36051cf6 100644
@@ -217,7 +217,7 @@ unsafe fn view_value(me: *mut $crate::list::ListLinks<$num>) -> *const Self {
                 // SAFETY: `me` originates from the most recent call to `prepare_to_insert`, so it
                 // points at the field `$field` in a value of type `Self`. Thus, reversing that
                 // operation is still in-bounds of the allocation.
-                $crate::container_of!(me, Self, $($field).*)
+                unsafe { $crate::container_of!(me, Self, $($field).*) }
             }
 
             // GUARANTEES:

@@ -242,7 +242,7 @@ unsafe fn post_remove(me: *mut $crate::list::ListLinks<$num>) -> *const Self {
                 // SAFETY: `me` originates from the most recent call to `prepare_to_insert`, so it
                 // points at the field `$field` in a value of type `Self`. Thus, reversing that
                 // operation is still in-bounds of the allocation.
-                $crate::container_of!(me, Self, $($field).*)
+                unsafe { $crate::container_of!(me, Self, $($field).*) }
             }
         }
     )*};

@@ -270,9 +270,10 @@ 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) };
 
-                let container = $crate::container_of!(
+                // SAFETY: By the same reasoning above, `links_field` is a valid pointer.
+                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.
                 let self_ptr = unsafe {

@@ -319,9 +320,11 @@ 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 {
-                let container = $crate::container_of!(
+                // SAFETY: The caller must guarantee that `links_field` is a valid pointer of type
+                // ListLinks.
+                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.
                 let self_ptr = unsafe {

2.49.0

Hmm... Where do these Reviewed-by's come from?

Ah, I see them now -- a copy of this patch was moved into another series:

https://lore.kernel.org/rust-for-linux/[email protected]/

I understand it was an RFC series, and perhaps you didn't intend to land the patch, so addressing the previous feedback wasn't important for the patch series.

Nevertheless, a note about it would have been nice -- we weren't Cc'd for some reason, and there was no notification about the move nor a link to the previous version.

I see Gary and Alice pointed out feedback that I had already given before -- that is why it is important to mention that this was not the first version of the patch or better, mention that there was feedback yet to be addressed. Otherwise, reviewers may end up mentioning again the same feedback, just like it happened here.

    /// # Safety
    ///
    /// The pointer passed to this macro, and the pointer returned by
this macro, must both be in
    /// bounds of the same allocation.

So I don't think this was tested with CLIPPY=1, because there are other safety comments missing even after this is applied.

Please note that CLIPPY=1 must remain clean for all Rust code:

https://rust-for-linux.com/contributing#submit-checklist-addendum

I have pushed to rust-fixes the patch with placeholders to show what is missing.

I am not sure if I will keep it there. I am not too happy introducing // SAFETY: TODO.s, but it isn't worse than the status (the comments were meant to be there), and at least this shows what is missing -- our pre-existing "good first issue" [1] may motivate new contributors to complete them properly.

Cheers,
Miguel

Feel free to drop it for now, I can go through it properly with the list author to see what the most accurate formulation would be.

The commit is in v7.0-rc1 -- please feel free to send the improvements on top of that.

Thanks!

Cheers,
Miguel

(I said it was applied, but for completeness, let me send my usual message here... The commit is in mainline already.).

Applied to rust-fixes -- thanks everyone!

[ As discussed, let's fix the build for those that want to use the
macro within the kernel crate now and we can discuss the proper safety comments afterwards. Thus I removed the ones from the patch.

However, we cannot just avoid the comments with CLIPPY=1, so I provided placeholders for now, like we did in the past. They were also needed for an unsafe impl.

While I am not happy about it, it isn't worse than the current status (the comments were meant to be there), and at least this shows what is missing -- our pre-existing "good first issue" [1] may motivate new contributors to complete them properly.

Finally, I moved one of the existing safety comments one line down so that Clippy could locate it.

- Miguel ]

[ Fixed formatting. Reworded to fix the lint suppression
explanation. Indent build error. - Miguel ]

Cheers,
Miguel