[PATCH 0/2][RFC] Better handling of insane CMOS values

[PATCH 0/2][RFC] Better handling of insane CMOS values

[John Stultz]

So CAI Qian noticed recent boot trouble on a machine that had its CMOS
clock configured for the year 8200. 
See: http://lkml.org/lkml/2012/7/29/188

While running with a crazy CMOS clock isn't advised, and a simple
"don't do that" might be reasonable, the behavior has in effect
regressed recently due to changes in the hrtimer/timekeeping
interactions.

This patchset tries to resolve this issue in two ways:
1) Change ktime_get_update_offsets to match ktime_get and avoid
possible precision loss with extremely large timespecs.

2) Catch any stop attempt to set the time to a value (circa the
year 2264) large enough to overflow ktime_t.

The end fix here might be an either/or/both combination of these
two changes, so I wanted to send them out for comment. I'm also
looking at further ways to test and improve robustness around
these more extreme time values.

I've also only been able to lightly test. If you want to try this out
you can add the following to timekeeping_init after the
read_persistent_clock() call:

	now.tv_sec = 196469280000LL;

thanks
-john


Cc: Ingo Molnar <mingo@kernel.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Prarit Bhargava <prarit@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Zhouping Liu <zliu@redhat.com>
Cc: CAI Qian <caiqian@redhat.com>


John Stultz (2):
  [RFC] time: Fix problem with large timespecs &
    ktime_get_update_offsets
  [RFC] time: Limit time values that would overflow ktime_t

 kernel/time/timekeeping.c |   40 ++++++++++++++++++++++++++++++----------
 1 file changed, 30 insertions(+), 10 deletions(-)

-- 
1.7.9.5

[PATCH 1/2] [RFC] time: Fix problem with large timespecs & ktime_get_update_offsets

[John Stultz]

There's currently a slight difference in ktime_get_update_offsets()
vs ktime_get() which can result in boot time crashes when booting
with insane CMOS clock values larger then ~2264.

ktime_get() does basically the following:
        return timespec_to_ktime(timespec_add(xtime, wall_to_monotonic))

Where as ktime_get_update_offsets does approximately:
        return ktime_sub(timespec_to_ktime(xtime), realtime_offset);

The problem is, at boot we set xtime = year 8200 and
wall_to_monotonic = year -8200,  ktime_get adds both values, mostly
nulling the difference out (leaving only how long the system has been
up), then converts that relatively small value to a ktime_t properly
without losing any information.

ktime_get_update_offsets however, since it converts xtime (again set
to some value greater then year 8200), to a ktime, it gets clamped at
KTIME_MAX, then we subtract realtime_offset, which is _also_ clamped
at KTIME_MAX, resulting in us always returning almost[1] zero. This
causes us to stop expiring timers.

Now, one of the reasons Thomas and I changed the logic was that using
the precalculated realtime_offset was slightly more efficient then
re-adding xtime and wall_to_monotonic's components separately. But
how valuable this unmeasured slight efficiency is vs extra
robustness for crazy time values is questionable.

So switch back to the ktime_get implementation for
ktime_get_update_offsets

