Received-SPF: pass (google.com: domain of linux-kernel+bounces-143491-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Feedback-ID: iad51458e:Fastmail
Date: Fri, 12 Apr 2024 18:30:34 -0700
From: Boqun Feng <boqun.feng@gmail.com>
To: Miguel Ojeda <miguel.ojeda.sandonis@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>
Subject: Re: [PATCH 2/2] rust: time: Use wrapping_sub() for Ktime::sub()
Message-ID: <ZhngOs7Xze65i9Qy@Boquns-Mac-mini.home>
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>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CANiq72ka4UvJzb4dN12fpA1WirgDHXcvPurvc7B9t+iPUfWnew@mail.gmail.com>

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