| Message ID | 20210914191045.2234020-12-samitolvanen@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86: Add support for Clang CFI | expand |
On Tue, Sep 14, 2021 at 12:11 PM Sami Tolvanen <samitolvanen@google.com> wrote: > > Disable CONFIG_CFI_CLANG for the stand-alone purgatory.ro. > > Signed-off-by: Sami Tolvanen <samitolvanen@google.com> I kind of prefer the existing convention that has explicit guards on specific configs (ie. CONFIG_FUNCTION_TRACER, CONFIG_STACKPROTECTOR, CONFIG_STACKPROTECTOR_STRONG, CONFIG_RETPOLINE); it's more obvious which configs may introduce which flags that are problematic. This patch is ok as is, but it kind of makes this Makefile more inconsistent. I would prefer we had the explicit checks. Does CFI actually do any instrumentation in these object files? I guess issues in purgatory cause silent/hard to debug kexec failures? > --- > arch/x86/purgatory/Makefile | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/x86/purgatory/Makefile b/arch/x86/purgatory/Makefile > index 95ea17a9d20c..ed46ad780130 100644 > --- a/arch/x86/purgatory/Makefile > +++ b/arch/x86/purgatory/Makefile > @@ -31,7 +31,7 @@ KCOV_INSTRUMENT := n > # These are adjustments to the compiler flags used for objects that > # make up the standalone purgatory.ro > > -PURGATORY_CFLAGS_REMOVE := -mcmodel=kernel > +PURGATORY_CFLAGS_REMOVE := -mcmodel=kernel $(CC_FLAGS_CFI) > PURGATORY_CFLAGS := -mcmodel=large -ffreestanding -fno-zero-initialized-in-bss -g0 > PURGATORY_CFLAGS += $(DISABLE_STACKLEAK_PLUGIN) -DDISABLE_BRANCH_PROFILING > PURGATORY_CFLAGS += -fno-stack-protector > -- > 2.33.0.309.g3052b89438-goog >
On Tue, Sep 14, 2021 at 1:02 PM Nick Desaulniers <ndesaulniers@google.com> wrote: > > On Tue, Sep 14, 2021 at 12:11 PM Sami Tolvanen <samitolvanen@google.com> wrote: > > > > Disable CONFIG_CFI_CLANG for the stand-alone purgatory.ro. > > > > Signed-off-by: Sami Tolvanen <samitolvanen@google.com> > > I kind of prefer the existing convention that has explicit guards on > specific configs (ie. CONFIG_FUNCTION_TRACER, CONFIG_STACKPROTECTOR, > CONFIG_STACKPROTECTOR_STRONG, CONFIG_RETPOLINE); it's more obvious > which configs may introduce which flags that are problematic. This > patch is ok as is, but it kind of makes this Makefile more > inconsistent. I would prefer we had the explicit checks. The Makefile does already use DISABLE_STACKLEAK_PLUGIN in a similar way, but I don't have a strong preference here. I can move this into an ifdef if it makes things cleaner. > Does CFI actually do any instrumentation in these object files? I > guess issues in purgatory cause silent/hard to debug kexec failures? The compiler shouldn't add any actual CFI instrumentation here right now, but I would prefer to avoid issues in future. Sami
On Tue, Sep 14, 2021 at 1:30 PM Sami Tolvanen <samitolvanen@google.com> wrote: > > On Tue, Sep 14, 2021 at 1:02 PM Nick Desaulniers > <ndesaulniers@google.com> wrote: > > > > On Tue, Sep 14, 2021 at 12:11 PM Sami Tolvanen <samitolvanen@google.com> wrote: > > > > > > Disable CONFIG_CFI_CLANG for the stand-alone purgatory.ro. > > > > > > Signed-off-by: Sami Tolvanen <samitolvanen@google.com> > > > > I kind of prefer the existing convention that has explicit guards on > > specific configs (ie. CONFIG_FUNCTION_TRACER, CONFIG_STACKPROTECTOR, > > CONFIG_STACKPROTECTOR_STRONG, CONFIG_RETPOLINE); it's more obvious > > which configs may introduce which flags that are problematic. This > > patch is ok as is, but it kind of makes this Makefile more > > inconsistent. I would prefer we had the explicit checks. > > The Makefile does already use DISABLE_STACKLEAK_PLUGIN in a similar > way, but I don't have a strong preference here. mmm...DISABLE_STACKLEAK_PLUGIN adds to PURGATORY_CFLAGS. This patch adds to PURGATORY_CFLAGS_REMOVE. > I can move this into > an ifdef if it makes things cleaner. > > > Does CFI actually do any instrumentation in these object files? I > > guess issues in purgatory cause silent/hard to debug kexec failures? > > The compiler shouldn't add any actual CFI instrumentation here right > now, but I would prefer to avoid issues in future. Ok, good to know.
On Tue, Sep 14, 2021 at 03:31:14PM -0700, Nick Desaulniers wrote: > On Tue, Sep 14, 2021 at 1:30 PM Sami Tolvanen <samitolvanen@google.com> wrote: > > > > On Tue, Sep 14, 2021 at 1:02 PM Nick Desaulniers > > <ndesaulniers@google.com> wrote: > > > > > > On Tue, Sep 14, 2021 at 12:11 PM Sami Tolvanen <samitolvanen@google.com> wrote: > > > > > > > > Disable CONFIG_CFI_CLANG for the stand-alone purgatory.ro. > > > > > > > > Signed-off-by: Sami Tolvanen <samitolvanen@google.com> > > > > > > I kind of prefer the existing convention that has explicit guards on > > > specific configs (ie. CONFIG_FUNCTION_TRACER, CONFIG_STACKPROTECTOR, > > > CONFIG_STACKPROTECTOR_STRONG, CONFIG_RETPOLINE); it's more obvious > > > which configs may introduce which flags that are problematic. This > > > patch is ok as is, but it kind of makes this Makefile more > > > inconsistent. I would prefer we had the explicit checks. Can you explain your reasoning a bit more? It seems like redundant open-coded logic to me, but I do see this idiom repeated in the kernel. And/or maybe I've misunderstood you? It seems like it's better to have a single variable (like in the proposed patch: CC_FLAGS_CFI) that has all the details internal -- no tests needed. i.e.: instead of this in many places: ifdef CONFIG_FEATURE PURGATORY_CFLAGS_REMOVE += -feature-flag endif do this once: CC_FEATURE_CFLAGS := -feature-flag ... KBUILD_CFLAGS += $(CC_FEATURE_CFLAGS) and only repeat a single line in for targets: CFLAGS_REMOVE_target.o += $(CC_FEATURE_CFLAGS) > > > > The Makefile does already use DISABLE_STACKLEAK_PLUGIN in a similar > > way, but I don't have a strong preference here. > > mmm...DISABLE_STACKLEAK_PLUGIN adds to PURGATORY_CFLAGS. This patch > adds to PURGATORY_CFLAGS_REMOVE. CFI is "simple" in that regard; its options can just be left off. This isn't true for some weirder stuff. Stack protector is a good one, in that just removing the options may not disable it depending on distro patches (which may turn it on by default), so both target_CFLAGS and target_REMOVE are needed there. (In the case of the plugins, yes, I think they could be rearranged to use the target_REMOVE method, but I have a memory of REMOVE not working there for some weird thing? Hmm.)
diff --git a/arch/x86/purgatory/Makefile b/arch/x86/purgatory/Makefile index 95ea17a9d20c..ed46ad780130 100644 --- a/arch/x86/purgatory/Makefile +++ b/arch/x86/purgatory/Makefile @@ -31,7 +31,7 @@ KCOV_INSTRUMENT := n # These are adjustments to the compiler flags used for objects that # make up the standalone purgatory.ro -PURGATORY_CFLAGS_REMOVE := -mcmodel=kernel +PURGATORY_CFLAGS_REMOVE := -mcmodel=kernel $(CC_FLAGS_CFI) PURGATORY_CFLAGS := -mcmodel=large -ffreestanding -fno-zero-initialized-in-bss -g0 PURGATORY_CFLAGS += $(DISABLE_STACKLEAK_PLUGIN) -DDISABLE_BRANCH_PROFILING PURGATORY_CFLAGS += -fno-stack-protector
Disable CONFIG_CFI_CLANG for the stand-alone purgatory.ro. Signed-off-by: Sami Tolvanen <samitolvanen@google.com> --- arch/x86/purgatory/Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)