[PATCH v2 0/2] objtool: Add option to fail build on vmlinux warnings

[PATCH v2 0/2] objtool: Add option to fail build on vmlinux warnings

[Brendan Jackman]

This adds an option to objtool to exit with an error when it enounters
warnings.

Then, it adds a config to enable that flag. This enables you to fail
the build e.g. when noinstr is violated.

Signed-off-by: Brendan Jackman <jackmanb@google.com>
---
Changes in v2:
- Renamed flag/config to -Werror/CONFIG_*_WERROR
- Applied to all objool runs instead of just vmlinux.
- Link to v1: https://lore.kernel.org/r/20241213-objtool-strict-v1-0-fd388f9d971f@google.com

---
Brendan Jackman (2):
      objtool: Add --Werror
      kbuild: Add option to fail build on vmlinux objtool issues

 lib/Kconfig.debug                       | 10 ++++++++++
 scripts/Makefile.lib                    |  1 +
 tools/objtool/builtin-check.c           |  6 ++++++
 tools/objtool/check.c                   |  7 ++-----
 tools/objtool/include/objtool/builtin.h |  1 +
 5 files changed, 20 insertions(+), 5 deletions(-)
---
base-commit: f932fb9b40749d1c9a539d89bb3e288c077aafe5
change-id: 20241213-objtool-strict-cb9a0a75139e

Best regards,
-- 
Brendan Jackman <jackmanb@google.com>

[PATCH v2 1/2] objtool: Add --Werror

[Brendan Jackman]

At present objtool only prints to the terminal when observing "fatal
warnings". This option lets you have it produce an error instead.

The use case for this is noinstr validation; so far I've never seen any
false warnings here, but it quite often detects real bugs. It would
be useful for the build to fail when I have those bugs.

Signed-off-by: Brendan Jackman <jackmanb@google.com>
---
 tools/objtool/builtin-check.c           | 6 ++++++
 tools/objtool/check.c                   | 7 ++-----
 tools/objtool/include/objtool/builtin.h | 1 +
 3 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/tools/objtool/builtin-check.c b/tools/objtool/builtin-check.c
index 387d56a7f5fb8da8435d0a3f5c05eeee66932c9b..0b28082df90710ff7127327deb857c0548f378c7 100644
--- a/tools/objtool/builtin-check.c
+++ b/tools/objtool/builtin-check.c
@@ -94,6 +94,12 @@ static const struct option check_options[] = {
 	OPT_BOOLEAN(0, "sec-address", &opts.sec_address, "print section addresses in warnings"),
 	OPT_BOOLEAN(0, "stats", &opts.stats, "print statistics"),
 	OPT_BOOLEAN('v', "verbose", &opts.verbose, "verbose warnings"),
+	/*
+	 *  For now, don't fail the kernel build on fatal warnings by default.
+	 *  These errors are still fairly common due to the growing matrix of
+	 *  supported toolchains and their recent pace of change.
+	 */
+	OPT_BOOLEAN(0, "Werror", &opts.werror, "fail on fatal warnings"),
 
 	OPT_END(),
 };
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 4ce176ad411fb12a10101bbedbb6180275941b4b..7c73517dc11c98cd7163f96dc8f4158389d58428 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -4941,10 +4941,7 @@ int check(struct objtool_file *file)
 	}
 
 out:
-	/*
-	 *  For now, don't fail the kernel build on fatal warnings.  These
-	 *  errors are still fairly common due to the growing matrix of
-	 *  supported toolchains and their recent pace of change.
-	 */
+	if (opts.werror && warnings)
+		return 1;
 	return 0;
 }
diff --git a/tools/objtool/include/objtool/builtin.h b/tools/objtool/include/objtool/builtin.h
index fcca6662c8b4b5e0048e54fada8694cc2e6ebc34..97d668010efadfa05bb6e25e1967a7d72bf77815 100644
--- a/tools/objtool/include/objtool/builtin.h
+++ b/tools/objtool/include/objtool/builtin.h
@@ -38,6 +38,7 @@ struct opts {
 	bool sec_address;
 	bool stats;
 	bool verbose;
+	bool werror;
 };
 
 extern struct opts opts;

-- 
2.47.1.613.gc27f4b7a9f-goog

[PATCH v2 2/2] kbuild: Add option to fail build on vmlinux objtool issues

[Brendan Jackman]

