Date: Fri, 12 Apr 2024 18:30:34 -0700
From: Boqun Feng <boqun.feng@...il.com>
To: Miguel Ojeda <miguel.ojeda.sandonis@...il.com>
Cc: Thomas Gleixner <tglx@...utronix.de>, Miguel Ojeda <ojeda@...nel.org>,
	John Stultz <jstultz@...gle.com>, Stephen Boyd <sboyd@...nel.org>,
	Alex Gaynor <alex.gaynor@...il.com>,
	Wedson Almeida Filho <wedsonaf@...il.com>,
	Gary Guo <gary@...yguo.net>, bjorn3_gh@...tonmail.com,
	Benno Lossin <benno.lossin@...ton.me>,
	Andreas Hindborg <a.hindborg@...sung.com>,
	Alice Ryhl <aliceryhl@...gle.com>, rust-for-linux@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	Vegard Nossum <vegard.nossum@...cle.com>
Subject: Re: [PATCH 2/2] rust: time: Use wrapping_sub() for Ktime::sub()

On Fri, Apr 12, 2024 at 04:41:26PM +0200, Miguel Ojeda wrote:
> On Fri, Apr 12, 2024 at 3:34 PM Boqun Feng <boqun.feng@...il.com> 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.

Seems we have a different reading of `ktime_sub()` ;-)

Based on your reply to Philipp, I take it that
CONFIG_RUST_CHECK_OVERFLOWS can be enabled in a production kernel,
right? IOW, it's not a debug-only feature like UBSAN (or maybe I'm way
wrong, that UBSAN is also a feature that production kernel can or
already use?). If so, then the current `Ktime::sub()` has a different
behavior compared to `ktime_sub()`: it will perform overflow checks and
panic (which is BUG()) in production kernels.

Now I wasn't trying to say substraction overflows shouldn't be checked
(by default), the thing is that `Ktime` is just a `ktime_t` wrapper, so
it's natural that it provides as least difference as possible. If it was
a standalone abstraction, then by all means let's add different APIs for
different purpose.

If you look at ktime API, ktime_sub() is the only one doing
substraction between two ktime_t, there is no raw or unsafe or safe API,
So as a minimal abstraction, it's natural for a user to expect
`Ktime::sub()` behaves like `ktime_sub()`.

That's my reasoning, but it depends one a few "if"s and what time
subsystem wants to do.

> `_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 it's wise to just wait for them to reply, I don't think you and I
have much disagree other than ktime_t API semantics ;-)

Regards,
Boqun

> > 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

Powered by blists - more mailing lists