[PATCH RFC] kbuild: Prevent compiler mismatch with external modules

[PATCH RFC] kbuild: Prevent compiler mismatch with external modules

[Josh Poimboeuf]

When building an external module, if the compiler version differs from
what the kernel was built with, bad things can happen.  Many kernel
features change based on available compiler features.  Silently removing
a compiler-dependent feature in the external module build can cause
unpredictable behavior.  Right now there are no checks to help prevent
such mismatches.

On the other hand, when a user is building an external module against a
distro kernel, the exact compiler version may not be installed, or in
some cases not even released yet.  In fact it's quite common for
external modules to be built with a slightly different version of GCC
than the kernel.

A minor version mismatch should be ok.  User space does it all the time.
New compiler features aren't added within a major version.

Add a check for compiler mismatch, but only check the major version.

Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
This is related to the previous RFC I posted:

  https://lkml.kernel.org/r/efe6b039a544da8215d5e54aa7c4b6d1986fc2b0.1611607264.git.jpoimboe@redhat.com

The discussion revealed gaps between developer perceptions and distro
realities with respect to external (out-of-tree) modules...

Backing up a bit, let's please decide on what exactly is supported (or
not supported) with respect to mismatched compiler versions.  Then let's
try to enforce and/or document the decision.

Please stick to technical arguments...


 Makefile | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Makefile b/Makefile
index b0e4767735dc..f281d2587fa5 100644
--- a/Makefile
+++ b/Makefile
@@ -1744,6 +1744,14 @@ help:
 # no-op for external module builds
 PHONY += prepare modules_prepare
 
+# External module compiler (major) version must match the kernel
+ifneq ($(shell echo $(CONFIG_GCC_VERSION) | cut -c-2), $(shell $(srctree)/scripts/gcc-version.sh $(CC) | cut -c-2))
+  $(error ERROR: Compiler version mismatch in external module build)
+endif
+ifneq ($(shell echo $(CONFIG_CLANG_VERSION) | cut -c-2), $(shell $(srctree)/scripts/clang-version.sh $(CC) | cut -c-2))
+  $(error ERROR: Compiler version mismatch in external module build)
+endif
+
 endif # KBUILD_EXTMOD
 
 # Single targets
-- 
2.29.2

Re: [PATCH RFC] kbuild: Prevent compiler mismatch with external modules

[Linus Torvalds]

On Thu, Jan 28, 2021 at 12:08 PM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>
> Add a check for compiler mismatch, but only check the major version.

I think this is wrong for multiple reasons.

The most fundamental reason is that it's pointless and doesn't
actually do what you claim it does.

Just doing a "make oldconfig" will reset the CONFIG_xyz_VERSION to
whatever is installed, and now your check doesn't actually do
anything, since you're not actually checking what the kernel was
compiled with!

So I think that check is pointless and entirely misleading. It doesn't
do what you want it to do, and what you claim it does.

I'm not convinced about the whole magic vs minor argument either. The
whole "new compiler features" thing is a red herring - even if you do
have new compiler features, that in itself has very little to do with
whether the resulting object files are compatible or not.

So I say NAK, on the basis that the patch is nonsensical, tests the
wrong thing, and doesn't really have a technical reason for it.

             Linus

Re: [PATCH RFC] kbuild: Prevent compiler mismatch with external modules

[Josh Poimboeuf]

On Thu, Jan 28, 2021 at 12:24:45PM -0800, Linus Torvalds wrote:
> On Thu, Jan 28, 2021 at 12:08 PM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> >
> > Add a check for compiler mismatch, but only check the major version.
> 
> I think this is wrong for multiple reasons.
> 
> The most fundamental reason is that it's pointless and doesn't
> actually do what you claim it does.
> 
> Just doing a "make oldconfig" will reset the CONFIG_xyz_VERSION to
> whatever is installed, and now your check doesn't actually do
> anything, since you're not actually checking what the kernel was
> compiled with!

Huh?  Why would you do a "make oldconfig" on a distro-released kernel
before building an OOT module?

Usually you build an OOT module against /lib/modules/$(uname -r)/build,
and the .config isn't even writable.

