LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] time, ntp: Do not update time_state in middle of leap second
@ 2015-01-29 13:35 Prarit Bhargava
  0 siblings, 0 replies; 6+ messages in thread
From: Prarit Bhargava @ 2015-01-29 13:35 UTC (permalink / raw)
  To: linux-kernel; +Cc: Prarit Bhargava

During leap second insertion testing it was noticed that a small window
exists where the time_state could be reset such that
time_state = TIME_OK, which then causes the leap second to not occur, or
causes the entire leap second state machine to fail.

While this is highly unlikely to ever happen in the real world it is
still something we should protect against, as breaking the state machine
is obviously bad.

If the time_state == TIME_OOP (ie, the leap second is in progress) do not
allow an external update to time_state.

Signed-off-by: Prarit Bhargava <prarit@redhat.com>
---
 kernel/time/ntp.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/kernel/time/ntp.c b/kernel/time/ntp.c
index 28bf91c..f9ebf06 100644
--- a/kernel/time/ntp.c
+++ b/kernel/time/ntp.c
@@ -534,7 +534,8 @@ void ntp_notify_cmos_timer(void) { }
  */
 static inline void process_adj_status(struct timex *txc, struct timespec64 *ts)
 {
-	if ((time_status & STA_PLL) && !(txc->status & STA_PLL)) {
+	if ((time_status & STA_PLL) && !(txc->status & STA_PLL) &&
+	    (time_state != TIME_OOP)) {
 		time_state = TIME_OK;
 		time_status = STA_UNSYNC;
 		/* restart PPS frequency calibration */
-- 
1.7.9.3


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] time, ntp: Do not update time_state in middle of leap second
  2015-02-10 23:47 ` John Stultz
@ 2015-02-11 10:47   ` Prarit Bhargava
  0 siblings, 0 replies; 6+ messages in thread
From: Prarit Bhargava @ 2015-02-11 10:47 UTC (permalink / raw)
  To: John Stultz; +Cc: lkml, Thomas Gleixner, Miroslav Lichvar



On 02/10/2015 06:47 PM, John Stultz wrote:
> On Sun, Feb 8, 2015 at 2:29 AM, Prarit Bhargava <prarit@redhat.com> wrote:
>> During leap second insertion testing it was noticed that a small window
>> exists where the time_state could be reset such that
>> time_state = TIME_OK, which then causes the leap second to not occur, or
>> causes the entire leap second state machine to fail.
> 
> 
> I think this description is fairly opaque, and probably needs the
> specific example of the state change transitions that motivates this
> patch.
> 
>> While this is highly unlikely to ever happen in the real world it is
>> still something we should protect against, as breaking the state machine
>> is obviously bad.
> 
> In this case it was a test-case bug where uninitialized data being
> passed to adjtimex (when the test intended to only read the time
> state) was causing an unexpected state change transition. So its not
> immediately obvious that resetting the state machine when the root
> called adjtimex is invalid, so it would be good to make this more
> clear and explicit (ie: show the expected state transitions and the
> command that caused the strange transition you saw).
> 
> Sorry for the slow response here, I've been on the fence as to if this
> is the right thing or not, and have needed to get some time to stare
> at this a bit more to see if I can convince myself its the right
> thing, so improving the commit message might make it more obvious to
> me and others. :)

Will do :)  I'll write up a proper and detailed description.  My bad.

P.

