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: <20250619.092359.585578352260473138.fujita.tomonori@gmail.com>
Date: Thu, 19 Jun 2025 09:23:59 +0900 (JST)
From: FUJITA Tomonori <fujita.tomonori@...il.com>
To: boqun.feng@...il.com
Cc: a.hindborg@...nel.org, fujita.tomonori@...il.com,
 alex.gaynor@...il.com, ojeda@...nel.org, aliceryhl@...gle.com,
 anna-maria@...utronix.de, bjorn3_gh@...tonmail.com, dakr@...nel.org,
 frederic@...nel.org, gary@...yguo.net, jstultz@...gle.com,
 linux-kernel@...r.kernel.org, lossin@...nel.org, lyude@...hat.com,
 rust-for-linux@...r.kernel.org, sboyd@...nel.org, tglx@...utronix.de,
 tmgross@...ch.edu
Subject: Re: [PATCH] rust: time: Seal the ClockSource trait

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?