[RFE PATCH] time, Fix setting of hardware clock in NTP code

[RFE PATCH] time, Fix setting of hardware clock in NTP code

[Prarit Bhargava]

I have found a long existing bug in the ntp code that causes a forwarding of
time equal to that of the local timezone every reboot.  This is the sequence
indicating what happens during boot.

+ start boot
|
+ rtc read, written as UTC into xtime/system clock.  This time is rtc0_time
  below.
|
|
+ ... rest of initial kernel boot, initramfs, etc.
|
|
+ systemd/initscript/etc: set timezone data through first call to settimeofday()
        - if LOCAL, a timezone offset is applied so that all applications "see"
           the system time as UTC, ie) sys_tz = {offset from UTC, 0}
        - if UTC, no timezone offset is applied, ie) sys_tz = {0,0};
+ The first settimeofday() calls warp_clock().  xtime (system time) is set to
  rtc time + sys_tz.tz_minuteswest .
  On my system, the difference between the rtc and the system time is now
  300 mins.
|
|
+ ntpd starts
+       - RHEL7: the first adjtimex() clears the STA_PLL flag.  This causes
          STA_UNSYNC to be cleared and the system time/xtime to be written to
          the rtc via update_persistent_clock().
                - if LOCAL, this means that the rtc now reads
                  rtc + sys_tz.tz_minuteswest; on my system this is rtc + 300.
                - if UTC, this means that the rtc on my system reads rtc + 0.

The problem with this model is what happens if /etc/adjtime is LOCAL, ie)
the rtc is set to localtime:

Reboot the system, on the next boot,
        rtc0_time = rtc + sys_tz.tz_minuteswest
Reboot the system, on the next boot,
        rtc0_time = rtc + sys_tz.tz_minuteswest + sys_tz.tz_minuteswest

AFAICT the only call to update_persistent_clock() in the kernel is the
ntp code.  It is wired in to allow ntp to occasionally update the system
clock.  Other calls to update the rtc are made directly through the
RTC_SET_TIME ioctl.

I believe that the value passed into update_persistent_time() is wrong.  It
should take into account the sys_tz data.  If the rtc is UTC, then the
offset is 0.  If the system is LOCAL, then there should be a 300 min offset
for the value of now.

We do not see this manifest on some architectures because they limit changes
to the rtc to +/-15 minutes of the current value of the rtc (x86, alpha,
mn10300).  Other arches do nothing (cris, mips, sh), and only a few seem to
show this problem (power, sparc).  I can reproduce this reliably on powerpc
with the latest Fedoras (17, 18, rawhide), as well as an Ubuntu powerpc spin.
I can also reproduce it "older" OSes such as RHEL6.

A few things about the patch.  'sys_time_offset' certainly could have a
better name and it could be a bool.  Also, technically I do not need to add the
'adjust' struct in sync_cmos_clock() as the value of now.tv_sec is not used
after being passed into update_persistent_clock().  However, I think the code
is easier to follow if I do add 'adjust'.

------8<---------

Take the timezone offset applied in warp_clock() into account when writing
the hardware clock in the ntp code.  This patch adds sys_time_offset which
indicates that an offset has been applied in warp_clock().

Signed-off-by: Prarit Bhargava <prarit@redhat.com>
Cc: John Stultz <johnstul@us.ibm.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
---
 include/linux/time.h |    1 +
 kernel/time.c        |    8 ++++++++
 kernel/time/ntp.c    |    8 ++++++--
 3 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/include/linux/time.h b/include/linux/time.h
index 4d358e9..02754b5 100644
--- a/include/linux/time.h
+++ b/include/linux/time.h
@@ -117,6 +117,7 @@ static inline bool timespec_valid_strict(const struct timespec *ts)
 
 extern void read_persistent_clock(struct timespec *ts);
 extern void read_boot_clock(struct timespec *ts);
+extern int sys_time_offset;
 extern int update_persistent_clock(struct timespec now);
 void timekeeping_init(void);
 extern int timekeeping_suspended;
