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; \