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

[chenhan <chenhan0017.work@gmail.com>]

Hi Andreas, Miguel, Tomonori and Georgios,

Thanks for the reviews. Here are the details behind the changes in v2.

Miguel wrote:
> This does not add much information, since we don't have the reproducer
> here, i.e. what is `CONFIG_SAMPLE_RUST_REPRO`? I would instead explain
> what the reproducer is, if it is important to do so.

You're right; CONFIG_SAMPLE_RUST_REPRO was a local Kconfig option for
samples/rust/rust_repro.rs, and neither was included in v1. Setting it to
y built the test module into the kernel. Its init function read a dividend
(i64), a divisor (i32) and an operation selector from module parameters,
then evaluated one of these expressions using kernel::time::Delta:

    Delta::from_nanos(dividend) / Delta::from_nanos(i64::from(divisor))
    Delta::from_nanos(dividend).rem_nanos(divisor).as_nanos()

The inputs came from the kernel command line. Each failing operation ran
in a separate QEMU boot, since a panic stops that test. The expected
results with the fix are:

    dividend    divisor    division        remainder (nanoseconds)
    10          0          panic           panic
    i64::MIN    -1         panic           panic
    10          3          3               1

V2 replaces the unexplained config name with a description of the test.
For v2 I used an expanded test module: it passes operands through
core::hint::black_box to exercise the actual APIs, checks 26 valid
divisions and 21 valid remainders per architecture, and runs the four
panic cases above in separate boots. All passed on x86-64 and ARMv7.
The division tests also cover full-width i64 divisors. Both kernel test
configurations had CONFIG_RUST_OVERFLOW_CHECKS=y; I have not boot-tested
with it disabled. The guards themselves are unconditional, consistent
with the Rust semantics discussed in the issue and cited in v2.

On Andreas' and Miguel's suggestion to move generic helpers to math.rs,
Tomonori wrote:
> Do you have other users in mind? If not, each helper has only one
> caller, so they could go directly into div() and rem_nanos().

I tried the math.rs version, then checked for other users. Each helper
still had only one caller. Time-unit conversions use fixed positive
divisors, to_jiffies_timeout() uses unsigned arithmetic, and
num/bounded.rs operates on generic integer types. None is a concrete
extra user for these fixed-type signed helpers. V2 therefore follows
Tomonori's suggestion: the checks are directly in the two methods, with
no generic free functions in time.rs and no new math module.

Miguel wrote:
> However, this changes behavior -- did you check all callers etc.?

I checked the tracked Rust sources, including drivers, samples and lib,
at timekeeping-next 2ea0119f72db (the v2 base) and rust-next c82c75ae11fa.
I searched named calls and inspected division expressions in Delta and
Instant users, including values obtained through elapsed(). I found no
in-tree calls to either API outside their definitions. Existing users
construct timeouts, compare elapsed times, or convert to scalar units.
Valid-input arithmetic is unchanged; invalid inputs on 32-bit now panic.
The audit does not cover out-of-tree users or all stable branches. I kept
the stable Cc alongside Fixes as suggested; please let me know if this
consistency fix is unsuitable for backporting without an affected caller.

Miguel wrote:
> What "both operands are passed by value" is trying to say? i.e. what
> precondition are you trying to satisfy?
> [...]
> What happens in the `min / -1` case?

Passing by value did not establish a relevant safety precondition. The
comments now state that the divisor is non-zero and the quotient is
representable. div_s64_rem() computes a quotient even though we discard
it. The MIN / -1 guard was already before the C call in v1; v2 keeps it
and makes that condition explicit in the SAFETY comment. Without the
guard, the generic 32-bit C implementation can produce a wrapped quotient
and a zero remainder under the kernel's C arithmetic flags. Rust's signed
remainder operator panics for this input even though the mathematical
remainder is zero, so we reject it to match Rust's semantics. The pointer
comment also explains that &mut rem provides a valid, aligned, writable
i32 for the call.

For the documentation comments from Miguel and Tomonori, I moved
# Panics immediately above fn div(), removed the redundant architecture
sentence and private-helper references, and added intra-doc links for
i32, i64 and i64::MIN. I also removed the claim of identical inputs:
Delta division takes a Delta holding i64 nanoseconds, while rem_nanos()
takes an i32 divisor. Each method documents its own panic conditions.

Miguel wrote:
> The kernel requires a "known identity", is "chenhan" one?

Yes. chenhan is the identity I consistently use, and
chenhan0017.work@gmail.com is my email address. I also participate in
issue #1254 as WindDevil on GitHub. V2 uses that identity consistently in
From and Signed-off-by.

V2 includes all M: and R: contacts in both requested MAINTAINERS entries:
DELAY, SLEEP, TIMEKEEPING, TIMERS [RUST] and RUST. It also retains the
discussion participants and mailing lists.

Georgios: I added the requested tag to the commit message:
Reported-by: Georgios Androutsopoulos <georgeandrout13@gmail.com>

Andreas: sorry about the HTML reply and sending it only to you. I will
use plain text and reply-all for this discussion.

Best regards,
chenhan