timekeeping: always make sure wall_to_monotonic isn't positive
PATCH v2 1/1 timekeeping: always make sure wall_to_monotonic isn't positive
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.
---
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(-)
kernel/time/timekeeping.c +10 -3
@@ -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; }
Signature · 1 line
1.8.5.6.2.g3d8a54e.dirty
Quoted 34 lines · 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/armdoes 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 positivewall_to_monotonicpush boot time back to the time before Epoch, getboottime will return negative value.In issue 1:
negative boot time causeget_expiryoverflowtime_twhen input expire time is 2147483647, thencache_flushalways clear entries just added inip_map_parse.
In issue 2:
show_statuse "unsigned long" to print
negative value return by getboottime.
Quoted 1 line
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.
Quoted 4 lines
Note: this patch will cause we can't use settimeofday with time
earlier than current time on system whichtimekeeping_initinitialize 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?
Quoted 34 lines
Signed-off-by Wang YanQing <[email protected]>---
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(-)kernel/time/timekeeping.c +2
-0@@ -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 ) {
kernel/time/timekeeping.c int do_settimeofday64(const struct timespec64 *ts) · line 921
Quoted 10 lines
kernel/time/timekeeping.c +5
-1+ 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?
Quoted 12 lines
kernel/time/timekeeping.c +1
-1write_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);
kernel/time/timekeeping.c int timekeeping_inject_offset(struct timespec *ts) · line 962
Quoted 9 lines
kernel/time/timekeeping.c +2
-1@@ -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
Quoted 42 lines · John Stultz
Quoted 34 lines · 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/armdoes 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 positivewall_to_monotonicpush boot time back to the time before Epoch, getboottime will return negative value.In issue 1:
negative boot time causeget_expiryoverflowtime_twhen input expire time is 2147483647, thencache_flushalways clear entries just added inip_map_parse.
In issue 2:
show_statuse "unsigned long" to print
negative value return by getboottime.Quoted 1 line
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.
Quoted 8 lines
Quoted 4 lines
Note: this patch will cause we can't use settimeofday with time
earlier than current time on system whichtimekeeping_initinitialize 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.
Quoted 2 lines
This basically seems to come down to the fact that you can't set the
CLOCK_REALTIMEtime prior to (1970 + system uptime), right?
Yes, that's strict and precise representation!
Quoted 42 lines
Quoted 42 lines
Signed-off-by Wang YanQing <[email protected]>---
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(-)kernel/time/timekeeping.c +6
-0@@ -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));
Quoted 6 lines
kernel/time/timekeeping.c int do_settimeofday64(const struct timespec64 *ts) · line 921
Quoted 4 lines
kernel/time/timekeeping.c +1
-1tk_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_updatehere?
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.