NOINSTR_VALIDATION is pretty helpful for detecting bugs, it would be
helpful for the build to fail when those bugs arise.

If necessary it would be possible to enable this for individual
warnings, it seems unlikely there's a use-case for that though. So
for now just add a global setting.

Signed-off-by: Brendan Jackman <jackmanb@google.com>
---
 lib/Kconfig.debug    | 10 ++++++++++
 scripts/Makefile.lib |  1 +
 2 files changed, 11 insertions(+)

diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index f3d72370587936fa373129cc9b246f15dac907be..3ee92da4733a3a504991d5dbb4d0cee84f519d64 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -545,6 +545,16 @@ config FRAME_POINTER
 config OBJTOOL
 	bool
 
+config OBJTOOL_WERROR
+	bool "Run objtool with warnings as errors"
+	default n
+	depends on OBJTOOL
+	help
+	  Fail the build when objtool produces warnings.
+
+	  By default, objtool just prints warnings to the terminal without
+	  causing a build failure. This config changes that.
+
 config STACK_VALIDATION
 	bool "Compile-time stack metadata validation"
 	depends on HAVE_STACK_VALIDATION && UNWINDER_FRAME_POINTER
diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
index 7395200538da89a2f6e6d21f8959f3f60d291d79..a53e052ae0532e886fcb2019025cf7216e484bd2 100644
--- a/scripts/Makefile.lib
+++ b/scripts/Makefile.lib
@@ -277,6 +277,7 @@ objtool-args-$(CONFIG_HAVE_STATIC_CALL_INLINE)		+= --static-call
 objtool-args-$(CONFIG_HAVE_UACCESS_VALIDATION)		+= --uaccess
 objtool-args-$(CONFIG_GCOV_KERNEL)			+= --no-unreachable
 objtool-args-$(CONFIG_PREFIX_SYMBOLS)			+= --prefix=$(CONFIG_FUNCTION_PADDING_BYTES)
+objtool-args-$(CONFIG_OBJTOOL_WERROR)			+= --Werror
 
 objtool-args = $(objtool-args-y)					\
 	$(if $(delay-objtool), --link)					\

-- 
2.47.1.613.gc27f4b7a9f-goog

Re: [PATCH v2 2/2] kbuild: Add option to fail build on vmlinux objtool issues

[Josh Poimboeuf]

On Wed, Dec 18, 2024 at 02:58:56PM +0000, Brendan Jackman wrote:
> NOINSTR_VALIDATION is pretty helpful for detecting bugs, it would be
> helpful for the build to fail when those bugs arise.
> 
> If necessary it would be possible to enable this for individual
> warnings, it seems unlikely there's a use-case for that though. So
> for now just add a global setting.
> 
> Signed-off-by: Brendan Jackman <jackmanb@google.com>

The patch looks good to me, though the subject and commit log still need
to be updated.  It's no longer specific to vmlinux or
NOINSTR_VALIDATION.  And the justification is similar to what I
mentioned before: objtool warnings (even seemingly "harmless" ones)
often indicate serious bugs which can break the kernel.

-- 
Josh

Re: [PATCH v2 0/2] objtool: Add option to fail build on vmlinux warnings

[Andrew Morton]

On Wed, 18 Dec 2024 14:58:54 +0000 Brendan Jackman <jackmanb@google.com> wrote:

> This adds an option to objtool to exit with an error when it enounters
> warnings.

Why is it optional?  Can we always fail the build so stuff gets fixed?

Re: [PATCH v2 0/2] objtool: Add option to fail build on vmlinux warnings

[Josh Poimboeuf]

On Wed, Dec 18, 2024 at 04:06:56PM -0800, Andrew Morton wrote:
> On Wed, 18 Dec 2024 14:58:54 +0000 Brendan Jackman <jackmanb@google.com> wrote:
> 
> > This adds an option to objtool to exit with an error when it enounters
> > warnings.
> 
> Why is it optional?  Can we always fail the build so stuff gets fixed?

Eventually, yes.  But right now, I'm not so sure...

it will likely break a lot of robot builds, there are some obscure
randconfigs out there triggering some "interesting" edge cases, e.g.:

  - ftrace branch profiling which is fundamentally incompatible with
    uaccess STAC/CLAC rules

  - similar weirdnesses with UBSAN/KASAN/etc and other compiler
    features/plugins

  - obscure toolchain bugs in certain compiler versions which do weird
    things with control flow.  some of these bugs break the kernel, some
    don't.

