Skip to contentSkip to conversation

ntp bug fix

PATCH In review Richard Cochran 2 participants 9 messages Apr 26, 2012 – May 5, 2012 Timekeeping, clocksource core, NTP, alarmtimer

lore
Share & tools
lore
#0 lore raw

cover ntp bug fix

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(-)
Signature · 1 line
1.7.2.5
#1 lore raw

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

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.

---
 kernel/time/ntp.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
kernel/time/ntp.c +1 -1
index f03fd83..e8c8671 100644
@@ -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;
 
Signature · 1 line
1.7.2.5
#2 lore raw

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

Quoted 6 lines · 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.

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

#3 lore raw

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

Quoted 11 lines · John Stultz
Quoted 6 lines · 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.

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

#4 lore raw

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

Quoted 8 lines · Richard Cochran
Quoted 7 lines · John Stultz
Quoted 6 lines · 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.

Quoted 8 lines
Quoted 3 lines

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.

Quoted 9 lines

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

#5 lore raw

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

Quoted 8 lines · John Stultz
Quoted 3 lines · Richard Cochran

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. ]

Quoted 6 lines
Quoted 6 lines

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.

Quoted 6 lines
Quoted 3 lines

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

#6 lore raw

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

Quoted 10 lines · Richard Cochran
Quoted 8 lines · John Stultz
Quoted 3 lines · Richard Cochran

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.

Quoted 6 lines

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

#7 lore raw

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

Quoted 12 lines · Richard Cochran
Quoted 11 lines · Richard Cochran
Quoted 6 lines · John Stultz
Quoted 2 lines · Richard Cochran

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.

Quoted 7 lines
Quoted 2 lines

[ 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

#8 lore raw

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

In reply to Richard Cochran
Quoted 4 lines · 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.

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

Quoted 25 lines
---
  kernel/time/ntp.c |    2 +-
  1 files changed, 1 insertions(+), 1 deletions(-)
kernel/time/ntp.c +1 -1
index f03fd83..e8c8671 100644
@@ -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;