> 
> thanks
> -john
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] time, ntp: Do not update time_state in middle of leap second
  2015-02-07 18:29 Prarit Bhargava
  2015-02-10 14:01 ` Peter Zijlstra
@ 2015-02-10 23:47 ` John Stultz
  2015-02-11 10:47   ` Prarit Bhargava
  1 sibling, 1 reply; 6+ messages in thread
From: John Stultz @ 2015-02-10 23:47 UTC (permalink / raw)
  To: Prarit Bhargava; +Cc: lkml, Thomas Gleixner, Miroslav Lichvar

On Sun, Feb 8, 2015 at 2:29 AM, Prarit Bhargava <prarit@redhat.com> wrote:
> During leap second insertion testing it was noticed that a small window
> exists where the time_state could be reset such that
> time_state = TIME_OK, which then causes the leap second to not occur, or
> causes the entire leap second state machine to fail.


I think this description is fairly opaque, and probably needs the
specific example of the state change transitions that motivates this
patch.

> While this is highly unlikely to ever happen in the real world it is
> still something we should protect against, as breaking the state machine
> is obviously bad.

In this case it was a test-case bug where uninitialized data being
passed to adjtimex (when the test intended to only read the time
state) was causing an unexpected state change transition. So its not
immediately obvious that resetting the state machine when the root
called adjtimex is invalid, so it would be good to make this more
clear and explicit (ie: show the expected state transitions and the
command that caused the strange transition you saw).

Sorry for the slow response here, I've been on the fence as to if this
is the right thing or not, and have needed to get some time to stare
at this a bit more to see if I can convince myself its the right
thing, so improving the commit message might make it more obvious to
me and others. :)

thanks
-john

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] time, ntp: Do not update time_state in middle of leap second
  2015-02-07 18:29 Prarit Bhargava
@ 2015-02-10 14:01 ` Peter Zijlstra
  2015-02-10 23:47 ` John Stultz
  1 sibling, 0 replies; 6+ messages in thread
From: Peter Zijlstra @ 2015-02-10 14:01 UTC (permalink / raw)
  To: Prarit Bhargava
  Cc: linux-kernel, John Stultz, Thomas Gleixner, Miroslav Lichvar

On Sat, Feb 07, 2015 at 01:29:39PM -0500, Prarit Bhargava wrote:
> During leap second insertion testing it was noticed that a small window
> exists where the time_state could be reset such that
> time_state = TIME_OK, which then causes the leap second to not occur, or
> causes the entire leap second state machine to fail.
> 
> While this is highly unlikely to ever happen in the real world it is
> still something we should protect against, as breaking the state machine
> is obviously bad.
> 
> If the time_state == TIME_OOP (ie, the leap second is in progress) do not
> allow an external update to time_state.
> 
> [v2]: Only block time_state change when TIME_OOP
> 
> Signed-off-by: Prarit Bhargava <prarit@redhat.com>
> Cc: John Stultz <john.stultz@linaro.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Miroslav Lichvar <mlichvar@redhat.com>

John, ACK?

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH] time, ntp: Do not update time_state in middle of leap second
@ 2015-02-07 18:29 Prarit Bhargava
  2015-02-10 14:01 ` Peter Zijlstra
  2015-02-10 23:47 ` John Stultz
  0 siblings, 2 replies; 6+ messages in thread
From: Prarit Bhargava @ 2015-02-07 18:29 UTC (permalink / raw)
  To: linux-kernel
  Cc: Prarit Bhargava, John Stultz, Thomas Gleixner, Miroslav Lichvar

During leap second insertion testing it was noticed that a small window
exists where the time_state could be reset such that
time_state = TIME_OK, which then causes the leap second to not occur, or
causes the entire leap second state machine to fail.

While this is highly unlikely to ever happen in the real world it is
still something we should protect against, as breaking the state machine
is obviously bad.

If the time_state == TIME_OOP (ie, the leap second is in progress) do not
allow an external update to time_state.

[v2]: Only block time_state change when TIME_OOP

Signed-off-by: Prarit Bhargava <prarit@redhat.com>
Cc: John Stultz <john.stultz@linaro.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Miroslav Lichvar <mlichvar@redhat.com>
---
 kernel/time/ntp.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/kernel/time/ntp.c b/kernel/time/ntp.c
index 28bf91c..6ff5cd5 100644
--- a/kernel/time/ntp.c
+++ b/kernel/time/ntp.c
@@ -535,7 +535,8 @@ void ntp_notify_cmos_timer(void) { }
 static inline void process_adj_status(struct timex *txc, struct timespec64 *ts)
 {
 	if ((time_status & STA_PLL) && !(txc->status & STA_PLL)) {
-		time_state = TIME_OK;
+		if (time_state != TIME_OOP)
+			time_state = TIME_OK;
 		time_status = STA_UNSYNC;
 		/* restart PPS frequency calibration */
 		pps_reset_freq_interval();
-- 
1.7.9.3


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH] time, ntp: Do not update time_state in middle of leap second
@ 2015-02-04 12:28 Prarit Bhargava
  0 siblings, 0 replies; 6+ messages in thread
From: Prarit Bhargava @ 2015-02-04 12:28 UTC (permalink / raw)
  To: linux-kernel; +Cc: Prarit Bhargava, John Stultz, Thomas Gleixner

Resending ...

P.

----8<----

During leap second insertion testing it was noticed that a small window
exists where the time_state could be reset such that
time_state = TIME_OK, which then causes the leap second to not occur, or
causes the entire leap second state machine to fail.

While this is highly unlikely to ever happen in the real world it is
still something we should protect against, as breaking the state machine
is obviously bad.

If the time_state == TIME_OOP (ie, the leap second is in progress) do not
allow an external update to time_state.

Signed-off-by: Prarit Bhargava <prarit@redhat.com>
Cc: John Stultz <john.stultz@linaro.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
---
 kernel/time/ntp.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/kernel/time/ntp.c b/kernel/time/ntp.c
index 28bf91c..f9ebf06 100644
--- a/kernel/time/ntp.c
+++ b/kernel/time/ntp.c
@@ -534,7 +534,8 @@ void ntp_notify_cmos_timer(void) { }
  */
 static inline void process_adj_status(struct timex *txc, struct timespec64 *ts)
 {
-	if ((time_status & STA_PLL) && !(txc->status & STA_PLL)) {
+	if ((time_status & STA_PLL) && !(txc->status & STA_PLL) &&
+	    (time_state != TIME_OOP)) {
 		time_state = TIME_OK;
 		time_status = STA_UNSYNC;
 		/* restart PPS frequency calibration */
-- 
1.7.9.3


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2015-02-11 10:47 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-01-29 13:35 [PATCH] time, ntp: Do not update time_state in middle of leap second Prarit Bhargava
2015-02-04 12:28 Prarit Bhargava
2015-02-07 18:29 Prarit Bhargava
2015-02-10 14:01 ` Peter Zijlstra
2015-02-10 23:47 ` John Stultz
2015-02-11 10:47   ` Prarit Bhargava

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).