Problem is, it usually falls on the objtool maintainers to figure out
the root of the problem and the resolution, neither of which is
necessarily straightforward, especially the latter.  There's only two of
us maintainers at the moment, with limited bandwidth.

So yes, it *should* always fail the build.  But unless we get more
maintainer bandwidth, I don't think we're ready for that.

We might end up being able to make CONFIG_OBJTOOL_WERROR=y the default,
and then just require broken features to depend on
CONFIG_OBJTOOL_WERROR=n.  And then print a big fat warning message at
build and/or runtime in the case of warnings.

We also might need to add some features, like a way to mark certain
compiler versions as bad, or a way to silence objtool warnings for
certain known harmless cases, or improve the specificity and usefulness
of certain vague warnings.

But as a first step I'll planning on just throwing these patches on a
robot-monitored branch with CONFIG_OBJTOOL_WERROR=y over the holidays to
see how bad the damage is.

-- 
Josh

Re: [PATCH v2 2/2] kbuild: Add option to fail build on vmlinux objtool issues

[Brendan Jackman]

On Wed, 18 Dec 2024 at 20:04, Josh Poimboeuf <jpoimboe@kernel.org> wrote:
> The patch looks good to me, though the subject and commit log still need
> to be updated.

Agh, sorry. I'll wait a couple of days before posting the update to
avoid spamming in case anyone wants to weigh in on the discussion.

Re: [PATCH v2 0/2] objtool: Add option to fail build on vmlinux warnings

[Nathan Chancellor]

Hi Josh,

On Wed, Dec 18, 2024 at 05:00:54PM -0800, Josh Poimboeuf wrote:
...
>   - obscure toolchain bugs in certain compiler versions which do weird
>     things with control flow.  some of these bugs break the kernel, some
>     don't.
> 
> Problem is, it usually falls on the objtool maintainers to figure out
> the root of the problem and the resolution, neither of which is
> necessarily straightforward, especially the latter.  There's only two of
> us maintainers at the moment, with limited bandwidth.
> 
> So yes, it *should* always fail the build.  But unless we get more
> maintainer bandwidth, I don't think we're ready for that.
> 
> We might end up being able to make CONFIG_OBJTOOL_WERROR=y the default,
> and then just require broken features to depend on
> CONFIG_OBJTOOL_WERROR=n.  And then print a big fat warning message at
> build and/or runtime in the case of warnings.
> 
> We also might need to add some features, like a way to mark certain
> compiler versions as bad, or a way to silence objtool warnings for
> certain known harmless cases, or improve the specificity and usefulness
> of certain vague warnings.
> 
> But as a first step I'll planning on just throwing these patches on a
> robot-monitored branch with CONFIG_OBJTOOL_WERROR=y over the holidays to
> see how bad the damage is.

For the record, I plan to monitor these reports for LLVM and try to
investigate and triage all other known objtool warnings for LLVM after
the holidays to try and prepare for this. I felt blind sided by the
compiler -Werror change so I'd rather not go through that again :) one
reason I would like to be objtool clean is to catch changed compiler
behavior quicker, as I tend to notice it is easier to get problems
addressed when the problem is reported as close as possible to the
original change.

I do agree with you that figuring our the root problem and resolution to
some of these warnings is not always the easiest, especially when they
are on the toolchain side, so I have often kicked the can down the road.
I know there is some documentation in objtool.txt around various
warnings, is that pretty up to date/accurate? Are there any other
resources I could look at to help with this work? I know Arnd just
recently fixed a set [1] that I saw in our builds as well due to a bare
unreachable(), which I think tend to hurt Clang more than GCC but maybe
I am imagining things there.

Some objtool reports get sent to only llvm@lists.linux.dev when clang is
involved (due to a historical filter IIRC, I cannot find the original
request), so you may want to glance at [2] to see if anything new pops
up.

[1]: https://git.kernel.org/netdev/net/c/cff865c700711ecc3824b2dfe181637f3ed23c80
[2]: https://lore.kernel.org/llvm/?q=objtool+f:lkp@intel.com

Cheers,
Nathan

Re: [PATCH v2 0/2] objtool: Add option to fail build on vmlinux warnings

[Josh Poimboeuf]