diff --git a/kernel/time.c b/kernel/time.c
index d226c6a..66533d3 100644
--- a/kernel/time.c
+++ b/kernel/time.c
@@ -115,6 +115,12 @@ SYSCALL_DEFINE2(gettimeofday, struct timeval __user *, tv,
 }
 
 /*
+ * Indicates if there is an offset between the system clock and the hardware
+ * clock/persistent clock/rtc.
+ */
+int sys_time_offset;
+
+/*
  * Adjust the time obtained from the CMOS to be UTC time instead of
  * local time.
  *
@@ -135,6 +141,8 @@ static inline void warp_clock(void)
 	struct timespec adjust;
 
 	adjust = current_kernel_time();
+	if (sys_tz.tz_minuteswest > 0)
+		sys_time_offset = 1;
 	adjust.tv_sec += sys_tz.tz_minuteswest * 60;
 	do_settimeofday(&adjust);
 }
diff --git a/kernel/time/ntp.c b/kernel/time/ntp.c
index 24174b4..39b88c4 100644
--- a/kernel/time/ntp.c
+++ b/kernel/time/ntp.c
@@ -510,8 +510,12 @@ static void sync_cmos_clock(struct work_struct *work)
 	}
 
 	getnstimeofday(&now);
-	if (abs(now.tv_nsec - (NSEC_PER_SEC / 2)) <= tick_nsec / 2)
-		fail = update_persistent_clock(now);
+	if (abs(now.tv_nsec - (NSEC_PER_SEC / 2)) <= tick_nsec / 2) {
+		struct timespec adjust = now;
+		if (sys_time_offset)
+			adjust.tv_sec -= (sys_tz.tz_minuteswest * 60);
+		fail = update_persistent_clock(adjust);
+	}
 
 	next.tv_nsec = (NSEC_PER_SEC / 2) - now.tv_nsec - (TICK_NSEC / 2);
 	if (next.tv_nsec <= 0)
-- 
1.7.9.3

Re: [RFE PATCH] time, Fix setting of hardware clock in NTP code

[John Stultz]

On Thu, Feb 7, 2013 at 5:29 AM, Prarit Bhargava <prarit@redhat.com> wrote:
> The problem with this model is what happens if /etc/adjtime is LOCAL, ie)
> the rtc is set to localtime:
>
> Reboot the system, on the next boot,
>         rtc0_time = rtc + sys_tz.tz_minuteswest
> Reboot the system, on the next boot,
>         rtc0_time = rtc + sys_tz.tz_minuteswest + sys_tz.tz_minuteswest
>
> AFAICT the only call to update_persistent_clock() in the kernel is the
> ntp code.  It is wired in to allow ntp to occasionally update the system
> clock.  Other calls to update the rtc are made directly through the
> RTC_SET_TIME ioctl.
>
> I believe that the value passed into update_persistent_time() is wrong.  It
> should take into account the sys_tz data.  If the rtc is UTC, then the
> offset is 0.  If the system is LOCAL, then there should be a 300 min offset
> for the value of now.
>
> We do not see this manifest on some architectures because they limit changes
> to the rtc to +/-15 minutes of the current value of the rtc (x86, alpha,
> mn10300).  Other arches do nothing (cris, mips, sh), and only a few seem to
> show this problem (power, sparc).  I can reproduce this reliably on powerpc
> with the latest Fedoras (17, 18, rawhide), as well as an Ubuntu powerpc spin.
> I can also reproduce it "older" OSes such as RHEL6.

Interesting.
Yea, local RTC time is probably pretty rare outside of x86 (due to windows).
And the +/- 15minute trick has always explicitly masked this issue there.


> A few things about the patch.  'sys_time_offset' certainly could have a
> better name and it could be a bool.  Also, technically I do not need to add the
> 'adjust' struct in sync_cmos_clock() as the value of now.tv_sec is not used
> after being passed into update_persistent_clock().  However, I think the code
> is easier to follow if I do add 'adjust'.
>
> ------8<---------
>
> Take the timezone offset applied in warp_clock() into account when writing
> the hardware clock in the ntp code.  This patch adds sys_time_offset which
> indicates that an offset has been applied in warp_clock().
>
> Signed-off-by: Prarit Bhargava <prarit@redhat.com>
> Cc: John Stultz <johnstul@us.ibm.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> ---
>  include/linux/time.h |    1 +
>  kernel/time.c        |    8 ++++++++
>  kernel/time/ntp.c    |    8 ++++++--
>  3 files changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/time.h b/include/linux/time.h
> index 4d358e9..02754b5 100644
> --- a/include/linux/time.h
> +++ b/include/linux/time.h
> @@ -117,6 +117,7 @@ static inline bool timespec_valid_strict(const struct timespec *ts)
>
>  extern void read_persistent_clock(struct timespec *ts);
>  extern void read_boot_clock(struct timespec *ts);
> +extern int sys_time_offset;
>  extern int update_persistent_clock(struct timespec now);
>  void timekeeping_init(void);
>  extern int timekeeping_suspended;
> diff --git a/kernel/time.c b/kernel/time.c
> index d226c6a..66533d3 100644
> --- a/kernel/time.c
> +++ b/kernel/time.c
> @@ -115,6 +115,12 @@ SYSCALL_DEFINE2(gettimeofday, struct timeval __user *, tv,
>  }
>
>  /*
> + * Indicates if there is an offset between the system clock and the hardware
> + * clock/persistent clock/rtc.
> + */
> +int sys_time_offset;

