Re: [PATCH v4 2/3] rust: time: make Delta division and remainder fail consistently

[Andreas Hindborg]

"chenhan" <chenhan0017.work@xxxxxxxxx> writes:

> Delta's division and rem_nanos() use Rust operators on 64-bit and C
> helpers on 32-bit. The report shows that rem_nanos(0) can return 10 for
> a 10 ns dividend on ARMv7, while the same call panics on x86-64. The
> i64::MIN / -1 case also differs, and neither API documents these inputs.
>
> Check for zero divisors and i64::MIN with -1 before the architecture-
> specific code, and document the panic conditions on each method. This
> matches Rust's signed division and remainder operators, including when
> overflow checks are disabled. Valid-input results and API signatures
> are unchanged; invalid inputs now panic on 32-bit too.
>
> No in-tree calls to either API were found at the base commit. The checks
> stay in the existing methods so the fix does not depend on introducing
> a math module.
>
> Fixes: 4521438fb076 ("rust: time: Implement basic arithmetic operations for Delta")
> Reported-by: Georgios Androutsopoulos <georgeandrout13@xxxxxxxxx>
> Closes: https://github.com/Rust-for-Linux/linux/issues/1254
> Cc: stable@xxxxxxxxxxxxxxx
> Assisted-by: LLM
> Signed-off-by: chenhan <chenhan0017.work@xxxxxxxxx>
> ---
> Changes since v3: none.
>
>  rust/kernel/time.rs | 30 ++++++++++++++++++++++++++++--
>  1 file changed, 28 insertions(+), 2 deletions(-)
>
> diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
> index 16b44e82d2ac..4415e91a5227 100644
> --- a/rust/kernel/time.rs
> +++ b/rust/kernel/time.rs
> @@ -412,8 +412,20 @@ fn mul_assign(&mut self, rhs: i64) {
>  impl ops::Div for Delta {
>      type Output = i64;
>
> +    /// # Panics
> +    ///
> +    /// Panics if `rhs` is zero, or if `self` represents [`i64::MIN`] nanoseconds
> +    /// and `rhs` represents `-1` nanosecond.
>      #[inline]
>      fn div(self, rhs: Self) -> Self::Output {
> +        if rhs.value == 0 {
> +            panic!("attempt to divide by zero");
> +        }
> +
> +        if self.value == i64::MIN && rhs.value == -1 {
> +            panic!("attempt to divide with overflow");
> +        }
> +
>          #[cfg(CONFIG_64BIT)]
>          {
>              self.value / rhs.value
> @@ -421,7 +433,7 @@ fn div(self, rhs: Self) -> Self::Output {
>
>          #[cfg(not(CONFIG_64BIT))]
>          {
> -            // SAFETY: This function is always safe to call regardless of the input values
> +            // SAFETY: The checks above exclude a zero divisor and `i64::MIN / -1`.

Why change the safety comment here? If the function is always safe to
call, the exclusion is not relevant. Was the old comment wrong?

>              unsafe { bindings::div64_s64(self.value, rhs.value) }
>          }
>      }
> @@ -615,8 +627,21 @@ pub fn to_jiffies_timeout(self) -> Delta<Jiffy> {
>      }
>
>      /// Return `self % divisor` where `divisor` is in nanoseconds.
> +    ///
> +    /// # Panics
> +    ///
> +    /// Panics if `divisor` is zero, or if `self` represents [`i64::MIN`]
> +    /// nanoseconds and `divisor` is `-1`.
>      #[inline]
>      pub fn rem_nanos(self, divisor: i32) -> Self {
> +        if divisor == 0 {
> +            panic!("attempt to calculate the remainder with a divisor of zero");
> +        }
> +
> +        if self.value == i64::MIN && divisor == -1 {
> +            panic!("attempt to calculate the remainder with overflow");
> +        }
> +
>          #[cfg(CONFIG_64BIT)]
>          {
>              Self {
> @@ -628,7 +653,8 @@ pub fn rem_nanos(self, divisor: i32) -> Self {
>          {
>              let mut rem = 0;
>
> -            // SAFETY: `rem` is in the stack, so we can always provide a valid pointer to it.
> +            // SAFETY: The checks above exclude a zero divisor and `i64::MIN / -1`.
> +            // `&mut rem` provides a valid, aligned, writable pointer for this call.

Again, was the old safety comment wrong?

Once kunit supports the `#[should_panic]` attribute, we can assert the
behavior on all platforms.

Best regards,
Andreas Hindborg