netdev - Re: [PATCH net-next v3 4/8] rust: time: Implement addition of Ktime and Delta

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: <ZxFfLyXiDqOva5jN@boqun-archlinux>
Date: Thu, 17 Oct 2024 12:02:07 -0700
From: Boqun Feng <boqun.feng@...il.com>
To: Miguel Ojeda <miguel.ojeda.sandonis@...il.com>
Cc: FUJITA Tomonori <fujita.tomonori@...il.com>, netdev@...r.kernel.org,
	rust-for-linux@...r.kernel.org, andrew@...n.ch,
	hkallweit1@...il.com, tmgross@...ch.edu, ojeda@...nel.org,
	alex.gaynor@...il.com, gary@...yguo.net, bjorn3_gh@...tonmail.com,
	benno.lossin@...ton.me, a.hindborg@...sung.com,
	aliceryhl@...gle.com, anna-maria@...utronix.de, frederic@...nel.org,
	tglx@...utronix.de, arnd@...db.de, jstultz@...gle.com,
	sboyd@...nel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next v3 4/8] rust: time: Implement addition of Ktime
 and Delta

On Thu, Oct 17, 2024 at 08:10:17PM +0200, Miguel Ojeda wrote:
> On Thu, Oct 17, 2024 at 7:03 PM Boqun Feng <boqun.feng@...il.com> wrote:
> >
> > The point I tried to make is that `+` operator of Ktime can cause
> > overflow because of *user inputs*, unlike the `-` operator of Ktime,
> > which cannot cause overflow as long as Ktime is implemented correctly
> > (as a timestamp). Because the overflow possiblity is exposed to users,
> > then we need to 1) document it and 2) provide saturating_add() (maybe
> > also checked_add() and overflowing_add()) so that users won't need to do
> > the saturating themselves:
> 
> Generally, operators are expected to possibly wrap/panic, so that
> would be fine, no?
> 

Yes, but I guess I need to make it clear: the current `+` operator
implemention is fine for me. What I'm trying to say is: since we have a
`+` that expects users to not provide inputs that causes overflow, then
it makes sense to provide a saturating_add() at the same time for the
API completeness.

> It may be surprising to `ktime_t` users, and that is bad. It is one
> more reason to avoid using the same name for the type, too.
> 
> My only concern is that people may expect this sort of thing (timing
> related) to "usually/just work" and not expect panics. That is bad,
> but if we remain consistent, it may be best to pay the cost now. In
> other words, when someone sees an operator, it is saying it is never
> supposed to wrap, and that is easy to remember. Perhaps some types
> could avoid providing the operators if it is too dangerous to use
> them.
> 

Sounds reasonable to me.

> The other option is be inconsistent in our use of operators, and
> instead give operators the "best" semantics for each type. That can
> definitely provide better ergonomics in some cases, but there is a
> high risk of forgetting what each operator does for each type --
> operator overloading can be quite risky.
> 

Agreed.

> So that is why I think it may be best/easier to generally say "we
> don't do operator overloading in general unless the operator really
> behaves like a core one", especially early on.
> 
> >         kt = kt.saturating_add(d);
> 
> Yeah, that is what we want (I may be missing something in your argument though).
> 
> > but one thing I'm not sure is since it looks like saturating to
> > KTIME_SEC_MAX is the current C choice, if we want to do the same, should
> > we use the name `add_safe()` instead of `saturating_add()`? FWIW, it
> > seems harmless to saturate at KTIME_MAX to me. So personally, I like
> > what Alice suggested.
> 
> Do you mean it would be confusing to not saturate to the highest the
> lower/inner level type can hold?
> 

Yes.

> I mean, nothing says we need to saturate to the highest -- we could
> have a type invariant. (One more reason to avoid the same name).
> 
> Worst case, we can have two saturating methods, or two types, if really needed.
> 
> Thomas can probably tell us what are the most important use cases
> needed, and whether it is a good opportunity to clean/redesign this in
> Rust differently.
> 

Works for me!

Regards,
Boqun

> Cheers,
> Miguel