linux-kernel - Re: [PATCH v2 0/5] rust: add `TryFrom` and `Into` derive macros

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: <CANiq72nbERSOWKRUTJ96YYKCDeAoHLBuiVP+XkDUKg7JYkoyiA@mail.gmail.com>
Date: Tue, 30 Sep 2025 22:12:42 +0200
From: Miguel Ojeda <miguel.ojeda.sandonis@...il.com>
To: Jesung Yang <y.j3ms.n@...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 <lossin@...nel.org>, Andreas Hindborg <a.hindborg@...nel.org>, 
	Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>, 
	Danilo Krummrich <dakr@...nel.org>, Alexandre Courbot <acourbot@...dia.com>, linux-kernel@...r.kernel.org, 
	rust-for-linux@...r.kernel.org, nouveau@...ts.freedesktop.org
Subject: Re: [PATCH v2 0/5] rust: add `TryFrom` and `Into` derive macros

On Fri, Aug 15, 2025 at 7:32 AM Jesung Yang <y.j3ms.n@...il.com> wrote:
>
> This patch series introduces derive macros for `TryFrom` and `Into`
> traits.

I took a quick look -- it is a nice series!

Some notes:

  - My biggest concern is the overflow caveat, which is a fairly big
one if one, especially if one is dealing with runtime values.

    Can we do better? Accessing the discriminant via `as` is available
in const context, and you already have every variant easily available,
so can we check that every variant fits in the relevant target types?

    For instance, by creating some item-level const blocks
(`static_assert!`s) -- dummy example for an unsigned-to-unsigned case:

        const _: () = { assert! (E::A as u128 <= u8::MAX as u128); };
        const _: () = { assert! (E::B as u128 <= u8::MAX as u128); };
        ...

    and so on. There may be better ways to do it -- I just quickly
brute forced it that unsigned case with what you already had for
handling variants:

        variants.iter().map(|variant| {
            quote! {
                const _: () = { assert!(#enum_ident::#variant as u128
<= #ty::MAX as u128); };
            }
        });

    Maybe this was already discussed elsewhere and there is a reason
not to do something like this, but it seems to me that we should try
to avoid that risk.

  - On other news, I will post in the coming days the `syn` patches,
and my plan is to merge them for next cycle, so when those are out,
Benno thought you could give them a go (we can still merge this with
your current approach and then convert, but still, more `syn` users
and converting the existing macros would be nice :).

    (By the way, the linked patches about converting the existing
macros to `syn` are an RFC in the sense that they cannot be applied,
but having `syn` itself is something we already agreed on a long time
ago.)

  - Couple nits: typo arise -> arises, and I would do `repr-rust`
instead of `repr-rs` since that is the anchor in the reference that
you are linking.

Thanks a lot!

Cheers,
Miguel