[PATCH] rust: kernel: update `container_of` to not require unsafe blocks

[PATCH] rust: kernel: update `container_of` to not require unsafe blocks

[Florian Rädiker]

The calculation done by `container_of` does not have to be unsafe, so
change this.

Inspired by [0] by Wedson Almeida Filho <walmeida@microsoft.com>

[0] https://github.com/Rust-for-Linux/linux/commit/ecc54b2bbd08f62f8cd16420d3d6c60b517a05cd

Signed-off-by: Florian Rädiker <git@flra.eu>
---

Hi,

this is my first-ever patch submission :)
I thought this might serve as a "good first issue".

Best,
flra

 rust/kernel/lib.rs    | 13 ++++++------
 rust/kernel/rbtree.rs | 46 +++++++++++++++++++++----------------------
 2 files changed, 30 insertions(+), 29 deletions(-)

diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 545d1170ee63..343d8b9ea32e 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -153,8 +153,8 @@ fn panic(info: &core::panic::PanicInfo<'_>) -> ! {
 ///
 /// # Safety
 ///
-/// The pointer passed to this macro, and the pointer returned by this macro, must both be in
-/// bounds of the same allocation.
+/// This operation itself is always safe, but using the result is only safe if the resulting
+/// pointer, and the pointer passed to this macro, are in bounds of the same allocation.
 ///
 /// # Examples
 ///
@@ -167,9 +167,9 @@ fn panic(info: &core::panic::PanicInfo<'_>) -> ! {
 ///
 /// let test = Test { a: 10, b: 20 };
 /// let b_ptr = &test.b;
-/// // SAFETY: The pointer points at the `b` field of a `Test`, so the resulting pointer will be
-/// // in-bounds of the same allocation as `b_ptr`.
-/// let test_alias = unsafe { container_of!(b_ptr, Test, b) };
+/// // The pointer points at the `b` field of a `Test`, so the resulting pointer will be in-bounds
+/// // of the same allocation as `b_ptr`.
+/// let test_alias = container_of!(b_ptr, Test, b);
 /// assert!(core::ptr::eq(&test, test_alias));
 /// ```
 #[macro_export]
@@ -177,7 +177,8 @@ macro_rules! container_of {
     ($ptr:expr, $type:ty, $($f:tt)*) => {{
         let ptr = $ptr as *const _ as *const u8;
         let offset: usize = ::core::mem::offset_of!($type, $($f)*);
-        ptr.sub(offset) as *const $type
+        $crate::build_assert!(offset <= isize::MAX as usize);
+        ptr.wrapping_sub(offset) as *const $type
     }}
 }
 
diff --git a/rust/kernel/rbtree.rs b/rust/kernel/rbtree.rs
index ee2731dad72d..f61d7f80c2ca 100644
--- a/rust/kernel/rbtree.rs
+++ b/rust/kernel/rbtree.rs
@@ -328,8 +328,8 @@ fn raw_entry(&mut self, key: &K) -> RawEntry<'_, K, V> {
             unsafe { &mut (*raw_self).root.rb_node };
         while !(*child_field_of_parent).is_null() {
             let curr = *child_field_of_parent;
-            // SAFETY: All links fields we create are in a `Node<K, V>`.
-            let node = unsafe { container_of!(curr, Node<K, V>, links) };
+            // All links fields we create are in a `Node<K, V>`, so `node` is safe to dereference.
+            let node = container_of!(curr, Node<K, V>, links);
 
             // SAFETY: `node` is a non-null node so it is valid by the type invariants.
             match key.cmp(unsafe { &(*node).key }) {
@@ -375,9 +375,9 @@ pub fn find_mut(&mut self, key: &K) -> Option<OccupiedEntry<'_, K, V>> {
     pub fn get(&self, key: &K) -> Option<&V> {
         let mut node = self.root.rb_node;
         while !node.is_null() {
-            // SAFETY: By the type invariant of `Self`, all non-null `rb_node` pointers stored in `self`
-            // point to the links field of `Node<K, V>` objects.
-            let this = unsafe { container_of!(node, Node<K, V>, links) };
+            // By the type invariant of `Self`, all non-null `rb_node` pointers stored in `self`
+            // point to the links field of `Node<K, V>` objects, so `this` is safe to dereference.
+            let this = container_of!(node, Node<K, V>, links);
             // SAFETY: `this` is a non-null node so it is valid by the type invariants.
             node = match key.cmp(unsafe { &(*this).key }) {
                 // SAFETY: `node` is a non-null node so it is valid by the type invariants.
@@ -422,9 +422,9 @@ pub fn cursor_lower_bound(&mut self, key: &K) -> Option<Cursor<'_, K, V>>
         let mut node = self.root.rb_node;
         let mut best_match: Option<NonNull<Node<K, V>>> = None;
         while !node.is_null() {
-            // SAFETY: By the type invariant of `Self`, all non-null `rb_node` pointers stored in `self`
-            // point to the links field of `Node<K, V>` objects.
-            let this = unsafe { container_of!(node, Node<K, V>, links) }.cast_mut();
+            // By the type invariant of `Self`, all non-null `rb_node` pointers stored in `self`
+            // point to the links field of `Node<K, V>` objects, so `this` is safe to dereference.
+            let this = container_of!(node, Node<K, V>, links).cast_mut();
             // SAFETY: `this` is a non-null node so it is valid by the type invariants.
             let this_key = unsafe { &(*this).key };
             // SAFETY: `node` is a non-null node so it is valid by the type invariants.
@@ -485,8 +485,8 @@ fn drop(&mut self) {
 
         // INVARIANT: The loop invariant is that all tree nodes from `next` in postorder are valid.
         while !next.is_null() {
-            // SAFETY: All links fields we create are in a `Node<K, V>`.
-            let this = unsafe { container_of!(next, Node<K, V>, links) };
+            // All links fields we create are in a `Node<K, V>`, so `this` is safe to use.
+            let this = container_of!(next, Node<K, V>, links);
 
             // Find out what the next node is before disposing of the current one.
             // SAFETY: `next` and all nodes in postorder are still valid.
@@ -759,9 +759,9 @@ pub fn current_mut(&mut self) -> (&K, &mut V) {
     pub fn remove_current(self) -> (Option<Self>, RBTreeNode<K, V>) {
         let prev = self.get_neighbor_raw(Direction::Prev);
         let next = self.get_neighbor_raw(Direction::Next);
-        // SAFETY: By the type invariant of `Self`, all non-null `rb_node` pointers stored in `self`
-        // point to the links field of `Node<K, V>` objects.
-        let this = unsafe { container_of!(self.current.as_ptr(), Node<K, V>, links) }.cast_mut();
+        // By the type invariant of `Self`, all non-null `rb_node` pointers stored in `self`
+        // point to the links field of `Node<K, V>` objects, so `this` is safe to dereference.
+        let this = container_of!(self.current.as_ptr(), Node<K, V>, links).cast_mut();
         // SAFETY: `this` is valid by the type invariants as described above.
         let node = unsafe { KBox::from_raw(this) };
         let node = RBTreeNode { node };
@@ -804,9 +804,9 @@ fn remove_neighbor(&mut self, direction: Direction) -> Option<RBTreeNode<K, V>>
             // SAFETY: The reference to the tree used to create the cursor outlives the cursor, so
             // the tree cannot change. By the tree invariant, all nodes are valid.
             unsafe { bindings::rb_erase(neighbor, addr_of_mut!(self.tree.root)) };
-            // SAFETY: By the type invariant of `Self`, all non-null `rb_node` pointers stored in `self`
-            // point to the links field of `Node<K, V>` objects.
-            let this = unsafe { container_of!(neighbor, Node<K, V>, links) }.cast_mut();
+            // By the type invariant of `Self`, all non-null `rb_node` pointers stored in `self`
+            // point to the links field of `Node<K, V>` objects, so `this` is safe to dereference.
+            let this = container_of!(neighbor, Node<K, V>, links).cast_mut();
             // SAFETY: `this` is valid by the type invariants as described above.
             let node = unsafe { KBox::from_raw(this) };
             return Some(RBTreeNode { node });
@@ -910,9 +910,9 @@ unsafe fn to_key_value_mut<'b>(node: NonNull<bindings::rb_node>) -> (&'b K, &'b
     /// - `node` must be a valid pointer to a node in an [`RBTree`].
     /// - The caller has immutable access to the key for the duration of 'b.
     unsafe fn to_key_value_raw<'b>(node: NonNull<bindings::rb_node>) -> (&'b K, *mut V) {
-        // SAFETY: By the type invariant of `Self`, all non-null `rb_node` pointers stored in `self`
-        // point to the links field of `Node<K, V>` objects.
-        let this = unsafe { container_of!(node.as_ptr(), Node<K, V>, links) }.cast_mut();
+        // By the type invariant of `Self`, all non-null `rb_node` pointers stored in `self`
+        // point to the links field of `Node<K, V>` objects, so `this` is safe to dereference.
+        let this = container_of!(node.as_ptr(), Node<K, V>, links).cast_mut();
         // SAFETY: The passed `node` is the current node or a non-null neighbor,
         // thus `this` is valid by the type invariants.
         let k = unsafe { &(*this).key };
@@ -1019,14 +1019,14 @@ fn next(&mut self) -> Option<Self::Item> {
             return None;
         }
 
-        // SAFETY: By the type invariant of `IterRaw`, `self.next` is a valid node in an `RBTree`,
-        // and by the type invariant of `RBTree`, all nodes point to the links field of `Node<K, V>` objects.
-        let cur = unsafe { container_of!(self.next, Node<K, V>, links) }.cast_mut();
+        // By the type invariant of `IterRaw`, `self.next` is a valid node in an `RBTree`, and by the
+        // type invariant of `RBTree`, all nodes point to the links field of `Node<K, V>` objects.
+        let cur = container_of!(self.next, Node<K, V>, links).cast_mut();
 
         // SAFETY: `self.next` is a valid tree node by the type invariants.
         self.next = unsafe { bindings::rb_next(self.next) };
 
-        // SAFETY: By the same reasoning above, it is safe to dereference the node.
+        // SAFETY: By the reasoning above, it is safe to dereference `cur`.
         Some(unsafe { (addr_of_mut!((*cur).key), addr_of_mut!((*cur).value)) })
     }
 }

base-commit: ceff0757f5dafb5be5205988171809c877b1d3e3
-- 
2.47.1

Re: [PATCH] rust: kernel: update `container_of` to not require unsafe blocks

[Alice Ryhl]

On Thu, Jan 30, 2025 at 11:12 AM Florian Rädiker <git@flra.eu> wrote:
>
> The calculation done by `container_of` does not have to be unsafe, so
> change this.
>
> Inspired by [0] by Wedson Almeida Filho <walmeida@microsoft.com>
>
> [0] https://github.com/Rust-for-Linux/linux/commit/ecc54b2bbd08f62f8cd16420d3d6c60b517a05cd
>
> Signed-off-by: Florian Rädiker <git@flra.eu>

When we added container_of! to the kernel, I argued that it should be
unsafe. It has the advantage of using pointer::sub instead of
wrapping_sub, which gives more information to the compiler that it is
better able to optimize. The unsafe is rarely a problem in practice,
since you're always going to immediately dereference the pointer after
performing the container_of operation, so you need what it requires to
be true either way.

> Hi,
>
> this is my first-ever patch submission :)
> I thought this might serve as a "good first issue".

Was this listed as a good first issue on the issue tracker? I hope not ...

Alice

Re: [PATCH] rust: kernel: update `container_of` to not require unsafe blocks

[Raediker, Florian]

On Thu, 2025-01-30 at 11:15 +0100, Alice Ryhl wrote:
> When we added container_of! to the kernel, I argued that it should be
> unsafe. It has the advantage of using pointer::sub instead of
> wrapping_sub, which gives more information to the compiler that it is
> better able to optimize. The unsafe is rarely a problem in practice,
> since you're always going to immediately dereference the pointer after
> performing the container_of operation, so you need what it requires to
> be true either way.

Thanks for the explanation!

> > Hi,
> > 
> > this is my first-ever patch submission :)
> > I thought this might serve as a "good first issue".
> 
> Was this listed as a good first issue on the issue tracker? I hope not ...

No, I was simply working with Wedson's VFS abstractions and thought this smaller
commit might be a good first submission.

Best,
flra