[PATCH] rust: time: make Delta division and remainder fail consistently

[PATCH] rust: time: make Delta division and remainder fail consistently

[chenhan]

On 64-bit, Delta division and remainder use Rust operators, which panic
for a zero divisor and for the i64::MIN / -1 overflow case. On 32-bit,
the C helpers do not provide the same behavior, so the same API calls can
return architecture-dependent values or emit a divide-by-zero diagnostic.

Check both invalid inputs before selecting the architecture-specific
implementation. This gives both APIs the same panic conditions as i64's
`/` and `%` operators and documents them in the public API.

Tested on x86-64 and ARMv7 QEMU with CONFIG_SAMPLE_RUST_REPRO=y: zero
divisors and i64::MIN / -1 panic for both APIs, while 10 / 3 returns 3
and 10 % 3 returns 1 on both architectures.

Fixes: 4521438fb076 ("rust: time: Implement basic arithmetic operations for Delta")
Closes: https://github.com/Rust-for-Linux/linux/issues/1254
Assisted-by: LLM
Signed-off-by: chenhan <chenhan0017.work@gmail.com>
---
 rust/kernel/time.rs | 103 +++++++++++++++++++++++++++++++-------------
 1 file changed, 72 insertions(+), 31 deletions(-)

diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
index 6c0a5e8090d0..55c90365ba73 100644
--- a/rust/kernel/time.rs
+++ b/rust/kernel/time.rs
@@ -405,21 +405,65 @@ fn mul_assign(&mut self, rhs: i64) {
     }
 }
 
+#[inline]
+fn div_s64_or_panic(dividend: i64, divisor: i64) -> i64 {
+    if divisor == 0 {
+        panic!("attempt to divide by zero");
+    }
+
+    if dividend == i64::MIN && divisor == -1 {
+        panic!("attempt to divide with overflow");
+    }
+
+    #[cfg(CONFIG_64BIT)]
+    {
+        dividend / divisor
+    }
+
+    #[cfg(not(CONFIG_64BIT))]
+    {
+        // SAFETY: `divisor` is non-zero, and both operands are passed by value.
+        unsafe { bindings::div64_s64(dividend, divisor) }
+    }
+}
+
+#[inline]
+fn rem_s64_or_panic(dividend: i64, divisor: i32) -> i64 {
+    if divisor == 0 {
+        panic!("attempt to calculate the remainder with a divisor of zero");
+    }
+
+    if dividend == i64::MIN && divisor == -1 {
+        panic!("attempt to calculate the remainder with overflow");
+    }
+
+    #[cfg(CONFIG_64BIT)]
+    {
+        dividend % i64::from(divisor)
+    }
+
+    #[cfg(not(CONFIG_64BIT))]
+    {
+        let mut rem = 0;
+
+        // SAFETY: `rem` points to a local variable and `divisor` is non-zero.
+        unsafe { bindings::div_s64_rem(dividend, divisor, &mut rem) };
+
+        i64::from(rem)
+    }
+}
+
+/// # Panics
+///
+/// Panics if `rhs` is zero, or if the quotient overflows, i.e. if `self` is
+/// `Delta::from_nanos(i64::MIN)` and `rhs` is `Delta::from_nanos(-1)`. Both
+/// cases panic on 32-bit as well as on 64-bit; see `div_s64_or_panic()`.
 impl ops::Div for Delta {
     type Output = i64;
 
     #[inline]
     fn div(self, rhs: Self) -> Self::Output {
-        #[cfg(CONFIG_64BIT)]
-        {
-            self.value / rhs.value
-        }
-
-        #[cfg(not(CONFIG_64BIT))]
-        {
-            // SAFETY: This function is always safe to call regardless of the input values
-            unsafe { bindings::div64_s64(self.value, rhs.value) }
-        }
+        div_s64_or_panic(self.value, rhs.value)
     }
 }
 
@@ -554,29 +598,26 @@ pub fn as_millis_ceil(self) -> i64 {
         }
     }
 
-    /// Return `self % dividend` where `dividend` is in nanoseconds.
+    /// Return `self % divisor`, where `divisor` is a number of nanoseconds.
+    ///
+    /// The result has the sign of `self`, and its magnitude is strictly smaller
+    /// than that of `divisor`.
     ///
