[PATCH] fix adjtimex() freq return error

[PATCH] fix adjtimex() freq return error

[john stultz]

Martin Ziegler reported a bug in adjtimex(), where odd positive 
frequencies are reduced by 1 while negative values are correct.

See http://bugzilla.kernel.org/show_bug.cgi?id=11370 for details

The issue was introduced in the following commit that increased 
time_freq to a 64bit value and increased its resolution:

http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=074b3b87941c99bc0ce35385b5817924b1ed0c23;hp=eea83d896e318bda54be2d2770d2c5d6668d11db

The problem is how we convert from our internal high res value back to
userland. Instead of dividing, we shift, then mult, then shift again,
causing the slight error seen in the bug.

I don't think adjtimex() is a terribly hot path, so this patch simply
uses div_s64() to convert back to the external resolution and removes 
the unused shift defines to resolve the issue.

Signed-off-by: John Stultz <johnstul@us.ibm.com>

Index: 2.6-git/kernel/time/ntp.c
===================================================================
--- 2.6-git.orig/kernel/time/ntp.c
+++ 2.6-git/kernel/time/ntp.c
@@ -402,9 +402,7 @@ int do_adjtimex(struct timex *txc)
 		if (!(time_status & STA_NANO))
 			txc->offset /= NSEC_PER_USEC;
 	}
-	txc->freq	   = shift_right((s32)(time_freq >> PPM_SCALE_INV_SHIFT) *
-					 (s64)PPM_SCALE_INV,
-					 NTP_SCALE_SHIFT);
+	txc->freq	   = div_s64(time_freq, PPM_SCALE);
 	txc->maxerror	   = time_maxerror;
 	txc->esterror	   = time_esterror;
 	txc->status	   = time_status;
Index: 2.6-git/include/linux/timex.h
===================================================================
--- 2.6-git.orig/include/linux/timex.h
+++ 2.6-git/include/linux/timex.h
@@ -82,9 +82,6 @@
  */
 #define SHIFT_USEC 16		/* frequency offset scale (shift) */
 #define PPM_SCALE (NSEC_PER_USEC << (NTP_SCALE_SHIFT - SHIFT_USEC))
-#define PPM_SCALE_INV_SHIFT 20
-#define PPM_SCALE_INV ((1ll << (PPM_SCALE_INV_SHIFT + NTP_SCALE_SHIFT)) / \
-		       PPM_SCALE + 1)
 
 #define MAXPHASE 500000000l	/* max phase error (ns) */
 #define MAXFREQ 500000		/* max frequency error (ns/s) */

Re: [PATCH] fix adjtimex() freq return error

[Andrew Morton]

On Tue, 19 Aug 2008 19:29:06 -0700 john stultz <johnstul@us.ibm.com> wrote:

> Martin Ziegler reported a bug in adjtimex(), where odd positive 
> frequencies are reduced by 1 while negative values are correct.
> 
> See http://bugzilla.kernel.org/show_bug.cgi?id=11370 for details
> 
> The issue was introduced in the following commit that increased 
> time_freq to a 64bit value and increased its resolution:
> 
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=074b3b87941c99bc0ce35385b5817924b1ed0c23;hp=eea83d896e318bda54be2d2770d2c5d6668d11db
> 
> The problem is how we convert from our internal high res value back to
> userland. Instead of dividing, we shift, then mult, then shift again,
> causing the slight error seen in the bug.
> 
> I don't think adjtimex() is a terribly hot path, so this patch simply
> uses div_s64() to convert back to the external resolution and removes 
> the unused shift defines to resolve the issue.
> 
> Signed-off-by: John Stultz <johnstul@us.ibm.com>
> 
> Index: 2.6-git/kernel/time/ntp.c
> ===================================================================
> --- 2.6-git.orig/kernel/time/ntp.c
> +++ 2.6-git/kernel/time/ntp.c
> @@ -402,9 +402,7 @@ int do_adjtimex(struct timex *txc)
>  		if (!(time_status & STA_NANO))
>  			txc->offset /= NSEC_PER_USEC;
>  	}
> -	txc->freq	   = shift_right((s32)(time_freq >> PPM_SCALE_INV_SHIFT) *
> -					 (s64)PPM_SCALE_INV,
> -					 NTP_SCALE_SHIFT);
> +	txc->freq	   = div_s64(time_freq, PPM_SCALE);
>  	txc->maxerror	   = time_maxerror;
>  	txc->esterror	   = time_esterror;
>  	txc->status	   = time_status;
> Index: 2.6-git/include/linux/timex.h
> ===================================================================
> --- 2.6-git.orig/include/linux/timex.h
> +++ 2.6-git/include/linux/timex.h
> @@ -82,9 +82,6 @@
>   */
>  #define SHIFT_USEC 16		/* frequency offset scale (shift) */
>  #define PPM_SCALE (NSEC_PER_USEC << (NTP_SCALE_SHIFT - SHIFT_USEC))
> -#define PPM_SCALE_INV_SHIFT 20
> -#define PPM_SCALE_INV ((1ll << (PPM_SCALE_INV_SHIFT + NTP_SCALE_SHIFT)) / \
> -		       PPM_SCALE + 1)
>  
>  #define MAXPHASE 500000000l	/* max phase error (ns) */
>  #define MAXFREQ 500000		/* max frequency error (ns/s) */
> 

Thanks, John.  And Martin for the perfect bug report.

Based on Martin's observation that the bug has been there since 2.6.21
I marked this as needed in 2.6.25.x and in 2.6.26.x.  However the patch
doesn't apply at all to 2.6.25, so someone(tm) will have a bit of
rework to do if we want to fix 2.6.25.x.

This fix caused Roman's
ntp-fix-adj_offset_ss_read-bug-and-do_adjtimex-cleanup.patch (which I
have queued for when Thomas wakes up) to toss a reject, but fixing that
seemed pretty simple.  I think.  The scripts/kconfig/conf segfaults are
being a bit of a PITA at present.

Re: [PATCH] fix adjtimex() freq return error

[Roman Zippel]

Hi,

On Tue, 19 Aug 2008, Andrew Morton wrote:

> On Tue, 19 Aug 2008 19:29:06 -0700 john stultz <johnstul@us.ibm.com> wrote:
> 
> > Martin Ziegler reported a bug in adjtimex(), where odd positive 
> > frequencies are reduced by 1 while negative values are correct.

To put this in perspective one should also know that 1 here means an error 
of 2^-16ppm or less than 0.02nsec.
When I wrote this code, I noticed that the rounding isn't perfect, but 
OTOH the error is so small that it can't have any practical relevance.
AFAIC the old code is fine as is.

bye, Roman

Re: [PATCH] fix adjtimex() freq return error

[Roman Zippel]

Hi,

On Tue, 19 Aug 2008, Andrew Morton wrote:

> This fix caused Roman's
> ntp-fix-adj_offset_ss_read-bug-and-do_adjtimex-cleanup.patch (which I
> have queued for when Thomas wakes up) to toss a reject, but fixing that
> seemed pretty simple.  I think.  The scripts/kconfig/conf segfaults are
> being a bit of a PITA at present.

Below is a better patch on top of the above patch, note that this not a 
bug fix and thus not an issue for stable.

bye, Roman


[PATCH] improve adjtimex frequency rounding

Change PPM_SCALE_INV_SHIFT so that it doesn't throw away any input bits
(19 is the amount of the factor 2 in PPM_SCALE), the output frequency
can then be calculated back to its input value, as the inverse divide
produce a slightly larger value, which is then correctly rounded by the
final shift.

Signed-off-by: Roman Zippel <zippel@linux-m68k.org>

---
 include/linux/timex.h |    2 +-
 kernel/time/ntp.c     |    5 ++---
 2 files changed, 3 insertions(+), 4 deletions(-)

Index: linux-2.6-mm/include/linux/timex.h
===================================================================
--- linux-2.6-mm.orig/include/linux/timex.h
+++ linux-2.6-mm/include/linux/timex.h
@@ -82,7 +82,7 @@
  */
 #define SHIFT_USEC 16		/* frequency offset scale (shift) */
 #define PPM_SCALE (NSEC_PER_USEC << (NTP_SCALE_SHIFT - SHIFT_USEC))
-#define PPM_SCALE_INV_SHIFT 20
+#define PPM_SCALE_INV_SHIFT 19
 #define PPM_SCALE_INV ((1ll << (PPM_SCALE_INV_SHIFT + NTP_SCALE_SHIFT)) / \
 		       PPM_SCALE + 1)
 
Index: linux-2.6-mm/kernel/time/ntp.c
===================================================================
--- linux-2.6-mm.orig/kernel/time/ntp.c
+++ linux-2.6-mm/kernel/time/ntp.c
@@ -402,9 +402,8 @@ int do_adjtimex(struct timex *txc)
 		if (!(time_status & STA_NANO))
 			txc->offset /= NSEC_PER_USEC;
 	}
-	txc->freq	   = shift_right((s32)(time_freq >> PPM_SCALE_INV_SHIFT) *
-					 (s64)PPM_SCALE_INV,
-					 NTP_SCALE_SHIFT);
+	txc->freq	   = shift_right((time_freq >> PPM_SCALE_INV_SHIFT) *
+					 (s64)PPM_SCALE_INV, NTP_SCALE_SHIFT);
 	txc->maxerror	   = time_maxerror;
 	txc->esterror	   = time_esterror;
 	txc->status	   = time_status;