> So I think that check is pointless and entirely misleading. It doesn't
> do what you want it to do, and what you claim it does.

It's not a magic bullet, but doesn't it catch the vast majority of use
cases?  Which makes it a lot better than what we have now (nothing).

> I'm not convinced about the whole magic vs minor argument either. The
> whole "new compiler features" thing is a red herring - even if you do
> have new compiler features, that in itself has very little to do with
> whether the resulting object files are compatible or not.

Hm?  Are you saying the check is too strict, since GCC9 binaries _might_
be compatible with GCC10?

-- 
Josh

Re: [PATCH RFC] kbuild: Prevent compiler mismatch with external modules

[Linus Torvalds]

On Thu, Jan 28, 2021 at 12:52 PM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>
> Huh?  Why would you do a "make oldconfig" on a distro-released kernel
> before building an OOT module?

I guarantee you that this patch will *make* people do that.

> Hm?  Are you saying the check is too strict, since GCC9 binaries _might_
> be compatible with GCC10?

I'm saying that your argument about minor and major versions is bogus.

There is absolutely nothing that makes gcc9 object files not
compatible with gcc10.

And this is not just some theoretical issue: this is a fundamental
fact of EVERY SINGLE LIBRARY OUT THERE.

Do you think that when you compile your binaries with gcc-10, you need
to link against a standard library that has been compiled with gcc-10?

I really think the whole compiler version check is purely voodoo programming.

                 Linus

Re: [PATCH RFC] kbuild: Prevent compiler mismatch with external modules

[Linus Torvalds]

On Thu, Jan 28, 2021 at 1:03 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> I really think the whole compiler version check is purely voodoo programming.

.. but there are obviously potentially things we - in the kernel - do
that may make certain compiler versions incompatible. We long long ago
used to have things like "you can't have an empty struct because gcc
version x.y.z doesn't support it", so even a UP spinlock would be

       typedef struct { int gcc_is_buggy; } raw_spinlock_t;

but only if you compiled it with a version of gcc older than 3.0. So
compiling one file with one compiler, and another with a newer one,
would result in the data structures simply not having the same layout.

That's not because of compiler versions per se, it's because of our
version checks.

THAT workaround is long gone, but I didn't check what other ones we
might have now. But the gcc version checks we _do_ have are not
necessarily about major versions at all (ie I trivially found checks
for 4.9, 4.9.2, 5.1, 7.2 and 9.1).

               Linus

Re: [PATCH RFC] kbuild: Prevent compiler mismatch with external modules

[Josh Poimboeuf]

On Thu, Jan 28, 2021 at 01:23:11PM -0800, Linus Torvalds wrote:
> On Thu, Jan 28, 2021 at 1:03 PM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > I really think the whole compiler version check is purely voodoo programming.
> 
> .. but there are obviously potentially things we - in the kernel - do
> that may make certain compiler versions incompatible. We long long ago
> used to have things like "you can't have an empty struct because gcc
> version x.y.z doesn't support it", so even a UP spinlock would be
> 
>        typedef struct { int gcc_is_buggy; } raw_spinlock_t;
> 
> but only if you compiled it with a version of gcc older than 3.0. So
> compiling one file with one compiler, and another with a newer one,
> would result in the data structures simply not having the same layout.
> 
> That's not because of compiler versions per se, it's because of our
> version checks.

Right, this is what I'm trying to say.  We have features based on
compiler version checks.  Peterz pointed out asm goto as a previous
example.

> THAT workaround is long gone, but I didn't check what other ones we
> might have now. But the gcc version checks we _do_ have are not
> necessarily about major versions at all (ie I trivially found checks
> for 4.9, 4.9.2, 5.1, 7.2 and 9.1).

Then maybe the check should be same major.minor?

And convert it to a strongly worded warning/disclaimer?

-- 
Josh

Re: [PATCH RFC] kbuild: Prevent compiler mismatch with external modules

[Linus Torvalds]

