Re: [PATCH] timekeeping: always make sure wall_to_monotonic is negative

[John Stultz]

On Mon, Mar 16, 2015 at 12:28 PM, Wang YanQing <udknight@xxxxxxxxx> 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@xxxxxxxxx>
> ---
>  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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/