[tip: x86/build] x86/build: Don't build CONFIG_X86_32 as -ffreestanding

tip-bot2 for Nick Desaulniers posted 1 patch 4 years, 1 month ago
arch/x86/Makefile | 3 ---
1 file changed, 3 deletions(-)
[tip: x86/build] x86/build: Don't build CONFIG_X86_32 as -ffreestanding
Posted by tip-bot2 for Nick Desaulniers 4 years, 1 month ago
The following commit has been merged into the x86/build branch of tip:

Commit-ID:     9b2687f29bc1a050ffd63b425129aa9db987e4f3
Gitweb:        https://git.kernel.org/tip/9b2687f29bc1a050ffd63b425129aa9db987e4f3
Author:        Nick Desaulniers <ndesaulniers@google.com>
AuthorDate:    Thu, 03 Feb 2022 12:40:25 -08:00
Committer:     Borislav Petkov <bp@suse.de>
CommitterDate: Thu, 07 Apr 2022 11:55:42 +02:00

x86/build: Don't build CONFIG_X86_32 as -ffreestanding

-ffreestanding typically inhibits "libcall optimizations" where calls to
certain library functions can be replaced by the compiler in certain
cases to calls to other library functions that may be more efficient.
This can be problematic for embedded targets that don't provide full
libc implementations.

-ffreestanding inhibits all such optimizations, which is the safe
choice, but generally we want the optimizations that are performed. The
Linux kernel does implement a fair amount of libc routines. Instead of
-ffreestanding (which makes more sense in smaller images like kexec's
purgatory image), prefer -fno-builtin-* flags to disable the compiler
from emitting calls to functions which may not be defined.

If you see a linkage failure due to a missing symbol that's typically
defined in a libc, and not explicitly called from the source code, then
the compiler may have done such a transform. You can either implement
such a function (i.e. in lib/string.c) or disable the transform outright
via -fno-builtin-* flag (where * is the name of the library routine,
i.e. -fno-builtin-bcmp).

i386_defconfig build+boot tested with GCC and Clang. Removes a pretty
old TODO from the code base.

[kees: These libcall optimizations are specifically needed to allow Clang
to correctly optimize the string functions under CONFIG_FORTIFY_SOURCE.]

Fixes: 6edfba1b33c7 ("[PATCH] x86_64: Don't define string functions to builtin")
Suggested-by: Arvind Sankar <nivedita@alum.mit.edu>
Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Borislav Petkov <bp@suse.de>
Reviewed-by: Fangrui Song <maskray@google.com>
Reviewed-by: Kees Cook <keescook@chromium.org>
Link: https://lore.kernel.org/r/20200817220212.338670-5-ndesaulniers@google.com
Link: https://lore.kernel.org/r/20220203204025.1153397-1-keescook@chromium.org
---
 arch/x86/Makefile | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/arch/x86/Makefile b/arch/x86/Makefile
index 1abd7cc..670fe40 100644
--- a/arch/x86/Makefile
+++ b/arch/x86/Makefile
@@ -100,9 +100,6 @@ ifeq ($(CONFIG_X86_32),y)
         include $(srctree)/arch/x86/Makefile_32.cpu
         KBUILD_CFLAGS += $(cflags-y)
 
-        # temporary until string.h is fixed
-        KBUILD_CFLAGS += -ffreestanding
-
 	ifeq ($(CONFIG_STACKPROTECTOR),y)
 		ifeq ($(CONFIG_SMP),y)
 			KBUILD_CFLAGS += -mstack-protector-guard-reg=fs -mstack-protector-guard-symbol=__stack_chk_guard
Re: [tip: x86/build] x86/build: Don't build CONFIG_X86_32 as -ffreestanding
Posted by Nick Desaulniers 4 years, 1 month ago
On Thu, Apr 7, 2022 at 8:34 AM tip-bot2 for Nick Desaulniers
<tip-bot2@linutronix.de> wrote:
>
> The following commit has been merged into the x86/build branch of tip:
>
> Commit-ID:     9b2687f29bc1a050ffd63b425129aa9db987e4f3
> Gitweb:        https://git.kernel.org/tip/9b2687f29bc1a050ffd63b425129aa9db987e4f3
> Author:        Nick Desaulniers <ndesaulniers@google.com>
> AuthorDate:    Thu, 03 Feb 2022 12:40:25 -08:00
> Committer:     Borislav Petkov <bp@suse.de>
> CommitterDate: Thu, 07 Apr 2022 11:55:42 +02:00
>
> x86/build: Don't build CONFIG_X86_32 as -ffreestanding
>
> -ffreestanding typically inhibits "libcall optimizations" where calls to
> certain library functions can be replaced by the compiler in certain
> cases to calls to other library functions that may be more efficient.
> This can be problematic for embedded targets that don't provide full
> libc implementations.
>
> -ffreestanding inhibits all such optimizations, which is the safe
> choice, but generally we want the optimizations that are performed. The
> Linux kernel does implement a fair amount of libc routines. Instead of
> -ffreestanding (which makes more sense in smaller images like kexec's
> purgatory image), prefer -fno-builtin-* flags to disable the compiler
> from emitting calls to functions which may not be defined.
>
> If you see a linkage failure due to a missing symbol that's typically
> defined in a libc, and not explicitly called from the source code, then
> the compiler may have done such a transform. You can either implement
> such a function (i.e. in lib/string.c) or disable the transform outright
> via -fno-builtin-* flag (where * is the name of the library routine,
> i.e. -fno-builtin-bcmp).
>
> i386_defconfig build+boot tested with GCC and Clang. Removes a pretty
> old TODO from the code base.
>
> [kees: These libcall optimizations are specifically needed to allow Clang
> to correctly optimize the string functions under CONFIG_FORTIFY_SOURCE.]

