[PATCH] timekeeping: Force upper bound for setting CLOCK_REALTIME

[PATCH] timekeeping: Force upper bound for setting CLOCK_REALTIME

[Thomas Gleixner]

Several people reported testing failures after setting CLOCK_REALTIME close
to the limits of the kernel internal representation in nanoseconds,
i.e. year 2262.

The failures are exposed in subsequent operations, i.e. when arming timers
or when the advancing CLOCK_MONOTONIC makes the calculation of
CLOCK_REALTIME overflow into negative space.

Now people start to paper over the underlying problem by clamping
calculations to the valid range, but that's just wrong because such
workarounds will prevent detection of real issues as well.

It is reasonable to force an upper bound for the various methods of setting
CLOCK_REALTIME. Year 2262 is the absolute upper bound. Assume a maximum
uptime of 30 years which is plenty enough even for esoteric embedded
systems. That results in an upper bound of year 2232 for setting the time.

Once that limit is reached in reality this limit is only a small part of
the problem space. But until then this stops people from trying to paper
over the problem at the wrong places.

Reported-by: Xiongfeng Wang <wangxiongfeng2@huawei.com>
Reported-by: Hongbo Yao <yaohongbo@huawei.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: John Stultz <john.stultz@linaro.org>
Cc: Stephen Boyd <sboyd@kernel.org>
Cc: Miroslav Lichvar <mlichvar@redhat.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Richard Cochran <richardcochran@gmail.com>
---
 include/linux/time64.h    |   21 +++++++++++++++++++++
 kernel/time/time.c        |    2 +-
 kernel/time/timekeeping.c |    6 +++---
 3 files changed, 25 insertions(+), 4 deletions(-)

