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: <51e768e7-91af-86c5-3732-2e529364d376@redhat.com>
Date:   Fri, 24 Feb 2017 15:33:37 -0800
From:   Laura Abbott <labbott@...hat.com>
To:     Ard Biesheuvel <ard.biesheuvel@...aro.org>,
        John Stultz <john.stultz@...aro.org>
Cc:     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 02/24/2017 01:45 PM, Ard Biesheuvel wrote:
> 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 :       \
> 

No, still see the same issue.

Thanks,
Laura