linux-kernel - Re: [PATCH v7] rust: transmute: Add methods for FromBytes trait

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CANiq72mnxRquFmjoJemb=3LSq+ZdUfs9J+HXTwM6AavprsVNUg@mail.gmail.com>
Date: Sun, 15 Jun 2025 11:34:39 +0200
From: Miguel Ojeda <miguel.ojeda.sandonis@...il.com>
To: Every2 <christiansantoslima21@...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, ~lkcamp/patches@...ts.sr.ht, 
	richard120310@...il.com
Subject: Re: [PATCH v7] rust: transmute: Add methods for FromBytes trait

On Sun, Jun 15, 2025 at 9:20 AM Every2 <christiansantoslima21@...il.com> wrote:
>
> Methods receive a slice and perform size check to add a valid way to make
> conversion safe. An Option is used, in error case just return `None`.

Please start with something like "Add methods ... and ... They are
useful to ...", i.e. the idea is to try to explain the "what" and the
"why" (what is being added, and why is it being added).

> Signed-off-by: Every2 <christiansantoslima21@...il.com>

The previous version used your name in the SoB -- is this expected?

> +/// # Example

Please use the plural: # Examples

(and empty line between it and the ```).

> +/// match result {
> +///     Some(x) => assert_eq!(*x, 0x4030201),
> +///     None => unreachable!()
> +/// }

If we are going to use something like `unreachable!()`, then I would
just unwrap.

But recently we are trying to make examples look more like normal
kernel code, so please use `?` instead.

> +                    && (bytes.as_ptr() as usize) % core::mem::align_of::<$t>() == 0

Can we do the cast earlier and then use `.is_aligned()` instead?

Also, since this is inside a macro, we should try to avoid assuming
anything about the caller's code, so please use `::core` instead of
`core`.

> -    // SAFETY: If all bit patterns are acceptable for individual values in an array, then all bit
> -    // patterns are also acceptable for arrays of that type.
> -    {<T: FromBytes>} [T],
>      {<T: FromBytes, const N: usize>} [T; N],

Don't we still want this safety comment for the array case?

Also, this is still missing safety comments on top of a couple
`unsafe` blocks (pointed out in an earlier version). Are you building
with `make ..... CLIPPY=1`?

Thanks!

Cheers,
Miguel