--- a/include/linux/time64.h
+++ b/include/linux/time64.h
@@ -33,6 +33,17 @@ struct itimerspec64 {
 #define KTIME_MAX			((s64)~((u64)1 << 63))
 #define KTIME_SEC_MAX			(KTIME_MAX / NSEC_PER_SEC)
 
+/*
+ * Limits for settimeofday():
+ *
+ * To prevent setting the time close to the wraparound point time setting
+ * is limited so a reasonable uptime can be accomodated. Uptime of 30 years
+ * should be really sufficient, which means the cutoff is 2232. At that
+ * point the cutoff is just a small part of the larger problem.
+ */
+#define TIME_UPTIME_SEC_MAX		(30LL * 365 * 24 *3600)
+#define TIME_SETTOD_SEC_MAX		(KTIME_SEC_MAX - TIME_UPTIME_SEC_MAX)
+
 static inline int timespec64_equal(const struct timespec64 *a,
 				   const struct timespec64 *b)
 {
@@ -99,6 +110,16 @@ static inline bool timespec64_valid_stri
 		return false;
 	return true;
 }
+
+static inline bool timespec64_valid_settod(const struct timespec64 *ts)
+{
+	if (!timespec64_valid(ts))
+		return false;
+	/* Disallow values which cause overflow issues vs. CLOCK_REALTIME */
+	if ((unsigned long long)ts->tv_sec >= TIME_SETTOD_SEC_MAX)
+		return false;
+	return true;
+}
 
 /**
  * timespec64_to_ns - Convert timespec64 to nanoseconds
--- a/kernel/time/time.c
+++ b/kernel/time/time.c
@@ -171,7 +171,7 @@ int do_sys_settimeofday64(const struct t
 	static int firsttime = 1;
 	int error = 0;
 
-	if (tv && !timespec64_valid(tv))
+	if (tv && !timespec64_valid_settod(tv))
 		return -EINVAL;
 
 	error = security_settime64(tv, tz);
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -1221,7 +1221,7 @@ int do_settimeofday64(const struct times
 	unsigned long flags;
 	int ret = 0;
 
-	if (!timespec64_valid_strict(ts))
+	if (!timespec64_valid_settod(ts))
 		return -EINVAL;
 
 	raw_spin_lock_irqsave(&timekeeper_lock, flags);
@@ -1278,7 +1278,7 @@ static int timekeeping_inject_offset(con
 	/* Make sure the proposed value is valid */
 	tmp = timespec64_add(tk_xtime(tk), *ts);
 	if (timespec64_compare(&tk->wall_to_monotonic, ts) > 0 ||
-	    !timespec64_valid_strict(&tmp)) {
+	    !timespec64_valid_settod(&tmp)) {
 		ret = -EINVAL;
 		goto error;
 	}
@@ -1527,7 +1527,7 @@ void __init timekeeping_init(void)
 	unsigned long flags;
 
 	read_persistent_wall_and_boot_offset(&wall_time, &boot_offset);
-	if (timespec64_valid_strict(&wall_time) &&
+	if (timespec64_valid_settod(&wall_time) &&
 	    timespec64_to_ns(&wall_time) > 0) {
 		persistent_clock_exists = true;
 	} else if (timespec64_to_ns(&wall_time) != 0) {

Re: [PATCH] timekeeping: Force upper bound for setting CLOCK_REALTIME

[Miroslav Lichvar]

On Sat, Mar 23, 2019 at 11:36:19AM +0100, Thomas Gleixner wrote:
> It is reasonable to force an upper bound for the various methods of setting
> CLOCK_REALTIME. Year 2262 is the absolute upper bound. Assume a maximum
> uptime of 30 years which is plenty enough even for esoteric embedded
> systems. That results in an upper bound of year 2232 for setting the time.

The patch looks good to me.

I like this approach better than using a larger value closer to the
overflow (e.g. one week) and stepping the clock back automatically
when the clock reaches that time, but I suspect it might possibly
break more tests (or any unusual applications messing with time) as a
much larger interval is now EINVAL.

Thanks,

-- 
Miroslav Lichvar

Re: [PATCH] timekeeping: Force upper bound for setting CLOCK_REALTIME

[Thomas Gleixner]

On Tue, 26 Mar 2019, Miroslav Lichvar wrote:
> On Sat, Mar 23, 2019 at 11:36:19AM +0100, Thomas Gleixner wrote:
> > It is reasonable to force an upper bound for the various methods of setting
> > CLOCK_REALTIME. Year 2262 is the absolute upper bound. Assume a maximum
> > uptime of 30 years which is plenty enough even for esoteric embedded
> > systems. That results in an upper bound of year 2232 for setting the time.
> 
> The patch looks good to me.
> 
> I like this approach better than using a larger value closer to the
> overflow (e.g. one week) and stepping the clock back automatically
> when the clock reaches that time, but I suspect it might possibly
> break more tests (or any unusual applications messing with time) as a
> much larger interval is now EINVAL.

I'm fine with breaking a few tests on the way rather than having undefined
behaviour and the constant flow of patches tackling the wrong end of the
stick.

Thanks,

	tglx

Re: [PATCH] timekeeping: Force upper bound for setting CLOCK_REALTIME

[Arnd Bergmann]

On Tue, Mar 26, 2019 at 1:31 PM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> On Tue, 26 Mar 2019, Miroslav Lichvar wrote:
> > On Sat, Mar 23, 2019 at 11:36:19AM +0100, Thomas Gleixner wrote:
> > > It is reasonable to force an upper bound for the various methods of setting
> > > CLOCK_REALTIME. Year 2262 is the absolute upper bound. Assume a maximum
> > > uptime of 30 years which is plenty enough even for esoteric embedded
> > > systems. That results in an upper bound of year 2232 for setting the time.
> >
> > The patch looks good to me.
> >
> > I like this approach better than using a larger value closer to the
> > overflow (e.g. one week) and stepping the clock back automatically
> > when the clock reaches that time, but I suspect it might possibly
> > break more tests (or any unusual applications messing with time) as a
> > much larger interval is now EINVAL.
>
> I'm fine with breaking a few tests on the way rather than having undefined
> behaviour and the constant flow of patches tackling the wrong end of the
> stick.

I think the one downside of your approach is that it introduces a second
arbitrary cut-off point after which the system almost functions perfectly,
but is no longer able to do ntp updates or set the right time after a reboot.

That said, all other ideas I've managed to come up with are worse,
 so I agree on going ahead with this version.

We could still bikeshed over the exact cutoff time, as the one you
picked isn't particularly intuitive. It's almost exactly 30 years before
the final end point, but your calculation is off by a few days because
of leap years. And no, I don't have a particular preference for any
other color of this bikeshed either, it's probably as good as any other
time within 20 years of what you suggested.

          Arnd

Re: [PATCH] timekeeping: Force upper bound for setting CLOCK_REALTIME

[Thomas Gleixner]

On Tue, 26 Mar 2019, Arnd Bergmann wrote:
> On Tue, Mar 26, 2019 at 1:31 PM Thomas Gleixner <tglx@linutronix.de> wrote:
> >
> > On Tue, 26 Mar 2019, Miroslav Lichvar wrote:
> > > On Sat, Mar 23, 2019 at 11:36:19AM +0100, Thomas Gleixner wrote:
> > > > It is reasonable to force an upper bound for the various methods of setting
> > > > CLOCK_REALTIME. Year 2262 is the absolute upper bound. Assume a maximum
> > > > uptime of 30 years which is plenty enough even for esoteric embedded
> > > > systems. That results in an upper bound of year 2232 for setting the time.
> > >
> > > The patch looks good to me.
> > >
> > > I like this approach better than using a larger value closer to the
> > > overflow (e.g. one week) and stepping the clock back automatically
> > > when the clock reaches that time, but I suspect it might possibly
> > > break more tests (or any unusual applications messing with time) as a
> > > much larger interval is now EINVAL.
> >
> > I'm fine with breaking a few tests on the way rather than having undefined
> > behaviour and the constant flow of patches tackling the wrong end of the
> > stick.
> 
> I think the one downside of your approach is that it introduces a second
> arbitrary cut-off point after which the system almost functions perfectly,
> but is no longer able to do ntp updates or set the right time after a reboot.

Yes, I'm aware of that. But we talk about 113 years from now. Assume we can
fix that proper before the two of us retire. Then you'd need a system which
runs an 80-100 years old kernel in 2232 to run into that problem for real.

There is actually a proper solution for this (ignore RTCs). All user space
interfaces are going to be timespec64 based soon. So they can accomodate
more than 1e11 years.

Now if the kernel internally uses special functions to convert from and to
timespec64 for all interfaces which deal with CLOCK_REALTIME absolute time,
then we still can manage the internal representation in u64 nanoseconds and
have an offset added/subtracted on the relevant interfaces.

That's going to be a bit hairy when time is set back or forth so it needs
to adjust that internal offset, but for regular operation it might be good
enough to have the possible time setting limited to a fixed range depending
on the initial offset.

But even updating the offset should be managable. The conversion functions
would need a seqcount loop and the resulting internal values would be a
struct containing the value and the offset at conversion time. That'd allow
to fix them up at any boundary later on. Not that I want to to that, but if
absolutely necessary, it can be done.

> That said, all other ideas I've managed to come up with are worse,
>  so I agree on going ahead with this version.
> 
> We could still bikeshed over the exact cutoff time, as the one you
> picked isn't particularly intuitive. It's almost exactly 30 years before
> the final end point, but your calculation is off by a few days because
> of leap years. And no, I don't have a particular preference for any
> other color of this bikeshed either, it's probably as good as any other
> time within 20 years of what you suggested.

Haha, we surely could bikeshed that until retirement and then hand it over
to the next generations which might come to an agreement shortly before
2262 :)

Thanks,

	tglx

[tip:timers/core] timekeeping: Force upper bound for setting CLOCK_REALTIME

[tip-bot for Thomas Gleixner]

Commit-ID:  7a8e61f8478639072d402a26789055a4a4de8f77
Gitweb:     https://git.kernel.org/tip/7a8e61f8478639072d402a26789055a4a4de8f77
Author:     Thomas Gleixner <tglx@linutronix.de>
AuthorDate: Sat, 23 Mar 2019 11:36:19 +0100
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Thu, 28 Mar 2019 13:41:06 +0100

timekeeping: Force upper bound for setting CLOCK_REALTIME

Several people reported testing failures after setting CLOCK_REALTIME close
to the limits of the kernel internal representation in nanoseconds,
i.e. year 2262.

The failures are exposed in subsequent operations, i.e. when arming timers
or when the advancing CLOCK_MONOTONIC makes the calculation of
CLOCK_REALTIME overflow into negative space.

Now people start to paper over the underlying problem by clamping
calculations to the valid range, but that's just wrong because such
workarounds will prevent detection of real issues as well.

It is reasonable to force an upper bound for the various methods of setting
CLOCK_REALTIME. Year 2262 is the absolute upper bound. Assume a maximum
uptime of 30 years which is plenty enough even for esoteric embedded
systems. That results in an upper bound of year 2232 for setting the time.

Once that limit is reached in reality this limit is only a small part of
the problem space. But until then this stops people from trying to paper
over the problem at the wrong places.

Reported-by: Xiongfeng Wang <wangxiongfeng2@huawei.com>
Reported-by: Hongbo Yao <yaohongbo@huawei.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: John Stultz <john.stultz@linaro.org>
Cc: Stephen Boyd <sboyd@kernel.org>
Cc: Miroslav Lichvar <mlichvar@redhat.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Richard Cochran <richardcochran@gmail.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Link: https://lkml.kernel.org/r/alpine.DEB.2.21.1903231125480.2157@nanos.tec.linutronix.de

---
 include/linux/time64.h    | 21 +++++++++++++++++++++
 kernel/time/time.c        |  2 +-
 kernel/time/timekeeping.c |  6 +++---
 3 files changed, 25 insertions(+), 4 deletions(-)

diff --git a/include/linux/time64.h b/include/linux/time64.h
index f38d382ffec1..a620ee610b9f 100644
--- a/include/linux/time64.h
+++ b/include/linux/time64.h
@@ -33,6 +33,17 @@ struct itimerspec64 {
 #define KTIME_MAX			((s64)~((u64)1 << 63))
 #define KTIME_SEC_MAX			(KTIME_MAX / NSEC_PER_SEC)
 
+/*
+ * Limits for settimeofday():
+ *
+ * To prevent setting the time close to the wraparound point time setting
+ * is limited so a reasonable uptime can be accomodated. Uptime of 30 years
+ * should be really sufficient, which means the cutoff is 2232. At that
+ * point the cutoff is just a small part of the larger problem.
+ */
+#define TIME_UPTIME_SEC_MAX		(30LL * 365 * 24 *3600)
+#define TIME_SETTOD_SEC_MAX		(KTIME_SEC_MAX - TIME_UPTIME_SEC_MAX)
+
 static inline int timespec64_equal(const struct timespec64 *a,
 				   const struct timespec64 *b)
 {
@@ -100,6 +111,16 @@ static inline bool timespec64_valid_strict(const struct timespec64 *ts)
 	return true;
 }
 
+static inline bool timespec64_valid_settod(const struct timespec64 *ts)
+{
+	if (!timespec64_valid(ts))
+		return false;
+	/* Disallow values which cause overflow issues vs. CLOCK_REALTIME */
+	if ((unsigned long long)ts->tv_sec >= TIME_SETTOD_SEC_MAX)
+		return false;
+	return true;
+}
+
 /**
  * timespec64_to_ns - Convert timespec64 to nanoseconds
  * @ts:		pointer to the timespec64 variable to be converted
diff --git a/kernel/time/time.c b/kernel/time/time.c
index c3f756f8534b..86656bbac232 100644
--- a/kernel/time/time.c
+++ b/kernel/time/time.c
@@ -171,7 +171,7 @@ int do_sys_settimeofday64(const struct timespec64 *tv, const struct timezone *tz
 	static int firsttime = 1;
 	int error = 0;
 
-	if (tv && !timespec64_valid(tv))
+	if (tv && !timespec64_valid_settod(tv))
 		return -EINVAL;
 
 	error = security_settime64(tv, tz);
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index 540145da33da..5716e28bfa3c 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -1221,7 +1221,7 @@ int do_settimeofday64(const struct timespec64 *ts)
 	unsigned long flags;
 	int ret = 0;
 
-	if (!timespec64_valid_strict(ts))
+	if (!timespec64_valid_settod(ts))
 		return -EINVAL;
 
 	raw_spin_lock_irqsave(&timekeeper_lock, flags);
@@ -1278,7 +1278,7 @@ static int timekeeping_inject_offset(const struct timespec64 *ts)
 	/* Make sure the proposed value is valid */
 	tmp = timespec64_add(tk_xtime(tk), *ts);
 	if (timespec64_compare(&tk->wall_to_monotonic, ts) > 0 ||
-	    !timespec64_valid_strict(&tmp)) {
+	    !timespec64_valid_settod(&tmp)) {
 		ret = -EINVAL;
 		goto error;
 	}
@@ -1527,7 +1527,7 @@ void __init timekeeping_init(void)
 	unsigned long flags;
 
 	read_persistent_wall_and_boot_offset(&wall_time, &boot_offset);
-	if (timespec64_valid_strict(&wall_time) &&
+	if (timespec64_valid_settod(&wall_time) &&
 	    timespec64_to_ns(&wall_time) > 0) {
 		persistent_clock_exists = true;
 	} else if (timespec64_to_ns(&wall_time) != 0) {