Cc: Ingo Molnar <mingo@kernel.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Prarit Bhargava <prarit@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Zhouping Liu <zliu@redhat.com>
Cc: CAI Qian <caiqian@redhat.com>
Reported-by: CAI Qian <caiqian@redhat.com>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 kernel/time/timekeeping.c |   12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index 3447cfa..96179ab 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -1283,15 +1283,15 @@ void get_xtime_and_monotonic_and_sleep_offset(struct timespec *xtim,
  */
 ktime_t ktime_get_update_offsets(ktime_t *offs_real, ktime_t *offs_boot)
 {
-	ktime_t now;
 	unsigned int seq;
 	u64 secs, nsecs;
 
 	do {
 		seq = read_seqbegin(&timekeeper.lock);
-
-		secs = timekeeper.xtime.tv_sec;
-		nsecs = timekeeper.xtime.tv_nsec;
+		secs = timekeeper.xtime.tv_sec +
+				timekeeper.wall_to_monotonic.tv_sec;
+		nsecs = timekeeper.xtime.tv_nsec +
+				timekeeper.wall_to_monotonic.tv_nsec;
 		nsecs += timekeeping_get_ns();
 		/* If arch requires, add in gettimeoffset() */
 		nsecs += arch_gettimeoffset();
@@ -1300,9 +1300,7 @@ ktime_t ktime_get_update_offsets(ktime_t *offs_real, ktime_t *offs_boot)
 		*offs_boot = timekeeper.offs_boot;
 	} while (read_seqretry(&timekeeper.lock, seq));
 
-	now = ktime_add_ns(ktime_set(secs, 0), nsecs);
-	now = ktime_sub(now, *offs_real);
-	return now;
+	return ktime_add_ns(ktime_set(secs, 0), nsecs);
 }
 #endif
 
-- 
1.7.9.5

Re: [PATCH 1/2] [RFC] time: Fix problem with large timespecs & ktime_get_update_offsets

[Thomas Gleixner]

On Tue, 31 Jul 2012, John Stultz wrote:
> There's currently a slight difference in ktime_get_update_offsets()
> vs ktime_get() which can result in boot time crashes when booting
> with insane CMOS clock values larger then ~2264.
> 
> ktime_get() does basically the following:
>         return timespec_to_ktime(timespec_add(xtime, wall_to_monotonic))
> 
> Where as ktime_get_update_offsets does approximately:
>         return ktime_sub(timespec_to_ktime(xtime), realtime_offset);
> 
> The problem is, at boot we set xtime = year 8200 and
> wall_to_monotonic = year -8200,  ktime_get adds both values, mostly
> nulling the difference out (leaving only how long the system has been
> up), then converts that relatively small value to a ktime_t properly
> without losing any information.
> 
> ktime_get_update_offsets however, since it converts xtime (again set
> to some value greater then year 8200), to a ktime, it gets clamped at
> KTIME_MAX, then we subtract realtime_offset, which is _also_ clamped
> at KTIME_MAX, resulting in us always returning almost[1] zero. This
> causes us to stop expiring timers.
> 
> Now, one of the reasons Thomas and I changed the logic was that using
> the precalculated realtime_offset was slightly more efficient then
> re-adding xtime and wall_to_monotonic's components separately. But
> how valuable this unmeasured slight efficiency is vs extra
> robustness for crazy time values is questionable.
> 
> So switch back to the ktime_get implementation for
> ktime_get_update_offsets

NAK.

You're papering over the real problem: Using crap values without
sanity checking them in the first place.

All your patch does is papering over the problem. Heck, year 8200 is
obvious bullshit, so we can detect and reject it.

Thanks,

	tglx

 

Re: [PATCH 1/2] [RFC] time: Fix problem with large timespecs & ktime_get_update_offsets

[John Stultz]

On 07/31/2012 11:52 PM, Thomas Gleixner wrote:
> On Tue, 31 Jul 2012, John Stultz wrote:
>> There's currently a slight difference in ktime_get_update_offsets()
>> vs ktime_get() which can result in boot time crashes when booting
>> with insane CMOS clock values larger then ~2264.
>>
>> ktime_get() does basically the following:
>>          return timespec_to_ktime(timespec_add(xtime, wall_to_monotonic))
>>
>> Where as ktime_get_update_offsets does approximately:
>>          return ktime_sub(timespec_to_ktime(xtime), realtime_offset);
>>
>> The problem is, at boot we set xtime = year 8200 and
>> wall_to_monotonic = year -8200,  ktime_get adds both values, mostly
>> nulling the difference out (leaving only how long the system has been
>> up), then converts that relatively small value to a ktime_t properly
>> without losing any information.
>>
>> ktime_get_update_offsets however, since it converts xtime (again set
>> to some value greater then year 8200), to a ktime, it gets clamped at
>> KTIME_MAX, then we subtract realtime_offset, which is _also_ clamped
>> at KTIME_MAX, resulting in us always returning almost[1] zero. This
>> causes us to stop expiring timers.
>>
>> Now, one of the reasons Thomas and I changed the logic was that using
>> the precalculated realtime_offset was slightly more efficient then
>> re-adding xtime and wall_to_monotonic's components separately. But
>> how valuable this unmeasured slight efficiency is vs extra
>> robustness for crazy time values is questionable.
>>
>> So switch back to the ktime_get implementation for
>> ktime_get_update_offsets
> NAK.
>
> You're papering over the real problem: Using crap values without
> sanity checking them in the first place.
>
> All your patch does is papering over the problem. Heck, year 8200 is
> obvious bullshit, so we can detect and reject it.

Ok, sounds good. I'll drop this one and just keep the sanity checking patch.

thanks
-john

[PATCH 2/2] [RFC] time: Limit time values that would overflow ktime_t

[John Stultz]

We could observe unexpected behavior if the time is set
to a value large enough to overflow a 64bit ktime_t
(which is something larger then the year 2264).

So check timekeeping inputs to make sure we don't set
the time to a value that overflows ktime_t.

Note: This does not protect from setting the time close to
overflowing ktime_t and then letting natural accumulation
cause the overflow.

Cc: Ingo Molnar <mingo@kernel.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Prarit Bhargava <prarit@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Zhouping Liu <zliu@redhat.com>
Cc: CAI Qian <caiqian@redhat.com>
Reported-by: CAI Qian <caiqian@redhat.com>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 kernel/time/timekeeping.c |   28 +++++++++++++++++++++++++---
 1 file changed, 25 insertions(+), 3 deletions(-)

diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index 96179ab..78bccd0 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -92,7 +92,7 @@ __cacheline_aligned_in_smp DEFINE_SEQLOCK(xtime_lock);
 /* flag for if timekeeping is suspended */
 int __read_mostly timekeeping_suspended;
 
-
+#define TWENTY_YEARS (20LL*365*24*60*60)
 
 /**
  * timekeeper_setup_internals - Set up internals to use clocksource clock.
@@ -387,6 +387,9 @@ int do_settimeofday(const struct timespec *tv)
 	if ((unsigned long)tv->tv_nsec >= NSEC_PER_SEC)
 		return -EINVAL;
 
+	if ((unsigned long long)tv->tv_sec >= KTIME_SEC_MAX)
+		return -EINVAL;
+
 	write_seqlock_irqsave(&timekeeper.lock, flags);
 
 	timekeeping_forward_now();
@@ -418,6 +421,8 @@ EXPORT_SYMBOL(do_settimeofday);
 int timekeeping_inject_offset(struct timespec *ts)
 {
 	unsigned long flags;
+	struct timespec tmp;
+	int ret = 0;
 
 	if ((unsigned long)ts->tv_nsec >= NSEC_PER_SEC)
 		return -EINVAL;
@@ -426,10 +431,17 @@ int timekeeping_inject_offset(struct timespec *ts)
 
 	timekeeping_forward_now();
 
-	timekeeper.xtime = timespec_add(timekeeper.xtime, *ts);
+	/* Make sure the increased value won't cause ktime_t trouble */
+	tmp = timespec_add(timekeeper.xtime, *ts);
+	if ((unsigned long long)tmp.tv_sec >= KTIME_SEC_MAX) {
+		ret= -EINVAL;
+		goto error;
+	}
+	timekeeper.xtime = tmp;
 	timekeeper.wall_to_monotonic =
 				timespec_sub(timekeeper.wall_to_monotonic, *ts);
 
+error: /* even if we error out, we forwarded the time, so call update */
 	timekeeping_update(true);
 
 	write_sequnlock_irqrestore(&timekeeper.lock, flags);
@@ -437,7 +449,7 @@ int timekeeping_inject_offset(struct timespec *ts)
 	/* signal hrtimers about time change */
 	clock_was_set();
 
-	return 0;
+	return ret;
 }
 EXPORT_SYMBOL(timekeeping_inject_offset);
 
