Skip to contentSkip to conversation

Avoid time_offset overflows

PATCH In review john stultz 5 participants 6 messages Mar 24, 2007 – Mar 25, 2007 Timekeeping, clocksource core, NTP, alarmtimer

lore
Share & tools
lore

john stultz

author
#0 lore raw

PATCH 1/1 Avoid time_offset overflows

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 <[email protected]>
kernel/time/ntp.c +10 -10
index eb12509..eaa3d24 100644
@@ -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);
#1 lore raw

Hi,

kernel/time/ntp.c #endif · line 321

Quoted 11 lines · john stultz
kernel/time/ntp.c +2 -2
@@ -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

#2 lore raw
Quoted 13 lines · On Sat, 24 Mar 2007 06:20:45 +0100 (CET) Roman Zippel <[email protected]>…

Hi,

Quoted 9 lines · john stultz
kernel/time/ntp.c +2 -2
@@ -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 */
Quoted 9 lines

kernel/time/ntp.c #endif · line 321

Quoted 2 lines
kernel/time/ntp.c +0 -0
 	    } /* 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?

#3 lore raw
Quoted 25 lines · Andrew Morton
Quoted 13 lines · On Sat, 24 Mar 2007 06:20:45 +0100 (CET) Roman Zippel <[email protected]>…

Hi,

Quoted 9 lines · john stultz
(quoted diff) +2 -2
@@ -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 */
Quoted 9 lines

Review #endif · line 321

Quoted 2 lines
(quoted diff) +0 -0
 	    } /* 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).

Quoted 1 line

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

kernel/time/ntp.c +4 -2
Index: 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 */
#4 lore raw

PATCH 1/1 ntp: avoid integer overflow in do_adjtimex()

Quoted 3 lines · Thomas Gleixner

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 <[email protected]>
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 <[email protected]>
Signed-off-by Ingo Molnar <[email protected]>

---

kernel/time/ntp.c +4 -2
Index: 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 */
#5 lore raw

Re: [patch] ntp: avoid integer overflow in do_adjtimex()

Quoted 8 lines · Ingo Molnar
Quoted 3 lines · Thomas Gleixner

Here you go. It's ugly, but it should do the trick for now.

tglx

here's your patch with proper metadata:

Quoted 8 lines

--------------------->
From: Thomas Gleixner <[email protected]>
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