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]

Date: Fri, 12 Apr 2024 09:43:24 +0200
From: Philipp Stanner <pstanner@...hat.com>
To: Miguel Ojeda <miguel.ojeda.sandonis@...il.com>, 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, 2024-04-12 at 09:14 +0200, Miguel Ojeda wrote:
> 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`).

Is that going to remain enabled by default or what was the plan here?

P.


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