[PATCH v4 1/3] rust: time: rename the rem_nanos() divisor

[PATCH v4 1/3] rust: time: rename the rem_nanos() divisor

[chenhan]

The argument to rem_nanos() is the divisor, while self holds the
dividend. Rename the argument and correct the documentation. Drop the
architecture-specific implementation explanation from the public API
documentation; the argument type already states the divisor's width.

This does not change the function's type or behavior. It is a preparation
for documenting and checking the invalid divisors in the next patch.

Cc: stable@vger.kernel.org # prerequisite for the Delta division/remainder fix
Reviewed-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
Assisted-by: LLM
Signed-off-by: chenhan <chenhan0017.work@gmail.com>
---
Changes since v3:
- Carry FUJITA Tomonori's Reviewed-by; code unchanged.

 rust/kernel/time.rs | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
index 5ae377e70dd8..16b44e82d2ac 100644
--- a/rust/kernel/time.rs
+++ b/rust/kernel/time.rs
@@ -614,16 +614,13 @@ pub fn to_jiffies_timeout(self) -> Delta<Jiffy> {
         Delta::<Jiffy>::from_jiffies(jiffies)
     }
 
-    /// Return `self % dividend` where `dividend` is in nanoseconds.
-    ///
-    /// The kernel doesn't have any emulation for `s64 % s64` on 32 bit platforms, so this is
-    /// limited to 32 bit dividends.
+    /// Return `self % divisor` where `divisor` is in nanoseconds.
     #[inline]
-    pub fn rem_nanos(self, dividend: i32) -> Self {
+    pub fn rem_nanos(self, divisor: i32) -> Self {
         #[cfg(CONFIG_64BIT)]
         {
             Self {
-                value: self.as_nanos() % i64::from(dividend),
+                value: self.as_nanos() % i64::from(divisor),
             }
         }
 
@@ -632,7 +629,7 @@ pub fn rem_nanos(self, dividend: i32) -> Self {
             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) };
+            unsafe { bindings::div_s64_rem(self.as_nanos(), divisor, &mut rem) };
 
             Self {
                 value: i64::from(rem),
-- 
2.34.1

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

[chenhan]

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@gmail.com>
Closes: https://github.com/Rust-for-Linux/linux/issues/1254
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: chenhan <chenhan0017.work@gmail.com>
---
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`.
             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.
             unsafe { bindings::div_s64_rem(self.as_nanos(), divisor, &mut rem) };
 
             Self {
-- 
2.34.1

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

[Andreas Hindborg]

"chenhan" <chenhan0017.work@gmail.com> 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@gmail.com>
> Closes: https://github.com/Rust-for-Linux/linux/issues/1254
> Cc: stable@vger.kernel.org
> Assisted-by: LLM
> Signed-off-by: chenhan <chenhan0017.work@gmail.com>
> ---
> 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

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

[chenhan]

Hi Andreas,

Thanks for asking. I should have distinguished the non-zero-divisor
requirement from the MIN/-1 check in those comments.

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

The old "regardless of the input values" statement is too broad. In the
generic 32-bit implementation, div64_s64() goes through div64_u64() and
div_u64_rem() to do_div(). The latter contains C division and remainder
operations which require a non-zero divisor. The kernel's
-fno-strict-overflow does not define division by zero. An architecture's
divide-by-zero handling therefore does not establish that the C call is
safe for every input across implementations.

The zero-divisor check above the call supplies that precondition. I have
updated the local comments for the next revision to cite that check
explicitly, without grouping it with MIN/-1. For the division call:

    // SAFETY: `rhs.value` is non-zero due to the check above.

The MIN/-1 check preserves Rust's panic semantics; the generic
32-bit helpers use unsigned magnitude division and wrapping sign
arithmetic, so I should not present its rejection as a necessary safety
precondition of those implementations.

> Again, was the old safety comment wrong?

The old pointer explanation was correct: the local rem supplies valid
storage for the output. I expanded it to make the validity, alignment
and writability of &mut rem explicit. The signed remainder helper also
reaches the division path above, so the comment should explain both
the non-zero divisor, established by the check, and the valid output
pointer. The updated comments keep these separate from the MIN/-1
behavior check; both runtime checks are unchanged.

I have made the corresponding comment changes in the math helpers in
patch 3 as well, so the explanation remains consistent after extraction.

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

Agreed; that would let us keep these panic cases as in-tree regression
tests when the support is available.

Best regards,
chenhan