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

[Miguel Ojeda]

On Fri, Apr 12, 2024 at 3:34 PM Boqun Feng <boqun.feng@xxxxxxxxx> wrote:
>
> That works for me, although I would prefer `Ktime::sub()` is wrapping
> sub and we have another function doing a safe version of sub.

Why? It goes against the "normal" case in integers. It is also not
what `ktime_sub()` does, which is the "normal" case here, vs.
`_unsafe()` and `_safe()` ones.

> Exactly, ktime_add_safe() doesn't panic if overflow happens, right?
> I think that's pretty clear on how time subsystem wants to handle
> overflow (saturating it, or zeroing it instead of panicing).

There are three variants in C (for addition) that I can see:

  - No suffix: not supposed to wrap.
  - `_unsafe()`: wraps.
  - `_safe()`: saturates.

The first one, in normal C, would be UB. In kernel C, it wraps but may
be detected by UBSAN (this is what Kees is re-introducing very
recently with 557f8c582a9b ("ubsan: Reintroduce signed overflow
sanitizer")).

So, in Rust terms, the three options above would map to:

  - Raw operators.
  - `wrapping_`.
  - `saturating_`.

Because the raw operators are what we use for arithmetic that is "not
supposed to wrap" too. That is, they wrap, but may be checked by the
Kconfig option. Of course, it may be worth having an intermediate
option that does not actually go for a full-blown Rust-panic for that,
but the point is that the current "not supposed to wrap" methods are
the raw operators.

All three, in fact, are "safe" in Rust terms, since none can actually
trigger UB (in kernel C at least -- it would be different in normal C:
the first one would map to an unsafe Rust method, i.e. `unchecked_`).

Instead, in the C side, `_unsafe()` seems to be used to mean instead
"you should be checking for overflow if needed, because it will never
be reported by UBSAN unlike the raw one". Again, this is based on my
reading of that commit and the docs on `_unsafe()`. It may be wrong,
or maybe the subtraction is supposed to be different. It should
probably be clarified in the C side anyway.

And, relatedly, I see that when the `union` was removed in commit
2456e8553544 ("ktime: Get rid of the union"), `ktime_add_unsafe()`
stopped returning a `ktime_t` even when both inputs are `ktime_t`s
themselves:

    static_assert(_Generic(ktime_add(a, b), ktime_t: true, default:
false)); // OK
    static_assert(_Generic(ktime_add_unsafe(a, b), ktime_t: true,
default: false)); // Bad

It returns an `u64` now, which could surprise users, and probably
should be fixed. The only user just puts the result into a `ktime_t`,
so there is no actual issue today.

> I must defer this to Thomas.

Yeah, the question on the C API was meant for Thomas et al.

> Maybe, however neither of this function probably shouldn't have the
> panic-on-overflow behavior. So I agree that overflow checking is not a
> bad thing, but when to check and how to handle overflow should be
> controlled by the users, and making the default behavior
> panic-on-overflow doesn't look reasonable to me.

Yes, it should be controlled by callers, but the point above is that,
from the looks of it, these interfaces are not meant to overflow to
begin with.

Cheers,
Miguel