[PATCH] rust: check type of `$ptr` in `container_of!`

[PATCH] rust: check type of `$ptr` in `container_of!`

[Tamir Duberstein]

Add a compile-time check that `*$ptr` is of the type of `$type->$($f)*`.

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>
---
 rust/kernel/lib.rs | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 1df11156302a..da9e36aa7967 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -200,7 +200,10 @@ fn panic(info: &core::panic::PanicInfo<'_>) -> ! {
 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>()
+        let container = $ptr.byte_sub(offset).cast::<$type>();
+        fn assert_same_type<T>(_: T, _: T) {}
+        assert_same_type($ptr, ::core::mem::addr_of!((*container).$($f)*).cast_mut());
+        container
     }}
 }
 

---
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>

Re: [PATCH] rust: check type of `$ptr` in `container_of!`

[Alice Ryhl]

On Fri, Apr 11, 2025 at 4:31 PM Tamir Duberstein <tamird@gmail.com> wrote:
>
> Add a compile-time check that `*$ptr` is of the type of `$type->$($f)*`.
>
> 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>
> ---
>  rust/kernel/lib.rs | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index 1df11156302a..da9e36aa7967 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -200,7 +200,10 @@ fn panic(info: &core::panic::PanicInfo<'_>) -> ! {
>  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>()
> +        let container = $ptr.byte_sub(offset).cast::<$type>();
> +        fn assert_same_type<T>(_: T, _: T) {}
> +        assert_same_type($ptr, ::core::mem::addr_of!((*container).$($f)*).cast_mut());

Perhaps it would be better to wrap the type check in an `if false` to
avoid evaluating the expressions at runtime?

> +        container
>      }}
>  }
>
>
> ---
> 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>
>

Re: [PATCH] rust: check type of `$ptr` in `container_of!`

[Tamir Duberstein]