So why is this extra flag necessary instead of just using if
(sys_tz.tz_minuteswest) ?


> +
> +/*
>   * Adjust the time obtained from the CMOS to be UTC time instead of
>   * local time.
>   *
> @@ -135,6 +141,8 @@ static inline void warp_clock(void)
>         struct timespec adjust;
>
>         adjust = current_kernel_time();
> +       if (sys_tz.tz_minuteswest > 0)
> +               sys_time_offset = 1;

This seems like it wouldn't work for localtimes that are east of GMT.
Or am I misunderstanding this? I feel like the warp_clock code has
always been a bit unloved, and I've never worked out all the
subtleties around it.

thanks
-john

Re: [RFE PATCH] time, Fix setting of hardware clock in NTP code

[Prarit Bhargava]

On 02/07/2013 12:24 PM, John Stultz wrote:
> On Thu, Feb 7, 2013 at 5:29 AM, Prarit Bhargava <prarit@redhat.com> wrote:
>> We do not see this manifest on some architectures because they limit changes
>> to the rtc to +/-15 minutes of the current value of the rtc (x86, alpha,
>> mn10300).  Other arches do nothing (cris, mips, sh), and only a few seem to
>> show this problem (power, sparc).  I can reproduce this reliably on powerpc
>> with the latest Fedoras (17, 18, rawhide), as well as an Ubuntu powerpc spin.
>> I can also reproduce it "older" OSes such as RHEL6.
> 
> Interesting.
> Yea, local RTC time is probably pretty rare outside of x86 (due to windows).
> And the +/- 15minute trick has always explicitly masked this issue there.
> 

I'm not sure I understand the purpose behind the +/-15 minute window?  Is it
just to prevent a wild swing on the RTC?  I can understand that to some degree,
however, I'm not sure I agree with it being the default behaviour.

Here's a real-world scenario:

My RTC on my laptop is set to 1:30PM Jan 7, 2013.  I boot, systemd and ntp do
their magic, and the system time comes up as Feb 7, 12:48PM.  I never will
notice that the RTC is wrong.

Now I go somewhere and have to work on a plane.  I have no internet connection
and then boot.  Now the system time will be 1:30PM Jan 7, 2013.  That's actually
happened to me and I remember filing it away for a bug to be looked at.

AFAIK, no other OS does that ... if I install Windows or use a Mac in the
no-internet connection case, the time is *always* corrected.  I tried to see if
I could get this to happen on a Mac and I can't.

99.99999% of Linux users out there are using some sort of time protocol (usually
NTP, but PTP is starting to catch on) to sync their systems.  NTP is a trusted
source of timekeeping IMO.  How often do we see systems that run NTP but don't
trust the numbers that come from it?

We should be doing a full sync of the RTC in NTP, or at least it should be an
option/CONFIG option (FYI: I want to patch for that ... it'll give me something
to do).

> 
>> A few things about the patch.  'sys_time_offset' certainly could have a
>> better name and it could be a bool.  Also, technically I do not need to add the
>> 'adjust' struct in sync_cmos_clock() as the value of now.tv_sec is not used
>> after being passed into update_persistent_clock().  However, I think the code
>> is easier to follow if I do add 'adjust'.
>>
>> ------8<---------
>>
>> Take the timezone offset applied in warp_clock() into account when writing
>> the hardware clock in the ntp code.  This patch adds sys_time_offset which
>> indicates that an offset has been applied in warp_clock().
>>
>> Signed-off-by: Prarit Bhargava <prarit@redhat.com>
>> Cc: John Stultz <johnstul@us.ibm.com>
>> Cc: Thomas Gleixner <tglx@linutronix.de>
>> ---
>>  include/linux/time.h |    1 +
>>  kernel/time.c        |    8 ++++++++
>>  kernel/time/ntp.c    |    8 ++++++--
>>  3 files changed, 15 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/linux/time.h b/include/linux/time.h
>> index 4d358e9..02754b5 100644
>> --- a/include/linux/time.h
>> +++ b/include/linux/time.h
>> @@ -117,6 +117,7 @@ static inline bool timespec_valid_strict(const struct timespec *ts)
>>
>>  extern void read_persistent_clock(struct timespec *ts);
>>  extern void read_boot_clock(struct timespec *ts);
>> +extern int sys_time_offset;
>>  extern int update_persistent_clock(struct timespec now);
>>  void timekeeping_init(void);
>>  extern int timekeeping_suspended;
>> diff --git a/kernel/time.c b/kernel/time.c
>> index d226c6a..66533d3 100644
>> --- a/kernel/time.c
>> +++ b/kernel/time.c
>> @@ -115,6 +115,12 @@ SYSCALL_DEFINE2(gettimeofday, struct timeval __user *, tv,
>>  }
>>
>>  /*
>> + * Indicates if there is an offset between the system clock and the hardware
>> + * clock/persistent clock/rtc.
>> + */
>> +int sys_time_offset;
> 
> So why is this extra flag necessary instead of just using if
> (sys_tz.tz_minuteswest) ?

