Received-SPF: pass (google.com: domain of linux-kernel+bounces-143517-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240411230801.1504496-1-boqun.feng@gmail.com>
 <20240411230801.1504496-3-boqun.feng@gmail.com> <CANiq72nSSAVkynVaAq7bQKoL6N8K2JUXp8AOVvu7vN+siAhk-Q@mail.gmail.com>
 <Zhk4Uqc4LImR2n_r@Boquns-Mac-mini.home> <CANiq72ka4UvJzb4dN12fpA1WirgDHXcvPurvc7B9t+iPUfWnew@mail.gmail.com>
 <ZhngOs7Xze65i9Qy@Boquns-Mac-mini.home>
In-Reply-To: <ZhngOs7Xze65i9Qy@Boquns-Mac-mini.home>
From: Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>
Date: Sat, 13 Apr 2024 04:16:52 +0200
Message-ID: <CANiq72mwx6uA4cWRjpUxu4E1MLZhnofuwXLBnYFn-eUG7-aN3w@mail.gmail.com>
Subject: Re: [PATCH 2/2] rust: time: Use wrapping_sub() for Ktime::sub()
To: Boqun Feng <boqun.feng@gmail.com>
Cc: Thomas Gleixner <tglx@linutronix.de>, Miguel Ojeda <ojeda@kernel.org>, 
	John Stultz <jstultz@google.com>, Stephen Boyd <sboyd@kernel.org>, 
	Alex Gaynor <alex.gaynor@gmail.com>, Wedson Almeida Filho <wedsonaf@gmail.com>, Gary Guo <gary@garyguo.net>, 
	bjorn3_gh@protonmail.com, Benno Lossin <benno.lossin@proton.me>, 
	Andreas Hindborg <a.hindborg@samsung.com>, Alice Ryhl <aliceryhl@google.com>, 
	rust-for-linux@vger.kernel.org, linux-kernel@vger.kernel.org, 
	Vegard Nossum <vegard.nossum@oracle.com>, Kees Cook <kees@kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Sat, Apr 13, 2024 at 3:30=E2=80=AFAM Boqun Feng <boqun.feng@gmail.com> w=
rote:
>
> 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

Yeah, it is intended to be used in production (for those that want it,
i.e. not everybody will want it; and probably we will want to provide
other modes of operation when the check fails as I was mentioning,
e.g. report and continue with wrap).

> wrong, that UBSAN is also a feature that production kernel can or
> already use?). If so, then the current `Ktime::sub()` has a different

Generally userspace sanitizers aren't, but they may be (like the
minimal runtime in Clang).

Not sure about the "status" of each kernel one, Cc'ing Kees (sorry,
you got dropped), but I imagine things like the integer overflow ones
with `UBSAN_TRAP=3Dy` may be fine in production.

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

Agreed that we should generally avoid surprises, but here the C side
may be actually expecting the same (i.e. no overflows).

But if that is not the case, and then you think we should call this a
different name than `Ktime` to avoid confusion, that is fair.

> 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()`.

Yeah, but as I was mentioning, in the `add` case, it seems like it is
not intended to overflow. Thus one could assume (perhaps naively) the
subtraction isn't, either.

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

Indeed :)

Cheers,
Miguel