@@ -599,6 +611,16 @@ void __init timekeeping_init(void)
 	read_persistent_clock(&now);
 	read_boot_clock(&boot);
 
+	/*
+	 * Check to make sure the persistent clock
+	 * didn't return something crazy.
+	 */
+	if (now.tv_sec > (KTIME_SEC_MAX - TWENTY_YEARS)) {
+		printk("WARNING: Persistent clock returned a year greater then"
+			" 2242. Capping at 2242.\n");
+		now.tv_sec = KTIME_SEC_MAX - TWENTY_YEARS;
+	}
+
 	seqlock_init(&timekeeper.lock);
 
 	ntp_init();
-- 
1.7.9.5

Re: [PATCH 0/2][RFC] Better handling of insane CMOS values

[James Courtier-Dutton]

On 31 July 2012 07:35, John Stultz <john.stultz@linaro.org> wrote:
> So CAI Qian noticed recent boot trouble on a machine that had its CMOS
> clock configured for the year 8200.
> See: http://lkml.org/lkml/2012/7/29/188
>
> While running with a crazy CMOS clock isn't advised, and a simple
> "don't do that" might be reasonable, the behavior has in effect
> regressed recently due to changes in the hrtimer/timekeeping
> interactions.
>
Would it not be easier to work out which CMOS clock values we can
handle correctly, and then do input value validation on the CMOS
values, and if a value is outside the acceptable range, assume a value
equal to the minimum acceptable value.

