linux-kernel - Re: [PATCH] rust: check type of `$ptr` in `container

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CANiq72kmGPF+MmYcYmPz93g5L2rr5AspZGYYLfG=gYSfF703+Q@mail.gmail.com>
Date: Thu, 29 May 2025 15:28:41 +0200
From: Miguel Ojeda <miguel.ojeda.sandonis@...il.com>
To: Tamir Duberstein <tamird@...il.com>
Cc: Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>, 
	Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>, 
	Björn Roy Baron <bjorn3_gh@...tonmail.com>, 
	Benno Lossin <benno.lossin@...ton.me>, Andreas Hindborg <a.hindborg@...nel.org>, 
	Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>, 
	Danilo Krummrich <dakr@...nel.org>, rust-for-linux@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] rust: check type of `$ptr` in `container_of!`

On Fri, Apr 11, 2025 at 4:31 PM Tamir Duberstein <tamird@...il.com> wrote:
>
> Add a compile-time check that `*$ptr` is of the type of `$type->$($f)*`.
>
> Suggested-by: Alice Ryhl <aliceryhl@...gle.com>
> Link: https://lore.kernel.org/all/CAH5fLgh6gmqGBhPMi2SKn7mCmMWfOSiS0WP5wBuGPYh9ZTAiww@mail.gmail.com/
> Signed-off-by: Tamir Duberstein <tamird@...il.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