| Message ID | 7fad83ecde03540e65677959034315f8fbb3755e.1649434832.git.jpoimboe@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | kbuild: Remove CONFIG_DEBUG_SECTION_MISMATCH | expand |
(+CC: Changbin Du, Arnd Bergmann, who were working on the -Og builds) On Sat, Apr 9, 2022 at 1:23 AM Josh Poimboeuf <jpoimboe@redhat.com> wrote: > > Modpost's section mismatch detection warns when a non-init function > references an __init function or __initdata. Those warnings are very > useful, because __init memory is freed during boot, so the non-init > function might end up referencing or calling into some random memory. > > CONFIG_DEBUG_SECTION_MISMATCH is intended to root out even more of these > issues by adding the -fno-inline-functions-called-once compiler flag, > which forces once-called static functions to not be inlined. As > described in the option's help description: > > - Add the option -fno-inline-functions-called-once to gcc commands. > When inlining a function annotated with __init in a non-init > function, we would lose the section information and thus > the analysis would not catch the illegal reference. > This option tells gcc to inline less (but it does result in > a larger kernel). > > So it's basically a debug (non-production) option, which has the goal of > rooting out potential issues which might exist on *other* configs which > might somehow trigger different inlining decisions, without having to > build all those other configs to see the warnings directly. > > But with -O2, once-called static functions are almost always inlined, so "always inlined" per GCC manual. -O2, -O3, -O3 enables -finline-functions-called-once Clang does not enable this flag because DEBUG_SECTION_MISMATCH depends on CC_IS_GCC > its usefulness for rooting out mismatch warnings on other configs is > somewhere between extremely limited and non-existent. And nowadays we > have build bots everywhere doing randconfigs continuously, which are > great for rooting out such edge cases. > > Somewhat ironically, the existence of those build bots means we get a > lot of unrealistic objtool warnings being reported, due to unrealistic > inlines caused by CONFIG_DEBUG_SECTION_MISMATCH, where the only way to > silence the warnings is to force a single-called function to be inlined > with '__always_inline'. > > So the limited, hypothetical benefit of "rooting out configs with > section mismatches" is outweighed by the very real downside of "adding > lots of unnecessary '__always_inline' annotations". I am confused with the description because you are talking about two different warnings. [1] modpost warning (section mismatch) [2] objtool warnings For [1], you can add __init marker to the callers, and that is the right thing to do. Is [2] caused by dead code that was not optimized out due to the unusual inlining decisions by the compiler ? If the issues are all about objtool, "depends on !STACK_VALIDATION" might be an alternative approach? config DEBUG_SECTION_MISMATCH bool "Enable full Section mismatch analysis" depends on CC_IS_GCC depends on !STACK_VALIDATION # do not confuse objtool > > In fact I suspect this option has been responsible for dozens of > "convert inline to __always_inline" patches over the years. Such > patches usually complain about the compiler's inlining decisions being > unpredictable. It turns out this config option is the main culprit. > > So considering the drawbacks of this option significantly outweigh the > benefits, especially now in the age of randconfig build bots, remove it. > > Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com> In the old days, CONFIG_DEBUG_SECTION_MISMATCH was controlling more features of modpost, but all of them were just annoying and useless. They were killed by these commits: b7dca6dd1e591ad19a9aae716f3898be8063f880 46c7dd56d54133e3fb9414844d90e563627f3feb 7657f60e8ffad587fabc74873b5f42083a60c3cf Only the remaining part is enabling the -fno-inline-functions-called-once flag. Testing this flag more actively was proposed as a part of work of CONFIG_CC_OPTIMIZE_FOR_DEBUGGING (-Og). [1] I thought -Og was useful for debugging, but Linus rejected it. [2] GCC manual says: "-finline-functions-called-once Consider all static functions called once for inlining into their caller even if they are not marked inline. If a call to a given function is integrated, then the function is not output as assembler code in its own right. Enabled at levels -O1, -O2, -O3 and -Os, but not -Og." Now that -Og was already rejected, the possible flag for building the kernel is -O2, O3, -Os. All of them enable -finline-functions-called-once. So, there is no practical case for -fno-inline-functions-called-once unless we explicitly enable it. I am OK with this patch, I am still wondering if this could be useful to detect missing __init markers. [1] https://lore.kernel.org/linux-kbuild/1528186420-6615-3-git-send-email-changbin.du@intel.com/ [2] https://lore.kernel.org/linux-kbuild/CAHk-=wiLt3rgeyM3BWAd5VJrKcnxxuHybwoQiDGMgyspo6oXDg@mail.gmail.com/ > --- > .../media/maintainer-entry-profile.rst | 2 +- > Makefile | 5 ----- > arch/arc/configs/tb10x_defconfig | 1 - > arch/s390/configs/debug_defconfig | 1 - > arch/s390/configs/defconfig | 1 - > kernel/configs/debug.config | 1 - > lib/Kconfig.debug | 22 ------------------- > .../selftests/wireguard/qemu/debug.config | 1 - > 8 files changed, 1 insertion(+), 33 deletions(-) > > diff --git a/Documentation/driver-api/media/maintainer-entry-profile.rst b/Documentation/driver-api/media/maintainer-entry-profile.rst > index ffc712a5f632..06106d7e7fae 100644 > --- a/Documentation/driver-api/media/maintainer-entry-profile.rst > +++ b/Documentation/driver-api/media/maintainer-entry-profile.rst > @@ -123,7 +123,7 @@ Those tests need to pass before the patches go upstream. > > Also, please notice that we build the Kernel with:: > > - make CF=-D__CHECK_ENDIAN__ CONFIG_DEBUG_SECTION_MISMATCH=y C=1 W=1 CHECK=check_script > + make CF=-D__CHECK_ENDIAN__ C=1 W=1 CHECK=check_script > > Where the check script is:: > > diff --git a/Makefile b/Makefile > index 8c7de9a72ea2..3d7ea1a23558 100644 > --- a/Makefile > +++ b/Makefile > @@ -871,11 +871,6 @@ KBUILD_CFLAGS += $(CC_FLAGS_FTRACE) $(CC_FLAGS_USING) > KBUILD_AFLAGS += $(CC_FLAGS_USING) > endif > > -# We trigger additional mismatches with less inlining > -ifdef CONFIG_DEBUG_SECTION_MISMATCH > -KBUILD_CFLAGS += -fno-inline-functions-called-once > -endif > - > ifdef CONFIG_LD_DEAD_CODE_DATA_ELIMINATION > KBUILD_CFLAGS_KERNEL += -ffunction-sections -fdata-sections > LDFLAGS_vmlinux += --gc-sections > diff --git a/arch/arc/configs/tb10x_defconfig b/arch/arc/configs/tb10x_defconfig > index a12656ec0072..5acf8cc3e7b0 100644 > --- a/arch/arc/configs/tb10x_defconfig > +++ b/arch/arc/configs/tb10x_defconfig > @@ -96,7 +96,6 @@ CONFIG_STRIP_ASM_SYMS=y > CONFIG_DEBUG_FS=y > CONFIG_HEADERS_INSTALL=y > CONFIG_HEADERS_CHECK=y > -CONFIG_DEBUG_SECTION_MISMATCH=y > CONFIG_MAGIC_SYSRQ=y > CONFIG_DEBUG_MEMORY_INIT=y > CONFIG_DEBUG_STACKOVERFLOW=y > diff --git a/arch/s390/configs/debug_defconfig b/arch/s390/configs/debug_defconfig > index 498bed9b261b..66a4c65a4bd8 100644 > --- a/arch/s390/configs/debug_defconfig > +++ b/arch/s390/configs/debug_defconfig > @@ -791,7 +791,6 @@ CONFIG_DEBUG_INFO_DWARF4=y > CONFIG_DEBUG_INFO_BTF=y > CONFIG_GDB_SCRIPTS=y > CONFIG_HEADERS_INSTALL=y > -CONFIG_DEBUG_SECTION_MISMATCH=y > CONFIG_MAGIC_SYSRQ=y > CONFIG_DEBUG_PAGEALLOC=y > CONFIG_PAGE_OWNER=y > diff --git a/arch/s390/configs/defconfig b/arch/s390/configs/defconfig > index 61e36b999f67..0e6ae36cf401 100644 > --- a/arch/s390/configs/defconfig > +++ b/arch/s390/configs/defconfig > @@ -776,7 +776,6 @@ CONFIG_DEBUG_INFO=y > CONFIG_DEBUG_INFO_DWARF4=y > CONFIG_DEBUG_INFO_BTF=y > CONFIG_GDB_SCRIPTS=y > -CONFIG_DEBUG_SECTION_MISMATCH=y > CONFIG_MAGIC_SYSRQ=y > CONFIG_DEBUG_WX=y > CONFIG_PTDUMP_DEBUGFS=y > diff --git a/kernel/configs/debug.config b/kernel/configs/debug.config > index e8db8d938661..0c1a5d64febb 100644 > --- a/kernel/configs/debug.config > +++ b/kernel/configs/debug.config > @@ -18,7 +18,6 @@ CONFIG_SYMBOLIC_ERRNAME=y > # > CONFIG_DEBUG_INFO=y > CONFIG_DEBUG_INFO_DWARF_TOOLCHAIN_DEFAULT=y > -CONFIG_DEBUG_SECTION_MISMATCH=y > CONFIG_FRAME_WARN=2048 > CONFIG_SECTION_MISMATCH_WARN_ONLY=y > # > diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug > index 075cd25363ac..e52f851e9e3b 100644 > --- a/lib/Kconfig.debug > +++ b/lib/Kconfig.debug > @@ -425,28 +425,6 @@ config HEADERS_INSTALL > user-space program samples. It is also needed by some features such > as uapi header sanity checks. > > -config DEBUG_SECTION_MISMATCH > - bool "Enable full Section mismatch analysis" > - depends on CC_IS_GCC > - help > - The section mismatch analysis checks if there are illegal > - references from one section to another section. > - During linktime or runtime, some sections are dropped; > - any use of code/data previously in these sections would > - most likely result in an oops. > - In the code, functions and variables are annotated with > - __init,, etc. (see the full list in include/linux/init.h), > - which results in the code/data being placed in specific sections. > - The section mismatch analysis is always performed after a full > - kernel build, and enabling this option causes the following > - additional step to occur: > - - Add the option -fno-inline-functions-called-once to gcc commands. > - When inlining a function annotated with __init in a non-init > - function, we would lose the section information and thus > - the analysis would not catch the illegal reference. > - This option tells gcc to inline less (but it does result in > - a larger kernel). > - > config SECTION_MISMATCH_WARN_ONLY > bool "Make section mismatch errors non-fatal" > default y > diff --git a/tools/testing/selftests/wireguard/qemu/debug.config b/tools/testing/selftests/wireguard/qemu/debug.config > index 2b321b8a96cf..e737ce3b324e 100644 > --- a/tools/testing/selftests/wireguard/qemu/debug.config > +++ b/tools/testing/selftests/wireguard/qemu/debug.config > @@ -57,7 +57,6 @@ CONFIG_USER_STACKTRACE_SUPPORT=y > CONFIG_DEBUG_SG=y > CONFIG_DEBUG_NOTIFIERS=y > CONFIG_X86_DEBUG_FPU=y > -CONFIG_DEBUG_SECTION_MISMATCH=y > CONFIG_DEBUG_PAGEALLOC=y > CONFIG_DEBUG_PAGEALLOC_ENABLE_DEFAULT=y > CONFIG_DEBUG_WW_MUTEX_SLOWPATH=y > -- > 2.34.1 >
On Sat, Apr 09, 2022 at 03:29:21AM +0900, Masahiro Yamada wrote: > Is [2] caused by dead code that was not optimized out > due to the unusual inlining decisions by the compiler ? The complaint is due to SMAP validation; objtool will scream if there's a CALL in between STAC/CLAC. The thinking is that since they open a security window, we want tight code between them. We also very much don't want tracing and other funnies to happen there. As such, any CALL is dis-allowed. This weird option is having us upgrade quite a few 'inline' to '__always_inline'.
Lore thread start for newly cc'ed ML readers: https://lore.kernel.org/lkml/7fad83ecde03540e65677959034315f8fbb3755e.1649434832.git.jpoimboe@redhat.com/ On Fri, Apr 8, 2022 at 12:14 PM Peter Zijlstra <peterz@infradead.org> wrote: > > On Sat, Apr 09, 2022 at 03:29:21AM +0900, Masahiro Yamada wrote: > > Is [2] caused by dead code that was not optimized out > > due to the unusual inlining decisions by the compiler ? > > The complaint is due to SMAP validation; objtool will scream if there's > a CALL in between STAC/CLAC. The thinking is that since they open a > security window, we want tight code between them. We also very much > don't want tracing and other funnies to happen there. As such, any CALL > is dis-allowed. Just indirect calls, which might be manipulated, or static calls, too? > > This weird option is having us upgrade quite a few 'inline' to > '__always_inline'. As is, the assumption that __init functions only call other __init functions or __always_inline is a brittle house of cards that leads to a "what color is your function" [0] scenario, and leads to code that happens to not emit warnings for compiler X (or compiler X version Y). There's also curious exceptions in modpost that look like memory leaks to me. We already have such toolchain portability issues for different toolchains and different configs; warnings from section mismatches, and objtool STAC/CLAC checks. I feel that Josh's patch would sweep more of those under the rug, so I'm not in favor of it, but could be convinced otherwise. TBH, I kind of think that we could use a C extension to permit __attribute__((always_inline)) to additionally be a statement attribute, rather than just a function attribute because of cases like this; we need the flexibility to make one call site __always_inline without necessarily forcing ALL callsites to be __always_inline'd. void y (void); void x (void) { __attribute__((always_inline)) y(); }; (This is already expressable in LLVM IR; not (yet) in C. I'm not sure yet _why_ this was added to LLVM; whether a different language front end can express this, if C can and I'm mistaken, or whether it's only used for optimizations). I think that would give developers maximal flexibility to defer as much to the compiler's inlining decisions when they don't care, and express precisely what they need when they do [care]. [0] https://journal.stuffwithstuff.com/2015/02/01/what-color-is-your-function/
On Fri, Apr 8, 2022 at 1:08 PM Nick Desaulniers <ndesaulniers@google.com> wrote: > > Lore thread start for newly cc'ed ML readers: > https://lore.kernel.org/lkml/7fad83ecde03540e65677959034315f8fbb3755e.1649434832.git.jpoimboe@redhat.com/ > > On Fri, Apr 8, 2022 at 12:14 PM Peter Zijlstra <peterz@infradead.org> wrote: > > > > This weird option is having us upgrade quite a few 'inline' to > > '__always_inline'. > > As is, the assumption that __init functions only call other __init > functions or __always_inline is a brittle house of cards that leads to > a "what color is your function" [0] scenario, and leads to code that > happens to not emit warnings for compiler X (or compiler X version Y). > There's also curious exceptions in modpost that look like memory leaks > to me. These assumptions perhaps made more sense in a world prior to commit 889b3c1245de ("compiler: remove CONFIG_OPTIMIZE_INLINING entirely") https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=889b3c1245de48ed0cacf7aebb25c489d3e4a3e9 (I view 889b3c1245de favorably; perhaps this whole thread is just fallout from that change though. It's also interesting to note that CONFIG_OPTIMIZE_INLINING was enabled in the i386 and x86_64 defconfigs. That might color some folk's experience with the use of `inline` in the kernel sources and whether "inline means __attribute__((always_inline))").
On Fri, Apr 8, 2022 at 9:23 AM Josh Poimboeuf <jpoimboe@redhat.com> wrote: > > So it's basically a debug (non-production) option, which has the goal of So I think I agree that seeing it used in defconfigs is curious to me; I'm kind of in favor of removing its use in defconfigs. I wish the defconfigs for more architectures were something closer to what a user should reasonably start with; I don't think we'd want to encourage people to actually ship kernels built with this option enabled.
On Fri, Apr 08, 2022 at 01:08:47PM -0700, Nick Desaulniers wrote: > Lore thread start for newly cc'ed ML readers: > https://lore.kernel.org/lkml/7fad83ecde03540e65677959034315f8fbb3755e.1649434832.git.jpoimboe@redhat.com/ > > On Fri, Apr 8, 2022 at 12:14 PM Peter Zijlstra <peterz@infradead.org> wrote: > > > > On Sat, Apr 09, 2022 at 03:29:21AM +0900, Masahiro Yamada wrote: > > > Is [2] caused by dead code that was not optimized out > > > due to the unusual inlining decisions by the compiler ? > > > > The complaint is due to SMAP validation; objtool will scream if there's > > a CALL in between STAC/CLAC. The thinking is that since they open a > > security window, we want tight code between them. We also very much > > don't want tracing and other funnies to happen there. As such, any CALL > > is dis-allowed. > > Just indirect calls, which might be manipulated, or static calls, too? Any CALL instruction is a no-no. Only 'simple' code is allowed between STAC and CLAC. > > This weird option is having us upgrade quite a few 'inline' to > > '__always_inline'. > > As is, the assumption that __init functions only call other __init > functions or __always_inline is a brittle house of cards that leads to > a "what color is your function" [0] scenario, and leads to code that > happens to not emit warnings for compiler X (or compiler X version Y). > There's also curious exceptions in modpost that look like memory leaks > to me. > > We already have such toolchain portability issues for different > toolchains and different configs; warnings from section mismatches, > and objtool STAC/CLAC checks. I feel that Josh's patch would sweep > more of those under the rug, so I'm not in favor of it, but could be > convinced otherwise. > > TBH, I kind of think that we could use a C extension to permit > __attribute__((always_inline)) to additionally be a statement > attribute, rather than just a function attribute because of cases like > this; we need the flexibility to make one call site __always_inline > without necessarily forcing ALL callsites to be __always_inline'd. > > void y (void); > void x (void) { __attribute__((always_inline)) y(); }; > > (This is already expressable in LLVM IR; not (yet) in C. I'm not sure > yet _why_ this was added to LLVM; whether a different language front > end can express this, if C can and I'm mistaken, or whether it's only > used for optimizations). > > I think that would give developers maximal flexibility to defer as > much to the compiler's inlining decisions when they don't care, and > express precisely what they need when they do [care]. > > [0] https://journal.stuffwithstuff.com/2015/02/01/what-color-is-your-function/ So in the case of that latest __always_inline patch, there was only a single caller. New syntax would buy us absolutely nothing there. If we're talking extentions, I'd much rather have function spaces. That is, being able to tag functions *AND* function pointers with an address space qualifier. I want to be able to create a function pointer that can only be assigned functions from the noinstr space for example. Ideally calling such a functino pointer would only be possible from within that space. Anyway, let me go read that blog you linked.
On Fri, Apr 08, 2022 at 10:32:28PM +0200, Peter Zijlstra wrote: > > > This weird option is having us upgrade quite a few 'inline' to > > > '__always_inline'. > > > > As is, the assumption that __init functions only call other __init > > functions or __always_inline is a brittle house of cards that leads to > > a "what color is your function" [0] scenario, and leads to code that > > happens to not emit warnings for compiler X (or compiler X version Y). > > There's also curious exceptions in modpost that look like memory leaks > > to me. So I don't see __always_inline that way (also I'm in the 'inline' should be '__always_inline' camp). To me inline is more like: 'instantiate that pattern *here*'. It's like CPP macros, only less horrible. You get the code generated according to the local rules (instrumentation yes/no, section, and whatever other function attributes we have that affect code-gen). So with inline we can get the same pattern instantiated a number of different times, leading to different actual code, without having to type the whole thing multiple times (which would be terrible for maintenance) etc.. Combine __always_inline with constant propagation of inline function 'pointers' and you get do beautiful things ;-) /me runs
On Sat, Apr 09, 2022 at 03:29:21AM +0900, Masahiro Yamada wrote: > > But with -O2, once-called static functions are almost always inlined, so > > "always inlined" per GCC manual. > -O2, -O3, -O3 enables -finline-functions-called-once Not necessarily. The manual also says: -finline-functions-called-once Consider all "static" functions called once for inlining into their caller even if they are not marked "inline". If a call to a given function is integrated, then the function is not output as assembler code in its own right. So it "considers" inlining them, but it doesn't guarantee it. And when I looked at the GCC code some months ago I remember seeing a few edge cases where it would inline. > > its usefulness for rooting out mismatch warnings on other configs is > > somewhere between extremely limited and non-existent. And nowadays we > > have build bots everywhere doing randconfigs continuously, which are > > great for rooting out such edge cases. > > > > Somewhat ironically, the existence of those build bots means we get a > > lot of unrealistic objtool warnings being reported, due to unrealistic > > inlines caused by CONFIG_DEBUG_SECTION_MISMATCH, where the only way to > > silence the warnings is to force a single-called function to be inlined > > with '__always_inline'. > > > > So the limited, hypothetical benefit of "rooting out configs with > > section mismatches" is outweighed by the very real downside of "adding > > lots of unnecessary '__always_inline' annotations". > > > I am confused with the description because > you are talking about two different warnings. > > [1] modpost warning (section mismatch) > [2] objtool warnings It's all a bit confusing. To clarify: in theory, the point of CONFIG_DEBUG_SECTION_MISMATCH was to trigger more *modpost* warnings. (It may do that, but the extra warnings are probably not realistic. And even if they were realistic on some configs, they would be found by modpost warnings on those configs found by build bots.) But CONFIG_DEBUG_SECTION_MISMATCH is also triggering more *objtool* warnings, which is a problem because the warnings are not realistic... > For [1], you can add __init marker to the callers, > and that is the right thing to do. Yes, either add __init to the caller or remove __init from the callee. But even if such "inlined single caller" modpost warnings correspond to real world configs (which is very questionable) then we'd expect them to show up in real-world randconfig bot testing, without having the need for CONFIG_DEBUG_SECTION_MISMATCH to root out the rare edge cases. > Is [2] caused by dead code that was not optimized out > due to the unusual inlining decisions by the compiler ? As Peter mentioned it was due to validation of SMAP uaccess rules. > If the issues are all about objtool, > "depends on !STACK_VALIDATION" might be > an alternative approach? > > config DEBUG_SECTION_MISMATCH > bool "Enable full Section mismatch analysis" > depends on CC_IS_GCC > depends on !STACK_VALIDATION # do not confuse objtool Yes, that would be another way to handle it. Though that means the option would effectively be dead on x86-64. > Now that -Og was already rejected, the possible flag for building the kernel is > -O2, O3, -Os. > All of them enable -finline-functions-called-once. > > So, there is no practical case for -fno-inline-functions-called-once > unless we explicitly enable it. Agreed. > I am OK with this patch, I am still wondering if this could be useful > to detect missing __init markers. So there's two ways to look at this question: at a source level and at a binary level. At a source level, if there's a non-init function which inlines a single-called __init function, and modpost doesn't notice it (because the __init function doesn't access any __init symbols), then the __init function wrongly has the __init annotation. But calling that a bug is very questionable, because it's not a real bug in the binary. IMO it's not worth worrying about, when we have real bugs to fix. At a binary level, if there's a real section mismatch bug with a given config, modpost will warn about it. This option doesn't affect that.
On Fri, Apr 8, 2022 at 9:14 PM Peter Zijlstra <peterz@infradead.org> wrote: > > On Sat, Apr 09, 2022 at 03:29:21AM +0900, Masahiro Yamada wrote: > > Is [2] caused by dead code that was not optimized out > > due to the unusual inlining decisions by the compiler ? > > The complaint is due to SMAP validation; objtool will scream if there's > a CALL in between STAC/CLAC. The thinking is that since they open a > security window, we want tight code between them. We also very much > don't want tracing and other funnies to happen there. As such, any CALL > is dis-allowed. > > This weird option is having us upgrade quite a few 'inline' to > '__always_inline'. There is also __attribute__((flatten)), which you can add to the caller to tell gcc to inline everything it can into this function, without having to inline it everywhere else. Would that work here? It sounds like this is what STAC/CLAC actually requires. Arnd
On Fri, Apr 8, 2022 at 5:48 PM Josh Poimboeuf <jpoimboe@redhat.com> wrote: > > On Sat, Apr 09, 2022 at 03:29:21AM +0900, Masahiro Yamada wrote: > > > But with -O2, once-called static functions are almost always inlined, so > > > > "always inlined" per GCC manual. > > -O2, -O3, -O3 enables -finline-functions-called-once > > Not necessarily. The manual also says: > > -finline-functions-called-once > Consider all "static" functions called once for inlining into > their caller even if they are not marked "inline". If a call > to a given function is integrated, then the function is not > output as assembler code in its own right. > > So it "considers" inlining them, but it doesn't guarantee it. And when > I looked at the GCC code some months ago I remember seeing a few edge > cases where it would inline. > > > > its usefulness for rooting out mismatch warnings on other configs is > > > somewhere between extremely limited and non-existent. And nowadays we > > > have build bots everywhere doing randconfigs continuously, which are > > > great for rooting out such edge cases. > > > > > > Somewhat ironically, the existence of those build bots means we get a > > > lot of unrealistic objtool warnings being reported, due to unrealistic > > > inlines caused by CONFIG_DEBUG_SECTION_MISMATCH, where the only way to > > > silence the warnings is to force a single-called function to be inlined > > > with '__always_inline'. > > > > > > So the limited, hypothetical benefit of "rooting out configs with > > > section mismatches" is outweighed by the very real downside of "adding > > > lots of unnecessary '__always_inline' annotations". > > > > > > I am confused with the description because > > you are talking about two different warnings. > > > > [1] modpost warning (section mismatch) > > [2] objtool warnings > > It's all a bit confusing. > > To clarify: in theory, the point of CONFIG_DEBUG_SECTION_MISMATCH was to > trigger more *modpost* warnings. (It may do that, but the extra > warnings are probably not realistic. And even if they were realistic on > some configs, they would be found by modpost warnings on those configs > found by build bots.) > > But CONFIG_DEBUG_SECTION_MISMATCH is also triggering more *objtool* > warnings, which is a problem because the warnings are not realistic... > > > For [1], you can add __init marker to the callers, > > and that is the right thing to do. > > Yes, either add __init to the caller or remove __init from the callee. > > But even if such "inlined single caller" modpost warnings correspond to > real world configs (which is very questionable) then we'd expect them to > show up in real-world randconfig bot testing, without having the need > for CONFIG_DEBUG_SECTION_MISMATCH to root out the rare edge cases. > > > Is [2] caused by dead code that was not optimized out > > due to the unusual inlining decisions by the compiler ? > > As Peter mentioned it was due to validation of SMAP uaccess rules. > > > If the issues are all about objtool, > > "depends on !STACK_VALIDATION" might be > > an alternative approach? > > > > config DEBUG_SECTION_MISMATCH > > bool "Enable full Section mismatch analysis" > > depends on CC_IS_GCC > > depends on !STACK_VALIDATION # do not confuse objtool > > Yes, that would be another way to handle it. Though that means the > option would effectively be dead on x86-64. Does this series help (or is it related to this thread)? https://lore.kernel.org/lkml/cover.1650300597.git.jpoimboe@redhat.com/ Patch 17/25 seems to make STACK_VALIDATION unwinder-dependent (on CONFIG_UNWINDER_FRAME_POINTER)? > > > Now that -Og was already rejected, the possible flag for building the kernel is > > -O2, O3, -Os. > > All of them enable -finline-functions-called-once. > > > > So, there is no practical case for -fno-inline-functions-called-once > > unless we explicitly enable it. > > Agreed. > > > I am OK with this patch, I am still wondering if this could be useful > > to detect missing __init markers. > > So there's two ways to look at this question: at a source level and at a > binary level. > > At a source level, if there's a non-init function which inlines a > single-called __init function, and modpost doesn't notice it (because > the __init function doesn't access any __init symbols), then the __init > function wrongly has the __init annotation. But calling that a bug is > very questionable, because it's not a real bug in the binary. IMO it's > not worth worrying about, when we have real bugs to fix. > > At a binary level, if there's a real section mismatch bug with a given > config, modpost will warn about it. This option doesn't affect that. > > -- > Josh >
On Wed, Apr 20, 2022 at 10:03:18AM -0700, Nick Desaulniers wrote: > > > If the issues are all about objtool, > > > "depends on !STACK_VALIDATION" might be > > > an alternative approach? > > > > > > config DEBUG_SECTION_MISMATCH > > > bool "Enable full Section mismatch analysis" > > > depends on CC_IS_GCC > > > depends on !STACK_VALIDATION # do not confuse objtool > > > > Yes, that would be another way to handle it. Though that means the > > option would effectively be dead on x86-64. > > Does this series help (or is it related to this thread)? > https://lore.kernel.org/lkml/cover.1650300597.git.jpoimboe@redhat.com/ > Patch 17/25 seems to make STACK_VALIDATION unwinder-dependent (on > CONFIG_UNWINDER_FRAME_POINTER)? Not really, that series just splits objtool into more granular features, so objtool is no longer just equivalent to CONFIG_STACK_VALIDATION. So the above suggestion would probably need to be changed to something like depends on !HAVE_UACCESS_VALIDATION Or maybe depends on !(HAVE_UACCESS_VALIDATION || NOINSTR_VALIDATION) But uaccess validation is still mandatory for x86-64, so that would still unconditionally disable CONFIG_DEBUG_SECTION_MISMATCH for x86-64.
diff --git a/Documentation/driver-api/media/maintainer-entry-profile.rst b/Documentation/driver-api/media/maintainer-entry-profile.rst index ffc712a5f632..06106d7e7fae 100644 --- a/Documentation/driver-api/media/maintainer-entry-profile.rst +++ b/Documentation/driver-api/media/maintainer-entry-profile.rst @@ -123,7 +123,7 @@ Those tests need to pass before the patches go upstream. Also, please notice that we build the Kernel with:: - make CF=-D__CHECK_ENDIAN__ CONFIG_DEBUG_SECTION_MISMATCH=y C=1 W=1 CHECK=check_script + make CF=-D__CHECK_ENDIAN__ C=1 W=1 CHECK=check_script Where the check script is:: diff --git a/Makefile b/Makefile index 8c7de9a72ea2..3d7ea1a23558 100644 --- a/Makefile +++ b/Makefile @@ -871,11 +871,6 @@ KBUILD_CFLAGS += $(CC_FLAGS_FTRACE) $(CC_FLAGS_USING) KBUILD_AFLAGS += $(CC_FLAGS_USING) endif -# We trigger additional mismatches with less inlining -ifdef CONFIG_DEBUG_SECTION_MISMATCH -KBUILD_CFLAGS += -fno-inline-functions-called-once -endif - ifdef CONFIG_LD_DEAD_CODE_DATA_ELIMINATION KBUILD_CFLAGS_KERNEL += -ffunction-sections -fdata-sections LDFLAGS_vmlinux += --gc-sections diff --git a/arch/arc/configs/tb10x_defconfig b/arch/arc/configs/tb10x_defconfig index a12656ec0072..5acf8cc3e7b0 100644 --- a/arch/arc/configs/tb10x_defconfig +++ b/arch/arc/configs/tb10x_defconfig @@ -96,7 +96,6 @@ CONFIG_STRIP_ASM_SYMS=y CONFIG_DEBUG_FS=y CONFIG_HEADERS_INSTALL=y CONFIG_HEADERS_CHECK=y -CONFIG_DEBUG_SECTION_MISMATCH=y CONFIG_MAGIC_SYSRQ=y CONFIG_DEBUG_MEMORY_INIT=y CONFIG_DEBUG_STACKOVERFLOW=y diff --git a/arch/s390/configs/debug_defconfig b/arch/s390/configs/debug_defconfig index 498bed9b261b..66a4c65a4bd8 100644 --- a/arch/s390/configs/debug_defconfig +++ b/arch/s390/configs/debug_defconfig @@ -791,7 +791,6 @@ CONFIG_DEBUG_INFO_DWARF4=y CONFIG_DEBUG_INFO_BTF=y CONFIG_GDB_SCRIPTS=y CONFIG_HEADERS_INSTALL=y -CONFIG_DEBUG_SECTION_MISMATCH=y CONFIG_MAGIC_SYSRQ=y CONFIG_DEBUG_PAGEALLOC=y CONFIG_PAGE_OWNER=y diff --git a/arch/s390/configs/defconfig b/arch/s390/configs/defconfig index 61e36b999f67..0e6ae36cf401 100644 --- a/arch/s390/configs/defconfig +++ b/arch/s390/configs/defconfig @@ -776,7 +776,6 @@ CONFIG_DEBUG_INFO=y CONFIG_DEBUG_INFO_DWARF4=y CONFIG_DEBUG_INFO_BTF=y CONFIG_GDB_SCRIPTS=y -CONFIG_DEBUG_SECTION_MISMATCH=y CONFIG_MAGIC_SYSRQ=y CONFIG_DEBUG_WX=y CONFIG_PTDUMP_DEBUGFS=y diff --git a/kernel/configs/debug.config b/kernel/configs/debug.config index e8db8d938661..0c1a5d64febb 100644 --- a/kernel/configs/debug.config +++ b/kernel/configs/debug.config @@ -18,7 +18,6 @@ CONFIG_SYMBOLIC_ERRNAME=y # CONFIG_DEBUG_INFO=y CONFIG_DEBUG_INFO_DWARF_TOOLCHAIN_DEFAULT=y -CONFIG_DEBUG_SECTION_MISMATCH=y CONFIG_FRAME_WARN=2048 CONFIG_SECTION_MISMATCH_WARN_ONLY=y # diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug index 075cd25363ac..e52f851e9e3b 100644 --- a/lib/Kconfig.debug +++ b/lib/Kconfig.debug @@ -425,28 +425,6 @@ config HEADERS_INSTALL user-space program samples. It is also needed by some features such as uapi header sanity checks. -config DEBUG_SECTION_MISMATCH - bool "Enable full Section mismatch analysis" - depends on CC_IS_GCC - help - The section mismatch analysis checks if there are illegal - references from one section to another section. - During linktime or runtime, some sections are dropped; - any use of code/data previously in these sections would - most likely result in an oops. - In the code, functions and variables are annotated with - __init,, etc. (see the full list in include/linux/init.h), - which results in the code/data being placed in specific sections. - The section mismatch analysis is always performed after a full - kernel build, and enabling this option causes the following - additional step to occur: - - Add the option -fno-inline-functions-called-once to gcc commands. - When inlining a function annotated with __init in a non-init - function, we would lose the section information and thus - the analysis would not catch the illegal reference. - This option tells gcc to inline less (but it does result in - a larger kernel). - config SECTION_MISMATCH_WARN_ONLY bool "Make section mismatch errors non-fatal" default y diff --git a/tools/testing/selftests/wireguard/qemu/debug.config b/tools/testing/selftests/wireguard/qemu/debug.config index 2b321b8a96cf..e737ce3b324e 100644 --- a/tools/testing/selftests/wireguard/qemu/debug.config +++ b/tools/testing/selftests/wireguard/qemu/debug.config @@ -57,7 +57,6 @@ CONFIG_USER_STACKTRACE_SUPPORT=y CONFIG_DEBUG_SG=y CONFIG_DEBUG_NOTIFIERS=y CONFIG_X86_DEBUG_FPU=y -CONFIG_DEBUG_SECTION_MISMATCH=y CONFIG_DEBUG_PAGEALLOC=y CONFIG_DEBUG_PAGEALLOC_ENABLE_DEFAULT=y CONFIG_DEBUG_WW_MUTEX_SLOWPATH=y
Modpost's section mismatch detection warns when a non-init function references an __init function or __initdata. Those warnings are very useful, because __init memory is freed during boot, so the non-init function might end up referencing or calling into some random memory. CONFIG_DEBUG_SECTION_MISMATCH is intended to root out even more of these issues by adding the -fno-inline-functions-called-once compiler flag, which forces once-called static functions to not be inlined. As described in the option's help description: - Add the option -fno-inline-functions-called-once to gcc commands. When inlining a function annotated with __init in a non-init function, we would lose the section information and thus the analysis would not catch the illegal reference. This option tells gcc to inline less (but it does result in a larger kernel). So it's basically a debug (non-production) option, which has the goal of rooting out potential issues which might exist on *other* configs which might somehow trigger different inlining decisions, without having to build all those other configs to see the warnings directly. But with -O2, once-called static functions are almost always inlined, so its usefulness for rooting out mismatch warnings on other configs is somewhere between extremely limited and non-existent. And nowadays we have build bots everywhere doing randconfigs continuously, which are great for rooting out such edge cases. Somewhat ironically, the existence of those build bots means we get a lot of unrealistic objtool warnings being reported, due to unrealistic inlines caused by CONFIG_DEBUG_SECTION_MISMATCH, where the only way to silence the warnings is to force a single-called function to be inlined with '__always_inline'. So the limited, hypothetical benefit of "rooting out configs with section mismatches" is outweighed by the very real downside of "adding lots of unnecessary '__always_inline' annotations". In fact I suspect this option has been responsible for dozens of "convert inline to __always_inline" patches over the years. Such patches usually complain about the compiler's inlining decisions being unpredictable. It turns out this config option is the main culprit. So considering the drawbacks of this option significantly outweigh the benefits, especially now in the age of randconfig build bots, remove it. Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com> --- .../media/maintainer-entry-profile.rst | 2 +- Makefile | 5 ----- arch/arc/configs/tb10x_defconfig | 1 - arch/s390/configs/debug_defconfig | 1 - arch/s390/configs/defconfig | 1 - kernel/configs/debug.config | 1 - lib/Kconfig.debug | 22 ------------------- .../selftests/wireguard/qemu/debug.config | 1 - 8 files changed, 1 insertion(+), 33 deletions(-)