On Thu, Jan 28, 2021 at 1:34 PM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>
> On Thu, Jan 28, 2021 at 01:23:11PM -0800, Linus Torvalds wrote:
> > THAT workaround is long gone, but I didn't check what other ones we
> > might have now. But the gcc version checks we _do_ have are not
> > necessarily about major versions at all (ie I trivially found checks
> > for 4.9, 4.9.2, 5.1, 7.2 and 9.1).
>
> Then maybe the check should be same major.minor?

Well, how many of them are actually about things that generate
incompatible object code?

The main one I can think of is the KASAN ABI version checks, but
honestly, I think that's irrelevant. I really hope no distros enable
KASAN in user kernels.

Another version check I looked at was the one that just checks whether
the compiler natively supports __builtin_mul_overflow() or not - it
doesn't generate incompatible object code, it just takes advantage of
a compiler feature if one exists. You can mix and match those kinds of
things well enough.

So I'd really like to hear actual hard technical reasons with
examples, for why you'd want to add this test in the first place.

No hand-waving "different compiler versions don't work together".
Because that's simply not true.

> And convert it to a strongly worded warning/disclaimer?

A warning might be better for the simple reason that it wouldn't cause
people to just fix it with "make oldconfig".

Maybe you haven't looked at people who compile external modules, but
they always have various "this is how to work around issues with
version XYZ". That "make oldconfig" would simply just become the
workaround for any build errors.

And a warning might be more palatable even if different compiler
version work fine together. Just a heads up on "it looks like you
might be mixing compiler versions" is a valid note, and isn't
necessarily wrong. Even when they work well together, maybe you want
to have people at least _aware_ of it.

              Linus

Re: [PATCH RFC] kbuild: Prevent compiler mismatch with external modules

[Josh Poimboeuf]

On Thu, Jan 28, 2021 at 01:45:51PM -0800, Linus Torvalds wrote:
> On Thu, Jan 28, 2021 at 1:34 PM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> >
> > On Thu, Jan 28, 2021 at 01:23:11PM -0800, Linus Torvalds wrote:
> > > THAT workaround is long gone, but I didn't check what other ones we
> > > might have now. But the gcc version checks we _do_ have are not
> > > necessarily about major versions at all (ie I trivially found checks
> > > for 4.9, 4.9.2, 5.1, 7.2 and 9.1).
> >
> > Then maybe the check should be same major.minor?
> 
> Well, how many of them are actually about things that generate
> incompatible object code?
> 
> The main one I can think of is the KASAN ABI version checks, but
> honestly, I think that's irrelevant. I really hope no distros enable
> KASAN in user kernels.
> 
> Another version check I looked at was the one that just checks whether
> the compiler natively supports __builtin_mul_overflow() or not - it
> doesn't generate incompatible object code, it just takes advantage of
> a compiler feature if one exists. You can mix and match those kinds of
> things well enough.
> 
> So I'd really like to hear actual hard technical reasons with
> examples, for why you'd want to add this test in the first place.

Unfortunately I don't have technical reasons beyond what we've already
discussed, found from code inspection.

This patch was born from a discussion where wildly different opinions
were expressed about whether such a mismatch scenario (or even external
modules in general!) would be supported at all.

So I guess the goal is to clarify (in the code base) to what extent
compiler mismatches are supported/allowed/encouraged.  Because they
definitely happen in the real world, but a lot of people seem to be
sticking their head in the sand about it.

If we decide it's not a cut-and-dry makefile check, then the policy
should at least be documented.

I'd prefer the warning though, since nobody's going to read the docs.

> No hand-waving "different compiler versions don't work together".
> Because that's simply not true.
> 
> > And convert it to a strongly worded warning/disclaimer?
> 
> A warning might be better for the simple reason that it wouldn't cause
> people to just fix it with "make oldconfig".
> 
> Maybe you haven't looked at people who compile external modules, but
> they always have various "this is how to work around issues with
> version XYZ". That "make oldconfig" would simply just become the
> workaround for any build errors.
> 
> And a warning might be more palatable even if different compiler
> version work fine together. Just a heads up on "it looks like you
> might be mixing compiler versions" is a valid note, and isn't
> necessarily wrong. Even when they work well together, maybe you want
> to have people at least _aware_ of it.

