| Message ID | 1531186516-15764-1-git-send-email-yamada.masahiro@socionext.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] objtool: move libelf detection to Kconfig from Makefile | expand |
On Tue, Jul 10, 2018 at 10:35:16AM +0900, Masahiro Yamada wrote: > Currently, users are allowed to enable STACK_VALIDATION regardless > of the compiler capability. The top-level Makefile warns or breaks > the build if it turns out that the host compiler cannot link libelf. > > Move the libelf test to Kconfig so that users can enable the feature > only when the host compiler can build the objtool. The ugly check > in the Makefile will go away. > > Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> > Acked-by: Josh Poimboeuf <jpoimboe@redhat.com> Actually, looking at this again, I want to rescind my ack. This patches changes the behavior in a subtle (but important) way. Before, if I did "make defconfig", it would *always* choose the ORC unwinder. Then, if I didn't have libelf-devel, the build would fail and it would tell me what I need to install. But now with this patch, if I do "make defconfig", the generated config actually changes based on what I happen to have installed on my build system. If I don't have libelf-devel, then it silently chooses the non-default unwinder (frame pointer). This is a subtle difference, but IMO it's dangerous, because it will silently enable the frame pointer unwinder for the majority of new systems, even though it's not the default. I strongly prefer the original behavior, because we really want to encourage people to use the ORC unwinder, even if that means they have to install a package to build it. Also -- in general -- I suspect that silently changing the defaults like this will create a lot of bad surprises. The output of "make defconfig" should be predictable and not dependent on what RPMs I happen to have installed. -- Josh
Hi. 2018-07-10 11:29 GMT+09:00 Josh Poimboeuf <jpoimboe@redhat.com>: > On Tue, Jul 10, 2018 at 10:35:16AM +0900, Masahiro Yamada wrote: >> Currently, users are allowed to enable STACK_VALIDATION regardless >> of the compiler capability. The top-level Makefile warns or breaks >> the build if it turns out that the host compiler cannot link libelf. >> >> Move the libelf test to Kconfig so that users can enable the feature >> only when the host compiler can build the objtool. The ugly check >> in the Makefile will go away. >> >> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> >> Acked-by: Josh Poimboeuf <jpoimboe@redhat.com> > > Actually, looking at this again, I want to rescind my ack. > > This patches changes the behavior in a subtle (but important) way. > > Before, if I did "make defconfig", it would *always* choose the ORC > unwinder. Then, if I didn't have libelf-devel, the build would fail and > it would tell me what I need to install. > > But now with this patch, if I do "make defconfig", the generated config > actually changes based on what I happen to have installed on my build > system. If I don't have libelf-devel, then it silently chooses the > non-default unwinder (frame pointer). > > This is a subtle difference, but IMO it's dangerous, because it will > silently enable the frame pointer unwinder for the majority of new > systems, even though it's not the default. > > I strongly prefer the original behavior, because we really want to > encourage people to use the ORC unwinder, even if that means they have > to install a package to build it. > > Also -- in general -- I suspect that silently changing the defaults like > this will create a lot of bad surprises. The output of "make defconfig" > should be predictable and not dependent on what RPMs I happen to have > installed. Actually, we had similar discussion for stack protector. First, Kees liked to let the build fail instead of disabling the stack protector silently: https://lkml.org/lkml/2018/2/9/697 Linus said: But yes, I also reacted to your earlier " It can't silently rewrite it to _REGULAR because the compiler support for _STRONG regressed." Because it damn well can. If the compiler doesn't support -fstack-protector-strong, we can just fall back on -fstack-protector. Silently. No extra crazy complex logic for that either. (https://lkml.org/lkml/2018/2/10/281) I hope this is the same pattern. -- Best Regards Masahiro Yamada
On Tue, Jul 10, 2018 at 12:47:49PM +0900, Masahiro Yamada wrote: > Hi. > > > 2018-07-10 11:29 GMT+09:00 Josh Poimboeuf <jpoimboe@redhat.com>: > > On Tue, Jul 10, 2018 at 10:35:16AM +0900, Masahiro Yamada wrote: > >> Currently, users are allowed to enable STACK_VALIDATION regardless > >> of the compiler capability. The top-level Makefile warns or breaks > >> the build if it turns out that the host compiler cannot link libelf. > >> > >> Move the libelf test to Kconfig so that users can enable the feature > >> only when the host compiler can build the objtool. The ugly check > >> in the Makefile will go away. > >> > >> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> > >> Acked-by: Josh Poimboeuf <jpoimboe@redhat.com> > > > > Actually, looking at this again, I want to rescind my ack. > > > > This patches changes the behavior in a subtle (but important) way. > > > > Before, if I did "make defconfig", it would *always* choose the ORC > > unwinder. Then, if I didn't have libelf-devel, the build would fail and > > it would tell me what I need to install. > > > > But now with this patch, if I do "make defconfig", the generated config > > actually changes based on what I happen to have installed on my build > > system. If I don't have libelf-devel, then it silently chooses the > > non-default unwinder (frame pointer). > > > > This is a subtle difference, but IMO it's dangerous, because it will > > silently enable the frame pointer unwinder for the majority of new > > systems, even though it's not the default. > > > > I strongly prefer the original behavior, because we really want to > > encourage people to use the ORC unwinder, even if that means they have > > to install a package to build it. > > > > Also -- in general -- I suspect that silently changing the defaults like > > this will create a lot of bad surprises. The output of "make defconfig" > > should be predictable and not dependent on what RPMs I happen to have > > installed. > > > > Actually, we had similar discussion for stack protector. > > > First, Kees liked to let the build fail > instead of disabling the stack protector silently: > > https://lkml.org/lkml/2018/2/9/697 > > > > Linus said: > But yes, I also reacted to your earlier " It can't silently rewrite it > to _REGULAR because the compiler support for _STRONG regressed." > Because it damn well can. If the compiler doesn't support > -fstack-protector-strong, we can just fall back on -fstack-protector. > Silently. No extra crazy complex logic for that either. > > (https://lkml.org/lkml/2018/2/10/281) > > > > I hope this is the same pattern. I wasn't a part of the -fstack-protector conversation, but I doubt it's the same pattern. We're trying to phase out frame pointers, for several reasons. One big reason is that they cause a general slowdown across the entire kernel. Since we switched the x86_64 default to the ORC unwinder, a lot of people have switched over. But this patch will reverse (or at least slow down) that trend, because almost nobody has the libelf devel packaged installed by default. So over time, it will effectively make frame pointers the default again in many cases. That's exactly what we *don't* want to do. It will also cause people to accidentally re-enable frame pointers when they thought they had ORC. -- Josh
* Josh Poimboeuf <jpoimboe@redhat.com> wrote: > Since we switched the x86_64 default to the ORC unwinder, a lot of > people have switched over. But this patch will reverse (or at least > slow down) that trend, because almost nobody has the libelf devel > packaged installed by default. So over time, it will effectively make > frame pointers the default again in many cases. That's exactly what we > *don't* want to do. It will also cause people to accidentally re-enable > frame pointers when they thought they had ORC. Yeah, agreed - turning ORC off like that is a non-starter. Thanks, Ingo
On Mon, Jul 9, 2018 at 9:26 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote: > I wasn't a part of the -fstack-protector conversation, but I doubt it's > the same pattern. We're trying to phase out frame pointers, for several > reasons. One big reason is that they cause a general slowdown across > the entire kernel. My primary concern with stack-protector was that I wanted to avoid a disconnect between what was visible in CONFIG_* and how the kernel actually got built. i.e. a kernel config had CONFIG_STACKPROTECTOR_STRONG, it was actually built with -fstack-protector-strong. Having it silently downgrade to -fstack-protector while keeping CONFIG_STACKPROTECTOR_STRONG would lead to serious confusion. The second issue was that I wanted the best stack protector a compiler supported, and at the time it wasn't possible to do this from kconfig. Masahiro fixed both of these now. :) (Thank you!) > Since we switched the x86_64 default to the ORC unwinder, a lot of > people have switched over. But this patch will reverse (or at least > slow down) that trend, because almost nobody has the libelf devel > packaged installed by default. So over time, it will effectively make > frame pointers the default again in many cases. That's exactly what we > *don't* want to do. It will also cause people to accidentally re-enable > frame pointers when they thought they had ORC. This is more like the gcc-plugins: kconfig will just not make the plugin CONFIG_*s visible if the gcc plugin dev package is missing on the build host. However, having or not having these isn't something we're trying to phase in or out, so the ORC case is more like how stack-protector was originally: fail the build if your CONFIG requires some additional build host package. What might be interesting is having "make *config" report certain CONFIG_* failures with helpful text. "WARNING: missing libelf for CONFIG_ORC..." or "Warning: missing gcc-plugin-dev for CONFIG_GCC_PLUGINS" etc? -Kees -- Kees Cook Pixel Security
diff --git a/Makefile b/Makefile index 925c55f..65befa0 100644 --- a/Makefile +++ b/Makefile @@ -927,19 +927,7 @@ endif export mod_sign_cmd ifdef CONFIG_STACK_VALIDATION - has_libelf := $(call try-run,\ - echo "int main() {}" | $(HOSTCC) -xc -o /dev/null -lelf -,1,0) - ifeq ($(has_libelf),1) - objtool_target := tools/objtool FORCE - else - ifdef CONFIG_UNWINDER_ORC - $(error "Cannot generate ORC metadata for CONFIG_UNWINDER_ORC=y, please install libelf-dev, libelf-devel or elfutils-libelf-devel") - else - $(warning "Cannot use CONFIG_STACK_VALIDATION=y, please install libelf-dev, libelf-devel or elfutils-libelf-devel") - endif - SKIP_STACK_VALIDATION := 1 - export SKIP_STACK_VALIDATION - endif + objtool_target := tools/objtool endif diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig index f1dbb4e..c1ded99 100644 --- a/arch/x86/Kconfig +++ b/arch/x86/Kconfig @@ -434,7 +434,7 @@ config GOLDFISH config RETPOLINE bool "Avoid speculative indirect branches in kernel" default y - select STACK_VALIDATION if HAVE_STACK_VALIDATION + imply STACK_VALIDATION help Compile kernel with the retpoline compiler options to guard against kernel-to-user data leaks by avoiding speculative indirect diff --git a/arch/x86/Kconfig.debug b/arch/x86/Kconfig.debug index c6dd1d9..4713b3c 100644 --- a/arch/x86/Kconfig.debug +++ b/arch/x86/Kconfig.debug @@ -364,7 +364,7 @@ choice config UNWINDER_ORC bool "ORC unwinder" depends on X86_64 - select STACK_VALIDATION + depends on STACK_VALIDATION ---help--- This option enables the ORC (Oops Rewind Capability) unwinder for unwinding kernel stack traces. It uses a custom data format which is diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug index 8838d11..f4d48af 100644 --- a/lib/Kconfig.debug +++ b/lib/Kconfig.debug @@ -364,7 +364,7 @@ config FRAME_POINTER config STACK_VALIDATION bool "Compile-time stack metadata validation" depends on HAVE_STACK_VALIDATION - default n + depends on $(success,echo "int main() {}" | $(HOSTCC) -xc -o /dev/null -lelf -) help Add compile-time checks to validate stack metadata, including frame pointers (if CONFIG_FRAME_POINTER is enabled). This helps ensure @@ -373,6 +373,10 @@ config STACK_VALIDATION This is also a prerequisite for generation of ORC unwind data, which is needed for CONFIG_UNWINDER_ORC. + To enable this, the host compiler needs to be able to link libelf. + If it is missing on your system, please install libelf-dev, + libelf-devel or elfutils-libelf-devel. + For more information, see tools/objtool/Documentation/stack-validation.txt. diff --git a/scripts/Makefile.build b/scripts/Makefile.build index e7889f4..154205b 100644 --- a/scripts/Makefile.build +++ b/scripts/Makefile.build @@ -243,7 +243,6 @@ endif # -record-mcount endif # CONFIG_FTRACE_MCOUNT_RECORD ifdef CONFIG_STACK_VALIDATION -ifneq ($(SKIP_STACK_VALIDATION),1) __objtool_obj := $(objtree)/tools/objtool/objtool @@ -282,7 +281,6 @@ objtool_obj = $(if $(patsubst y%,, \ $(OBJECT_FILES_NON_STANDARD_$(basetarget).o)$(OBJECT_FILES_NON_STANDARD)n), \ $(__objtool_obj)) -endif # SKIP_STACK_VALIDATION endif # CONFIG_STACK_VALIDATION # Rebuild all objects when objtool changes, or is enabled/disabled.