| 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
| ||
|
Message-Id: <DF8QDONK951M.10NYLJ40UNNY1@kernel.org>
Date: Sat, 27 Dec 2025 05:57:07 +0100
From: "Benno Lossin" <lossin@...nel.org>
To: "Jesung Yang" <y.j3ms.n@...il.com>
Cc: "Miguel Ojeda" <ojeda@...nel.org>, "Boqun Feng" <boqun.feng@...il.com>,
"Gary Guo" <gary@...yguo.net>, Björn Roy Baron
<bjorn3_gh@...tonmail.com>, "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 v4 1/4] rust: macros: add derive macro for `Into`
On Fri Dec 26, 2025 at 10:36 AM CET, Jesung Yang wrote:
> On Fri, Dec 26, 2025 at 2:40 AM Benno Lossin <lossin@...nel.org> wrote:
>> On Thu Dec 25, 2025 at 9:37 AM CET, Jesung Yang via B4 Relay wrote:
>> > +fn parse_attrs(target: DeriveTarget, attrs: &[Attribute]) -> syn::Result<(Vec<ValidTy>, Ident)> {
>> > + let helper = target.get_helper_name();
>> > +
>> > + let mut repr_ty = None;
>> > + let mut helper_tys = Vec::new();
>> > + for attr in attrs {
>> > + if attr.path().is_ident("repr") {
>> > + attr.parse_nested_meta(|meta| {
>> > + let ident = meta.path.get_ident();
>> > + if ident.is_some_and(is_valid_primitive) {
>> > + repr_ty = ident.cloned();
>> > + }
>>
>> While this works for now, writing `repr(C, {integer})` might become
>> meaningful in the future, see [1]. We should just accept it now as well.
>>
>> [1]: https://github.com/rust-lang/rust/issues/68585
>
> I think it's worth noting. I'll add a small comment for this.
My suggestion was to just accept `repr(C, {integer})` and make it result
in the same as `repr({integer})`.
>> > + // Delegate `repr` attribute validation to rustc.
>> > + Ok(())
>> > + })?;
>> > + } else if attr.path().is_ident(helper) && helper_tys.is_empty() {
>> > + let args = attr.parse_args_with(Punctuated::<ValidTy, Token![,]>::parse_terminated)?;
>> > + helper_tys.extend(args);
>> > + }
>> > + }
>> > +
>> > + // Note on field-less `repr(C)` enums (quote from [1]):
>> > + //
>> > + // In C, enums with discriminants that do not all fit into an `int` or all fit into an
>> > + // `unsigned int` are a portability hazard: such enums are only permitted since C23, and not
>> > + // supported e.g. by MSVC.
>> > + //
>> > + // Furthermore, Rust interprets the discriminant values of `repr(C)` enums as expressions of
>> > + // type `isize`. This makes it impossible to implement the C23 behavior of enums where the
>> > + // enum discriminants have no predefined type and instead the enum uses a type large enough
>> > + // to hold all discriminants.
>> > + //
>> > + // Therefore, `repr(C)` enums in Rust require that either all discriminants to fit into a C
>> > + // `int` or they all fit into an `unsigned int`.
>> > + //
>> > + // As such, `isize` is a reasonable representation for `repr(C)` enums, as it covers the range
>> > + // of both `int` and `unsigned int`.
>> > + //
>> > + // For more information, see:
>> > + // - https://github.com/rust-lang/rust/issues/124403
>> > + // - https://github.com/rust-lang/rust/pull/147017
>> > + // - https://github.com/rust-lang/rust/blob/2ca7bcd03b87b52f7055a59b817443b0ac4a530d/compiler/rustc_lint_defs/src/builtin.rs#L5251-L5263 [1]
>> > +
>> > + // Extract the representation passed by `#[repr(...)]` if present. If nothing is
>> > + // specified, the default is `Rust` representation, which uses `isize` for its
>> > + // discriminant type.
>> > + // See: https://doc.rust-lang.org/reference/items/enumerations.html#r-items.enum.discriminant.repr-rust
>>
>> I think we should error when no `#[repr({integer})]` attribute is
>> specified.
>
> Not a blocker but just out of curiosity: are you concerned that the
> default size might change in the future, leading to silent side
> effects?
`isize` already changes size when you switch between 64 and 32 bit
architectures. I think the author of an enum they want to convert into
integers should think about which size the enum should be.
They already can choose `repr(isize)` if that is correct in that case.
As a default, I would have choosen `i32` (but that conflicts with Rust's
default, so we can't do it).
>> > + let repr_ty = repr_ty.unwrap_or_else(|| Ident::new("isize", Span::call_site()));
>> > + Ok((helper_tys, repr_ty))
>> > +}
>> > +
>> > +fn derive_for_enum(
>> > + target: DeriveTarget,
>> > + enum_ident: &Ident,
>> > + variants: &[Ident],
>> > + helper_tys: &[ValidTy],
>> > + repr_ty: &Ident,
>> > +) -> TokenStream {
>> > + let impl_fn = match target {
>> > + DeriveTarget::Into => impl_into,
>> > + };
>> > +
>> > + let qualified_repr_ty: syn::Path = parse_quote! { ::core::primitive::#repr_ty };
>> > +
>> > + return if helper_tys.is_empty() {
>> > + let ty = ValidTy::Primitive(repr_ty.clone());
>> > + let impls =
>> > + std::iter::once(ty).map(|ty| impl_fn(enum_ident, variants, &qualified_repr_ty, &ty));
>> > + ::quote::quote! { #(#impls)* }
>> > + } else {
>> > + let impls = helper_tys
>> > + .iter()
>> > + .map(|ty| impl_fn(enum_ident, variants, &qualified_repr_ty, ty));
>> > + ::quote::quote! { #(#impls)* }
>> > + };
>>
>> Let's just do this when we still have the `helper_tys` vector:
>>
>> helper_tys.push(ValidTy::Primitive(repr_ty));
>
> The current implementation completely ignores what's in `#[repr(...)]`
> when the `#[into(...)]` or `#[try_from(...)]` helper attributes are
> specified. But users might expect, as you did, the macros to generate
> impls not only for types specified in the helper attributes but also
> for the one in `repr`. I think I should deduplicate `helper_tys` after
> the push, but anyway, I'll change this in v5.
Oh yeah I missed this, please do make this change :)
>> > +
>> > + fn impl_into(
>> > + enum_ident: &Ident,
>> > + variants: &[Ident],
>> > + repr_ty: &syn::Path,
>> > + input_ty: &ValidTy,
>> > + ) -> TokenStream {
>> > + let param = Ident::new("value", Span::call_site());
>> > +
>> > + let overflow_assertion = emit_overflow_assert(enum_ident, variants, repr_ty, input_ty);
>> > + let cast = match input_ty {
>> > + ValidTy::Bounded(inner) => {
>> > + let base_ty = inner.emit_qualified_base_ty();
>> > + let expr = parse_quote! { #param as #base_ty };
>> > + // Since the discriminant of `#param`, an enum variant, is determined
>> > + // at compile-time, we can rely on `Bounded::from_expr()`. It requires
>> > + // the provided expression to be verifiable at compile-time to avoid
>> > + // triggering a build error.
>> > + inner.emit_from_expr(&expr)
>> > + }
>> > + ValidTy::Primitive(ident) if ident == "bool" => {
>> > + ::quote::quote! { (#param as #repr_ty) == 1 }
>> > + }
>> > + qualified @ ValidTy::Primitive(_) => ::quote::quote! { #param as #qualified },
>> > + };
>> > +
>> > + ::quote::quote! {
>> > + #[automatically_derived]
>> > + impl ::core::convert::From<#enum_ident> for #input_ty {
>> > + fn from(#param: #enum_ident) -> #input_ty {
>> > + #overflow_assertion
>> > +
>> > + #cast
>> > + }
>> > + }
>> > + }
>> > + }
>> > +
>> > + fn emit_overflow_assert(
>> > + enum_ident: &Ident,
>> > + variants: &[Ident],
>> > + repr_ty: &syn::Path,
>> > + input_ty: &ValidTy,
>> > + ) -> TokenStream {
>>
>> I feel like we should track this via traits rather than using a const
>> assert. That approach will require & generate much less code.
>
> Sorry, but could you elaborate? A small example of what you have in
> mind would help a lot.
Oh yeah sorry, I had something different in mind compared to what I'll
describe now, but it achieves the same thing without introducing new
traits:
We have two options:
1) We use `<input_ty as TryFrom<repr_ty>>::try_from` instead of writing
the `fits` function ourself.
2) We require `input_ty: From<repr_ty>`.
The first option would still check every variant and should behave the
same as your current code.
Option 2 allows us to avoid the const altogether, but requires us to
choose the smallest integer as the representation (and if we want to be
able to use both `i8` and `u8`, we can't). I missed this before, so
using option 1 might be the only way to allow conversions of this kind.
>> > + let qualified_i128: syn::Path = parse_quote! { ::core::primitive::i128 };
>> > + let qualified_u128: syn::Path = parse_quote! { ::core::primitive::u128 };
>> > +
>> > + let input_min = input_ty.emit_min();
>> > + let input_max = input_ty.emit_max();
>> > +
>> > + let variant_fits = variants.iter().map(|variant| {
>> > + let msg = format!(
>> > + "enum discriminant overflow: \
>> > + `{enum_ident}::{variant}` does not fit in `{input_ty}`",
>> > + );
>> > + ::quote::quote! {
>> > + ::core::assert!(fits(#enum_ident::#variant as #repr_ty), #msg);
>> > + }
>> > + });
>> > +
>> > + ::quote::quote! {
>> > + const _: () = {
>> > + const fn fits(d: #repr_ty) -> ::core::primitive::bool {
>> > + // For every integer type, its minimum value always fits in `i128`.
>> > + let dst_min = #input_min;
>> > + // For every integer type, its maximum value always fits in `u128`.
>> > + let dst_max = #input_max;
>> > +
>> > + #[allow(unused_comparisons)]
>> > + let is_src_signed = #repr_ty::MIN < 0;
>> > + #[allow(unused_comparisons)]
>> > + let is_dst_signed = dst_min < 0;
>> > +
>> > + if is_src_signed && is_dst_signed {
>> > + // Casting from a signed value to `i128` does not overflow since
>> > + // `i128` is the largest signed primitive integer type.
>> > + (d as #qualified_i128) >= (dst_min as #qualified_i128)
>> > + && (d as #qualified_i128) <= (dst_max as #qualified_i128)
>> > + } else if is_src_signed && !is_dst_signed {
>> > + // Casting from a signed value greater than 0 to `u128` does not
>> > + // overflow since `u128::MAX` is greater than `i128::MAX`.
>> > + d >= 0 && (d as #qualified_u128) <= (dst_max as #qualified_u128)
>> > + } else {
>> > + // Casting from an unsigned value to `u128` does not overflow since
>> > + // `u128` is the largest unsigned primitive integer type.
>> > + (d as #qualified_u128) <= (dst_max as #qualified_u128)
>> > + }
>> > + }
>> > +
>> > + #(#variant_fits)*
>> > + };
>> > + }
>> > + }
>> > +}
>>
>> > +
>> > +impl fmt::Display for ValidTy {
>> > + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
>> > + match self {
>> > + Self::Bounded(inner) => inner.fmt(f),
>> > + Self::Primitive(ident) => ident.fmt(f),
>> > + }
>> > + }
>> > +}
>> > +
>> > +struct Bounded {
>> > + name: Ident,
>> > + open_angle: Token![<],
>> > + base_ty: Ident,
>>
>> We should allow a type here from syntax and then emit an error when it
>> isn't a primitive.
>
> Assuming you're talking about `syn::Type`, that seems better. I think I
> should do the same thing for `ValidTy::Primitive`.
Sounds good!
>> > + comma: Token![,],
>> > + bits: LitInt,
>> > + close_angle: Token![>],
>> > +}
>>
>> > diff --git a/rust/macros/lib.rs b/rust/macros/lib.rs
>> > index b38002151871a33f6b4efea70be2deb6ddad38e2..02528d7212b75d28788f0c33479edb272fa12e27 100644
>> > --- a/rust/macros/lib.rs
>> > +++ b/rust/macros/lib.rs
>> > @@ -14,6 +14,7 @@
>> > #[macro_use]
>> > mod quote;
>> > mod concat_idents;
>> > +mod convert;
>> > mod export;
>> > mod fmt;
>> > mod helpers;
>> > @@ -23,6 +24,10 @@
>> > mod vtable;
>> >
>> > use proc_macro::TokenStream;
>> > +use syn::{
>> > + parse_macro_input,
>> > + DeriveInput, //
>> > +};
>> >
>> > /// Declares a kernel module.
>> > ///
>> > @@ -475,3 +480,155 @@ pub fn paste(input: TokenStream) -> TokenStream {
>> > pub fn kunit_tests(attr: TokenStream, ts: TokenStream) -> TokenStream {
>> > kunit::kunit_tests(attr, ts)
>> > }
>> > +
>> > +/// A derive macro for providing an implementation of the [`Into`] trait.
>> > +///
>> > +/// This macro automatically derives the [`Into`] trait for a given enum by generating
>> > +/// the relevant [`From`] implementation. Currently, it only supports [unit-only enum]s.
>> > +///
>> > +/// [unit-only enum]: https://doc.rust-lang.org/reference/items/enumerations.html#r-items.enum.unit-only
>> > +///
>> > +/// # Notes
>> > +///
>> > +/// - Unlike its name suggests, the macro actually generates [`From`] implementations
>> > +/// which automatically provide corresponding [`Into`] implementations.
>> > +///
>> > +/// - The macro uses the `into` custom attribute or `repr` attribute to generate [`From`]
>> > +/// implementations. `into` always takes precedence over `repr`.
>>
>> What do you mean by "precedence" here? You always generate it for all
>> helper_tys and the repr?
>
> To quote my reply above:
>
> The current implementation completely ignores what's in
> `#[repr(...)]` when the `#[into(...)]` or `#[try_from(...)]` helper
> attributes are specified.
>
> That's why I used the term "precedence." But as I said, I'll change the
> logic to generate impls for both.
Oh yeah now it makes sense :)
> Thanks a lot for the time and effort you put into reviewing it!
My pleasure :)
Cheers,
Benno
Powered by blists - more mailing lists