[PATCH 0/1] ntp bug fix

[PATCH 0/1] ntp bug fix

[Richard Cochran]

I have been cooking up a patch series to keep the core time in TAI
instead of UTC. In the course of that work I noticed a bug in the
current NTP code. During a leap second insertion, the kernel will
advertise the wrong TAI offset.

There is a nice explanation of this issue in the NIST leap second
list, which I quote below. With reference to that, the Linux kernel
handles the UTC time value like in the first case, but sets the TAI
offset as in the second case, resulting in a discontinuity in the
apparent TAI time.

* Excerpt from ftp://time.nist.gov/pub/leap-seconds.list

   If your system keeps time as the number of seconds since some epoch
   (e.g., NTP timestamps), then the algorithm for assigning a UTC time
   stamp to an event that happens during a positive leap second is not
   well defined. The official name of that leap second is 23:59:60,
   but there is no way of representing that time in these systems.

   Many systems of this type effectively stop the system clock for one
   second during the leap second and use a time that is equivalent to
   23:59:59 UTC twice. For these systems, the corresponding TAI
   timestamp would be obtained by advancing to the next entry in the
   following table when the time equivalent to 23:59:59 UTC is used
   for the second time. Thus the leap second which occurred on 30 June
   1972 at 23:59:59 UTC would have TAI timestamps computed as follows:

   30 June 1972 23:59:59 (2287785599, first time):  TAI= UTC + 10 seconds
   30 June 1972 23:59:60 (2287785599,second time):  TAI= UTC + 11 seconds
   1  July 1972 00:00:00 (2287785600)               TAI= UTC + 11 seconds

   If your system realizes the leap second by repeating 00:00:00 UTC
   twice (this is possible but not usual), then the advance to the
   next entry in the table must occur the second time that a time
   equivlent to 00:00:00 UTC is used. Thus, using the same example as
   above:

   30 June 1972 23:59:59 (2287785599):              TAI= UTC + 10 seconds
   30 June 1972 23:59:60 (2287785600, first time):  TAI= UTC + 10 seconds
   1  July 1972 00:00:00 (2287785600,second time):  TAI= UTC + 11 seconds

   in both cases the use of timestamps based on TAI produces a smooth
   time scale with no discontinuity in the time interval.


Richard Cochran (1):
  ntp: advertise correct TAI offset during leap second

 kernel/time/ntp.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

-- 
1.7.2.5

[PATCH 1/1] ntp: advertise correct TAI offset during leap second

[Richard Cochran]

When repeating a UTC time value during a leap second (when the UTC
time should be 23:59:60), the TAI timescale should not stop. The kernel
NTP code increments the TAI offset one second too late. This patch fixes
the issue by incrementing the offset during the leap second itself.

Signed-off-by: Richard Cochran <richardcochran@gmail.com>
---
 kernel/time/ntp.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/kernel/time/ntp.c b/kernel/time/ntp.c
index f03fd83..e8c8671 100644
--- a/kernel/time/ntp.c
+++ b/kernel/time/ntp.c
@@ -412,6 +412,7 @@ int second_overflow(unsigned long secs)
 		if (secs % 86400 == 0) {
 			leap = -1;
 			time_state = TIME_OOP;
+			time_tai++;
 			printk(KERN_NOTICE
 				"Clock: inserting leap second 23:59:60 UTC\n");
 		}
@@ -426,7 +427,6 @@ int second_overflow(unsigned long secs)
 		}
 		break;
 	case TIME_OOP:
-		time_tai++;
 		time_state = TIME_WAIT;
 		break;
 
-- 
1.7.2.5

Re: [PATCH 1/1] ntp: advertise correct TAI offset during leap second

[John Stultz]

On 04/26/2012 05:11 AM, Richard Cochran wrote:
> When repeating a UTC time value during a leap second (when the UTC
> time should be 23:59:60), the TAI timescale should not stop. The kernel
> NTP code increments the TAI offset one second too late. This patch fixes
> the issue by incrementing the offset during the leap second itself.
>
> Signed-off-by: Richard Cochran<richardcochran@gmail.com>

This looks good to me. Although, have you actually tested against an ntp 
client that sets the tai offset to make sure you're not duplicating any 
ADJ_TAI adjustment it might make?

thanks
-john

Re: [PATCH 1/1] ntp: advertise correct TAI offset during leap second

[Richard Cochran]

On Fri, Apr 27, 2012 at 03:23:17PM -0700, John Stultz wrote:
> On 04/26/2012 05:11 AM, Richard Cochran wrote:
> >When repeating a UTC time value during a leap second (when the UTC
> >time should be 23:59:60), the TAI timescale should not stop. The kernel
> >NTP code increments the TAI offset one second too late. This patch fixes
> >the issue by incrementing the offset during the leap second itself.
> >
> >Signed-off-by: Richard Cochran<richardcochran@gmail.com>
> 
> This looks good to me. Although, have you actually tested against an
> ntp client that sets the tai offset to make sure you're not
> duplicating any ADJ_TAI adjustment it might make?

