linux-kernel - Re: [PATCH v2] 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: <CAH5fLgjOKQdKViAGArMjvXjVmYpbo_sA8xoQGwu+DCW1FDFRXA@mail.gmail.com>
Date: Mon, 28 Oct 2024 16:17:53 +0100
From: Alice Ryhl <aliceryhl@...gle.com>
To: Daniel Almeida <daniel.almeida@...labora.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>, 
	Trevor Gross <tmgross@...ch.edu>, linux-kernel@...r.kernel.org, 
	rust-for-linux@...r.kernel.org
Subject: Re: [PATCH v2] rust: kernel: add support for bits/genmask macros

On Thu, Oct 24, 2024 at 4:18 PM Daniel Almeida
<daniel.almeida@...labora.com> wrote:
>
> These macros are converted from their kernel equivalent.
>
> ---
> - Added ticks around `BIT`, and `h >=l` (Thanks, Benno).
> - Decided to keep the arguments as `expr`, as I see no issues with that
> - Added a proper example, with an assert_eq!() (Thanks, Benno)
> - Fixed the condition h <= l, which should be h >= l.
> - Checked that the assert for the condition above is described in the
>   docs.
>
> Signed-off-by: Daniel Almeida <daniel.almeida@...labora.com>

My main question here is whether this should be a macro_rules! or const fn.

One factor in favor of a macro is that if you need it to work in const
context, then you can't use generics, which means that you must
restrict it so it only works with a single integer type. On the other
hand, genmask! already only works for the u64 type, so that argument
doesn't work for genmask!.

Either way, I'd like to see the reason for your choice in the commit message.

>  rust/kernel/bits.rs | 36 ++++++++++++++++++++++++++++++++++++
>  rust/kernel/lib.rs  |  1 +
>  2 files changed, 37 insertions(+)
>
> diff --git a/rust/kernel/bits.rs b/rust/kernel/bits.rs
> new file mode 100644
> index 0000000000000000000000000000000000000000..479883984f995f6b44272fa4566a08f1c1e6b143
> --- /dev/null
> +++ b/rust/kernel/bits.rs
> @@ -0,0 +1,36 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Bit manipulation macros.
> +//!
> +//! C header: [`include/linux/bits.h`](srctree/include/linux/bits.h)
> +
> +/// Produces a literal where bit `n` is set.
> +///
> +/// Equivalent to the kernel's `BIT` macro.
> +///
> +#[macro_export]
> +macro_rules! bit {
> +    ($n:expr) => {
> +        (1 << $n)
> +    };
> +}
> +
> +/// Create a contiguous bitmask starting at bit position `l` and ending at
> +/// position `h`, where `h >= l`.
> +///
> +/// # Examples
> +/// ```
> +///     use kernel::genmask;
> +///     let mask = genmask!(39, 21);
> +///     assert_eq!(mask, 0x000000ffffe00000);
> +/// ```
> +///
> +#[macro_export]
> +macro_rules! genmask {
> +    ($h:expr, $l:expr) => {{
> +        const _: () = {
> +            assert!($h >= $l);
> +        };

Based on the error reported by the bot, you probably need to use
`$crate::build_assert!` here instead of the stdlib `assert!` macro. I
don't think the stdlib macro works in const context.

> +        ((!0u64 - (1u64 << $l) + 1) & (!0u64 >> (64 - 1 - $h)))

The implementation looks okay. I tried it:
fn main() {
    println!("{:03b}", genmask!(0,0));
    println!("{:03b}", genmask!(1,0));
    println!("{:03b}", genmask!(2,0));
    println!("{:03b}", genmask!(2,1));
}
prints:
001
011
111
110

Alice