linux-kernel - Re: [PATCH v5 1/2] sched/clock: interface to allow timestamps early in boot

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: <alpine.DEB.2.20.1708260018160.2124@nanos>
Date:   Sat, 26 Aug 2017 00:54:39 +0200 (CEST)
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Pavel Tatashin <pasha.tatashin@...cle.com>
cc:     x86@...nel.org, linux-kernel@...r.kernel.org, mingo@...hat.com,
        peterz@...radead.org, hpa@...or.com, douly.fnst@...fujitsu.com
Subject: Re: [PATCH v5 1/2] sched/clock: interface to allow timestamps early
 in boot

On Wed, 23 Aug 2017, Pavel Tatashin wrote:
> 
> - Use weak sched_clock_early() interface to determine time from boot in
>   arch specific read_boot_clock64()

weak sched_clock_early() is not an interface. The weak implementation is
merily a place holder which can be overridden by a real implementation.

Aside of that this change is completely unrelated to the sched clock core
changes and wants to be split out into a separate patch.

> +/*
> + * Called once during to boot to initialize boot time.
> + */
> +void read_boot_clock64(struct timespec64 *ts)

And because its called only once, it does not need to be marked __init()
and must be kept around forever, right?

> +{
> +	u64 ns_boot = sched_clock_early(); /* nsec from boot */

Please do not use tail comments. They are a horrible habit.

Instead of adding this crap you'd have better spent time in adding proper
comments explaining the reasoning behind this function,

> +	struct timespec64 ts_now;
> +	bool valid_clock;
> +	u64 ns_now;
> +
> +	/* Time from epoch */
> +	read_persistent_clock64(&ts_now);
> +	ns_now = timespec64_to_ns(&ts_now);
> +	valid_clock = ns_boot && timespec64_valid_strict(&ts_now) &&
> +			(ns_now > ns_boot);
> +
> +	if (!valid_clock)
> +		*ts = (struct timespec64){0, 0};
> +	else
> +		*ts = ns_to_timespec64(ns_now - ns_boot);
> +}

This is really broken. Look at the time keeping init code. It does:

     read_persistent_clock64(&now);
     ...
     read_boot_clock64(&boot);
     ...
     tk_set_xtime(tk, &now);
     ...
     set_normalized_timespec64(&tmp, -boot.tv_sec, -boot.tv_nsec);
     tk_set_wall_to_mono(tk, tmp);

Lets assume that the initial read_persistent_clock64() happens right before
the second. For simplicity lets assume we get 1000 seconds since the epoch.

Now read_boot_clock() reads sched_clock_early() which returns 1 second.

The second read_persistent_clock64() returns 1001 seconds since the epoch
because the RTC advanced by now. So the resulting time stamp is going to be
1000s since the epoch.

In case the RTC still returns 100 since the epoch, the resulting time stamp
is 999s since the epoch.

A full second difference. That's time stamp lottery but nothing which we
want to base any boot time analysis on.

You have to come up with something more useful than that.

Thanks,

	tglx