linux-kernel - Re: [PATCH v2 17/21] rust: num: Add an upward alignment helper for usize

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: <D9LEQ1U1PLO8.3N22GRY380ZM3@nvidia.com>
Date: Fri, 02 May 2025 13:57:52 +0900
From: "Alexandre Courbot" <acourbot@...dia.com>
To: "Alexandre Courbot" <acourbot@...dia.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>, "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>, "David Airlie" <airlied@...il.com>,
 "Simona Vetter" <simona@...ll.ch>, "Maarten Lankhorst"
 <maarten.lankhorst@...ux.intel.com>, "Maxime Ripard" <mripard@...nel.org>,
 "Thomas Zimmermann" <tzimmermann@...e.de>, "Jonathan Corbet"
 <corbet@....net>
Cc: "John Hubbard" <jhubbard@...dia.com>, "Ben Skeggs" <bskeggs@...dia.com>,
 "Joel Fernandes" <joelagnelf@...dia.com>, "Timur Tabi" <ttabi@...dia.com>,
 "Alistair Popple" <apopple@...dia.com>, <linux-kernel@...r.kernel.org>,
 <rust-for-linux@...r.kernel.org>, <nouveau@...ts.freedesktop.org>,
 <dri-devel@...ts.freedesktop.org>
Subject: Re: [PATCH v2 17/21] rust: num: Add an upward alignment helper for
 usize

On Thu May 1, 2025 at 9:58 PM JST, Alexandre Courbot wrote:
> From: Joel Fernandes <joelagnelf@...dia.com>
>
> This will be used in the nova-core driver where we need to upward-align
> the image size to get to the next image in the VBIOS ROM.
>
> [acourbot@...dia.com: handled conflicts due to removal of patch creating
> num.rs]
>
> Signed-off-by: Joel Fernandes <joelagnelf@...dia.com>
> Signed-off-by: Alexandre Courbot <acourbot@...dia.com>
> ---
>  rust/kernel/lib.rs |  1 +
>  rust/kernel/num.rs | 21 +++++++++++++++++++++
>  2 files changed, 22 insertions(+)
>
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index ab0286857061d2de1be0279cbd2cd3490e5a48c3..be75b196aa7a29cf3eed7c902ed8fb98689bbb50 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -67,6 +67,7 @@
>  pub mod miscdevice;
>  #[cfg(CONFIG_NET)]
>  pub mod net;
> +pub mod num;
>  pub mod of;
>  pub mod page;
>  #[cfg(CONFIG_PCI)]
> diff --git a/rust/kernel/num.rs b/rust/kernel/num.rs
> new file mode 100644
> index 0000000000000000000000000000000000000000..953c6ab012601efb3c9387b4b299e22233670c4b
> --- /dev/null
> +++ b/rust/kernel/num.rs
> @@ -0,0 +1,21 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Numerical and binary utilities for primitive types.
> +
> +/// A trait providing alignment operations for `usize`.
> +pub trait UsizeAlign {
> +    /// Aligns `self` upwards to the nearest multiple of `align`.
> +    fn align_up(self, align: usize) -> usize;
> +}

As it turns out I will also need the same functionality for u64 in a
future patch. :) Could we turn this into a more generic trait? E.g:

    /// A trait providing alignment operations for `usize`.
    pub trait Align {
        /// Aligns `self` upwards to the nearest multiple of `align`.
        fn align_up(self, align: Self) -> Self;
    }

That way it can be implemented for all basic types. I'd suggest having a local
macro that takes an arbitrary number of arguments and generates the impl blocks
for each of them, which would be invoked as:

    impl_align!(i8, u8, i16, u16, ...);

> +impl UsizeAlign for usize {
> +    fn align_up(mut self, align: usize) -> usize {
> +        self = (self + align - 1) & !(align - 1);
> +        self
> +    }
> +}

Note that there is no need to mutate `self`, the body can just be:

    (self + align - 1) & !(align - 1)

This operation can trigger overflows/underflows, so I guess for safety it
should return a `Result` and use `checked_add` and `checked_sub`, after moving
`align - 1` into a local variable to avoid checking it twice.

There is also the interesting question of, what if `align` is not a power of 2.
We should probably document the fact that it is expected to be.

> +/// Aligns `val` upwards to the nearest multiple of `align`.
> +pub const fn usize_align_up(val: usize, align: usize) -> usize {
> +    (val + align - 1) & !(align - 1)
> +}

Let's add a statement in the documentation saying that this exists for use in
`const` contexts. The `impl` version can maybe call this one directly to avoid
repeating the same operation twice.

Actually I'm not sure whether we want the `impl` block at all since this
provides the same functionality, albeit in a slightly less cosmetic way. Can
core R4L folks provide guidance about that?

Cheers,
Alex.