linux-kernel - Re: [PATCH] rust: time: New module for timekeeping functions

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: <87v8jvnqq4.ffs@tglx>
Date:   Tue, 21 Feb 2023 13:32:51 +0100
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Asahi Lina <lina@...hilina.net>, Miguel Ojeda <ojeda@...nel.org>,
        Alex Gaynor <alex.gaynor@...il.com>,
        Wedson Almeida Filho <wedsonaf@...il.com>,
        Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>,
        Björn Roy Baron <bjorn3_gh@...tonmail.com>,
        John Stultz <jstultz@...gle.com>,
        Stephen Boyd <sboyd@...nel.org>
Cc:     linux-kernel@...r.kernel.org, rust-for-linux@...r.kernel.org,
        asahi@...ts.linux.dev, Asahi Lina <lina@...hilina.net>
Subject: Re: [PATCH] rust: time: New module for timekeeping functions

On Tue, Feb 21 2023 at 16:06, Asahi Lina wrote:
> +
> +use crate::bindings;
> +use core::time::Duration;
> +
> +/// Returns the kernel time elapsed since boot, excluding time spent sleeping, as a [`Duration`].
> +pub fn ktime_get() -> Duration {
> +    // SAFETY: Function has no side effects and no inputs.
> +    Duration::from_nanos(unsafe { bindings::ktime_get() }.try_into().unwrap())

Why is this a Duration? From the spec:

    Duration

        A Duration type to represent a span of time, typically used for
        system timeouts.

    Instant

        A measurement of a monotonically nondecreasing clock. Opaque and
        useful only with Duration.

In my understanding 'Duration' is a time span between two points, while
ktime_get() and ktime_get_boottime() return the current time of
monotonically nondecreasing clocks, i.e. they fall into the 'Instant'
category.

Now the problem is that 'Instant' in it's specification is bound to
CLOCK_MONOTONIC and there is no way to express CLOCK_BOOTTIME, but
that's a shortcoming of the spec which ignores CLOCK_BOOTTIME
completely. IOW, that's also a problem for user space.

This makes sense vs. the other representation:

     SystemTime

        A measurement of the system clock, useful for talking to
        external entities like the file system or other processes.

This maps to CLOCK_REALTIME and CLOCK_TAI, i.e. ktime_get_real_ns() and
ktime_get_clocktai().

Similar to 'Instant' 'SystemTime' is strictly bound to CLOCK_REALTIME
by specification and there is no way to read CLOCK_TAI.

Please fix this in the spec and do not try to work around that by
pretending that a clock read is a 'Duration'.

> +}
> +
> +/// Returns the kernel time elapsed since boot, including time spent sleeping, as a [`Duration`].
> +pub fn ktime_get_boottime() -> Duration {
> +    Duration::from_nanos(
> +        // SAFETY: Function has no side effects and no variable inputs.
> +        unsafe { bindings::ktime_get_with_offset(bindings::tk_offsets_TK_OFFS_BOOT) }

No. Please use ktime_get_boottime() and not the timekeeping internal function.

Thanks,

        tglx