Right, but...
I think we found that doing so leads to a boot regression for i386
when built w/ clang because:
https://github.com/ClangBuiltLinux/linux/issues/1583
https://github.com/llvm/llvm-project/issues/53645
TL;DR
In doing such libcall optimizations, LLVM drops the -mregparm=3
calling convention...on the caller's side and not the callee's.

Boris, Can I send you a patch to replace this one (with a guard for
clang) or a patch on top? i.e. what base would you prefer me to use.

Using mainline:

```
diff --git a/arch/x86/Makefile b/arch/x86/Makefile
index 63d50f65b828..c94de779e334 100644
--- a/arch/x86/Makefile
+++ b/arch/x86/Makefile
@@ -100,8 +100,13 @@ ifeq ($(CONFIG_X86_32),y)
         include $(srctree)/arch/x86/Makefile_32.cpu
         KBUILD_CFLAGS += $(cflags-y)

-        # temporary until string.h is fixed
+        # LLVM is dropping -mregparm=3 from callers when doing libcall
+        # optimization.
+        # https://github.com/ClangBuiltLinux/linux/issues/1583
+        # https://github.com/llvm/llvm-project/issues/53645
+        ifndef CONFIG_CC_IS_CLANG
         KBUILD_CFLAGS += -ffreestanding
+        endif

        ifeq ($(CONFIG_STACKPROTECTOR),y)
                ifeq ($(CONFIG_SMP),y)
```

but happy to rebase onto whichever and put a commit message on that.
We'll bump up the priority on getting that fixed.

>
> Fixes: 6edfba1b33c7 ("[PATCH] x86_64: Don't define string functions to builtin")
> Suggested-by: Arvind Sankar <nivedita@alum.mit.edu>
> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: Borislav Petkov <bp@suse.de>
> Reviewed-by: Fangrui Song <maskray@google.com>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Link: https://lore.kernel.org/r/20200817220212.338670-5-ndesaulniers@google.com
> Link: https://lore.kernel.org/r/20220203204025.1153397-1-keescook@chromium.org
> ---
>  arch/x86/Makefile | 3 ---
>  1 file changed, 3 deletions(-)
>
> diff --git a/arch/x86/Makefile b/arch/x86/Makefile
> index 1abd7cc..670fe40 100644
> --- a/arch/x86/Makefile
> +++ b/arch/x86/Makefile
> @@ -100,9 +100,6 @@ ifeq ($(CONFIG_X86_32),y)
>          include $(srctree)/arch/x86/Makefile_32.cpu
>          KBUILD_CFLAGS += $(cflags-y)
>
> -        # temporary until string.h is fixed
> -        KBUILD_CFLAGS += -ffreestanding
> -
>         ifeq ($(CONFIG_STACKPROTECTOR),y)
>                 ifeq ($(CONFIG_SMP),y)
>                         KBUILD_CFLAGS += -mstack-protector-guard-reg=fs -mstack-protector-guard-symbol=__stack_chk_guard



-- 
Thanks,
~Nick Desaulniers
Re: [tip: x86/build] x86/build: Don't build CONFIG_X86_32 as -ffreestanding
Posted by Borislav Petkov 4 years, 1 month ago
On Thu, Apr 07, 2022 at 10:01:32AM -0700, Nick Desaulniers wrote:
> Boris, Can I send you a patch to replace this one (with a guard for
> clang) or a patch on top? i.e. what base would you prefer me to use.

I don't mind much, whatever Kees wants. I can just as well drop this
one completely and wait until you guys have fixed clang... or do the
gcc-only thing... your call.

> Using mainline:

The branch to use is in the tip tree and there x86/build - it is in the
subject of the tip-bot2 message:

[tip: x86/build] ...

Thx.

-- 
Regards/Gruss,
    Boris.

SUSE Software Solutions Germany GmbH, GF: Ivo Totev, HRB 36809, AG Nürnberg