linux-kernel - Re: [PATCH 2/2] rust: time: Use wrapping

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CANiq72nSSAVkynVaAq7bQKoL6N8K2JUXp8AOVvu7vN+siAhk-Q@mail.gmail.com>
Date: Fri, 12 Apr 2024 09:14:03 +0200
From: Miguel Ojeda <miguel.ojeda.sandonis@...il.com>
To: Boqun Feng <boqun.feng@...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
Subject: Re: [PATCH 2/2] rust: time: Use wrapping_sub() for Ktime::sub()

On Fri, Apr 12, 2024 at 1:08 AM Boqun Feng <boqun.feng@...il.com> 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