Message-ID: <CANiq72kU+rXsw7SOtw=-fGmDyxWXjYY3zB2Kiixvn-o22BJKzA@mail.gmail.com>
Date: Wed, 24 Apr 2024 12:21:17 +0200
From: Miguel Ojeda <miguel.ojeda.sandonis@...il.com>
To: Kees Cook <keescook@...omium.org>
Cc: Boqun Feng <boqun.feng@...il.com>, 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, 
	linux-hardening@...r.kernel.org, justinstitt@...gle.com
Subject: Re: [PATCH 2/2] rust: time: Use wrapping_sub() for Ktime::sub()

On Wed, Apr 24, 2024 at 1:37 AM Kees Cook <keescook@...omium.org> wrote:
>
> While working on the signed (and unsigned) integer overflow sanitizer
> support on the C side for the kernel, I've also run into timekeeping
> being a questionable area[1]. I *think* from what I can tell, it's always
> expected to have wrapping behavior.

Thanks, that is useful. In that branch you link, since it is about
unsigned, I imagine you could have hit issues with
`ktime_add_unsafe()` -- after all, callers are expecting overflow
there. But there is a single user (which is `ktime_add_safe()`), and I
don't see that one annotated in your branch.

In any case, if `ktime_add()` is supposed to always be wrapping like
the other functions in the area as you mention, then I think
`ktime_add_unsafe()` (i.e. wrapping one) should be renamed into
`ktime_add()` and the existing one removed. And then we can discuss
whether to do (or not) that in Rust too (see below).

However, if the `ktime_add()` / `ktime_add_unsafe()` /
`ktime_add_safe()` split is there for a good reason (see [1] -- sorry,
you were not in Cc in that particular one), and callers are already
expected to respect it, then I think we should document it in the C
side better and we should just start with that approach in the Rust
side too.

> Can we define the type itself to be wrapping? (This has been my plan on
> the C side, but we're still waiting on a finalized implementation of the
> "wraps" attribute.[2])

Yeah, we can make that the "default", so to speak (i.e. what the
operators will do). But we can also have different methods with
different expectations too if needed (i.e. the usual "access" vs.
"type" discussion).

And given the different variations that exist in C (see [1]), it
seemed to me that `ktime_t` operations there may not be expected to
actually wrap, and thus that would be an argument for trying to be
explicit in Rust.

So for the Rust side, what we need here is the expectation of how
`ktime_t` is supposed to be used. Or, even better, how one would
ideally design `ktime_t` today if there were no worry about callers,
because we have the chance to improve here over the C side before we
have those callers.

If the answer is "everyone assumes those to be wrapping, and there are
almost no use cases where the callers know it is not supposed to wrap,
and we don't care about whether they wrap" etc., then yeah, we should
go with wrapping default semantics and accept that we will not gain
that information and thus not catch mistakes easily in the future.

But if the answer is "we would have liked that `ktime_t` was more
explicit, and that is why we have the `ktime_add{,_safe,_unsafe}()`
variations, but nobody uses them because everybody assumes wrapping"
or "we actually assume no wrapping in `ktime_t`, which is why
`ktime_add_*()` exist", then I think we should definitely try to be
explicit on the Rust side from the onset so that we can catch more
bugs in the future.

(I know you know all this, but I was trying to summarize and clarify
the discussion :)

[1] https://lore.kernel.org/rust-for-linux/CANiq72ka4UvJzb4dN12fpA1WirgDHXcvPurvc7B9t+iPUfWnew@mail.gmail.com/

Cheers,
Miguel

Powered by blists - more mailing lists