Re: [PATCH 2/2] rust: time: Use wrapping_sub() for Ktime::sub()

[Miguel Ojeda]

On Fri, Apr 12, 2024 at 1:08 AM Boqun Feng <boqun.feng@xxxxxxxxx> wrote:
>
> Currently since Rust code is compiled with "-Coverflow-checks=y", so a

Nit: it is enabled by default, but configurable (`CONFIG_RUST_OVERFLOW_CHECKS`).

> although overflow detection is nice to have, however this makes
> `Ktime::sub()` behave differently than `ktime_sub()`, moreover it's not
> clear that the overflow checking is helpful, since for example, the
> current binder usage[1] doesn't have the checking.
>
> Therefore make `Ktime::sub()` have the same semantics as `ktime_sub()`:
> overflow behaves like 2s-complement wrapping sub.

If `ktime_sub()`'s callers rely on wrapping in some cases, then an
alternative we should consider is having a method for explicitly
wrapping, like the integers. This would allow callers to decide and it
would make the expected semantics clear since the beginning (which is
the easiest time to add this kind of thing) for Rust code.

Otherwise, I agree we should at least document the preconditions clearly.

Having said that, I see a `ktime_add_unsafe()` too, which was added
due to a UBSAN report for `ktime_add()` in commit 979515c56458 ("time:
Avoid undefined behaviour in ktime_add_safe()"). There is also a
private `ktime_add_safe()` too, which is a saturating one.

So, given that, can callers actually rely on wrapping for these
functions, or not? The documentation on the C side could perhaps be
clarified here (including the mention of UB in `ktime_add_unsafe()` --
we use `-fno-strict-overflow`) and perhaps using the `wrapping_*()` C
functions too.

In addition, Binder calls `ktime_ms_delta()`, not `ktime_sub()`,
right? In that case the arguments are called `later` and `earlier`,
perhaps those have a different expectation even if `ktime_sub()` is
allowed to overflow and thus it would make sense to check in that
function only instead? (and document accordingly)

Thanks!

Cheers,
Miguel