linux-kernel - Re: gcc7 log2 compile issues in kernel/time/timekeeping.c

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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Wed, 1 Mar 2017 17:39:32 +0000
From:   Ard Biesheuvel <ard.biesheuvel@...aro.org>
To:     Laura Abbott <labbott@...hat.com>
Cc:     Markus Trippelsdorf <markus@...ppelsdorf.de>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Will Deacon <will.deacon@....com>,
        John Stultz <john.stultz@...aro.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: gcc7 log2 compile issues in kernel/time/timekeeping.c

On 1 March 2017 at 00:00, Laura Abbott <labbott@...hat.com> wrote:
> On 02/25/2017 03:50 AM, Ard Biesheuvel wrote:
>>
>>
>>> On 25 Feb 2017, at 11:23, Ard Biesheuvel <ard.biesheuvel@...aro.org> wrote:
>>>
>>> On 25 February 2017 at 11:09, Markus Trippelsdorf
>>> <markus@...ppelsdorf.de> wrote:
>>>> On 2017.02.25 at 09:11 +0000, Ard Biesheuvel wrote:
>>>>>> On 25 February 2017 at 08:18, Markus Trippelsdorf <markus@...ppelsdorf.de> wrote:
>>>>>>
>>>>>> Why not simply get rid of the ____ilog2_NaN thing altogether?
>>>>>>
>>>>>
>>>>> That would remove the issue, sure. But we lose an opportunity to spot
>>>>> incorrect code at compile time.
>>>>
>>>> In the case of kernel/time/timekeeping.c it is clearly a false positive.
>>>> Was ever incorrect code spotted by ____ilog2_NaN in the past?
>>>>
>>>>> My concern is that it by not pushing back on changes to the semantics
>>>>> of __builtin_constant_p() such as this one, we may start seeing other
>>>>> issues where we can no longer use it, and we lose a very useful tool.
>>>>
>>>> We had a long discussion in:
>>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=72785
>>>> As you can see there is no real consensus.
>>>> But ilog2 seems to be the only place where this ever popped up.
>>>> (There were several distro-wide mass rebuilds with gcc-7 and no other
>>>> __builtin_constant_p() issue was found yet.)
>>>>
>>>
>>> Well, given that it is really dead code that is being emitted, and
>>> that log2(0) is really undefined, perhaps we should simply replace
>>> ilog2_NaN() with __builtin_unreachable()?
>>
>> ... or perhaps it is better to just pass the constant == 0 to the runtime implementation?
>>
>> The second ilog2_NaN is really unreachable, given that it deals with unsigned values >0 without a single bit set.
>>
>
> naively throwing in __builtin_unreachable() doesn't seem to
> work:
>
> ./include/linux/log2.h: In function ‘__order_base_2’:
> ./include/linux/log2.h:155:10: error: void value not ignored as it ought to be
>
> I'm guessing unreachable is treated as void instead of all
> possible types and therefore gcc assumes that the entire
> function must be void?
>

Something like this perhaps? This will at least prevent incorrect uses
from being silently ignored, but maybe it is a bit overkill.

diff --git a/include/linux/log2.h b/include/linux/log2.h
index ef3d4f67118c..c670b3dfd5ca 100644
--- a/include/linux/log2.h
+++ b/include/linux/log2.h
@@ -18,8 +18,8 @@
 /*
  * deal with unrepresentable constant logarithms
  */
-extern __attribute__((const, noreturn))
-int ____ilog2_NaN(void);
+static noinline __attribute__((noreturn, warning("ilog2(0) is undefined!")))
+int ____ilog2_NaN(void) { unreachable(); }

 /*
  * non-constant log of base 2 calculators