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]

Message-ID: <CAKv+Gu_PuY1aHjyTtoVuUhFZbt-vL_LVC7frN+rtxm8PX0C2Cg@mail.gmail.com>
Date:   Fri, 24 Feb 2017 21:45:07 +0000
From:   Ard Biesheuvel <ard.biesheuvel@...aro.org>
To:     John Stultz <john.stultz@...aro.org>
Cc:     Laura Abbott <labbott@...hat.com>,
        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 24 February 2017 at 21:25, John Stultz <john.stultz@...aro.org> wrote:
> On Thu, Feb 23, 2017 at 10:43 AM, Laura Abbott <labbott@...hat.com> wrote:
>> Hi,
>>
>> Fedora was previously carrying a workaround for a gcc7 issue reported
>> on arm64 http://lists.infradead.org/pipermail/linux-arm-kernel/2016-October/461597.html.
>> The workaround got rid of __ilog2_NaN. I dropped the patch this morning
>> because a proper fix (29905b52fad0 ("log2: make order_base_2() behave
>> correctly on const input value zero")) was merged. This fixed the arm64
>> problem linked in the thread but there seems to be another issue in
>> timekeeping.c:
>>
>> /kernel/time/timekeeping.c:2051: undefined reference to `____ilog2_NaN'
>>
>> Fedora enables CONFIG_CLOCKSOURCE_VALIDATE_LAST_CYCLE so I think the
>> compiler is calculating a possible constant of 0 once again.
>>
>> Any ideas about a proper fix?
>
> Huh. So if I understand this, its because we don't explicit checks for
> offsec or cycle_interval being zero in:
>
>         shift = ilog2(offset) - ilog2(tk->cycle_interval);
>
> Right?
>
> Clearly that case isn't expected to happen, but if it did we'd want
> the result of ilog2 to return zero.  So I'm not sure if that
> order_base_2() function is maybe the right function to use as it has
> an explict zero check?
>

The problem is really that GCC splits off a constant folded clone
where one of these variables is a constant 0. In the order_base_2()
case, we could sidestep it by fixing an existing issue with the
function itself, but in this case, it is ilog2() itself that is
affected.

Laura, does the below make any difference at all?

diff --git a/include/linux/log2.h b/include/linux/log2.h
index fd7ff3d91e6a..cf4e5bb662bd 100644
--- a/include/linux/log2.h
+++ b/include/linux/log2.h
@@ -85,7 +85,8 @@ unsigned long __rounddown_pow_of_two(unsigned long n)
 #define ilog2(n)                               \
 (                                              \
        __builtin_constant_p(n) ? (             \
-               (n) < 1 ? ____ilog2_NaN() :     \
+               __builtin_expect((n) < 1, 0) ?  \
+                       ____ilog2_NaN() :       \
                (n) & (1ULL << 63) ? 63 :       \
                (n) & (1ULL << 62) ? 62 :       \
                (n) & (1ULL << 61) ? 61 :       \