(+CC: Changbin Du, Arnd Bergmann, who were working on the -Og builds)
On Sat, Apr 9, 2022 at 1:23 AM Josh Poimboeuf <[email protected]> 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 <[email protected]>
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/[email protected]/
[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
>
--
Best Regards
Masahiro Yamada
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.
--
Josh
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 <[email protected]> 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/
--
Thanks,
~Nick Desaulniers
On Fri, Apr 8, 2022 at 5:48 PM Josh Poimboeuf <[email protected]> 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/[email protected]/
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
>
--
Thanks,
~Nick Desaulniers
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/[email protected]/
> 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.
--
Josh