Sounds reasonable.

-- 
Josh

Re: [PATCH RFC] kbuild: Prevent compiler mismatch with external modules

[Masahiro Yamada]

On Fri, Jan 29, 2021 at 7:08 AM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>
> On Thu, Jan 28, 2021 at 01:45:51PM -0800, Linus Torvalds wrote:
> > On Thu, Jan 28, 2021 at 1:34 PM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > >
> > > On Thu, Jan 28, 2021 at 01:23:11PM -0800, Linus Torvalds wrote:
> > > > THAT workaround is long gone, but I didn't check what other ones we
> > > > might have now. But the gcc version checks we _do_ have are not
> > > > necessarily about major versions at all (ie I trivially found checks
> > > > for 4.9, 4.9.2, 5.1, 7.2 and 9.1).
> > >
> > > Then maybe the check should be same major.minor?
> >
> > Well, how many of them are actually about things that generate
> > incompatible object code?
> >
> > The main one I can think of is the KASAN ABI version checks, but
> > honestly, I think that's irrelevant. I really hope no distros enable
> > KASAN in user kernels.
> >
> > Another version check I looked at was the one that just checks whether
> > the compiler natively supports __builtin_mul_overflow() or not - it
> > doesn't generate incompatible object code, it just takes advantage of
> > a compiler feature if one exists. You can mix and match those kinds of
> > things well enough.
> >
> > So I'd really like to hear actual hard technical reasons with
> > examples, for why you'd want to add this test in the first place.
>
> Unfortunately I don't have technical reasons beyond what we've already
> discussed, found from code inspection.
>
> This patch was born from a discussion where wildly different opinions
> were expressed about whether such a mismatch scenario (or even external
> modules in general!) would be supported at all.
>
> So I guess the goal is to clarify (in the code base) to what extent
> compiler mismatches are supported/allowed/encouraged.  Because they
> definitely happen in the real world, but a lot of people seem to be
> sticking their head in the sand about it.
>
> If we decide it's not a cut-and-dry makefile check, then the policy
> should at least be documented.
>
> I'd prefer the warning though, since nobody's going to read the docs.
>
> > No hand-waving "different compiler versions don't work together".
> > Because that's simply not true.
> >
> > > And convert it to a strongly worded warning/disclaimer?
> >
> > A warning might be better for the simple reason that it wouldn't cause
> > people to just fix it with "make oldconfig".
> >
> > Maybe you haven't looked at people who compile external modules, but
> > they always have various "this is how to work around issues with
> > version XYZ". That "make oldconfig" would simply just become the
> > workaround for any build errors.
> >
> > And a warning might be more palatable even if different compiler
> > version work fine together. Just a heads up on "it looks like you
> > might be mixing compiler versions" is a valid note, and isn't
> > necessarily wrong. Even when they work well together, maybe you want
> > to have people at least _aware_ of it.
>
> Sounds reasonable.
>
> --
> Josh
>

[1]

First, let me explain how Kbuild works w.r.t the compiler version
check.

When working on the kernel tree, Kbuild automatically detects
the compiler upgrade (this is done by comparing the output
of '$(CC) --version'), and invokes Kconfig to sync the configuration.
So, the .config is updated even if you do not explicitly
do "make oldconfig".


When working on external modules, in contrast,
Kbuild does not attempt to update anything in the kernel tree.
This makes sense since the build tree, /lib/modules/$(uname -r)/build/
is read-only.
You cannot manually run Kconfig either because the config targets
are hidden for external modules.

$ make M=../qemu-build/extmod  oldconfig
make: *** No rule to make target 'oldconfig'.  Stop.



[2]

As for this patch, it is wrong to do this check in the Makefile
parse stage.

"make M=...  clean"
"make M=...  help"

etc. will fail.
Such targets do not require the compiler in the first place.

This check must be done before starting building something,


Also, this patch is not applicable.
gcc-version.sh and clang-version.sh do not exist.
See linux-next.



[3]
Peterz already pointed out asm-goto as an example of ABI mismatch.

