linux-kernel - Re: [PATCH v8] rust: kernel: add support for bits/genmask 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] [day] [month] [year] [list]

Message-Id: <DB7DDEDNP1HF.T0MDDNPF26P3@nvidia.com>
Date: Wed, 09 Jul 2025 17:08:56 +0900
From: "Alexandre Courbot" <acourbot@...dia.com>
To: "Daniel Almeida" <daniel.almeida@...labora.com>, "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>, "Andreas
 Hindborg" <a.hindborg@...nel.org>, "Alice Ryhl" <aliceryhl@...gle.com>,
 "Trevor Gross" <tmgross@...ch.edu>, "Danilo Krummrich" <dakr@...nel.org>,
 "Benno Lossin" <lossin@...nel.org>
Cc: <linux-kernel@...r.kernel.org>, <rust-for-linux@...r.kernel.org>
Subject: Re: [PATCH v8] rust: kernel: add support for bits/genmask macros

On Wed Jul 9, 2025 at 1:50 AM JST, Daniel Almeida wrote:
<snip>
> +macro_rules! impl_genmask_fn {
> +    (
> +        $ty:ty,
> +        $(#[$genmask_checked_ex:meta])*,
> +        $(#[$genmask_ex:meta])*
> +    ) => {
> +        paste! {
> +            /// Creates a contiguous bitmask for the given range by validating
> +            /// the range at runtime.
> +            ///
> +            /// Returns [`None`] if the range is invalid, i.e.: if the start is
> +            /// greater than or equal to the end or if the range is outside of

Should be: "if the start is greater than the end", as the two being
equal is valid.

> +            /// the representable range for the type.
> +            $(#[$genmask_checked_ex])*
> +            #[inline]
> +            pub fn [<genmask_checked_ $ty>](range: RangeInclusive<u32>) -> Option<$ty> {
> +                let start = *range.start();
> +                let end = *range.end();
> +
> +                if start > end {
> +                    return None;
> +                }
> +
> +                let high = [<checked_bit_ $ty>](end)?;
> +                let low = [<checked_bit_ $ty>](start)?;
> +                Some((high | (high - 1)) & !(low - 1))
> +            }
> +
> +            /// Creates a compile-time contiguous bitmask for the given range by
> +            /// performing a compile-time assertion that the range is valid.
> +            ///
> +            /// This version is the default and should be used if the range is known
> +            /// at compile time.
> +            $(#[$genmask_ex])*
> +            #[inline]
> +            pub const fn [<genmask_ $ty>](range: RangeInclusive<u32>) -> $ty {
> +                let start = *range.start();
> +                let end = *range.end();
> +
> +                build_assert!(start <= end);
> +
> +                let high = [<bit_ $ty>](end);
> +                let low = [<bit_ $ty>](start);
> +                (high | (high - 1)) & !(low - 1)
> +            }
> +        }
> +    };
> +}
> +
> +impl_genmask_fn!(
> +    u64,
> +    /// # Examples
> +    ///
> +    /// The example below highlights the default use case, i.e., when the range
> +    /// is being built from non-constant values, which are represented here as
> +    /// the function arguments `a` and `b`.
> +    ///
> +    /// ```
> +    /// fn build_mask(a: u32, b: u32) -> Option<u64> {
> +    ///     # use kernel::bits::genmask_checked_u64;
> +    ///     // Ensures that a valid mask can be constructed for the range
> +    ///     // `a..=b` by performing runtime checks.
> +    ///     genmask_checked_u64(a..=b)
> +    /// }
> +    /// ```

This example just wraps `genmask_checked` into another function and
doesn't test it at all. I think it would be more useful if it mirrored
the non-checked equivalent, and also showed cases where `None` is
returned:

    # use kernel::bits::genmask_u64;
    assert_eq!(genmask_checked_u64(21..=39), Some(0x0000_00ff_ffe0_0000));

    // `80` is out of the supported bit range.
    assert_eq!(genmask_checked_u64(21..=80), None);
    
    // Invalid range where the start is bigger than the end.
    assert_eq!(genmask_checked_u64(15..=8), None);

Sure, here one should just use `genmask_u64`, but the function's
description already says that - here we just want to show how it works,
and test that it behaves as expected.

> +    ///
> +    /// This example tests an edge case where only the first bit is
> +    /// supposed to be set.

I don't think these comments are necessary as the examples are rather
obvious. You can put all these examples into the same code block,
separated by a blank line, without the `mask` temporary variable. Just
add an inline comment if some explanation is required for some of them.

With these last things:

Reviewed-by: Alexandre Courbot <acourbot@...dia.com>