linux-kernel - Re: [PATCH] rust: time: Seal the ClockSource trait

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: <3da3c6d4-5745-432a-8771-561b03b0e9b2@app.fastmail.com>
Date: Wed, 18 Jun 2025 17:27:01 -0700
From: "Boqun Feng" <boqun.feng@...il.com>
To: "FUJITA Tomonori" <fujita.tomonori@...il.com>
Cc: "Andreas Hindborg" <a.hindborg@...nel.org>, alex.gaynor@...il.com,
 "Miguel Ojeda" <ojeda@...nel.org>, "Alice Ryhl" <aliceryhl@...gle.com>,
 "Anna-Maria Gleixner" <anna-maria@...utronix.de>, bjorn3_gh@...tonmail.com,
 "Danilo Krummrich" <dakr@...nel.org>,
 "Frederic Weisbecker" <frederic@...nel.org>, "Gary Guo" <gary@...yguo.net>,
 "John Stultz" <jstultz@...gle.com>, linux-kernel@...r.kernel.org,
 lossin@...nel.org, "Lyude Paul" <lyude@...hat.com>,
 rust-for-linux@...r.kernel.org, "Stephen Boyd" <sboyd@...nel.org>,
 "Thomas Gleixner" <tglx@...utronix.de>, "Trevor Gross" <tmgross@...ch.edu>
Subject: Re: [PATCH] rust: time: Seal the ClockSource trait



On Wed, Jun 18, 2025, at 5:23 PM, FUJITA Tomonori wrote:
> On Wed, 18 Jun 2025 12:29:55 -0700
> Boqun Feng <boqun.feng@...il.com> wrote:
>
>> On Wed, Jun 18, 2025 at 09:13:07PM +0200, Andreas Hindborg wrote:
>>> "Boqun Feng" <boqun.feng@...il.com> writes:
>>> 
>>> > On Tue, Jun 17, 2025 at 05:10:42PM -0700, Boqun Feng wrote:
>>> >> On Wed, Jun 18, 2025 at 08:20:53AM +0900, FUJITA Tomonori wrote:
>>> >> > Prevent downstream crates or drivers from implementing `ClockSource`
>>> >> > for arbitrary types, which could otherwise leads to unsupported
>>> >> > behavior.
>>> >> >
>>> >>
>>> >> Hmm.. I don't think other impl of `ClockSource` is a problem, IIUC, as
>>> >> long as the ktime_get() can return a value in [0, i64::MAX). Also this
>>> >> means ClockSource should be an `unsafe` trait, because the correct
>>> >> implementaion relies on ktime_get() returns the correct value. This is
>>> >> needed even if you sealed ClockSource trait.
>>> >>
>>> >> Could you drop this and fix that the ClockSource trait instead? Thanks!
>>> >>
>>> >
>>> > For example:
>>> >
>>> >     /// Trait for clock sources.
>>> >     ///
>>> >     /// ...
>>> >     /// # Safety
>>> >     ///
>>> >     /// Implementers must ensure `ktime_get()` return a value in [0,
>>> >     //  KTIME_MAX (i.e. i64::MAX)).
>>> >     pub unsafe trait ClockSource {
>>> >         ...
>>> >     }
>>> 
>>> Nice catch, it definitely needs to be unsafe. We should also require
>>> correlation between ID and the value fetched by `ktime_get`.
>>> 
>>> But I still think it is fine to seal the trait, why not?
>>> 
>> 
>> There could be potential users of a customized clock source, for
>> example, a device which also has a timestamp register itself:
>> 
>> 	https://lore.kernel.org/rust-for-linux/Z9xb1r1x5tOzAIZT@boqun-archlinux/
>> 
>> So I think with ClockSource being unsafe and well documented, making it
>> not sealed wouldn't be a problem. IMO, sealing is for the cases where we
>> must not have downstream impls, ClockSource is not such a case.
>
> Ah, I wasn't aware of that kind of use case. Indeed, a customized
> clock source seems like a better approach than reinventing Instant and
> Delta in a driver.
>
> On the other hand, I think that sealing HrTimerMode trait is the right
> approach:
>
> https://lore.kernel.org/rust-for-linux/20250617232806.3950141-1-fujita.tomonori@gmail.com/
>
> Firstly, HrTimerMode needs to be supported on the C side, so
> implementing a custom Rust HrTimerMode won't work.
>
> Secondly, if a developer adds a new HrTimerMode on the C side, we
> believe that the corresponding Rust support should be added in the
> time abstractions, not in drivers or other places.
>
>
> Does that make sense?

Agreed. :)

Regards,
Boqun