Also, surely all timer use within the kernel should use the monotonic
time source for timer expiry, and not depend on CMOS time.
I.e. If we want a thread to wake up once every 30 seconds, use the
monotonic time source for that.
conversion to local time should only be needed for timestamps.
We might need a compile/boot time option to assign useable ranges to
local time values.
E.g. For this boot, local time is between year 2000 and 2100
For next boot, local time is between year 2050 and 2150.
Obviously, make the range as wide as we can sensibly handle, so the
setting does not need to be changed very often.
The range will be determined by the amount of bits in the time values.
User space can work on much wider ranges for historical date storage,
but for accessing the current time, smaller ranges are workable.

Kind Regards

James








> This patchset tries to resolve this issue in two ways:
> 1) Change ktime_get_update_offsets to match ktime_get and avoid
> possible precision loss with extremely large timespecs.
>
> 2) Catch any stop attempt to set the time to a value (circa the
> year 2264) large enough to overflow ktime_t.
>
> The end fix here might be an either/or/both combination of these
> two changes, so I wanted to send them out for comment. I'm also
> looking at further ways to test and improve robustness around
> these more extreme time values.
>
> I've also only been able to lightly test. If you want to try this out
> you can add the following to timekeeping_init after the
> read_persistent_clock() call:
>
>         now.tv_sec = 196469280000LL;
>
> thanks
> -john
>
>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Cc: Prarit Bhargava <prarit@redhat.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Zhouping Liu <zliu@redhat.com>
> Cc: CAI Qian <caiqian@redhat.com>
>
>
> John Stultz (2):
>   [RFC] time: Fix problem with large timespecs &
>     ktime_get_update_offsets
>   [RFC] time: Limit time values that would overflow ktime_t
>
>  kernel/time/timekeeping.c |   40 ++++++++++++++++++++++++++++++----------
>  1 file changed, 30 insertions(+), 10 deletions(-)
>
> --
> 1.7.9.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 0/2][RFC] Better handling of insane CMOS values

[John Stultz]

On 07/31/2012 02:54 AM, James Courtier-Dutton wrote:
> On 31 July 2012 07:35, John Stultz <john.stultz@linaro.org> wrote:
>> So CAI Qian noticed recent boot trouble on a machine that had its CMOS
>> clock configured for the year 8200.
>> See: http://lkml.org/lkml/2012/7/29/188
>>
>> While running with a crazy CMOS clock isn't advised, and a simple
>> "don't do that" might be reasonable, the behavior has in effect
>> regressed recently due to changes in the hrtimer/timekeeping
>> interactions.
>>
> Would it not be easier to work out which CMOS clock values we can
> handle correctly, and then do input value validation on the CMOS
> values, and if a value is outside the acceptable range, assume a value
> equal to the minimum acceptable value.

I believe patch 2/2 does exactly this.

> Also, surely all timer use within the kernel should use the monotonic
> time source for timer expiry, and not depend on CMOS time.
> I.e. If we want a thread to wake up once every 30 seconds, use the
> monotonic time source for that.
> conversion to local time should only be needed for timestamps.
Timers can also be specified against various clockids like 
CLOCK_REALTIME (and absolute time values, like expire this timer at 
11:55 GMT, Oct 5 2012).  So while the kernel does manage most things 
internally with CLOCK_MONOTONIC, it still has to manage some timers 
against other clockids.

