LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] Avoid time_offset overflows
@ 2007-03-24 0:40 john stultz
2007-03-24 5:20 ` Roman Zippel
0 siblings, 1 reply; 6+ messages in thread
From: john stultz @ 2007-03-24 0:40 UTC (permalink / raw)
To: Andrew Morton; +Cc: Roman Zippel, Ingo Molnar, Thomas Gleixner, lkml
I've been seeing some odd NTP behavior recently on a few boxes and
finally narrowed it down to time_offset overflowing when converted to
SHIFT_UPDATE units (which was a side effect from my HZfreeNTP patch).
This patch converts time_offset from a long to a s64 which resolves the
issue.
thanks
-john
Signed-off-by: John Stultz <johnstul@us.ibm.com>
diff --git a/kernel/time/ntp.c b/kernel/time/ntp.c
index eb12509..eaa3d24 100644
--- a/kernel/time/ntp.c
+++ b/kernel/time/ntp.c
@@ -32,7 +32,7 @@ #define MAX_TICKADJ_SCALED (((u64)(MAX_T
/* TIME_ERROR prevents overwriting the CMOS clock */
static int time_state = TIME_OK; /* clock synchronization status */
int time_status = STA_UNSYNC; /* clock status bits */
-static long time_offset; /* time adjustment (ns) */
+static s64 time_offset; /* time adjustment (ns) */
static long time_constant = 2; /* pll time constant */
long time_maxerror = NTP_PHASE_LIMIT; /* maximum error (us) */
long time_esterror = NTP_PHASE_LIMIT; /* estimated error (us) */
@@ -196,7 +196,7 @@ void __attribute__ ((weak)) notify_arch_
*/
int do_adjtimex(struct timex *txc)
{
- long ltemp, mtemp, save_adjust;
+ long mtemp, save_adjust;
s64 freq_adj, temp64;
int result;
@@ -277,14 +277,14 @@ #endif
time_adjust = txc->offset;
}
else if (time_status & STA_PLL) {
- ltemp = txc->offset * NSEC_PER_USEC;
+ time_offset = txc->offset * NSEC_PER_USEC;
/*
* Scale the phase adjustment and
* clamp to the operating range.
*/
- time_offset = min(ltemp, MAXPHASE * NSEC_PER_USEC);
- time_offset = max(time_offset, -MAXPHASE * NSEC_PER_USEC);
+ time_offset = min(time_offset, (s64)MAXPHASE * NSEC_PER_USEC);
+ time_offset = max(time_offset, (s64)-MAXPHASE * NSEC_PER_USEC);
/*
* Select whether the frequency is to be controlled
@@ -297,11 +297,11 @@ #endif
mtemp = xtime.tv_sec - time_reftime;
time_reftime = xtime.tv_sec;
- freq_adj = (s64)time_offset * mtemp;
+ freq_adj = time_offset * mtemp;
freq_adj = shift_right(freq_adj, time_constant * 2 +
(SHIFT_PLL + 2) * 2 - SHIFT_NSEC);
if (mtemp >= MINSEC && (time_status & STA_FLL || mtemp > MAXSEC)) {
- temp64 = (s64)time_offset << (SHIFT_NSEC - SHIFT_FLL);
+ temp64 = time_offset << (SHIFT_NSEC - SHIFT_FLL);
if (time_offset < 0) {
temp64 = -temp64;
do_div(temp64, mtemp);
@@ -314,8 +314,8 @@ #endif
freq_adj += time_freq;
freq_adj = min(freq_adj, (s64)MAXFREQ_NSEC);
time_freq = max(freq_adj, (s64)-MAXFREQ_NSEC);
- time_offset = (time_offset / NTP_INTERVAL_FREQ)
- << SHIFT_UPDATE;
+ do_div(time_offset, NTP_INTERVAL_FREQ);
+ time_offset <<= SHIFT_UPDATE;
} /* STA_PLL */
} /* txc->modes & ADJ_OFFSET */
if (txc->modes & ADJ_TICK)
@@ -330,7 +330,7 @@ leave: if ((time_status & (STA_UNSYNC|ST
if ((txc->modes & ADJ_OFFSET_SINGLESHOT) == ADJ_OFFSET_SINGLESHOT)
txc->offset = save_adjust;
else
- txc->offset = shift_right(time_offset, SHIFT_UPDATE)
+ txc->offset = ((long)shift_right(time_offset, SHIFT_UPDATE))
* NTP_INTERVAL_FREQ / 1000;
txc->freq = (time_freq / NSEC_PER_USEC)
<< (SHIFT_USEC - SHIFT_NSEC);
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] Avoid time_offset overflows
2007-03-24 0:40 [PATCH] Avoid time_offset overflows john stultz
@ 2007-03-24 5:20 ` Roman Zippel
2007-03-25 9:09 ` Andrew Morton
0 siblings, 1 reply; 6+ messages in thread
From: Roman Zippel @ 2007-03-24 5:20 UTC (permalink / raw)
To: john stultz; +Cc: Andrew Morton, Ingo Molnar, Thomas Gleixner, lkml
Hi,
On Fri, 23 Mar 2007, john stultz wrote:
> @@ -314,8 +314,8 @@ #endif
> freq_adj += time_freq;
> freq_adj = min(freq_adj, (s64)MAXFREQ_NSEC);
> time_freq = max(freq_adj, (s64)-MAXFREQ_NSEC);
> - time_offset = (time_offset / NTP_INTERVAL_FREQ)
> - << SHIFT_UPDATE;
> + do_div(time_offset, NTP_INTERVAL_FREQ);
> + time_offset <<= SHIFT_UPDATE;
> } /* STA_PLL */
> } /* txc->modes & ADJ_OFFSET */
> if (txc->modes & ADJ_TICK)
This is wrong, time_offset is signed and do_div is unsigned.
In general I planned to do the same change, but the do_div API could use a
little cleanup to provide some clear function for signed/unsigned divide
(hopefully with a better name than div_long_long_rem_signed or
do_div_llr).
bye, Roman
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Avoid time_offset overflows
2007-03-24 5:20 ` Roman Zippel
@ 2007-03-25 9:09 ` Andrew Morton
2007-03-25 11:07 ` Thomas Gleixner
0 siblings, 1 reply; 6+ messages in thread
From: Andrew Morton @ 2007-03-25 9:09 UTC (permalink / raw)
To: Roman Zippel; +Cc: john stultz, Ingo Molnar, Thomas Gleixner, lkml
On Sat, 24 Mar 2007 06:20:45 +0100 (CET) Roman Zippel <zippel@linux-m68k.org> wrote:
> Hi,
>
> On Fri, 23 Mar 2007, john stultz wrote:
>
> > @@ -314,8 +314,8 @@ #endif
> > freq_adj += time_freq;
> > freq_adj = min(freq_adj, (s64)MAXFREQ_NSEC);
> > time_freq = max(freq_adj, (s64)-MAXFREQ_NSEC);
> > - time_offset = (time_offset / NTP_INTERVAL_FREQ)
> > - << SHIFT_UPDATE;
> > + do_div(time_offset, NTP_INTERVAL_FREQ);
> > + time_offset <<= SHIFT_UPDATE;
> > } /* STA_PLL */
> > } /* txc->modes & ADJ_OFFSET */
> > if (txc->modes & ADJ_TICK)
>
> This is wrong, time_offset is signed and do_div is unsigned.
> In general I planned to do the same change, but the do_div API could use a
> little cleanup to provide some clear function for signed/unsigned divide
> (hopefully with a better name than div_long_long_rem_signed or
> do_div_llr).
>
Can we do a minimal thing for 2.6.21, worry about API beautification later?
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Avoid time_offset overflows
2007-03-25 9:09 ` Andrew Morton
@ 2007-03-25 11:07 ` Thomas Gleixner
2007-03-25 11:14 ` [patch] ntp: avoid integer overflow in do_adjtimex() Ingo Molnar
0 siblings, 1 reply; 6+ messages in thread
From: Thomas Gleixner @ 2007-03-25 11:07 UTC (permalink / raw)
To: Andrew Morton; +Cc: Roman Zippel, john stultz, Ingo Molnar, lkml
On Sun, 2007-03-25 at 01:09 -0800, Andrew Morton wrote:
> On Sat, 24 Mar 2007 06:20:45 +0100 (CET) Roman Zippel <zippel@linux-m68k.org> wrote:
>
> > Hi,
> >
> > On Fri, 23 Mar 2007, john stultz wrote:
> >
> > > @@ -314,8 +314,8 @@ #endif
> > > freq_adj += time_freq;
> > > freq_adj = min(freq_adj, (s64)MAXFREQ_NSEC);
> > > time_freq = max(freq_adj, (s64)-MAXFREQ_NSEC);
> > > - time_offset = (time_offset / NTP_INTERVAL_FREQ)
> > > - << SHIFT_UPDATE;
> > > + do_div(time_offset, NTP_INTERVAL_FREQ);
> > > + time_offset <<= SHIFT_UPDATE;
> > > } /* STA_PLL */
> > > } /* txc->modes & ADJ_OFFSET */
> > > if (txc->modes & ADJ_TICK)
> >
> > This is wrong, time_offset is signed and do_div is unsigned.
> > In general I planned to do the same change, but the do_div API could use a
> > little cleanup to provide some clear function for signed/unsigned divide
> > (hopefully with a better name than div_long_long_rem_signed or
> > do_div_llr).
> >
>
> Can we do a minimal thing for 2.6.21, worry about API beautification later?
Here you go. It's ugly, but it should do the trick for now.
tglx
Index: linux-2.6/kernel/time/ntp.c
===================================================================
--- linux-2.6.orig/kernel/time/ntp.c
+++ linux-2.6/kernel/time/ntp.c
@@ -196,7 +196,7 @@ void __attribute__ ((weak)) notify_arch_
*/
int do_adjtimex(struct timex *txc)
{
- long mtemp, save_adjust;
+ long mtemp, save_adjust, rem;
s64 freq_adj, temp64;
int result;
@@ -314,7 +314,9 @@ int do_adjtimex(struct timex *txc)
freq_adj += time_freq;
freq_adj = min(freq_adj, (s64)MAXFREQ_NSEC);
time_freq = max(freq_adj, (s64)-MAXFREQ_NSEC);
- do_div(time_offset, NTP_INTERVAL_FREQ);
+ time_offset = div_long_long_rem_signed(time_offset,
+ NTP_INTERVAL_FREQ,
+ &rem);
time_offset <<= SHIFT_UPDATE;
} /* STA_PLL */
} /* txc->modes & ADJ_OFFSET */
^ permalink raw reply [flat|nested] 6+ messages in thread* [patch] ntp: avoid integer overflow in do_adjtimex()
2007-03-25 11:07 ` Thomas Gleixner
@ 2007-03-25 11:14 ` Ingo Molnar
2007-03-25 13:01 ` Thomas Gleixner
0 siblings, 1 reply; 6+ messages in thread
From: Ingo Molnar @ 2007-03-25 11:14 UTC (permalink / raw)
To: Thomas Gleixner; +Cc: Andrew Morton, Roman Zippel, john stultz, linux-kernel
* Thomas Gleixner <tglx@linutronix.de> wrote:
> Here you go. It's ugly, but it should do the trick for now.
>
> tglx
here's your patch with proper metadata:
--------------------->
From: Thomas Gleixner <tglx@linutronix.de>
Subject: [patch] ntp: avoid integer overflow in do_adjtimex()
John Stultz traced back ntpd problems to the following issue:
do_adjtimex() [used by ntpd] can overflow if there's a too long delay
between timer interrupts - as it can happen on NO_HZ. Expand the
time_adjust calculation/division from 32 bits to 64 bits.
Found-by: John Stultz <johnstul@us.ibm.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
Index: linux-2.6/kernel/time/ntp.c
===================================================================
--- linux-2.6.orig/kernel/time/ntp.c
+++ linux-2.6/kernel/time/ntp.c
@@ -196,7 +196,7 @@ void __attribute__ ((weak)) notify_arch_
*/
int do_adjtimex(struct timex *txc)
{
- long mtemp, save_adjust;
+ long mtemp, save_adjust, rem;
s64 freq_adj, temp64;
int result;
@@ -314,7 +314,9 @@ int do_adjtimex(struct timex *txc)
freq_adj += time_freq;
freq_adj = min(freq_adj, (s64)MAXFREQ_NSEC);
time_freq = max(freq_adj, (s64)-MAXFREQ_NSEC);
- do_div(time_offset, NTP_INTERVAL_FREQ);
+ time_offset = div_long_long_rem_signed(time_offset,
+ NTP_INTERVAL_FREQ,
+ &rem);
time_offset <<= SHIFT_UPDATE;
} /* STA_PLL */
} /* txc->modes & ADJ_OFFSET */
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [patch] ntp: avoid integer overflow in do_adjtimex()
2007-03-25 11:14 ` [patch] ntp: avoid integer overflow in do_adjtimex() Ingo Molnar
@ 2007-03-25 13:01 ` Thomas Gleixner
0 siblings, 0 replies; 6+ messages in thread
From: Thomas Gleixner @ 2007-03-25 13:01 UTC (permalink / raw)
To: Ingo Molnar; +Cc: Andrew Morton, Roman Zippel, john stultz, linux-kernel
On Sun, 2007-03-25 at 13:14 +0200, Ingo Molnar wrote:
> * Thomas Gleixner <tglx@linutronix.de> wrote:
>
> > Here you go. It's ugly, but it should do the trick for now.
> >
> > tglx
>
> here's your patch with proper metadata:
>
> --------------------->
> From: Thomas Gleixner <tglx@linutronix.de>
> Subject: [patch] ntp: avoid integer overflow in do_adjtimex()
>
> John Stultz traced back ntpd problems to the following issue:
> do_adjtimex() [used by ntpd] can overflow if there's a too long delay
> between timer interrupts - as it can happen on NO_HZ. Expand the
> time_adjust calculation/division from 32 bits to 64 bits.
Wrong metadata. This is a patch to fix John's patch vs. signedness.
tglx
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2007-03-25 12:53 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2007-03-24 0:40 [PATCH] Avoid time_offset overflows john stultz
2007-03-24 5:20 ` Roman Zippel
2007-03-25 9:09 ` Andrew Morton
2007-03-25 11:07 ` Thomas Gleixner
2007-03-25 11:14 ` [patch] ntp: avoid integer overflow in do_adjtimex() Ingo Molnar
2007-03-25 13:01 ` Thomas Gleixner
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).