Re: [PATCH v2] timekeeping: always make sure wall_to_monotonic isn't positive

[John Stultz]

On Sat, May 30, 2015 at 10:10 PM, Wang YanQing <udknight@xxxxxxxxx> wrote:
> I meet two issues on an IMX6 development board without enable
> RTC device(so timekeeping_init will initialize the boot time
> and monotonic to 0).
>
> Issue 1:exportfs -a generate:
>        "exportfs: /opt/nfs/arm does not support NFS export"
> Issue 2:cat /proc/stat:
>        "btime 4294967236"
>
> Exact reproduction of the same issues on x86 after run below
> code:
> "       int main(void)
>         {
>             struct timeval val;
>             int ret;
>
>             val.tv_sec = 0;
>             val.tv_usec = 0;
>             ret = settimeofday(&val, NULL);
>             return 0;
>         }
> "
> Reason:
> The reason is positive wall_to_monotonic push boot time back to the time
> before Epoch, getboottime will return negative value.
>
> In issue 1:
>           negative boot time cause get_expiry overflow time_t when input expire
>           time is 2147483647, then cache_flush always clear entries just added
>           in ip_map_parse.
> In issue 2:
>           show_stat use "unsigned long" to print
>           negative value return by getboottime.
>
> This patch fix these two issues.

If there is two issues, we probably should have two patches, each
clearly fixing one issue.  If there is one problem with multiple
symptoms, then a single patch is fine but we want to be clear there.


> Note: this patch will cause we can't use settimeofday with time
>       earlier than current time on system which timekeeping_init
>       initialize the xtime, boot and monotonic to 0 before set
>       current time to a more reasonable time point.

If everything is initialized to 0 (aka 1970), then setting the time to
prior to (relatively) shortly after boot is a pretty reasonable
constraint. So you might want to reword this a little bit.

This basically seems to come down to the fact that you can't set the
CLOCK_REALTIME time prior to (1970 + system uptime), right?


>
> Signed-off-by: Wang YanQing <udknight@xxxxxxxxx>
> ---
>  Changes v1-v2:
>  1: fix subject, use "isn't positive" instead of "is negative".
>  2: rewrite changelog.
>  3: simplify code as suggested by John Stultz.
>
>  It really take me some times to realize how stupid and
>  buggy the version 1 patch is, but I am ready to be told
>  this version is even stupider:)
>
>  Thanks.
>
>  kernel/time/timekeeping.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
> index 0d784b3..b501aa6 100644
> --- a/kernel/time/timekeeping.c
> +++ b/kernel/time/timekeeping.c
> @@ -895,6 +895,7 @@ int do_settimeofday64(const struct timespec64 *ts)
>         struct timekeeper *tk = &tk_core.timekeeper;
>         struct timespec64 ts_delta, xt;
>         unsigned long flags;
> +       int ret = 0;
>
>         if (!timespec64_valid_strict(ts))
>                 return -EINVAL;
> @@ -908,10 +909,15 @@ int do_settimeofday64(const struct timespec64 *ts)
>         ts_delta.tv_sec = ts->tv_sec - xt.tv_sec;
>         ts_delta.tv_nsec = ts->tv_nsec - xt.tv_nsec;
>
> +       if (timespec64_compare(&tk->wall_to_monotonic, &ts_delta) > 0) {
> +               ret = -EINVAL;
> +               goto out;
> +       }
> +
>         tk_set_wall_to_mono(tk, timespec64_sub(tk->wall_to_monotonic, ts_delta));
>
>         tk_set_xtime(tk, ts);
> -
> +out:
>         timekeeping_update(tk, TK_CLEAR_NTP | TK_MIRROR | TK_CLOCK_WAS_SET);

If we didn't set the time, should we be calling timekeeping_update here?


>
>         write_seqcount_end(&tk_core.seq);
> @@ -920,7 +926,7 @@ int do_settimeofday64(const struct timespec64 *ts)
>         /* signal hrtimers about time change */
>         clock_was_set();
>
> -       return 0;
> +       return ret;
>  }
>  EXPORT_SYMBOL(do_settimeofday64);
>
> @@ -949,7 +955,8 @@ int timekeeping_inject_offset(struct timespec *ts)
>
>         /* Make sure the proposed value is valid */
>         tmp = timespec64_add(tk_xtime(tk),  ts64);
> -       if (!timespec64_valid_strict(&tmp)) {
> +       if (timespec64_compare(&tk->wall_to_monotonic, &ts64) > 0 ||
> +           !timespec64_valid_strict(&tmp)) {
>                 ret = -EINVAL;
>                 goto error;
>         }

Other then the issues above, this is looking better then the previous try!

thanks
-john
--
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/