> We might need a compile/boot time option to assign useable ranges to
> local time values.
> E.g. For this boot, local time is between year 2000 and 2100
> For next boot, local time is between year 2050 and 2150.
> Obviously, make the range as wide as we can sensibly handle, so the
> setting does not need to be changed very often.
> The range will be determined by the amount of bits in the time values.
> User space can work on much wider ranges for historical date storage,
> but for accessing the current time, smaller ranges are workable.
Right. So one way to handle the range difference between the ktime_t and 
timespec values would be to have kernel internal ktime_t epochs that we 
remove and readd as needed.  So instead of using 1970, we pick some 
close value to the boot time like 2010, and then subtract 40 years off 
all the inputs, and it back on to the outputs. Aside from the extra 
computation, the problem is that if a system booted up like the one in 
this case w/ a year of 8200, and we selected that as this kernel epoch, 
we then would have problems setting the time back to 2012, as there may 
be CLOCK_REALTIME timers set for 8200 that we could not store within the 
292 year epoch.  We probably could decide that timers larger then the 
ktime_t epoch will expire "never" and then re-calculate if needed when 
the time is set.

Even so, I suspect the extra complication, along with the extra overhead 
required will make such a plan unpopular until its actually needed 
(around the year 2264). So I suspect the input sanitation is really the 
most likely approach for now.

thanks
-john

Re: [PATCH 0/2][RFC] Better handling of insane CMOS values

[Josh Boyer]

On Tue, Jul 31, 2012 at 2:35 AM, John Stultz <john.stultz@linaro.org> wrote:
> So CAI Qian noticed recent boot trouble on a machine that had its CMOS
> clock configured for the year 8200.
> See: http://lkml.org/lkml/2012/7/29/188
>
> While running with a crazy CMOS clock isn't advised, and a simple
> "don't do that" might be reasonable, the behavior has in effect
> regressed recently due to changes in the hrtimer/timekeeping
> interactions.
>
> This patchset tries to resolve this issue in two ways:
> 1) Change ktime_get_update_offsets to match ktime_get and avoid
> possible precision loss with extremely large timespecs.
>
> 2) Catch any stop attempt to set the time to a value (circa the
> year 2264) large enough to overflow ktime_t.
>
> The end fix here might be an either/or/both combination of these
> two changes, so I wanted to send them out for comment. I'm also
> looking at further ways to test and improve robustness around
> these more extreme time values.
>
> I've also only been able to lightly test. If you want to try this out
> you can add the following to timekeeping_init after the
> read_persistent_clock() call:
>
>         now.tv_sec = 196469280000LL;
>
> thanks
> -john
>
>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Cc: Prarit Bhargava <prarit@redhat.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Zhouping Liu <zliu@redhat.com>
> Cc: CAI Qian <caiqian@redhat.com>

These should be CC'd to stable, right?  CAI hit this with a 3.5-rcX
kernel, and the hrtimer stuff was backported to 3.4 and before I
thought.

josh

Re: [PATCH 0/2][RFC] Better handling of insane CMOS values

[John Stultz]

On 07/31/2012 04:31 AM, Josh Boyer wrote:
>
> These should be CC'd to stable, right?  CAI hit this with a 3.5-rcX
> kernel, and the hrtimer stuff was backported to 3.4 and before I
> thought.
Yes.  But I'm just looking for feedback on the approach for now, this 
isn't for submission yet.

thanks
-john

Re: [PATCH 0/2][RFC] Better handling of insane CMOS values

[Prarit Bhargava]

On 07/31/2012 02:35 AM, John Stultz wrote:
> So CAI Qian noticed recent boot trouble on a machine that had its CMOS
> clock configured for the year 8200. 
> See: http://lkml.org/lkml/2012/7/29/188

In case anyone was wondering, the system's date was very much screwed up:

│ System Time .......................................... 13:39:40              │
│ System Date .......................................... Tue Jul 31, 8212

After testing these patches I set the year to 2012.

P.

Re: [PATCH 0/2][RFC] Better handling of insane CMOS values

[John Stultz]

On 07/30/2012 11:35 PM, John Stultz wrote:
> I've also only been able to lightly test. If you want to try this out
> you can add the following to timekeeping_init after the
> read_persistent_clock() call:
>
> 	now.tv_sec = 196469280000LL;
Prarit noted that I implemented these patches against 3.5 (what the 
problem was reported against) and not Linus' head.

I'm mostly just looking for input on the approach, but just in case 
anyone else wanted to actually play with these patches, they'll need to 
do so against 3.5. I'll  update the patchset against Linus' tree before 
I send them out again.

Sorry about that.

thanks
-john