linux-kernel - Re: [GIT PULL] x86/core for 5.18

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:   Sun, 27 Mar 2022 10:47:38 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     "the arch/x86 maintainers" <x86@...nel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Alexei Starovoitov <alexei.starovoitov@...il.com>,
        Christophe Leroy <christophe.leroy@...roup.eu>
Subject: Re: [GIT PULL] x86/core for 5.18

On Sun, Mar 27, 2022 at 1:22 AM Peter Zijlstra <peterz@...radead.org> wrote:
>
> Duh! I pushed the tag to the wrong repo (peterz/queue.git)... I just
> pushed it out to tip/tip/git as well, so hopefully it should all work
> now.

Thanks, looks good.

There's a few merges that don't have good explanations ("avoid
conflicts" really is not an explanation on its own), but those weren't
actually yours.

Looking around, the only other thing I notice is that the CC_HAS_IBT
conditional is horrendously unreadable.

There is no reason to make a Kconfig option use one single expression
from hell, when the Kconfig language is designed to be able to do so
much better.

That

       def_bool ((CC_IS_GCC && $(cc-option, -fcf-protection=branch
-mindirect-branch-register)) || \
                 (CC_IS_CLANG && CLANG_VERSION >= 140000)) && \
                 $(as-instr,endbr64)

really is pretty horrendous. Yeah, we have other examples ot those
kinds of horrible expressions (*cough*KCSAN*cough*), but wouldn't it
be much nicer to just write those things legibly.

And it's nice to see the comments about _why_ some particular compiler
checks are done, but when it's all part of one very complicated thing
it's not exactly easy to read or understand.

I've obviously already pulled it (you should have seen the
pr-tracker-bot reply), but I really think it should have been written
in a much more straight-forward and obvious manner, something like:

  # work around https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93654
  config GCC_HAS_IBT
        def_bool y
        depends on CC_IS_GCC
        depends on $(cc-option, -fcf-protection=branch
-mindirect-branch-register)

  # Clang/LLVM >= 14
  # https://github.com/llvm/llvm-project/commit/e0b89df2e0f0130881bf6c39bf31d7f6aac00e0f
  # https://github.com/llvm/llvm-project/commit/dfcf69770bc522b9e411c66454934a37c1f35332
  config CLANG_HAS_IBT
        def_bool y
        depends on CC_IS_CLANG && CLANG_VERSION >= 140000

  # binutils >= 2.29 for 'endbr64' instruction
  config CC_HAS_IBT
        def_bool y
        depends on GCC_HAS_IBT || CLANG_HAS_IBT
        depends on $(as-instr,endbr64)

instead (ok, using "AS_HAS_IBT" and "LD_HAS_IBT" to clarify all
_those_ tests too would probably have been even more legible, but I
got bored).

The above suggested Kconfig snippet is entirely untested, and meant
purely as a "look, you can split these expressions up to be more
legible".

The actual *code* seems to be nicely abstracted out, which is why I
then reacted to that Kconfig mess.

           Linus