No, I cooked up my own test program that uses the adjtimex interface
directly. I really am not very familiar with the ntp.org software.

Wait a minute. If user space manages this variable, then shouldn't the
kernel leave it alone?

This David Mills paper [1] gives a leap second example that does it
the "other" way from Linux (see Figure 4), repeating the new epoch
rather than the leap second. It may well be that ntp.org servers do
behave that way. However, the NIST file claims that this way is
unusual.

So, you have a good question. But, if ntp.org uses the NIST second
method, shouldn't Linux do the same?

Thanks,
Richard

1. http://www.eecis.udel.edu/~mills/database/papers/time.pdf

Re: [PATCH 1/1] ntp: advertise correct TAI offset during leap second

[John Stultz]

On 04/27/2012 11:17 PM, Richard Cochran wrote:
> On Fri, Apr 27, 2012 at 03:23:17PM -0700, John Stultz wrote:
>> On 04/26/2012 05:11 AM, Richard Cochran wrote:
>>> When repeating a UTC time value during a leap second (when the UTC
>>> time should be 23:59:60), the TAI timescale should not stop. The kernel
>>> NTP code increments the TAI offset one second too late. This patch fixes
>>> the issue by incrementing the offset during the leap second itself.
>>>
>>> Signed-off-by: Richard Cochran<richardcochran@gmail.com>
>> This looks good to me. Although, have you actually tested against an
>> ntp client that sets the tai offset to make sure you're not
>> duplicating any ADJ_TAI adjustment it might make?
> No, I cooked up my own test program that uses the adjtimex interface
> directly. I really am not very familiar with the ntp.org software.
>
> Wait a minute. If user space manages this variable, then shouldn't the
> kernel leave it alone?

Right. That's why I'm asking. I actually haven't spent much time looking 
at how the tai value provided via adjtimex is handled, and I want to 
make sure its ok if we modify it from the kernel.


> This David Mills paper [1] gives a leap second example that does it
> the "other" way from Linux (see Figure 4), repeating the new epoch
> rather than the leap second. It may well be that ntp.org servers do
> behave that way. However, the NIST file claims that this way is
> unusual.
>
> So, you have a good question. But, if ntp.org uses the NIST second
> method, shouldn't Linux do the same?
>
Not sure I'm following here.  In Linux 23:59:60 is represented as 
23:59:59 + TIME_OOP.  Could you expand on what in particular is 
inconsistent here?

thanks
-john

Re: [PATCH 1/1] ntp: advertise correct TAI offset during leap second

[Richard Cochran]

On Mon, Apr 30, 2012 at 12:48:22PM -0700, John Stultz wrote:
> On 04/27/2012 11:17 PM, Richard Cochran wrote:
> >
> >Wait a minute. If user space manages this variable, then shouldn't the
> >kernel leave it alone?
> 
> Right. That's why I'm asking. I actually haven't spent much time
> looking at how the tai value provided via adjtimex is handled, and I
> want to make sure its ok if we modify it from the kernel.

We *are* already modifying it in kernel, but at the wrong time.

I don't know either what ntpd does, but I will find out.

[ But if ntpd just uses adjtimex() to use the kernel as storage for
  variables, then that is really stupid. ]

> >This David Mills paper [1] gives a leap second example that does it
> >the "other" way from Linux (see Figure 4), repeating the new epoch
> >rather than the leap second. It may well be that ntp.org servers do
> >behave that way. However, the NIST file claims that this way is
> >unusual.
> >
> >So, you have a good question. But, if ntp.org uses the NIST second
> >method, shouldn't Linux do the same?
> >
> Not sure I'm following here.  In Linux 23:59:60 is represented as
> 23:59:59 + TIME_OOP.  Could you expand on what in particular is
> inconsistent here?

I am talking about the numerical values of time_t. The NIST note
explains it well. There are two ways to insert a leap second, and
neither way is specified by any standard, or at least I think that is
what the note is saying.

So you can have either sequences [1] or [2] during a leap second, but
not [3], which is what you are doing. The Mills paper has a table that
looks like [2], but the NIST note says that is not usual. So, I wonder
what ntpd broadcasts during a leap second.

    NTP		OFF	TAI		UTC       STATUS
[1] ----------------------------------------------------
    2287785598	10	78796808  	78796798  ins
    2287785599	10	78796809  	78796799  ins
    2287785599	11	78796810  	78796799  oop
    2287785600	11	78796811  	78796800  wait
    2287785601	11	78796812  	78796801  wait
