| 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
| ||
|
Message-ID: <CALAqxLWPsztNZqo_xNvhzXpWrTbGXr_5M-kGvaeigZRo19yi4Q@mail.gmail.com>
Date: Wed, 13 May 2015 17:01:32 -0700
From: John Stultz <john.stultz@...aro.org>
To: Wang YanQing <udknight@...il.com>,
Thomas Gleixner <tglx@...utronix.de>,
John Stultz <john.stultz@...aro.org>,
lkml <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] timekeeping: always make sure wall_to_monotonic is negative
On Mon, Mar 16, 2015 at 12:28 PM, Wang YanQing <udknight@...il.com> wrote:
>
> When wall_to_monotonic become positive, then I get below two
> errors on an IMX6 development board without enable RTC device:
>
> 1:execute exportfs -a generate:
> "exportfs: /opt/nfs/arm does not support NFS export"
> 2:cat /proc/stat:
> "btime 4294967236"
>
> I can product the same issues on x86 with newest kernel in next
> tree with below c code:
> "
> int main(void)
> {
> struct timeval val;
> int ret;
>
> val.tv_sec = 0;
> val.tv_usec = 0;
> ret = settimeofday(&val, NULL);
> printf("ret:%d\n", ret);
> return 0;
> }
> "
> The reason is getboottime[64] return negative value, cause below
> codes don't work:
>
> nfs error:cache.h:get_expiry return negative value cause
> cache_flush always clear entries just added in
> ip_map_parse.
> proc/stat error:
> seq_printf use "unsigned long" to show
> a negative number return by getboottime.
>
> This patch fix it by validate new value of wall_to_monotonic
> before assign it.
Sorry for the slow response, and thanks for the problem report. Some
questions below...
So to restate the problem: setting the CLOCK_REALTIME back to a
timevalue that is less then the value of CLOCK_MONOTONIC causes
problems with various interfaces such as nfs and proc/stat.
Is that right?
> Signed-off-by: Wang YanQing <udknight@...il.com>
> ---
> kernel/time/timekeeping.c | 71 ++++++++++++++++++++++++++++++++++++-----------
> 1 file changed, 55 insertions(+), 16 deletions(-)
>
> diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
> index 91db941..799e323 100644
> --- a/kernel/time/timekeeping.c
> +++ b/kernel/time/timekeeping.c
> @@ -96,11 +96,29 @@ static void tk_xtime_add(struct timekeeper *tk, const struct timespec64 *ts)
> tk_normalize_xtime(tk);
> }
>
> -static void tk_set_wall_to_mono(struct timekeeper *tk, struct timespec64 wtm)
> +static inline bool timespec64_negative(struct timespec64 *ts)
> +{
> + if (ts->tv_sec > 0)
> + return false;
> + if (ts->tv_sec == 0 && ts->tv_nsec > 0)
> + return false;
> + return true;
> +}
> +
> +static bool tk_set_wall_to_mono(struct timekeeper *tk, struct timespec64 wtm)
> {
> struct timespec64 tmp;
>
> /*
> + * The current time
> + * wall_to_monotonic is what we need to add to xtime (or xtime corrected
> + * for sub jiffie times) to get to monotonic time. Monotonic is pegged
> + * at zero at system boot time, so wall_to_monotonic will be negative.
> + */
> + if (!timespec64_negative(&wtm))
> + return false;
> +
> + /*
> * Verify consistency of: offset_real = -wall_to_monotonic
> * before modifying anything
> */
> @@ -111,6 +129,7 @@ static void tk_set_wall_to_mono(struct timekeeper *tk, struct timespec64 wtm)
> set_normalized_timespec64(&tmp, -wtm.tv_sec, -wtm.tv_nsec);
> tk->offs_real = timespec64_to_ktime(tmp);
> tk->offs_tai = ktime_add(tk->offs_real, ktime_set(tk->tai_offset, 0));
> + return true;
> }
>
> static inline void tk_update_sleep_time(struct timekeeper *tk, ktime_t delta)
> @@ -796,8 +815,9 @@ EXPORT_SYMBOL(do_gettimeofday);
> int do_settimeofday64(const struct timespec64 *ts)
> {
> struct timekeeper *tk = &tk_core.timekeeper;
> - struct timespec64 ts_delta, xt;
> + struct timespec64 ts_delta, tmp;
> unsigned long flags;
> + int ret = 0;
>
> if (!timespec64_valid_strict(ts))
> return -EINVAL;
> @@ -807,23 +827,27 @@ int do_settimeofday64(const struct timespec64 *ts)
>
> timekeeping_forward_now(tk);
>
> - xt = tk_xtime(tk);
> - ts_delta.tv_sec = ts->tv_sec - xt.tv_sec;
> - ts_delta.tv_nsec = ts->tv_nsec - xt.tv_nsec;
> + tmp = tk_xtime(tk);
> + ts_delta.tv_sec = ts->tv_sec - tmp.tv_sec;
> + ts_delta.tv_nsec = ts->tv_nsec - tmp.tv_nsec;
>
> - tk_set_wall_to_mono(tk, timespec64_sub(tk->wall_to_monotonic, ts_delta));
> + tmp = timespec64_sub(tk->wall_to_monotonic, ts_delta);
> + if (!tk_set_wall_to_mono(tk, tmp)) {
> + ret = -EINVAL;
> + goto out;
> + }
It seems like a simpler check at the top of do_settimeofday64() would
be easier then embedding the check in set_wall_to_mono and trying to
deal with it later in the process of adjusting time.
>
> tk_set_xtime(tk, ts);
> -
> timekeeping_update(tk, TK_CLEAR_NTP | TK_MIRROR | TK_CLOCK_WAS_SET);
> -
> +out:
> write_seqcount_end(&tk_core.seq);
> raw_spin_unlock_irqrestore(&timekeeper_lock, flags);
>
> /* signal hrtimers about time change */
> - clock_was_set();
> + if (!ret)
> + clock_was_set();
>
> - return 0;
> + return ret;
> }
> EXPORT_SYMBOL(do_settimeofday64);
>
> @@ -857,8 +881,13 @@ int timekeeping_inject_offset(struct timespec *ts)
> goto error;
> }
>
> + tmp = timespec64_sub(tk->wall_to_monotonic, ts64);
> + if (!tk_set_wall_to_mono(tk, tmp)) {
> + ret = -EINVAL;
> + goto error;
> + }
> +
Similarly here. A simple check at the top before we make any
adjustments would make more sense to me.
> tk_xtime_add(tk, &ts64);
> - tk_set_wall_to_mono(tk, timespec64_sub(tk->wall_to_monotonic, ts64));
>
> error: /* even if we error out, we forwarded the time, so call update */
> timekeeping_update(tk, TK_CLEAR_NTP | TK_MIRROR | TK_CLOCK_WAS_SET);
> @@ -1140,14 +1169,21 @@ static struct timespec64 timekeeping_suspend_time;
> static void __timekeeping_inject_sleeptime(struct timekeeper *tk,
> struct timespec64 *delta)
> {
> + struct timespec64 tmp;
> +
> if (!timespec64_valid_strict(delta)) {
> printk_deferred(KERN_WARNING
> "__timekeeping_inject_sleeptime: Invalid "
> "sleep delta value!\n");
> return;
> }
> +
> + tmp = timespec64_sub(tk->wall_to_monotonic, *delta);
> + if (!tk_set_wall_to_mono(tk, tmp)) {
> + WARN_ON(1);
> + return;
> + }
Is this really possible? _inject_sleeptime should never be negative.
Or am I missing something?
> tk_xtime_add(tk, delta);
> - tk_set_wall_to_mono(tk, timespec64_sub(tk->wall_to_monotonic, *delta));
> tk_update_sleep_time(tk, timespec64_to_ktime(*delta));
> tk_debug_account_sleep_time(delta);
> }
> @@ -1539,14 +1575,17 @@ static inline unsigned int accumulate_nsecs_to_secs(struct timekeeper *tk)
> leap = second_overflow(tk->xtime_sec);
> if (unlikely(leap)) {
> struct timespec64 ts;
> -
> - tk->xtime_sec += leap;
> + bool ret;
>
> ts.tv_sec = leap;
> ts.tv_nsec = 0;
> - tk_set_wall_to_mono(tk,
> + ret = tk_set_wall_to_mono(tk,
> timespec64_sub(tk->wall_to_monotonic, ts));
How would adding time in accumulate_nsecs_to_secs ever cause
wall_to_monotonic to not be negative?
> -
> + if (!ret) {
> + WARN_ON_ONCE(1);
> + break;
> + }
> + tk->xtime_sec += leap;
> __timekeeping_set_tai_offset(tk, tk->tai_offset - leap);
>
> clock_set = TK_CLOCK_WAS_SET;
> --
> 2.2.2.dirty
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists