[PATCH 1/2] time: Fix timekeeping_freqadjust()'s incorrect use of abs() instead of abs64()

[PATCH 1/2] time: Fix timekeeping_freqadjust()'s incorrect use of abs() instead of abs64()

[John Stultz]

The internal clocksteering done for fine-grained error correction
uses a logarithmic approximation, so any time adjtimex() adjusts
the clock steering, timekeeping_freqadjust() quickly approximates
the correct clock frequency over a series of ticks.

Unfortunately, the logic in timekeeping_freqadjust(), introduced
in commit dc491596f639438 (Rework frequency adjustments to work
better w/ nohz), used the abs() function with a s64 error value
to calculate the size of the approximated adjustment to be made.

Per include/linux/kernel.h: "abs() should not be used for 64-bit
types (s64, u64, long long) - use abs64()".

Thus on 32-bit platforms, this resulted in the clocksteering to
take a quite dampended random walk trying to converge on the
proper frequency, which caused the adjustments to be made much
slower then intended (most easily observed when large adjustments
are made).

This patch fixes the issue by using abs64() instead.

Cc: Nuno Gonçalves <nunojpg@gmail.com>
Cc: Miroslav Lichvar <mlichvar@redhat.com>
Cc: Prarit Bhargava <prarit@redhat.com>
Cc: Richard Cochran <richardcochran@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: <stable@vger.kernel.org> # v3.17+
Reported-by: Nuno Gonçalves <nunojpg@gmail.com>
Tested-by: Nuno Goncalves <nunojpg@gmail.com>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 kernel/time/timekeeping.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index f6ee2e6..3739ac6 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -1614,7 +1614,7 @@ static __always_inline void timekeeping_freqadjust(struct timekeeper *tk,
 	negative = (tick_error < 0);
 
 	/* Sort out the magnitude of the correction */
-	tick_error = abs(tick_error);
+	tick_error = abs64(tick_error);
 	for (adj = 0; tick_error > interval; adj++)
 		tick_error >>= 1;
 
-- 
1.9.1

Re: [PATCH 1/2] time: Fix timekeeping_freqadjust()'s incorrect use of abs() instead of abs64()

[Ingo Molnar]

* John Stultz <john.stultz@linaro.org> wrote:

> The internal clocksteering done for fine-grained error correction
> uses a logarithmic approximation, so any time adjtimex() adjusts
> the clock steering, timekeeping_freqadjust() quickly approximates
> the correct clock frequency over a series of ticks.
> 
> Unfortunately, the logic in timekeeping_freqadjust(), introduced
> in commit dc491596f639438 (Rework frequency adjustments to work
> better w/ nohz), used the abs() function with a s64 error value
> to calculate the size of the approximated adjustment to be made.
> 
> Per include/linux/kernel.h: "abs() should not be used for 64-bit
> types (s64, u64, long long) - use abs64()".
> 
> Thus on 32-bit platforms, this resulted in the clocksteering to
> take a quite dampended random walk trying to converge on the
> proper frequency, which caused the adjustments to be made much
> slower then intended (most easily observed when large adjustments
> are made).
> 
> This patch fixes the issue by using abs64() instead.

>  	/* Sort out the magnitude of the correction */
> -	tick_error = abs(tick_error);
> +	tick_error = abs64(tick_error);

Ugh, and we had this bug for almost two years!

Would it be possible to make abs() warn about 64-bit types during build time,
to prevent such mishaps?

Thanks,

	Ingo

Re: [PATCH 1/2] time: Fix timekeeping_freqadjust()'s incorrect use of abs() instead of abs64()

[John Stultz]

[-- Attachment #1: Type: text/plain, Size: 2083 bytes --]

On Sun, Sep 13, 2015 at 1:32 AM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * John Stultz <john.stultz@linaro.org> wrote:
>
>> The internal clocksteering done for fine-grained error correction
>> uses a logarithmic approximation, so any time adjtimex() adjusts
>> the clock steering, timekeeping_freqadjust() quickly approximates
>> the correct clock frequency over a series of ticks.
>>
>> Unfortunately, the logic in timekeeping_freqadjust(), introduced
>> in commit dc491596f639438 (Rework frequency adjustments to work
>> better w/ nohz), used the abs() function with a s64 error value
>> to calculate the size of the approximated adjustment to be made.
>>
>> Per include/linux/kernel.h: "abs() should not be used for 64-bit
>> types (s64, u64, long long) - use abs64()".
>>
>> Thus on 32-bit platforms, this resulted in the clocksteering to
>> take a quite dampended random walk trying to converge on the
>> proper frequency, which caused the adjustments to be made much
>> slower then intended (most easily observed when large adjustments
>> are made).
>>
>> This patch fixes the issue by using abs64() instead.
>
>>       /* Sort out the magnitude of the correction */
>> -     tick_error = abs(tick_error);
>> +     tick_error = abs64(tick_error);
>
> Ugh, and we had this bug for almost two years!

Well. I sat on the patch for quite awhile, so the author date isn't
really representative. It was only included in mainline in 3.17, so
its been in use for a little over a year.  But yea, still.

> Would it be possible to make abs() warn about 64-bit types during build time,
> to prevent such mishaps?

Yea. I was surprised this wasn't something the compiler would catch previously.

So is BUILD_BUG_ON() the best option for this? Its catching a whole
bunch of other related issues (frighteningly, more then Joe's cocci
script). The down-side is BUILD_BUG_ON causes build errors, not
warnings, and its output isn't super easy to parse on first view.

Potential BUILD_BUG_ON patch attached. I'll also try to spin some
patches to fix the issues this one catches.

thanks
-john

[-- Attachment #2: abs-build-bug.patch --]
[-- Type: text/x-patch, Size: 687 bytes --]

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 5582410..753be99 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -8,6 +8,7 @@
 #include <linux/types.h>
 #include <linux/compiler.h>
 #include <linux/bitops.h>
+#include <linux/bug.h>
 #include <linux/log2.h>
 #include <linux/typecheck.h>
 #include <linux/printk.h>
@@ -208,6 +209,9 @@ extern int _cond_resched(void);
  */
 #define abs(x) ({						\
 		long ret;					\
+		BUILD_BUG_ON_MSG(				\
+			sizeof(typeof(x)) > sizeof(long),	\
+			"abs() should not be used for 64-bit types - use abs64()");\
 		if (sizeof(x) == sizeof(long)) {		\
 			long __x = (x);				\
 			ret = (__x < 0) ? -__x : __x;		\

Re: [tip:timers/urgent] time: Fix timekeeping_freqadjust()' s incorrect use of abs() instead of abs64()

[Nuno Gonçalves]

On Sun, Sep 13, 2015 at 12:07 PM, tip-bot for John Stultz
<tipbot@zytor.com> wrote:
> Commit-ID:  2619d7e9c92d524cb155ec89fd72875321512e5b
> Gitweb:     http://git.kernel.org/tip/2619d7e9c92d524cb155ec89fd72875321512e5b
> Author:     John Stultz <john.stultz@linaro.org>
> AuthorDate: Wed, 9 Sep 2015 16:07:30 -0700
> Committer:  Ingo Molnar <mingo@kernel.org>
> CommitDate: Sun, 13 Sep 2015 10:30:47 +0200
>
> time: Fix timekeeping_freqadjust()'s incorrect use of abs() instead of abs64()
>
> The internal clocksteering done for fine-grained error
> correction uses a logarithmic approximation, so any time
> adjtimex() adjusts the clock steering, timekeeping_freqadjust()
> quickly approximates the correct clock frequency over a series
> of ticks.
>
> Unfortunately, the logic in timekeeping_freqadjust(), introduced
> in commit:
>
>   dc491596f639 ("timekeeping: Rework frequency adjustments to work better w/ nohz")
>
> used the abs() function with a s64 error value to calculate the
> size of the approximated adjustment to be made.
>
> Per include/linux/kernel.h:
>
>   "abs() should not be used for 64-bit types (s64, u64, long long) - use abs64()".
>
> Thus on 32-bit platforms, this resulted in the clocksteering to
> take a quite dampended random walk trying to converge on the
> proper frequency, which caused the adjustments to be made much
> slower then intended (most easily observed when large
> adjustments are made).
>
> This patch fixes the issue by using abs64() instead.
>
> Reported-by: Nuno Gonçalves <nunojpg@gmail.com>
> Tested-by: Nuno Goncalves <nunojpg@gmail.com>
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> Cc: <stable@vger.kernel.org> # v3.17+
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: Miroslav Lichvar <mlichvar@redhat.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Prarit Bhargava <prarit@redhat.com>
> Cc: Richard Cochran <richardcochran@gmail.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Link: http://lkml.kernel.org/r/1441840051-20244-1-git-send-email-john.stultz@linaro.org
> Signed-off-by: Ingo Molnar <mingo@kernel.org>
> ---
>  kernel/time/timekeeping.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
> index f6ee2e6..3739ac6 100644
> --- a/kernel/time/timekeeping.c
> +++ b/kernel/time/timekeeping.c
> @@ -1614,7 +1614,7 @@ static __always_inline void timekeeping_freqadjust(struct timekeeper *tk,
>         negative = (tick_error < 0);
>
>         /* Sort out the magnitude of the correction */
> -       tick_error = abs(tick_error);
> +       tick_error = abs64(tick_error);
>         for (adj = 0; tick_error > interval; adj++)
>                 tick_error >>= 1;
>

I think this got lost on its way to the linux-stable queue (or I don't
understand the stable rules).

Thanks,
Nuno

Re: [tip:timers/urgent] time: Fix timekeeping_freqadjust()' s incorrect use of abs() instead of abs64()

[Greg KH]

On Mon, Oct 05, 2015 at 04:10:28PM +0100, Nuno Gon�alves wrote:
> On Sun, Sep 13, 2015 at 12:07 PM, tip-bot for John Stultz
> <tipbot@zytor.com> wrote:
> > Commit-ID:  2619d7e9c92d524cb155ec89fd72875321512e5b
> > Gitweb:     http://git.kernel.org/tip/2619d7e9c92d524cb155ec89fd72875321512e5b
> > Author:     John Stultz <john.stultz@linaro.org>
> > AuthorDate: Wed, 9 Sep 2015 16:07:30 -0700
> > Committer:  Ingo Molnar <mingo@kernel.org>
> > CommitDate: Sun, 13 Sep 2015 10:30:47 +0200
> >
> > time: Fix timekeeping_freqadjust()'s incorrect use of abs() instead of abs64()
> >
> > The internal clocksteering done for fine-grained error
> > correction uses a logarithmic approximation, so any time
> > adjtimex() adjusts the clock steering, timekeeping_freqadjust()
> > quickly approximates the correct clock frequency over a series
> > of ticks.
> >
> > Unfortunately, the logic in timekeeping_freqadjust(), introduced
> > in commit:
> >
> >   dc491596f639 ("timekeeping: Rework frequency adjustments to work better w/ nohz")
> >
> > used the abs() function with a s64 error value to calculate the
> > size of the approximated adjustment to be made.
> >
> > Per include/linux/kernel.h:
> >
> >   "abs() should not be used for 64-bit types (s64, u64, long long) - use abs64()".
> >
> > Thus on 32-bit platforms, this resulted in the clocksteering to
> > take a quite dampended random walk trying to converge on the
> > proper frequency, which caused the adjustments to be made much
> > slower then intended (most easily observed when large
> > adjustments are made).
> >
> > This patch fixes the issue by using abs64() instead.
> >
> > Reported-by: Nuno Gon�alves <nunojpg@gmail.com>
> > Tested-by: Nuno Goncalves <nunojpg@gmail.com>
> > Signed-off-by: John Stultz <john.stultz@linaro.org>
> > Cc: <stable@vger.kernel.org> # v3.17+
> > Cc: Linus Torvalds <torvalds@linux-foundation.org>
> > Cc: Miroslav Lichvar <mlichvar@redhat.com>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Prarit Bhargava <prarit@redhat.com>
> > Cc: Richard Cochran <richardcochran@gmail.com>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Link: http://lkml.kernel.org/r/1441840051-20244-1-git-send-email-john.stultz@linaro.org
> > Signed-off-by: Ingo Molnar <mingo@kernel.org>
> > ---
> >  kernel/time/timekeeping.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
> > index f6ee2e6..3739ac6 100644
> > --- a/kernel/time/timekeeping.c
> > +++ b/kernel/time/timekeeping.c
> > @@ -1614,7 +1614,7 @@ static __always_inline void timekeeping_freqadjust(struct timekeeper *tk,
> >         negative = (tick_error < 0);
> >
> >         /* Sort out the magnitude of the correction */
> > -       tick_error = abs(tick_error);
> > +       tick_error = abs64(tick_error);
> >         for (adj = 0; tick_error > interval; adj++)
> >                 tick_error >>= 1;
> >
> 
> I think this got lost on its way to the linux-stable queue (or I don't
> understand the stable rules).

It's still in the "todo" queue, along with 159 other patches that I have
yet to get to :(

thanks,

greg k-h