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

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

[Wang YanQing]

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.

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.

Signed-off-by: Wang YanQing <udknight@gmail.com>
---
 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);
 
 	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;
 	}
-- 
1.8.5.6.2.g3d8a54e.dirty

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@gmail.com> 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@gmail.com>
> ---
>  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

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

[Wang YanQing]

On Mon, Jun 01, 2015 at 04:55:48PM -0700, John Stultz wrote:
> On Sat, May 30, 2015 at 10:10 PM, Wang YanQing <udknight@gmail.com> 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.
I get it and will reword this in v3.

> This basically seems to come down to the fact that you can't set the
> CLOCK_REALTIME time prior to (1970 + system uptime), right?
Yes, that's strict and precise representation!

> 
> 
> >
> > Signed-off-by: Wang YanQing <udknight@gmail.com>
> > ---
> >  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?

Because we have called timekeeping_forward_now(tk), I found a same situation 
in timekeeping_inject_offset:

"error: /* even if we error out, we forwarded the time, so call update */"

Thanks.