sys_tz can be set during runtime via settimeofday() without affecting the
current system time.  The bug *only* happens if the system clock is "warped"
ahead relative to the hardware clock on the first call to settimeofday(), so
checking for sys_tz.tz_minuteswest isn't good enough of a test.

> 
> 
>> +
>> +/*
>>   * Adjust the time obtained from the CMOS to be UTC time instead of
>>   * local time.
>>   *
>> @@ -135,6 +141,8 @@ static inline void warp_clock(void)
>>         struct timespec adjust;
>>
>>         adjust = current_kernel_time();
>> +       if (sys_tz.tz_minuteswest > 0)
>> +               sys_time_offset = 1;
> 
> This seems like it wouldn't work for localtimes that are east of GMT.

Ah, good point.  I had it in my head that minuteswest was always negative.  That
should be

if (sys_tz.tz_minuteswest != 0)

I'll fix that in the next !RFE patch.

P.

Re: [RFE PATCH] time, Fix setting of hardware clock in NTP code

[John Stultz]

On 02/07/2013 10:20 AM, Prarit Bhargava wrote:
>
> On 02/07/2013 12:24 PM, John Stultz wrote:
>> On Thu, Feb 7, 2013 at 5:29 AM, Prarit Bhargava <prarit@redhat.com> wrote:
>>> We do not see this manifest on some architectures because they limit changes
>>> to the rtc to +/-15 minutes of the current value of the rtc (x86, alpha,
>>> mn10300).  Other arches do nothing (cris, mips, sh), and only a few seem to
>>> show this problem (power, sparc).  I can reproduce this reliably on powerpc
>>> with the latest Fedoras (17, 18, rawhide), as well as an Ubuntu powerpc spin.
>>> I can also reproduce it "older" OSes such as RHEL6.
>> Interesting.
>> Yea, local RTC time is probably pretty rare outside of x86 (due to windows).
>> And the +/- 15minute trick has always explicitly masked this issue there.
>>
> I'm not sure I understand the purpose behind the +/-15 minute window?  Is it
> just to prevent a wild swing on the RTC?  I can understand that to some degree,
> however, I'm not sure I agree with it being the default behaviour.

The 15 minute cap is totally an x86-ism, and I believe its due to the 
fact that the main concern is we don't reliably know the timezone data 
has been set properly, but we're expected to work well dual booting with 
Windows.

Imagine the case where he RTC is local, but the system doesn't set the 
timezone at boot, so warp-clock never runs, and then NTP jumps in and 
just sets the RTC to UTC, which then causes problems the next boot up 
with windows.


> Here's a real-world scenario:
>
> My RTC on my laptop is set to 1:30PM Jan 7, 2013.  I boot, systemd and ntp do
> their magic, and the system time comes up as Feb 7, 12:48PM.  I never will
> notice that the RTC is wrong.
>
> Now I go somewhere and have to work on a plane.  I have no internet connection
> and then boot.  Now the system time will be 1:30PM Jan 7, 2013.  That's actually
> happened to me and I remember filing it away for a bug to be looked at.
>
> AFAIK, no other OS does that ... if I install Windows or use a Mac in the
> no-internet connection case, the time is *always* corrected.  I tried to see if
> I could get this to happen on a Mac and I can't.

Yea, although I suspect you'd have problems dual-booting a Mac with Windows.
http://lifehacker.com/5742148/fix-windows-clock-issues-when-dual+booting-with-os-x

Though interestingly that link suggests a fix on the windows side.


> 99.99999% of Linux users out there are using some sort of time protocol (usually
> NTP, but PTP is starting to catch on) to sync their systems.  NTP is a trusted
> source of timekeeping IMO.  How often do we see systems that run NTP but don't
> trust the numbers that come from it?

I actually doubt ntp usage is that high, given that some popular distros 
don't install it by default, but that's a tangent. :)

Again, the quirks here are all about interacting with Windows on a 
dual-boot environment.

Though I think it might be reasonable at this point to say we'll set the 
RTC as accurately as we can with the given info, which requires the 
distro to trigger warp clock if the RTC is kept in local time.




>>>   /*
>>> + * Indicates if there is an offset between the system clock and the hardware
>>> + * clock/persistent clock/rtc.
>>> + */
>>> +int sys_time_offset;
>> So why is this extra flag necessary instead of just using if
>> (sys_tz.tz_minuteswest) ?
> sys_tz can be set during runtime via settimeofday() without affecting the
> current system time.  The bug *only* happens if the system clock is "warped"
> ahead relative to the hardware clock on the first call to settimeofday(), so
> checking for sys_tz.tz_minuteswest isn't good enough of a test.

