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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CALAqxLXYCvFDPd2jh+OhoRr0ncWhbHtDqRv5ev+NCuhu8_8n8w@mail.gmail.com>
Date:	Mon, 14 Sep 2015 16:24:23 -0700
From:	John Stultz <john.stultz@...aro.org>
To:	Ingo Molnar <mingo@...nel.org>
Cc:	Linus Torvalds <torvalds@...ux-foundation.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	Thomas Gleixner <tglx@...utronix.de>,
	LKML <linux-kernel@...r.kernel.org>,
	Nuno Gonçalves <nunojpg@...il.com>,
	Miroslav Lichvar <mlichvar@...hat.com>,
	Prarit Bhargava <prarit@...hat.com>,
	Richard Cochran <richardcochran@...il.com>,
	stable <stable@...r.kernel.org>, Joe Perches <joe@...ches.com>
Subject: Re: [PATCH 1/2] time: Fix timekeeping_freqadjust()'s incorrect use of
 abs() instead of abs64()

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

View attachment "abs-build-bug.patch" of type "text/x-patch" (687 bytes)