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

[John Stultz]

On Sun, Sep 13, 2015 at 1:32 AM, Ingo Molnar <mingo@xxxxxxxxxx> wrote:
>
> * John Stultz <john.stultz@xxxxxxxxxx> 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
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;		\