rust/kernel/lib.rs | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)
Add a compile-time check that `*$ptr` is of the type of `$type->$($f)*`.
Given the incorrect usage:
; diff --git a/rust/kernel/rbtree.rs b/rust/kernel/rbtree.rs
; index 8d978c896747..6a7089149878 100644
; --- a/rust/kernel/rbtree.rs
; +++ b/rust/kernel/rbtree.rs
; @@ -329,7 +329,7 @@ fn raw_entry(&mut self, key: &K) -> RawEntry<'_, K, V> {
; 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) };
; + let node = unsafe { container_of!(curr, Node<K, V>, key) };
;
; // SAFETY: `node` is a non-null node so it is valid by the type invariants.
; match key.cmp(unsafe { &(*node).key }) {
this patch produces the compilation error:
; error[E0308]: mismatched types
; --> rust/kernel/lib.rs:206:26
; |
; 206 | [$field_ptr, field_ptr]; // typeof(`$ptr_to_field`) == typeof(`$Container.$($fields)*`)
; | ^^^^^^^^^ expected `*mut rb_node`, found `*mut K`
; |
; ::: rust/kernel/rbtree.rs:270:6
; |
; 270 | impl<K, V> RBTree<K, V>
; | - found this type parameter
; ...
; 332 | let node = unsafe { container_of!(curr, Node<K, V>, key) };
; | ------------------------------------ in this macro invocation
; |
; = note: expected raw pointer `*mut bindings::rb_node`
; found raw pointer `*mut K`
; = note: this error originates in the macro `container_of` (in Nightly builds, run with -Z macro-backtrace for more info)
;
; error: aborting due to 1 previous error
Suggested-by: Alice Ryhl <aliceryhl@google.com>
Link: https://lore.kernel.org/all/CAH5fLgh6gmqGBhPMi2SKn7mCmMWfOSiS0WP5wBuGPYh9ZTAiww@mail.gmail.com/
Signed-off-by: Tamir Duberstein <tamird@gmail.com>
---
I also considered an implementation using a function, but the resulting
compilation error was not as concise:
; error[E0308]: mismatched types
; --> rust/kernel/lib.rs:207:42
; |
; 207 | assert_same_type($field_ptr, field_ptr);
; | ---------------- ^^^^^^^^^ expected `*const rb_node`, found `*mut K`
; | |
; | arguments to this function are incorrect
; |
; ::: rust/kernel/rbtree.rs:270:6
; |
; 270 | impl<K, V> RBTree<K, V>
; | - found this type parameter
; ...
; 332 | let node = unsafe { container_of!(curr, Node<K, V>, key) };
; | ------------------------------------ in this macro invocation
; |
; = note: expected raw pointer `*const bindings::rb_node`
; found raw pointer `*mut K`
; note: function defined here
; --> rust/kernel/lib.rs:205:16
; |
; 205 | fn assert_same_type<T>(_: *const T, _: *const T) {}
; | ^^^^^^^^^^^^^^^^ -----------
; |
; ::: rust/kernel/rbtree.rs:332:33
; |
; 332 | let node = unsafe { container_of!(curr, Node<K, V>, key) };
; | ------------------------------------ in this macro invocation
; = note: this error originates in the macro `container_of` (in Nightly builds, run with -Z macro-backtrace for more info)
;
; error: aborting due to 1 previous error
---
Changes in v2:
- Wrap in `if false` to improve unoptimized codegen. (Alice Ryhl)
- Shrink implementation using an array literal instead of a function.
- Link to v1: https://lore.kernel.org/r/20250411-b4-container-of-type-check-v1-1-08262ef67c95@gmail.com
---
rust/kernel/lib.rs | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 1df11156302a..6fbd4cc5afff 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -198,9 +198,14 @@ fn panic(info: &core::panic::PanicInfo<'_>) -> ! {
/// ```
#[macro_export]
macro_rules! container_of {
- ($ptr:expr, $type:ty, $($f:tt)*) => {{
- let offset: usize = ::core::mem::offset_of!($type, $($f)*);
- $ptr.byte_sub(offset).cast::<$type>()
+ ($field_ptr:expr, $Container:ty, $($fields:tt)*) => {{
+ let offset: usize = ::core::mem::offset_of!($Container, $($fields)*);
+ let container_ptr = $field_ptr.byte_sub(offset).cast::<$Container>();
+ if false {
+ let field_ptr = ::core::ptr::addr_of!((*container_ptr).$($fields)*).cast_mut();
+ [$field_ptr, field_ptr]; // typeof(`$ptr_to_field`) == typeof(`$Container.$($fields)*`)
+ }
+ container_ptr
}}
}
---
base-commit: 0af2f6be1b4281385b618cb86ad946eded089ac8
change-id: 20250411-b4-container-of-type-check-06af1c204f59
prerequisite-change-id: 20250409-container-of-mutness-b153dab4388d:v1
prerequisite-patch-id: 53d5889db599267f87642bb0ae3063c29bc24863
Best regards,
--
Tamir Duberstein <tamird@gmail.com>On Sat, Apr 12, 2025 at 02:16:08PM -0400, Tamir Duberstein wrote:
> + ($field_ptr:expr, $Container:ty, $($fields:tt)*) => {{
> + let offset: usize = ::core::mem::offset_of!($Container, $($fields)*);
> + let container_ptr = $field_ptr.byte_sub(offset).cast::<$Container>();
> + if false {
> + let field_ptr = ::core::ptr::addr_of!((*container_ptr).$($fields)*).cast_mut();
> + [$field_ptr, field_ptr]; // typeof(`$ptr_to_field`) == typeof(`$Container.$($fields)*`)
This evaluates $field_ptr twice. The `if false` avoids bad stuff if the
expression has side-effects, but still seems sub-optimal.
Alice
On Mon, Apr 14, 2025 at 10:04 AM Alice Ryhl <aliceryhl@google.com> wrote:
>
> On Sat, Apr 12, 2025 at 02:16:08PM -0400, Tamir Duberstein wrote:
> > + ($field_ptr:expr, $Container:ty, $($fields:tt)*) => {{
> > + let offset: usize = ::core::mem::offset_of!($Container, $($fields)*);
> > + let container_ptr = $field_ptr.byte_sub(offset).cast::<$Container>();
> > + if false {
> > + let field_ptr = ::core::ptr::addr_of!((*container_ptr).$($fields)*).cast_mut();
> > + [$field_ptr, field_ptr]; // typeof(`$ptr_to_field`) == typeof(`$Container.$($fields)*`)
>
> This evaluates $field_ptr twice. The `if false` avoids bad stuff if the
> expression has side-effects, but still seems sub-optimal.
I don't disagree but I intentionally made this choice so that the
compiler error was clear about the LHS element being one of the
arguments to the macro. But maybe the comment provides enough clarity.
The alternative error is
> error[E0308]: mismatched types
> --> rust/kernel/lib.rs:207:25
> |
> 207 | [field_ptr, container_field_ptr]; // typeof(`$field_ptr`) == typeof(`$Container.$($fields)*`)
> | ^^^^^^^^^^^^^^^^^^^ expected `*mut rb_node`, found `*mut K`
> |
> ::: rust/kernel/rbtree.rs:270:6
> |
> 270 | impl<K, V> RBTree<K, V>
> | - found this type parameter
> ...
> 332 | let node = unsafe { container_of!(curr, Node<K, V>, key) };
> | ------------------------------------ in this macro invocation
> |
> = note: expected raw pointer `*mut bindings::rb_node`
> found raw pointer `*mut K`
> = note: this error originates in the macro `container_of` (in Nightly builds, run with -Z macro-backtrace for more info)
>
> error: aborting due to 1 previous error
Seems OK to me, so I'll do this in v3.
On Sat Apr 12, 2025 at 8:16 PM CEST, Tamir Duberstein wrote:
> Add a compile-time check that `*$ptr` is of the type of `$type->$($f)*`.
>
> Given the incorrect usage:
>
> ; diff --git a/rust/kernel/rbtree.rs b/rust/kernel/rbtree.rs
> ; index 8d978c896747..6a7089149878 100644
> ; --- a/rust/kernel/rbtree.rs
> ; +++ b/rust/kernel/rbtree.rs
> ; @@ -329,7 +329,7 @@ fn raw_entry(&mut self, key: &K) -> RawEntry<'_, K, V> {
> ; 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) };
> ; + let node = unsafe { container_of!(curr, Node<K, V>, key) };
> ;
> ; // SAFETY: `node` is a non-null node so it is valid by the type invariants.
> ; match key.cmp(unsafe { &(*node).key }) {
>
> this patch produces the compilation error:
>
> ; error[E0308]: mismatched types
> ; --> rust/kernel/lib.rs:206:26
> ; |
> ; 206 | [$field_ptr, field_ptr]; // typeof(`$ptr_to_field`) == typeof(`$Container.$($fields)*`)
> ; | ^^^^^^^^^ expected `*mut rb_node`, found `*mut K`
> ; |
> ; ::: rust/kernel/rbtree.rs:270:6
> ; |
> ; 270 | impl<K, V> RBTree<K, V>
> ; | - found this type parameter
> ; ...
> ; 332 | let node = unsafe { container_of!(curr, Node<K, V>, key) };
> ; | ------------------------------------ in this macro invocation
> ; |
> ; = note: expected raw pointer `*mut bindings::rb_node`
> ; found raw pointer `*mut K`
> ; = note: this error originates in the macro `container_of` (in Nightly builds, run with -Z macro-backtrace for more info)
> ;
> ; error: aborting due to 1 previous error
>
> Suggested-by: Alice Ryhl <aliceryhl@google.com>
> Link: https://lore.kernel.org/all/CAH5fLgh6gmqGBhPMi2SKn7mCmMWfOSiS0WP5wBuGPYh9ZTAiww@mail.gmail.com/
> Signed-off-by: Tamir Duberstein <tamird@gmail.com>
Reviewed-by: Benno Lossin <benno.lossin@proton.me>
> ---
> I also considered an implementation using a function, but the resulting
> compilation error was not as concise:
Thanks for checking :)
---
Cheers,
Benno
On Sat, Apr 12, 2025 at 8:16 PM Tamir Duberstein <tamird@gmail.com> wrote: > > ; diff --git a/rust/kernel/rbtree.rs b/rust/kernel/rbtree.rs I am curious, what is the reason for the `;` notation? If is it to avoid issues with the `diff` marker, then I think indenting with 4 spaces as usual would work. Thanks! Cheers, Miguel
On Sun, Apr 13, 2025 at 4:31 PM Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote: > > On Sat, Apr 12, 2025 at 8:16 PM Tamir Duberstein <tamird@gmail.com> wrote: > > > > ; diff --git a/rust/kernel/rbtree.rs b/rust/kernel/rbtree.rs > > I am curious, what is the reason for the `;` notation? If is it to > avoid issues with the `diff` marker, then I think indenting with 4 > spaces as usual would work. `b4 prep --check` complains: ● checkpatch.pl: :207: ERROR: Avoid using diff content in the commit message - patch(1) might not work What do you suggest?
On Mon, Apr 14, 2025 at 3:27 PM Tamir Duberstein <tamird@gmail.com> wrote: > > `b4 prep --check` complains: > ● checkpatch.pl: :207: ERROR: Avoid using diff content in the commit > message - patch(1) might not work > > What do you suggest? (It is `checkpatch.pl` the one that complains, no?) I don't think it really matters, since `git am` is OK with it. So unless you are sending the patch to a subsystem that still uses `patch` or `quilt` or similar, and those are quite rare nowadays, I wouldn't worry. But if you care and want to be extra nice, then I would suggest doing what others do, i.e. checking the Git log. That tells me to use `>` or `:`, since they seem to be common. I don't see `;`. I would also recommend patching `patch`... :) Thanks for clarifying! Cheers, Miguel
On Mon, Apr 14, 2025 at 9:57 AM Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote: > > On Mon, Apr 14, 2025 at 3:27 PM Tamir Duberstein <tamird@gmail.com> wrote: > > > > `b4 prep --check` complains: > > ● checkpatch.pl: :207: ERROR: Avoid using diff content in the commit > > message - patch(1) might not work > > > > What do you suggest? > > (It is `checkpatch.pl` the one that complains, no?) Yes, of course, I just mentioned b4 because that is how I invoke it - better to over-communicate :) > > I don't think it really matters, since `git am` is OK with it. So > unless you are sending the patch to a subsystem that still uses > `patch` or `quilt` or similar, and those are quite rare nowadays, I > wouldn't worry. > > But if you care and want to be extra nice, then I would suggest doing > what others do, i.e. checking the Git log. That tells me to use `>` or > `:`, since they seem to be common. I don't see `;`. Will use `>` on respin since I need to fix up that comment anyhow. > I would also recommend patching `patch`... :) > > Thanks for clarifying! Thanks!
On Sat, Apr 12, 2025 at 2:16 PM Tamir Duberstein <tamird@gmail.com> wrote:
>
> [...]
>
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index 1df11156302a..6fbd4cc5afff 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -198,9 +198,14 @@ fn panic(info: &core::panic::PanicInfo<'_>) -> ! {
> /// ```
> #[macro_export]
> macro_rules! container_of {
> - ($ptr:expr, $type:ty, $($f:tt)*) => {{
> - let offset: usize = ::core::mem::offset_of!($type, $($f)*);
> - $ptr.byte_sub(offset).cast::<$type>()
> + ($field_ptr:expr, $Container:ty, $($fields:tt)*) => {{
> + let offset: usize = ::core::mem::offset_of!($Container, $($fields)*);
> + let container_ptr = $field_ptr.byte_sub(offset).cast::<$Container>();
> + if false {
> + let field_ptr = ::core::ptr::addr_of!((*container_ptr).$($fields)*).cast_mut();
> + [$field_ptr, field_ptr]; // typeof(`$ptr_to_field`) == typeof(`$Container.$($fields)*`)
The comment here should be s/ptr_to_field/field_ptr/. I missed this
when renaming this placeholder for clarity.
> + }
> + container_ptr
> }}
> }
© 2016 - 2025 Red Hat, Inc.