I remember a trouble reported in the past due
to the mismatch of -mstack-protector-guard-offset.

https://bugzilla.kernel.org/show_bug.cgi?id=201891

This has already been fixed,
and it will no longer happen though.





--
Best Regards




Masahiro Yamada

Re: [PATCH RFC] kbuild: Prevent compiler mismatch with external modules

[Josh Poimboeuf]

On Fri, Jan 29, 2021 at 08:17:51AM +0900, Masahiro Yamada wrote:
> [3]
> Peterz already pointed out asm-goto as an example of ABI mismatch.
> 
> I remember a trouble reported in the past due
> to the mismatch of -mstack-protector-guard-offset.
> 
> https://bugzilla.kernel.org/show_bug.cgi?id=201891
> 
> This has already been fixed,
> and it will no longer happen though.

This is kind of concerning though.  It would be nice to somehow store
KCLAGS in the config and warn if it changes unexpectedly.

This can be a problem not only for OOT modules, but for regular kernel
builds which have a .config copied from somewhere.

Because of the toolchain-dependent kconfig options, features can
silently disappear if the toolchain doesn't support them, due to a
different compiler version, or even a missing library.

> [2]
> 
> As for this patch, it is wrong to do this check in the Makefile
> parse stage.
> 
> "make M=...  clean"
> "make M=...  help"
> 
> etc. will fail.
> Such targets do not require the compiler in the first place.
> 
> This check must be done before starting building something,
>
> Also, this patch is not applicable.
> gcc-version.sh and clang-version.sh do not exist.
> See linux-next.

Something like so?

diff --git a/Makefile b/Makefile
index 95ab9856f357..10ca621369fb 100644
--- a/Makefile
+++ b/Makefile
@@ -1721,12 +1721,25 @@ KBUILD_MODULES := 1
 
 build-dirs := $(KBUILD_EXTMOD)
 PHONY += modules
-modules: $(MODORDER)
+modules: ext_compiler_check $(MODORDER)
 	$(Q)$(MAKE) -f $(srctree)/scripts/Makefile.modpost
 
 $(MODORDER): descend
 	@:
 
+orig_name   := $(if $(CONFIG_CC_IS_GCC),GCC,CLANG)
+orig_minor  := $(shell expr $(if $(CONFIG_CC_IS_GCC),$(CONFIG_GCC_VERSION),$(CONFIG_CLANG_VERSION)) / 100)
+cur_namever := $(shell $(srctree)/scripts/cc-version.sh $(CC))
+cur_name    := $(word 1,$(cur_namever))
+cur_minor   := $(shell expr $(word 2,$(cur_namever)) / 100)
+PHONY += ext_compiler_check
+ext_compiler_check:
+	@if [ $(orig_name) != $(cur_name) ] || [ $(orig_minor) != $(cur_minor) ]; then \
+		echo >&2 "warning: The compiler differs from the version which was used to build the kernel."; \
+		echo >&2 "warning: Some kernel features are compiler-dependent."; \
+		echo >&2 "warning: It's recommended that you change your compiler to match the version in the .config file."; \
+	fi
+
 PHONY += modules_install
 modules_install: _emodinst_ _emodinst_post
 

Re: [PATCH RFC] kbuild: Prevent compiler mismatch with external modules

[Masahiro Yamada]