-    /// The kernel doesn't have any emulation for `s64 % s64` on 32 bit platforms, so this is
-    /// limited to 32 bit dividends.
+    /// `divisor` is a 32-bit integer because the helper called on 32-bit
+    /// platforms, `div_s64_rem()`, takes an `s32` divisor. The dividend (`self`)
+    /// is a full `i64` on every architecture, so the width restriction applies
+    /// to all configurations, not only to 32-bit ones.
+    ///
+    /// # Panics
+    ///
+    /// Panics if `divisor` is zero, or if the division overflows, i.e. if `self`
+    /// is `Delta::from_nanos(i64::MIN)` and `divisor` is `-1`. These are the same
+    /// inputs [`ops::Div`] panics on, and the same ones `i64`'s `%` operator
+    /// panics on. See `rem_s64_or_panic()`.
     #[inline]
-    pub fn rem_nanos(self, dividend: i32) -> Self {
-        #[cfg(CONFIG_64BIT)]
-        {
-            Self {
-                value: self.as_nanos() % i64::from(dividend),
-            }
-        }
-
-        #[cfg(not(CONFIG_64BIT))]
-        {
-            let mut rem = 0;
-
-            // SAFETY: `rem` is in the stack, so we can always provide a valid pointer to it.
-            unsafe { bindings::div_s64_rem(self.as_nanos(), dividend, &mut rem) };
-
-            Self {
-                value: i64::from(rem),
-            }
+    pub fn rem_nanos(self, divisor: i32) -> Self {
+        Self {
+            value: rem_s64_or_panic(self.as_nanos(), divisor),
         }
     }
 }

base-commit: d266640c6c760c9bc215bf5a3ece122ca488b6f5
-- 
2.34.1

Re: [PATCH] rust: time: make Delta division and remainder fail consistently

[Andreas Hindborg]

"chenhan" <chenhan0017.work@gmail.com> writes:

> On 64-bit, Delta division and remainder use Rust operators, which panic
> for a zero divisor and for the i64::MIN / -1 overflow case. On 32-bit,
> the C helpers do not provide the same behavior, so the same API calls can
> return architecture-dependent values or emit a divide-by-zero diagnostic.
>
> Check both invalid inputs before selecting the architecture-specific
> implementation. This gives both APIs the same panic conditions as i64's
> `/` and `%` operators and documents them in the public API.
>
> Tested on x86-64 and ARMv7 QEMU with CONFIG_SAMPLE_RUST_REPRO=y: zero
> divisors and i64::MIN / -1 panic for both APIs, while 10 / 3 returns 3
> and 10 % 3 returns 1 on both architectures.
>
> Fixes: 4521438fb076 ("rust: time: Implement basic arithmetic operations for Delta")
> Closes: https://github.com/Rust-for-Linux/linux/issues/1254
> Assisted-by: LLM
> Signed-off-by: chenhan <chenhan0017.work@gmail.com>
> ---
>  rust/kernel/time.rs | 103 +++++++++++++++++++++++++++++++-------------
>  1 file changed, 72 insertions(+), 31 deletions(-)
>
> diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
> index 6c0a5e8090d0..55c90365ba73 100644
> --- a/rust/kernel/time.rs
> +++ b/rust/kernel/time.rs
> @@ -405,21 +405,65 @@ fn mul_assign(&mut self, rhs: i64) {
>      }
>  }
>
> +#[inline]
> +fn div_s64_or_panic(dividend: i64, divisor: i64) -> i64 {
> +    if divisor == 0 {
> +        panic!("attempt to divide by zero");
> +    }
> +
> +    if dividend == i64::MIN && divisor == -1 {
> +        panic!("attempt to divide with overflow");
> +    }
> +
> +    #[cfg(CONFIG_64BIT)]
> +    {
> +        dividend / divisor
> +    }
> +
> +    #[cfg(not(CONFIG_64BIT))]
> +    {
> +        // SAFETY: `divisor` is non-zero, and both operands are passed by value.
> +        unsafe { bindings::div64_s64(dividend, divisor) }
> +    }
> +}
> +
> +#[inline]
> +fn rem_s64_or_panic(dividend: i64, divisor: i32) -> i64 {
> +    if divisor == 0 {
> +        panic!("attempt to calculate the remainder with a divisor of zero");
> +    }
> +
> +    if dividend == i64::MIN && divisor == -1 {
> +        panic!("attempt to calculate the remainder with overflow");
> +    }
> +
> +    #[cfg(CONFIG_64BIT)]
> +    {
> +        dividend % i64::from(divisor)
> +    }
> +
> +    #[cfg(not(CONFIG_64BIT))]
> +    {
> +        let mut rem = 0;
> +
> +        // SAFETY: `rem` points to a local variable and `divisor` is non-zero.
> +        unsafe { bindings::div_s64_rem(dividend, divisor, &mut rem) };
> +
> +        i64::from(rem)
> +    }
> +}

We should probably find a better place for these. How about
rust/kernel/math.rs? Opinions?


Best regards,
Andreas Hindborg