So it would probably be better named as something like rtc_is_local.


So yea, I think if we include your patch, we can probably consider 
dropping the 15 min cap. There will probably be some situations where 
system setups don't have RTC local configured, so that flag isn't set 
and we'll fight with a dual-boot environment, but those hopefully should 
be rare.

I'd suggest we do this in two steps. First your current patch, adding 
rtc_is_local flag and the RTC timezone correction in 
update_persistent_clock(), then second a patch for x86 dropping the 15 
min cap that gets wide distribution so all the distros know its coming 
and can test it and object if necessary.

thanks
-john

Re: [RFE PATCH] time, Fix setting of hardware clock in NTP code

[Prarit Bhargava]

On 02/07/2013 02:52 PM, John Stultz wrote:
> On 02/07/2013 10:20 AM, Prarit Bhargava wrote:
>>
>> On 02/07/2013 12:24 PM, John Stultz wrote:
>>> On Thu, Feb 7, 2013 at 5:29 AM, Prarit Bhargava <prarit@redhat.com> wrote:
>>>
>> I'm not sure I understand the purpose behind the +/-15 minute window?  Is it
>> just to prevent a wild swing on the RTC?  I can understand that to some degree,
>> however, I'm not sure I agree with it being the default behaviour.
> 
> The 15 minute cap is totally an x86-ism, and I believe its due to the fact that
> the main concern is we don't reliably know the timezone data has been set
> properly, but we're expected to work well dual booting with Windows.

Heh :)  I think some of the other arches have copied what x86 does.  alpha and
mn10300 do the same thing. :)


> 
>> 99.99999% of Linux users out there are using some sort of time protocol (usually
>> NTP, but PTP is starting to catch on) to sync their systems.  NTP is a trusted
>> source of timekeeping IMO.  How often do we see systems that run NTP but don't
>> trust the numbers that come from it?
> 
> I actually doubt ntp usage is that high, given that some popular distros don't
> install it by default, but that's a tangent. :)

:D  Fair enough :D

> 
> Again, the quirks here are all about interacting with Windows on a dual-boot
> environment.
> 

Sure, that's been my understanding as well.

> Though I think it might be reasonable at this point to say we'll set the RTC as
> accurately as we can with the given info, which requires the distro to trigger
> warp clock if the RTC is kept in local time.

Okay.

> 
> 
> 
> 
>>>>   /*
>>>> + * Indicates if there is an offset between the system clock and the hardware
>>>> + * clock/persistent clock/rtc.
>>>> + */
>>>> +int sys_time_offset;
>>> So why is this extra flag necessary instead of just using if
>>> (sys_tz.tz_minuteswest) ?
>> sys_tz can be set during runtime via settimeofday() without affecting the
>> current system time.  The bug *only* happens if the system clock is "warped"
>> ahead relative to the hardware clock on the first call to settimeofday(), so
>> checking for sys_tz.tz_minuteswest isn't good enough of a test.
> 
> So it would probably be better named as something like rtc_is_local.

I'll do a [v2] with that change as well.

> 
> 
> So yea, I think if we include your patch, we can probably consider dropping the
> 15 min cap. There will probably be some situations where system setups don't
> have RTC local configured, so that flag isn't set and we'll fight with a
> dual-boot environment, but those hopefully should be rare.
> 
> I'd suggest we do this in two steps. First your current patch, adding
> rtc_is_local flag and the RTC timezone correction in update_persistent_clock(),
> then second a patch for x86 dropping the 15 min cap that gets wide distribution
> so all the distros know its coming and can test it and object if necessary.

That's what I was hoping for.  I'll post this as an actual patch and then get to
work on the full sync code next.

Thanks,

P.

> 
> thanks
> -john
> 
> 
> 
> 
>