linux-kernel - Re: [PATCH v4] printk: Add monotonic, boottime, and realtime timestamps

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:   Mon, 7 Aug 2017 19:15:13 +0200
From:   Peter Zijlstra <peterz@...radead.org>
To:     John Stultz <john.stultz@...aro.org>
Cc:     Prarit Bhargava <prarit@...hat.com>,
        lkml <linux-kernel@...r.kernel.org>,
        Mark Salyzyn <salyzyn@...roid.com>,
        Jonathan Corbet <corbet@....net>,
        Petr Mladek <pmladek@...e.com>,
        Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Stephen Boyd <sboyd@...eaurora.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
        Christoffer Dall <cdall@...aro.org>,
        Deepa Dinamani <deepa.kernel@...il.com>,
        Ingo Molnar <mingo@...nel.org>,
        Joel Fernandes <joelaf@...gle.com>,
        Kees Cook <keescook@...omium.org>,
        Geert Uytterhoeven <geert+renesas@...der.be>,
        "Luis R. Rodriguez" <mcgrof@...nel.org>,
        Nicholas Piggin <npiggin@...il.com>,
        "Jason A. Donenfeld" <Jason@...c4.com>,
        Olof Johansson <olof@...om.net>,
        Josh Poimboeuf <jpoimboe@...hat.com>, linux-doc@...r.kernel.org
Subject: Re: [PATCH v4] printk: Add monotonic, boottime, and realtime
 timestamps

On Mon, Aug 07, 2017 at 09:52:10AM -0700, John Stultz wrote:
> On Mon, Aug 7, 2017 at 8:52 AM, Prarit Bhargava <prarit@...hat.com> wrote:
> > +u64 ktime_get_real_log_ts(u64 *offset_real)
> > +{
> > +       *offset_real = ktime_to_ns(tk_core.timekeeper.offs_real);
> > +
> > +       if (timekeeping_active)
> > +               return ktime_get_mono_fast_ns();
> > +       else
> > +               return local_clock();
> > +}
> > +
> > +u64 ktime_get_boot_log_ts(void)
> > +{
> > +       if (timekeeping_active)
> > +               return ktime_get_boot_fast_ns();
> > +       else
> > +               return local_clock();
> > +}
> 
> This feels a little tacked on and duplicative.  I'd suggest having one
> function that returns the offset_real and offset_boot or have a
> separate get_mono_log_ts() so its at least consistent.   Additionally,
> in the commit message, you call out the lack of locking between
> grabing the offs_real and calling get_mono_fast_ns(), but I worry it
> may be particularly problematic on 32bit systems, and you don't have
> any notes in the actual code about it (it looks like an oversight).
> 
> Also, when timekeeping_active flips over, and we change from local
> clock to the specified clock, do we see a discontinuity in the log? I
> know folks use to gripe about that back in the day.

Yeah, yuck, this smells. Please don't mix clocks like this.

I expect all you want is to avoid the explosion you get from calling the
fast things too early, right? Please use the below, which should result
in it returning 0.

---
 kernel/time/timekeeping.c | 47 +++++++++++++++++++++++++++++++++--------------
 1 file changed, 33 insertions(+), 14 deletions(-)

diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index cedafa008de5..d111039e0245 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -60,8 +60,39 @@ struct tk_fast {
 	struct tk_read_base	base[2];
 };
 
-static struct tk_fast tk_fast_mono ____cacheline_aligned;
-static struct tk_fast tk_fast_raw  ____cacheline_aligned;
+/* Suspend-time cycles value for halted fast timekeeper. */
+static u64 cycles_at_suspend;
+
+static u64 dummy_clock_read(struct clocksource *cs)
+{
+	return cycles_at_suspend;
+}
+
+static struct clocksource dummy_clock = {
+	.read = dummy_clock_read,
+};
+
+static struct tk_fast tk_fast_mono ____cacheline_aligned = {
+	.base = {
+		(struct tk_read_base){
+			.clock = &dummy_clock,
+		},
+		(struct tk_read_base){
+			.clock = &dummy_clock,
+		},
+	},
+};
+
+static struct tk_fast tk_fast_raw  ____cacheline_aligned = {
+	.base = {
+		(struct tk_read_base){
+			.clock = &dummy_clock,
+		},
+		(struct tk_read_base){
+			.clock = &dummy_clock,
+		},
+	},
+};
 
 /* flag for if timekeeping is suspended */
 int __read_mostly timekeeping_suspended;
@@ -477,18 +508,6 @@ u64 notrace ktime_get_boot_fast_ns(void)
 }
 EXPORT_SYMBOL_GPL(ktime_get_boot_fast_ns);
 
-/* Suspend-time cycles value for halted fast timekeeper. */
-static u64 cycles_at_suspend;
-
-static u64 dummy_clock_read(struct clocksource *cs)
-{
-	return cycles_at_suspend;
-}
-
-static struct clocksource dummy_clock = {
-	.read = dummy_clock_read,
-};
-
 /**
  * halt_fast_timekeeper - Prevent fast timekeeper from accessing clocksource.
  * @tk: Timekeeper to snapshot.