linux-kernel - Re: [PATCH 1/4] time: add ktime_get

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]

Date:   Thu, 28 Sep 2023 22:15:27 -0700
From:   John Stultz <jstultz@...gle.com>
To:     Mahesh Bandewar <maheshb@...gle.com>
Cc:     Netdev <netdev@...r.kernel.org>,
        Linux <linux-kernel@...r.kernel.org>,
        David Miller <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Eric Dumazet <edumazet@...gle.com>,
        Paolo Abeni <pabeni@...hat.com>,
        Jonathan Corbet <corbet@....net>,
        Don Hatchett <hatch@...gle.com>,
        Yuliang Li <yuliangli@...gle.com>,
        Mahesh Bandewar <mahesh@...dewar.net>,
        Thomas Gleixner <tglx@...utronix.de>,
        Stephen Boyd <sboyd@...nel.org>,
        Richard Cochran <richardcochran@...il.com>
Subject: Re: [PATCH 1/4] time: add ktime_get_cycles64() api

On Thu, Sep 28, 2023 at 7:37 PM Mahesh Bandewar <maheshb@...gle.com> wrote:
>
> add a method to retrieve raw cycles in the same fashion as there are
> ktime_get_* methods available for supported time-bases. The method
> continues using the 'struct timespec64' since the UAPI uses 'struct
> ptp_clock_time'.
>
> The caller can perform operation equivalent of timespec64_to_ns() to
> retrieve raw-cycles value. The precision loss because of this conversion
> should be none for 64 bit cycle counters and nominal at 96 bit counters
> (considering UAPI of s64 + u32 of 'struct ptp_clock_time).
>
> Signed-off-by: Mahesh Bandewar <maheshb@...gle.com>
> CC: John Stultz <jstultz@...gle.com>
> CC: Thomas Gleixner <tglx@...utronix.de>
> CC: Stephen Boyd <sboyd@...nel.org>
> CC: Richard Cochran <richardcochran@...il.com>
> CC: netdev@...r.kernel.org
> CC: linux-kernel@...r.kernel.org
> ---
>  include/linux/timekeeping.h |  1 +
>  kernel/time/timekeeping.c   | 24 ++++++++++++++++++++++++
>  2 files changed, 25 insertions(+)
>
> diff --git a/include/linux/timekeeping.h b/include/linux/timekeeping.h
> index fe1e467ba046..5537700ad113 100644
> --- a/include/linux/timekeeping.h
> +++ b/include/linux/timekeeping.h
> @@ -43,6 +43,7 @@ extern void ktime_get_ts64(struct timespec64 *ts);
>  extern void ktime_get_real_ts64(struct timespec64 *tv);
>  extern void ktime_get_coarse_ts64(struct timespec64 *ts);
>  extern void ktime_get_coarse_real_ts64(struct timespec64 *ts);
> +extern void ktime_get_cycles64(struct timespec64 *ts);
>
>  void getboottime64(struct timespec64 *ts);
>
> diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
> index 266d02809dbb..35d603d21bd5 100644
> --- a/kernel/time/timekeeping.c
> +++ b/kernel/time/timekeeping.c
> @@ -989,6 +989,30 @@ void ktime_get_ts64(struct timespec64 *ts)
>  }
>  EXPORT_SYMBOL_GPL(ktime_get_ts64);
>
> +/**
> + * ktime_get_cycles64 - get the raw clock cycles in timespec64 format
> + * @ts:                pointer to timespec variable
> + *
> + * This function converts the raw clock cycles into timespce64 format
> + * in the varibale pointed to by @ts
> + */
> +void ktime_get_cycles64(struct timespec64 *ts)
> +{
> +       struct timekeeper *tk = &tk_core.timekeeper;
> +       unsigned int seq;
> +       u64 now;
> +
> +       WARN_ON_ONCE(timekeeping_suspended);
> +
> +       do {
> +               seq = read_seqcount_begin(&tk_core.seq);
> +               now = tk_clock_read(&tk->tkr_mono);
> +       } while (read_seqcount_retry(&tk_core.seq, seq));
> +
> +       *ts = ns_to_timespec64(now);
> +}

Hey Mahesh,
  Thanks for sending this out.  Unfortunately, I'm a bit confused by
this. It might be helpful to further explain what this would be used
for in more detail?

Some aspects that are particularly unclear:
1) You seem to be trying to stuff cycle values into a timespec64,
which is not very intuitive (and a type error of sorts). It's not
clear /why/ that type is useful.

2) Depending on your clocksource, this would have very strange
wrapping behavior, so I'm not sure it is generally safe to use.

3) Nit: The interface is called ktime_get_cycles64 (timespec64
returning interfaces usually are postfixed with ts64).

I guess could you clarify why you need this instead of using
CLOCK_MONOTONIC_RAW which tries to abstract raw cycles in a way that
is safe and avoids wrapping across various clocksources?

thanks
-john