[-- Attachment #1: Type: text/plain, Size: 3146 bytes --]

On Tue, Feb 2, 2021 at 6:13 AM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>
> On Fri, Jan 29, 2021 at 08:17:51AM +0900, Masahiro Yamada wrote:
> > [3]
> > Peterz already pointed out asm-goto as an example of ABI mismatch.
> >
> > I remember a trouble reported in the past due
> > to the mismatch of -mstack-protector-guard-offset.
> >
> > https://bugzilla.kernel.org/show_bug.cgi?id=201891
> >
> > This has already been fixed,
> > and it will no longer happen though.
>
> This is kind of concerning though.  It would be nice to somehow store
> KCLAGS in the config and warn if it changes unexpectedly.
>
> This can be a problem not only for OOT modules, but for regular kernel
> builds which have a .config copied from somewhere.
>
> Because of the toolchain-dependent kconfig options, features can
> silently disappear if the toolchain doesn't support them, due to a
> different compiler version, or even a missing library.
>
> > [2]
> >
> > As for this patch, it is wrong to do this check in the Makefile
> > parse stage.
> >
> > "make M=...  clean"
> > "make M=...  help"
> >
> > etc. will fail.
> > Such targets do not require the compiler in the first place.
> >
> > This check must be done before starting building something,
> >
> > Also, this patch is not applicable.
> > gcc-version.sh and clang-version.sh do not exist.
> > See linux-next.
>
> Something like so?

No, I do not think so.


>
> diff --git a/Makefile b/Makefile
> index 95ab9856f357..10ca621369fb 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1721,12 +1721,25 @@ KBUILD_MODULES := 1
>
>  build-dirs := $(KBUILD_EXTMOD)
>  PHONY += modules
> -modules: $(MODORDER)
> +modules: ext_compiler_check $(MODORDER)


For the single thread build, yes
GNU Make will run the targets from left to right,
so ext_compiler_check is run before compiling
any build artifact.

If -j <N> option is given, there is no guarantee
that ext_compiler_check finishes first.



>         $(Q)$(MAKE) -f $(srctree)/scripts/Makefile.modpost
>
>  $(MODORDER): descend
>         @:
>
> +orig_name   := $(if $(CONFIG_CC_IS_GCC),GCC,CLANG)
> +orig_minor  := $(shell expr $(if $(CONFIG_CC_IS_GCC),$(CONFIG_GCC_VERSION),$(CONFIG_CLANG_VERSION)) / 100)
> +cur_namever := $(shell $(srctree)/scripts/cc-version.sh $(CC))
> +cur_name    := $(word 1,$(cur_namever))
> +cur_minor   := $(shell expr $(word 2,$(cur_namever)) / 100)

These are still calculated by 'make M=... clean' or 'make M=... help'.
Using '=' assignment solves it, but the code is still ugly.


I attached my alternative implementation.


> +PHONY += ext_compiler_check
> +ext_compiler_check:
> +       @if [ $(orig_name) != $(cur_name) ] || [ $(orig_minor) != $(cur_minor) ]; then \
> +               echo >&2 "warning: The compiler differs from the version which was used to build the kernel."; \
> +               echo >&2 "warning: Some kernel features are compiler-dependent."; \
> +               echo >&2 "warning: It's recommended that you change your compiler to match the version in the .config file."; \
> +       fi
> +
>  PHONY += modules_install
>  modules_install: _emodinst_ _emodinst_post
>
>

--
Best Regards
Masahiro Yamada

[-- Attachment #2: 0001-kbuild-show-notice-if-a-different-compiler-is-used-f.patch --]
[-- Type: application/x-patch, Size: 1799 bytes --]

Re: [PATCH RFC] kbuild: Prevent compiler mismatch with external modules

[Josh Poimboeuf]

On Sat, Mar 06, 2021 at 01:28:22AM +0900, Masahiro Yamada wrote:
> > +orig_name   := $(if $(CONFIG_CC_IS_GCC),GCC,CLANG)
> > +orig_minor  := $(shell expr $(if $(CONFIG_CC_IS_GCC),$(CONFIG_GCC_VERSION),$(CONFIG_CLANG_VERSION)) / 100)
> > +cur_namever := $(shell $(srctree)/scripts/cc-version.sh $(CC))
> > +cur_name    := $(word 1,$(cur_namever))
> > +cur_minor   := $(shell expr $(word 2,$(cur_namever)) / 100)
> 
> These are still calculated by 'make M=... clean' or 'make M=... help'.
> Using '=' assignment solves it, but the code is still ugly.
> 
> 
> I attached my alternative implementation.

Thanks for the attached patch, yours looks much cleaner.  Looks like it
warns on *any* mismatch, rather than just a major.minor mismatch.  But
that's ok with me.

Acked-by: Josh Poimboeuf <jpoimboe@redhat.com>

-- 
Josh