linux-kernel - Re: "KVM: x86: generalize guest_cpuid_has

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: <CACT4Y+Z7tsX-0bFUnPOpQHMrnjZvN0HX4aD5L6Xw7u4DZ_1Oqg@mail.gmail.com>
Date:   Tue, 12 Sep 2017 17:54:21 +0200
From:   Dmitry Vyukov <dvyukov@...gle.com>
To:     Radim Krčmář <rkrcmar@...hat.com>
Cc:     David Hildenbrand <david@...hat.com>,
        Paolo Bonzini <pbonzini@...hat.com>,
        LKML <linux-kernel@...r.kernel.org>,
        KVM list <kvm@...r.kernel.org>,
        llvmlinux@...ts.linuxfoundation.org,
        Alexander Potapenko <glider@...gle.com>,
        andreyknvl <andreyknvl@...gle.com>,
        Michael Davidson <md@...gle.com>,
        Greg Hackmann <ghackmann@...gle.com>,
        Nick Desaulniers <ndesaulniers@...gle.com>
Subject: Re: "KVM: x86: generalize guest_cpuid_has_ helpers" breaks clang

On Tue, Sep 12, 2017 at 5:51 PM, Dmitry Vyukov <dvyukov@...gle.com> wrote:
> On Tue, Sep 12, 2017 at 5:18 PM, Radim Krčmář <rkrcmar@...hat.com> wrote:
>> 2017-09-12 16:42+0200, Dmitry Vyukov:
>>> Hi Radim,
>>>
>>> I've just noticed that your commit "KVM: x86: generalize
>>> guest_cpuid_has_ helpers" breaks clang build on this assert:
>>>
>>> static __always_inline struct cpuid_reg x86_feature_cpuid(unsigned x86_feature)
>>> {
>>>     unsigned x86_leaf = x86_feature / 32;
>>>
>>>     BUILD_BUG_ON(!__builtin_constant_p(x86_leaf));
>>>
>>>
>>> In clang __builtin_constant_p is never true for function arguments,
>>> it's true only for compile-time constants (what you can use as stack
>>> array size, or C++ template argument). What would work is an
>>> additional macro along the lines of:
>>
>> GCC optimizes it thanks to __always_inline, so the x86_feature is
>> constant in each instance of this function ... the goal is to have
>> compile-time input checking.
>>
>>> #define x86_feature_cpuid(x) (BUILD_BUG_ON(!__builtin_constant_p(x),
>>> __x86_feature_cpuid(x))
>>>
>>> But again assuming that caller pass the constant directly.
>>
>> The __builtin_constant_p() check is just a canary, the important ones
>> are
>>
>>   BUILD_BUG_ON(x86_leaf >= ARRAY_SIZE(reverse_cpuid));
>>   BUILD_BUG_ON(reverse_cpuid[x86_leaf].function == 0);
>>
>> and these would be very awkward if moved out of the function.
>>
>>> Could you please fix it?
>>
>> Sure, I can just make them BUG_ON (or WARN_ON with error handling), but
>> I tried with clang version 4.0.1 and got no errors -- are you using an
>> older version?  (or a command other than `make HOSTCC=clang CC=clang`)
>
>
> Interesting.
>
> I use clang version 6.0.0 (trunk 313027).
>
> I am on 8fac2f96ab86b0e14ec4e42851e21e9b518bdc55 on Linus tree. Here
> is my config (which is basically defconfig + kvm enabled):
> https://gist.githubusercontent.com/dvyukov/4360060ab49374b1e983312f587f1b4e/raw/2e4def6f4318bde81ab316546400513b02673bc9/gistfile1.txt
> Build with: make CC=/build/bin/clang
> and get:
>
>   MODPOST vmlinux.o
> arch/x86/kvm/x86.o: In function `kvm_set_apic_base':
> x86.c:(.text+0x4738): undefined reference to `__compiletime_assert_62'
> arch/x86/kvm/x86.o: In function `kvm_set_cr4':
> x86.c:(.text+0x54ae): undefined reference to `__compiletime_assert_62'
> x86.c:(.text+0x54dc): undefined reference to `__compiletime_assert_62'
> x86.c:(.text+0x5509): undefined reference to `__compiletime_assert_62'
> x86.c:(.text+0x5536): undefined reference to `__compiletime_assert_62'
> arch/x86/kvm/x86.o:x86.c:(.text+0x5563): more undefined references to
> `__compiletime_assert_62' follow
> make: *** [vmlinux] Error 1
>
>
> I've commented out the first BUILD_BUG_ON, and these did not cause build errors:
>
>   BUILD_BUG_ON(x86_leaf >= ARRAY_SIZE(reverse_cpuid));
>   BUILD_BUG_ON(reverse_cpuid[x86_leaf].function == 0);
>
> I guess clang still eliminates dead branches. Clang optimizer does
> know that these are constant, it just does not allow build
> success/failure nor runtime behavior depend on optimization level and
> compiler version. I.e. with gcc you can get build failure with only
> some compiler flags and/or compiler versions. Clang gives stable
> result. But the optimizer does use constant propagation, etc during
> optimization.


I've installed clang-3.9 (the closest version to yours my distribution
gives me) and still got the same error with it. I would expect that
4.0 should give the same result as well... Are you sure you enabled
KVM/intel/amd? (yes, I know you are maintaining KVM code :))