[2] ----------------------------------------------------
    2287785598	10	78796808  	78796798  ins
    2287785599	10	78796809  	78796799  ins
    2287785600	10	78796810  	78796800  oop
    2287785600	11	78796811  	78796800  wait
    2287785601	11	78796812  	78796801  wait
[3] ----------------------------------------------------
    2287785598	10	78796808  	78796798  ins
    2287785599	10	78796809  	78796799  ins
    2287785599	10	78796809 *  	78796799  oop
    2287785600	11	78796811 * 	78796800  wait
    2287785601	11	78796812  	78796801  wait

* Notice apparent jump in TAI value.

Thanks,
Richard

Re: [PATCH 1/1] ntp: advertise correct TAI offset during leap second

[Richard Cochran]

On Tue, May 01, 2012 at 08:16:05AM +0200, Richard Cochran wrote:
> On Mon, Apr 30, 2012 at 12:48:22PM -0700, John Stultz wrote:
> > On 04/27/2012 11:17 PM, Richard Cochran wrote:
> > >
> > >Wait a minute. If user space manages this variable, then shouldn't the
> > >kernel leave it alone?
> > 
> > Right. That's why I'm asking. I actually haven't spent much time
> > looking at how the tai value provided via adjtimex is handled, and I
> > want to make sure its ok if we modify it from the kernel.
> 
> We *are* already modifying it in kernel, but at the wrong time.
> 
> I don't know either what ntpd does, but I will find out.
> 
> [ But if ntpd just uses adjtimex() to use the kernel as storage for
>   variables, then that is really stupid. ]

I took a look at ntp-4.2.6p5, and ntpd unconditionally sets the TAI
offset using MOD_TAI when it thinks the offset has changed.

But I think it won't hurt if the kernel changes the offset
automatically, since ntpd will just try and set the same value again.

Thanks,
Richard

Re: [PATCH 1/1] ntp: advertise correct TAI offset during leap second

[John Stultz]

On 05/05/2012 03:02 AM, Richard Cochran wrote:
> On Tue, May 01, 2012 at 08:16:05AM +0200, Richard Cochran wrote:
>> On Mon, Apr 30, 2012 at 12:48:22PM -0700, John Stultz wrote:
>>> On 04/27/2012 11:17 PM, Richard Cochran wrote:
>>>> Wait a minute. If user space manages this variable, then shouldn't the
>>>> kernel leave it alone?
>>> Right. That's why I'm asking. I actually haven't spent much time
>>> looking at how the tai value provided via adjtimex is handled, and I
>>> want to make sure its ok if we modify it from the kernel.
>> We *are* already modifying it in kernel, but at the wrong time.
>>
>> I don't know either what ntpd does, but I will find out.
>>
>> [ But if ntpd just uses adjtimex() to use the kernel as storage for
>>    variables, then that is really stupid. ]
> I took a look at ntp-4.2.6p5, and ntpd unconditionally sets the TAI
> offset using MOD_TAI when it thinks the offset has changed.
>
> But I think it won't hurt if the kernel changes the offset
> automatically, since ntpd will just try and set the same value again.
Assuming that it doesn't happen right before the tick that adjusts the 
leapsecond.  :)
But that's a pre-existing bug and not a reason to block your patch that 
improves things.

Thanks for the extra due-diligence here!

thanks
-john

Re: [PATCH 1/1] ntp: advertise correct TAI offset during leap second

[John Stultz]

On 04/26/2012 05:11 AM, Richard Cochran wrote:
> When repeating a UTC time value during a leap second (when the UTC
> time should be 23:59:60), the TAI timescale should not stop. The kernel
> NTP code increments the TAI offset one second too late. This patch fixes
> the issue by incrementing the offset during the leap second itself.

Got a chance to look at this closer and I agree its the right fix.  So 
its queued.
thanks
-john

> Signed-off-by: Richard Cochran<richardcochran@gmail.com>
> ---
>   kernel/time/ntp.c |    2 +-
>   1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/kernel/time/ntp.c b/kernel/time/ntp.c
> index f03fd83..e8c8671 100644
> --- a/kernel/time/ntp.c
> +++ b/kernel/time/ntp.c
> @@ -412,6 +412,7 @@ int second_overflow(unsigned long secs)
>   		if (secs % 86400 == 0) {
>   			leap = -1;
>   			time_state = TIME_OOP;
> +			time_tai++;
>   			printk(KERN_NOTICE
>   				"Clock: inserting leap second 23:59:60 UTC\n");
>   		}
> @@ -426,7 +427,6 @@ int second_overflow(unsigned long secs)
>   		}
>   		break;
>   	case TIME_OOP:
> -		time_tai++;
>   		time_state = TIME_WAIT;
>   		break;
>