On Thu, Dec 19, 2024 at 03:19:13PM -0700, Nathan Chancellor wrote:
> I do agree with you that figuring our the root problem and resolution to
> some of these warnings is not always the easiest, especially when they
> are on the toolchain side, so I have often kicked the can down the road.
> I know there is some documentation in objtool.txt around various
> warnings, is that pretty up to date/accurate? Are there any other
> resources I could look at to help with this work?

I think the document is pretty up to date.  Some warnings are self
explanatory but others like "unreachable instruction" or "stack state
mismatch" do require digging.

One thing that can help is to "export OBJTOOL_VERBOSE=1", which will
tell objtool to disassemble any affected functions and show a backtrace
with all the taken branches leading up to the warning (if applicable).
Maybe that should be the default for --Werror.

I'd definitely like more people to be able to debug objtool warnings.
Any ideas on making that easier or educating people or improving
warnings are very welcome.  I'll be keeping that in mind when looking at
the build errors over the holidays.

> Some objtool reports get sent to only llvm@lists.linux.dev when clang is
> involved (due to a historical filter IIRC, I cannot find the original
> request), so you may want to glance at [2] to see if anything new pops
> up.

We need to figure out how to get that fixed, the commit author really
needs to know if their code causes a warning/error.

-- 
Josh

Re: [PATCH v2 0/2] objtool: Add option to fail build on vmlinux warnings

[Nick Desaulniers]

On Thu, Dec 19, 2024 at 2:19 PM Nathan Chancellor <nathan@kernel.org> wrote:
>
> Some objtool reports get sent to only llvm@lists.linux.dev when clang is
> involved (due to a historical filter IIRC, I cannot find the original
> request),

https://lore.kernel.org/all/CAKwvOdmJvWpmbP3GyzaZxyiuwooFXA8D7ui05QE7+f8Oaz+rXg@mail.gmail.com/
perhaps?
-- 
Thanks,
~Nick Desaulniers

Re: [PATCH v2 0/2] objtool: Add option to fail build on vmlinux warnings

[Brendan Jackman]

On Thu, 19 Dec 2024 at 22:56, Josh Poimboeuf <jpoimboe@kernel.org> wrote:
> One thing that can help is to "export OBJTOOL_VERBOSE=1", which will
> tell objtool to disassemble any affected functions and show a backtrace
> with all the taken branches leading up to the warning (if applicable).
> Maybe that should be the default for --Werror.

This sounds very useful, I will do this for v3 unless anyone objects.
If we're failing the build the risk of a mega-verbose splat seems like
a good tradeoff vs the risk of the user having to go and read the
objtool code.

Re: [PATCH v2 0/2] objtool: Add option to fail build on vmlinux warnings

[Nathan Chancellor]

On Thu, Dec 19, 2024 at 02:56:42PM -0800, Josh Poimboeuf wrote:
> On Thu, Dec 19, 2024 at 03:19:13PM -0700, Nathan Chancellor wrote:
> > I do agree with you that figuring our the root problem and resolution to
> > some of these warnings is not always the easiest, especially when they
> > are on the toolchain side, so I have often kicked the can down the road.
> > I know there is some documentation in objtool.txt around various
> > warnings, is that pretty up to date/accurate? Are there any other
> > resources I could look at to help with this work?
> 
> I think the document is pretty up to date.  Some warnings are self
> explanatory but others like "unreachable instruction" or "stack state
> mismatch" do require digging.
> 
> One thing that can help is to "export OBJTOOL_VERBOSE=1", which will
> tell objtool to disassemble any affected functions and show a backtrace
> with all the taken branches leading up to the warning (if applicable).
> Maybe that should be the default for --Werror.

Yeah, that does not sound like a bad idea. If the build is going to
break, it seems reasonable to give developers as much pertinent
information as possible so they can address the problem properly and
move on.

> I'd definitely like more people to be able to debug objtool warnings.
> Any ideas on making that easier or educating people or improving
> warnings are very welcome.  I'll be keeping that in mind when looking at
> the build errors over the holidays.

I will definitely ponder on that as well since I should have a good
perspective on that front.

> > Some objtool reports get sent to only llvm@lists.linux.dev when clang is
> > involved (due to a historical filter IIRC, I cannot find the original
> > request), so you may want to glance at [2] to see if anything new pops
> > up.
> 
> We need to figure out how to get that fixed, the commit author really
> needs to know if their code causes a warning/error.

I have started a new thread with the 0day folks to get that adjusted
with you and Peter on CC:

https://lore.kernel.org/20241220200617.GA936171@ax162/

Cheers,
Nathan