linux-kernel - Re: [PATCH v2 2/4] rust: add `Alignment` type

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: <CANiq72miQSuEEzKZsOhHKDah1kP+8PYcmODGqwtcaRwAggF-+g@mail.gmail.com>
Date: Mon, 4 Aug 2025 16:17:26 +0200
From: Miguel Ojeda <miguel.ojeda.sandonis@...il.com>
To: Alexandre Courbot <acourbot@...dia.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>, linux-kernel@...r.kernel.org, 
	rust-for-linux@...r.kernel.org, nouveau@...ts.freedesktop.org
Subject: Re: [PATCH v2 2/4] rust: add `Alignment` type

On Mon, Aug 4, 2025 at 1:45 PM Alexandre Courbot <acourbot@...dia.com> wrote:
>
> +/// align down/up operations. The alignment operations are done using the [`align_up!`] and
> +/// [`align_down!`] macros.

These intra-doc links don't work (they are not macros in this version at least).

> +    /// Returns the alignment of `T`.
> +    #[inline(always)]
> +    pub const fn of<T>() -> Self {
> +        // INVARIANT: `align_of` always returns a power of 2.
> +        Self(unsafe { NonZero::new_unchecked(align_of::<T>()) })

Missing safety comment (`CLIPPY=1` spots it).

Also, cannot we use `new()` here? i.e. the value will be known at compile-time.

> +        if !self.0.is_power_of_two() {
> +            // SAFETY: per the invariants, `self.0` is always a power of two so this block will
> +            // never be reached.
> +            unsafe { core::hint::unreachable_unchecked() }
> +        }

I guess this one is here to help optimize users after they inline the
cal? Is there a particular case you noticed? i.e. it may be worth
mentioning it.

> +    pub const fn mask(self) -> usize {
> +        // INVARIANT: `self.as_usize()` is guaranteed to be a power of two (i.e. non-zero), thus
> +        // `1` can safely be substracted from it.
> +        self.as_usize() - 1
> +    }

I am not sure why there is `// INVARIANT` here, since we are not
creating a new `Self`.

I guess by "safely" you are trying to say there is no overflow risk --
I would be explicit and avoid "safe", since it is safe to overflow.

Typo: subtracted

Cheers,
Miguel