On Fri, Apr 11, 2025 at 10:36 AM Alice Ryhl <aliceryhl@google.com> wrote:
>
> On Fri, Apr 11, 2025 at 4:31 PM Tamir Duberstein <tamird@gmail.com> wrote:
> >
> > Add a compile-time check that `*$ptr` is of the type of `$type->$($f)*`.
> >
> > 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>
> > ---
> >  rust/kernel/lib.rs | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> > index 1df11156302a..da9e36aa7967 100644
> > --- a/rust/kernel/lib.rs
> > +++ b/rust/kernel/lib.rs
> > @@ -200,7 +200,10 @@ fn panic(info: &core::panic::PanicInfo<'_>) -> ! {
> >  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>()
> > +        let container = $ptr.byte_sub(offset).cast::<$type>();
> > +        fn assert_same_type<T>(_: T, _: T) {}
> > +        assert_same_type($ptr, ::core::mem::addr_of!((*container).$($f)*).cast_mut());

I noticed I accidentally sent `::core::mem::addr_of` instead of
`::core::ptr::addr_of`; will fix once we agree below.

> Perhaps it would be better to wrap the type check in an `if false` to
> avoid evaluating the expressions at runtime?

It's optimized out at O1: https://godbolt.org/z/44Go5xnWr. Is it worth it?

Re: [PATCH] rust: check type of `$ptr` in `container_of!`

[Benno Lossin]

On Fri Apr 11, 2025 at 5:41 PM CEST, Tamir Duberstein wrote:
> On Fri, Apr 11, 2025 at 10:36 AM Alice Ryhl <aliceryhl@google.com> wrote:
>>
>> On Fri, Apr 11, 2025 at 4:31 PM Tamir Duberstein <tamird@gmail.com> wrote:
>> >
>> > Add a compile-time check that `*$ptr` is of the type of `$type->$($f)*`.
>> >
>> > 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>
>> > ---
>> >  rust/kernel/lib.rs | 5 ++++-
>> >  1 file changed, 4 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
>> > index 1df11156302a..da9e36aa7967 100644
>> > --- a/rust/kernel/lib.rs
>> > +++ b/rust/kernel/lib.rs
>> > @@ -200,7 +200,10 @@ fn panic(info: &core::panic::PanicInfo<'_>) -> ! {
>> >  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>()
>> > +        let container = $ptr.byte_sub(offset).cast::<$type>();
>> > +        fn assert_same_type<T>(_: T, _: T) {}
>> > +        assert_same_type($ptr, ::core::mem::addr_of!((*container).$($f)*).cast_mut());
>
> I noticed I accidentally sent `::core::mem::addr_of` instead of
> `::core::ptr::addr_of`; will fix once we agree below.
>
>> Perhaps it would be better to wrap the type check in an `if false` to
>> avoid evaluating the expressions at runtime?
>
> It's optimized out at O1: https://godbolt.org/z/44Go5xnWr. Is it worth it?

Wrapping in `if false` definitely doesn't hurt, since we get better
debug perf.

---
Cheers,
Benno

Re: [PATCH] rust: check type of `$ptr` in `container_of!`

[Tamir Duberstein]

On Fri, Apr 11, 2025 at 5:12 PM Benno Lossin <benno.lossin@proton.me> wrote:
>
> On Fri Apr 11, 2025 at 5:41 PM CEST, Tamir Duberstein wrote:
> > On Fri, Apr 11, 2025 at 10:36 AM Alice Ryhl <aliceryhl@google.com> wrote:
> >>
> >> On Fri, Apr 11, 2025 at 4:31 PM Tamir Duberstein <tamird@gmail.com> wrote:
> >> >
> >> > Add a compile-time check that `*$ptr` is of the type of `$type->$($f)*`.
> >> >
> >> > 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>
> >> > ---
> >> >  rust/kernel/lib.rs | 5 ++++-
> >> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >> >
> >> > diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> >> > index 1df11156302a..da9e36aa7967 100644
> >> > --- a/rust/kernel/lib.rs
> >> > +++ b/rust/kernel/lib.rs
> >> > @@ -200,7 +200,10 @@ fn panic(info: &core::panic::PanicInfo<'_>) -> ! {
> >> >  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>()
> >> > +        let container = $ptr.byte_sub(offset).cast::<$type>();
> >> > +        fn assert_same_type<T>(_: T, _: T) {}
> >> > +        assert_same_type($ptr, ::core::mem::addr_of!((*container).$($f)*).cast_mut());
> >
> > I noticed I accidentally sent `::core::mem::addr_of` instead of
> > `::core::ptr::addr_of`; will fix once we agree below.
> >
> >> Perhaps it would be better to wrap the type check in an `if false` to
> >> avoid evaluating the expressions at runtime?
> >
> > It's optimized out at O1: https://godbolt.org/z/44Go5xnWr. Is it worth it?
>
> Wrapping in `if false` definitely doesn't hurt, since we get better
> debug perf.

How's this?

if false { [$ptr, ::core::ptr::addr_of!((*container).$($f)*).cast_mut()]; }

Confirmed it's entirely gone from codegen: https://godbolt.org/z/fo3x13r5K.




Tamir

Re: [PATCH] rust: check type of `$ptr` in `container_of!`

[Benno Lossin]

On Fri Apr 11, 2025 at 11:39 PM CEST, Tamir Duberstein wrote:
> On Fri, Apr 11, 2025 at 5:12 PM Benno Lossin <benno.lossin@proton.me> wrote:
>>
>> On Fri Apr 11, 2025 at 5:41 PM CEST, Tamir Duberstein wrote:
>> > On Fri, Apr 11, 2025 at 10:36 AM Alice Ryhl <aliceryhl@google.com> wrote:
>> >>
>> >> On Fri, Apr 11, 2025 at 4:31 PM Tamir Duberstein <tamird@gmail.com> wrote:
>> >> >
>> >> > Add a compile-time check that `*$ptr` is of the type of `$type->$($f)*`.
>> >> >
>> >> > 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>
>> >> > ---
>> >> >  rust/kernel/lib.rs | 5 ++++-
>> >> >  1 file changed, 4 insertions(+), 1 deletion(-)
>> >> >
>> >> > diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
>> >> > index 1df11156302a..da9e36aa7967 100644
>> >> > --- a/rust/kernel/lib.rs
>> >> > +++ b/rust/kernel/lib.rs
>> >> > @@ -200,7 +200,10 @@ fn panic(info: &core::panic::PanicInfo<'_>) -> ! {
>> >> >  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>()
>> >> > +        let container = $ptr.byte_sub(offset).cast::<$type>();
>> >> > +        fn assert_same_type<T>(_: T, _: T) {}
>> >> > +        assert_same_type($ptr, ::core::mem::addr_of!((*container).$($f)*).cast_mut());
>> >
>> > I noticed I accidentally sent `::core::mem::addr_of` instead of
>> > `::core::ptr::addr_of`; will fix once we agree below.
>> >
>> >> Perhaps it would be better to wrap the type check in an `if false` to
>> >> avoid evaluating the expressions at runtime?
>> >
>> > It's optimized out at O1: https://godbolt.org/z/44Go5xnWr. Is it worth it?
>>
>> Wrapping in `if false` definitely doesn't hurt, since we get better
>> debug perf.
>
> How's this?
>
> if false { [$ptr, ::core::ptr::addr_of!((*container).$($f)*).cast_mut()]; }

How does the error look like if you use the wrong input pointer? I'd
prefer we use the variant that creates the best error report for the
user. I could imagine that the function gives a better error, but I
haven't checked.

---
Cheers,
Benno

Re: [PATCH] rust: check type of `$ptr` in `container_of!`

[Miguel Ojeda]

On Fri, Apr 11, 2025 at 4:31 PM Tamir Duberstein <tamird@gmail.com> wrote:
>
> Add a compile-time check that `*$ptr` is of the type of `$type->$($f)*`.
>
> 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>

Applied to `rust-next` -- thanks!

    [ I went with v1, since it seems to me like the obvious approach, the
      error messages seemed good enough and the debug performance should be
      fine, given the kernel is always built with -O2. Moreover, we could
      move the function out of this -- see [1]:

        With v1, we could also just put `assert_same_type` outside as a
        utility for others to use, i.e. in the `kernel` crate, which
        simplifies things and makes the error a bit shorter. Moving the
        function out makes the error slightly shorter, would also allow us to
        document its usage, including the suggestion to use `if false` in an
        example.

        Regarding the `if false`, the kernel is always built with at least
        -O2. Benno mentioned debug performance -- was that related to
        something like debug assertions being enabled or just optimization
        level? Either way, even with the assertions enabled, I don't see it in
        codegen.

      In particular, the error message for a example mistake like the one
      showcased by Tamir in v2 and v3:

          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 }) {

      looks like this (plus more details):

          error[E0308]: mismatched types
             --> rust/kernel/lib.rs:219:32
              |
          219 |         assert_same_type($ptr,
::core::ptr::addr_of!((*container).$($f)*).cast_mut());
              |         ----------------
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected `*mut
rb_node`, found `*mut K`
              |         |
              |         arguments to this function are incorrect

          - Miguel ]

    [ Fixed `mem` -> `ptr`. - Miguel ]

This caught three cases that require an extra cast in -next -- those
will be fixed on merge.

Cheers,
Miguel