linux-kernel - Re: [PATCH 2/2] rust: time: Implement basic arithmetic operations for Delta

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: <aIXVzIwBDvY1ZVjL@google.com>
Date: Sun, 27 Jul 2025 07:31:24 +0000
From: Alice Ryhl <aliceryhl@...gle.com>
To: Lyude Paul <lyude@...hat.com>
Cc: rust-for-linux@...r.kernel.org, Thomas Gleixner <tglx@...utronix.de>, 
	Boqun Feng <boqun.feng@...il.com>, linux-kernel@...r.kernel.org, 
	Andreas Hindborg <a.hindborg@...nel.org>, FUJITA Tomonori <fujita.tomonori@...il.com>, 
	Frederic Weisbecker <frederic@...nel.org>, Anna-Maria Behnsen <anna-maria@...utronix.de>, 
	John Stultz <jstultz@...gle.com>, Stephen Boyd <sboyd@...nel.org>, Miguel Ojeda <ojeda@...nel.org>, 
	Alex Gaynor <alex.gaynor@...il.com>, Gary Guo <gary@...yguo.net>, 
	"Björn Roy Baron" <bjorn3_gh@...tonmail.com>, Benno Lossin <lossin@...nel.org>, 
	Trevor Gross <tmgross@...ch.edu>, Danilo Krummrich <dakr@...nel.org>
Subject: Re: [PATCH 2/2] rust: time: Implement basic arithmetic operations for Delta

On Thu, Jul 24, 2025 at 02:54:07PM -0400, Lyude Paul wrote:
> While rvkms is only going to be using a few of these, since Deltas are
> basically the same as i64 it's easy enough to just implement all of the
> basic arithmetic operations for Delta types.
> 
> Note that for division and remainders, we currently limit these operations
> to CONFIG_64BIT as u64 / u64 and u64 % u64 is not supported on all 32 bit
> platforms natively. The correct solution we want to aim for here in the
> future is to use the kernel's math library for performing these operations
> so they're emulated on 32 bit platforms.

The CONFIG_64BIT restriction seems annoying. Could we not support 32-bit
from the get-go? Where is this going to be used?

After all, we have stuff like this:
https://lore.kernel.org/r/20250724165441.2105632-1-ojeda@kernel.org

> +impl ops::Mul for Delta {
> +    type Output = Self;
> +
> +    fn mul(self, rhs: Self) -> Self::Output {
> +        Self {
> +            nanos: self.nanos * rhs.nanos,
> +        }
> +    }
> +}
> +
> +impl ops::MulAssign for Delta {
> +    fn mul_assign(&mut self, rhs: Self) {
> +        self.nanos *= rhs.nanos;
> +    }
> +}

The units here do not make sense. I would not add multiplication of
Delta*Delta. It makes sense to have Delta*int, but it does not make
sense to multiply two Deltas together.

I would change the second type for both multiplication operators to be a
normal integer.

> +// TODO: When we get support for u64/u64 division and remainders helpers remove this, until then
> +// these ops only work on 64bit platforms.
> +#[cfg(CONFIG_64BIT)]
> +impl ops::Div for Delta {
> +    type Output = Self;
> +
> +    fn div(self, rhs: Self) -> Self::Output {
> +        Self {
> +            nanos: self.nanos / rhs.nanos,
> +        }
> +    }
> +}
> +
> +#[cfg(CONFIG_64BIT)]
> +impl ops::DivAssign for Delta {
> +    fn div_assign(&mut self, rhs: Self) {
> +        self.nanos /= rhs.nanos;
> +    }
> +}

Same here. The units don't work. If you divide two deltas by each other,
the correct unit is to return a kind of integer, not another Delta.

I would change Div to have an integer type as output and get rid of
DivAssign.

> +#[cfg(CONFIG_64BIT)]
> +impl ops::Rem for Delta {
> +    type Output = Self;
> +
> +    fn rem(self, rhs: Self) -> Self::Output {
> +        Self {
> +            nanos: self.nanos % rhs.nanos,
> +        }
> +    }
> +}
> +
> +#[cfg(CONFIG_64BIT)]
> +impl ops::RemAssign for Delta {
> +    fn rem_assign(&mut self, rhs: Self) {
> +        self.nanos %= rhs.nanos;
> +    }
> +}

The units here do make sense, so these are fine.

Alice