[tip:,objtool/core] jump_label, x86: Allow short NOPs
diff mbox series

Message ID 162082558708.29796.10992563428983424866.tip-bot2@tip-bot2
State Accepted
Commit ab3257042c26d0cd44793c741e2f89bf38b21fe8
Headers show
Series
  • [tip:,objtool/core] jump_label, x86: Allow short NOPs
Related show

Commit Message

irqchip-bot for Andy Shevchenko May 12, 2021, 1:19 p.m. UTC
The following commit has been merged into the objtool/core branch of tip:

Commit-ID:     ab3257042c26d0cd44793c741e2f89bf38b21fe8
Gitweb:        https://git.kernel.org/tip/ab3257042c26d0cd44793c741e2f89bf38b21fe8
Author:        Peter Zijlstra <peterz@infradead.org>
AuthorDate:    Thu, 06 May 2021 21:34:05 +02:00
Committer:     Ingo Molnar <mingo@kernel.org>
CommitterDate: Wed, 12 May 2021 14:54:56 +02:00

jump_label, x86: Allow short NOPs

Now that objtool is able to rewrite jump_label instructions, have the
compiler emit a JMP, such that it can decide on the optimal encoding,
and set jump_entry::key bit1 to indicate that objtool should rewrite
the instruction to a matching NOP.

For x86_64-allyesconfig this gives:

  jl\     NOP     JMP
  short:  22997   124
  long:   30874   90

IOW, we save (22997+124) * 3 bytes of kernel text in hotpaths.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Link: https://lore.kernel.org/r/20210506194158.216763632@infradead.org
---
 arch/x86/include/asm/jump_label.h | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

Comments

Peter Zijlstra May 18, 2021, 7:50 p.m. UTC | #1
On Wed, May 12, 2021 at 01:19:47PM -0000, tip-bot2 for Peter Zijlstra wrote:
> The following commit has been merged into the objtool/core branch of tip:
> 
> Commit-ID:     ab3257042c26d0cd44793c741e2f89bf38b21fe8
> Gitweb:        https://git.kernel.org/tip/ab3257042c26d0cd44793c741e2f89bf38b21fe8
> Author:        Peter Zijlstra <peterz@infradead.org>
> AuthorDate:    Thu, 06 May 2021 21:34:05 +02:00
> Committer:     Ingo Molnar <mingo@kernel.org>
> CommitterDate: Wed, 12 May 2021 14:54:56 +02:00
> 
> jump_label, x86: Allow short NOPs
> 
> Now that objtool is able to rewrite jump_label instructions, have the
> compiler emit a JMP, such that it can decide on the optimal encoding,
> and set jump_entry::key bit1 to indicate that objtool should rewrite
> the instruction to a matching NOP.
> 
> For x86_64-allyesconfig this gives:
> 
>   jl\     NOP     JMP
>   short:  22997   124
>   long:   30874   90
> 
> IOW, we save (22997+124) * 3 bytes of kernel text in hotpaths.
> 
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> Signed-off-by: Ingo Molnar <mingo@kernel.org>
> Link: https://lore.kernel.org/r/20210506194158.216763632@infradead.org

So Willy is having some trouble with this commit; for some reason his
kernel is no longer booting in his qemu thing, but I can't reproduce.

I've hacked up the below vmlinux.o validation, willy can you run this on
your vmlinux.o, something like:

	build/tools/objtool/objtool check -abdJsuld build/vmlinux.o

Where I'm assuming you build with O=build/. When I run it on my build
(with your .config) I get absolutely nothing :/

Alternatively, can you get me your vmlinux.o + bzImage ?

Also helpful might be trying to attach gdb to the qemu gdbstub and
looking where the boot fails.

---

diff --git a/tools/objtool/builtin-check.c b/tools/objtool/builtin-check.c
index 8b38b5d6fec7..100f3efa6136 100644
--- a/tools/objtool/builtin-check.c
+++ b/tools/objtool/builtin-check.c
@@ -20,7 +20,7 @@
 #include <objtool/objtool.h>
 
 bool no_fp, no_unreachable, retpoline, module, backtrace, uaccess, stats,
-     validate_dup, vmlinux, mcount, noinstr, backup;
+     validate_dup, vmlinux, mcount, noinstr, backup, validate_jl;
 
 static const char * const check_usage[] = {
 	"objtool check [<options>] file.o",
@@ -45,6 +45,7 @@ const struct option check_options[] = {
 	OPT_BOOLEAN('l', "vmlinux", &vmlinux, "vmlinux.o validation"),
 	OPT_BOOLEAN('M', "mcount", &mcount, "generate __mcount_loc"),
 	OPT_BOOLEAN('B', "backup", &backup, "create .orig files before modification"),
+	OPT_BOOLEAN('J', "jump-label", &validate_jl, "validate jump-label tables"),
 	OPT_END(),
 };
 
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 2c6a93edf27e..c3c82e40cbee 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -1225,6 +1225,33 @@ static int handle_jump_alt(struct objtool_file *file,
 			   struct instruction *orig_insn,
 			   struct instruction **new_insn)
 {
+	if (validate_jl) {
+#if 0
+		if (special_alt->key_addend & 2) {
+			WARN_FUNC("jump-label mod: %s", orig_insn->sec, orig_insn->offset,
+				  orig_insn->type == INSN_NOP ? "nop" : "jmp");
+		}
+#endif
+
+		if (orig_insn->len == 2) {
+			s32 disp;
+
+			if (special_alt->orig_sec != special_alt->new_sec) {
+				WARN_FUNC("short jump-label cannot cross sections",
+					  orig_insn->sec, orig_insn->offset);
+				return -1;
+			}
+
+			disp = special_alt->new_off - (special_alt->orig_off + 2);
+
+			if ((disp >> 31) != (disp >> 7)) {
+				WARN_FUNC("short jump-label, displacement too large: 0x%08x",
+					  orig_insn->sec, orig_insn->offset, disp);
+				return -1;
+			}
+		}
+	}
+
 	if (orig_insn->type == INSN_NOP) {
 do_nop:
 		if (orig_insn->len == 2)
@@ -1244,6 +1271,11 @@ static int handle_jump_alt(struct objtool_file *file,
 	if (special_alt->key_addend & 2) {
 		struct reloc *reloc = insn_reloc(file, orig_insn);
 
+		if (validate_jl) {
+			WARN_FUNC("jump-label unpatched", orig_insn->sec, orig_insn->offset);
+			return -1;
+		}
+
 		if (reloc) {
 			reloc->type = R_NONE;
 			elf_write_reloc(file->elf, reloc);
@@ -1341,6 +1373,8 @@ static int add_special_section_alts(struct objtool_file *file)
 	}
 
 	if (stats) {
+		if (validate_jl)
+			printf("validate-");
 		printf("jl\\\tNOP\tJMP\n");
 		printf("short:\t%ld\t%ld\n", file->jl_nop_short, file->jl_short);
 		printf("long:\t%ld\t%ld\n", file->jl_nop_long, file->jl_long);
diff --git a/tools/objtool/include/objtool/builtin.h b/tools/objtool/include/objtool/builtin.h
index 15ac0b7d3d6a..c9a00423ebd5 100644
--- a/tools/objtool/include/objtool/builtin.h
+++ b/tools/objtool/include/objtool/builtin.h
@@ -9,7 +9,7 @@
 
 extern const struct option check_options[];
 extern bool no_fp, no_unreachable, retpoline, module, backtrace, uaccess, stats,
-            validate_dup, vmlinux, mcount, noinstr, backup;
+            validate_dup, vmlinux, mcount, noinstr, backup, validate_jl;
 
 extern int cmd_parse_options(int argc, const char **argv, const char * const usage[]);
Peter Zijlstra May 18, 2021, 8:24 p.m. UTC | #2
+kbuild maintainers

On Tue, May 18, 2021 at 09:50:04PM +0200, Peter Zijlstra wrote:
> On Wed, May 12, 2021 at 01:19:47PM -0000, tip-bot2 for Peter Zijlstra wrote:
> > The following commit has been merged into the objtool/core branch of tip:
> > 
> > Commit-ID:     ab3257042c26d0cd44793c741e2f89bf38b21fe8
> > Gitweb:        https://git.kernel.org/tip/ab3257042c26d0cd44793c741e2f89bf38b21fe8
> > Author:        Peter Zijlstra <peterz@infradead.org>
> > AuthorDate:    Thu, 06 May 2021 21:34:05 +02:00
> > Committer:     Ingo Molnar <mingo@kernel.org>
> > CommitterDate: Wed, 12 May 2021 14:54:56 +02:00
> > 
> > jump_label, x86: Allow short NOPs
> > 
> > Now that objtool is able to rewrite jump_label instructions, have the
> > compiler emit a JMP, such that it can decide on the optimal encoding,
> > and set jump_entry::key bit1 to indicate that objtool should rewrite
> > the instruction to a matching NOP.
> > 
> > For x86_64-allyesconfig this gives:
> > 
> >   jl\     NOP     JMP
> >   short:  22997   124
> >   long:   30874   90
> > 
> > IOW, we save (22997+124) * 3 bytes of kernel text in hotpaths.
> > 
> > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > Signed-off-by: Ingo Molnar <mingo@kernel.org>
> > Link: https://lore.kernel.org/r/20210506194158.216763632@infradead.org
> 
> So Willy is having some trouble with this commit; for some reason his
> kernel is no longer booting in his qemu thing, but I can't reproduce.
> 
> I've hacked up the below vmlinux.o validation, willy can you run this on
> your vmlinux.o, something like:
> 
> 	build/tools/objtool/objtool check -abdJsuld build/vmlinux.o
> 
> Where I'm assuming you build with O=build/. When I run it on my build
> (with your .config) I get absolutely nothing :/
> 
> Alternatively, can you get me your vmlinux.o + bzImage ?
> 
> Also helpful might be trying to attach gdb to the qemu gdbstub and
> looking where the boot fails.

OK, willy followed up on IRC, and it turns out there's a kbuild
dependency missing; then objtool changes we don't rebuild:

  arch/x86/entry/vdso/vma.o

even though we should, this led to an unpatched 2 byte jump-label and
things went sideways. I'm not sure I understand the whole build
machinery well enough to know where to begin chasing this.

Now, this file is mighty magical, due to:

arch/x86/entry/vdso/Makefile:OBJECT_FILES_NON_STANDARD  := y
arch/x86/entry/vdso/Makefile:OBJECT_FILES_NON_STANDARD_vma.o    := n

Maybe that's related.
Josh Poimboeuf May 19, 2021, 12:44 a.m. UTC | #3
On Tue, May 18, 2021 at 10:24:43PM +0200, Peter Zijlstra wrote:
> OK, willy followed up on IRC, and it turns out there's a kbuild
> dependency missing; then objtool changes we don't rebuild:
> 
>   arch/x86/entry/vdso/vma.o
> 
> even though we should, this led to an unpatched 2 byte jump-label and
> things went sideways. I'm not sure I understand the whole build
> machinery well enough to know where to begin chasing this.
> 
> Now, this file is mighty magical, due to:
> 
> arch/x86/entry/vdso/Makefile:OBJECT_FILES_NON_STANDARD  := y
> arch/x86/entry/vdso/Makefile:OBJECT_FILES_NON_STANDARD_vma.o    := n
> 
> Maybe that's related.

I'm not exactly thrilled that objtool now has the power to easily brick
a system :-/  Is it really worth it?

Anyway, here's one way to fix it.  Maybe Masahiro has a better idea.

From f88b208677953bc445db08ac46b6e4259217bb8a Mon Sep 17 00:00:00 2001
Message-Id: <f88b208677953bc445db08ac46b6e4259217bb8a.1621384807.git.jpoimboe@redhat.com>
From: Josh Poimboeuf <jpoimboe@redhat.com>
Date: Tue, 18 May 2021 18:59:15 -0500
Subject: [PATCH] kbuild: Fix objtool dependency for
 'OBJECT_FILES_NON_STANDARD_<obj> := n'

"OBJECT_FILES_NON_STANDARD_vma.o := n" has a dependency bug.  When
objtool source is updated, the affected object doesn't get re-analyzed
by objtool.

Peter's new variable-sized jump label feature relies on objtool
rewriting the object file.  Otherwise the system can fail to boot.  That
effectively upgrades this minor dependency issue to a major bug.

The problem is that variables in prerequisites are expanded early,
during the read-in phase.  The '$(objtool_dep)' variable indirectly uses
'$@', which isn't yet available when the target prerequisites are
evaluated.

Use '.SECONDEXPANSION:' which causes '$(objtool_dep)' to be expanded in
a later phase, after the target-specific '$@' variable has been defined.

Fixes: b9ab5ebb14ec ("objtool: Add CONFIG_STACK_VALIDATION option")
Fixes: ab3257042c26 ("jump_label, x86: Allow short NOPs")
Reported-by: Matthew Wilcox <willy@infradead.org>
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 scripts/Makefile.build | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/scripts/Makefile.build b/scripts/Makefile.build
index 949f723efe53..34d257653fb4 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -268,7 +268,8 @@ define rule_as_o_S
 endef
 
 # Built-in and composite module parts
-$(obj)/%.o: $(src)/%.c $(recordmcount_source) $(objtool_dep) FORCE
+.SECONDEXPANSION:
+$(obj)/%.o: $(src)/%.c $(recordmcount_source) $$(objtool_dep) FORCE
 	$(call if_changed_rule,cc_o_c)
 	$(call cmd,force_checksrc)
 
@@ -349,7 +350,7 @@ cmd_modversions_S =								\
 	fi
 endif
 
-$(obj)/%.o: $(src)/%.S $(objtool_dep) FORCE
+$(obj)/%.o: $(src)/%.S $$(objtool_dep) FORCE
 	$(call if_changed_rule,as_o_S)
 
 targets += $(filter-out $(subdir-builtin), $(real-obj-y))
Peter Zijlstra May 19, 2021, 6:56 a.m. UTC | #4
On Tue, May 18, 2021 at 07:44:11PM -0500, Josh Poimboeuf wrote:

> I'm not exactly thrilled that objtool now has the power to easily brick
> a system :-/  Is it really worth it?

The way I look at it is that not running objtool is a bug either way,
bricking a system is ofcourse a somewhat more drastic failure mode than
missing ORC info for example, but neither are good.

As to worth, about half the jump labels are shorter now, this reduces I$
pressure on hot paths. Any little thing to offset the ever increasing
bulk seems like a good thing to me. But yes, it would be nice if the
assemblers wouldn't suck so bad and this wouldn't need objtool :/ But
I've tried poking the tools guys and they don't really seem interested
:-(

Also, only dirty builds are affected here; clean builds (always
recommended afaik, because dep trouble isn't unheard of) are fine.

> Anyway, here's one way to fix it.  Maybe Masahiro has a better idea.

Thanks! lemme go read up on this magic :-)
Matthew Wilcox June 29, 2021, 8 p.m. UTC | #5
So this got merged without the corresponding Kbuild update being merged,
and my kernel failed to boot.  Bisect got as far as

$ git bisect good
Bisecting: 4 revisions left to test after this (roughly 2 steps)
[ab3257042c26d0cd44793c741e2f89bf38b21fe8] jump_label, x86: Allow short NOPs

before my sluggish memory remembered this thread from six weeks ago.

So if anybody else hits this, do a make clean.

On Wed, May 19, 2021 at 08:56:33AM +0200, Peter Zijlstra wrote:
> On Tue, May 18, 2021 at 07:44:11PM -0500, Josh Poimboeuf wrote:
> 
> > I'm not exactly thrilled that objtool now has the power to easily brick
> > a system :-/  Is it really worth it?
> 
> The way I look at it is that not running objtool is a bug either way,
> bricking a system is ofcourse a somewhat more drastic failure mode than
> missing ORC info for example, but neither are good.
> 
> As to worth, about half the jump labels are shorter now, this reduces I$
> pressure on hot paths. Any little thing to offset the ever increasing
> bulk seems like a good thing to me. But yes, it would be nice if the
> assemblers wouldn't suck so bad and this wouldn't need objtool :/ But
> I've tried poking the tools guys and they don't really seem interested
> :-(
> 
> Also, only dirty builds are affected here; clean builds (always
> recommended afaik, because dep trouble isn't unheard of) are fine.
> 
> > Anyway, here's one way to fix it.  Maybe Masahiro has a better idea.
> 
> Thanks! lemme go read up on this magic :-)
Matthew Wilcox June 29, 2021, 8:35 p.m. UTC | #6
On Tue, Jun 29, 2021 at 09:01:25PM +0100, Matthew Wilcox wrote:
> So this got merged without the corresponding Kbuild update being merged,
> and my kernel failed to boot.  Bisect got as far as
> 
> $ git bisect good
> Bisecting: 4 revisions left to test after this (roughly 2 steps)
> [ab3257042c26d0cd44793c741e2f89bf38b21fe8] jump_label, x86: Allow short NOPs
> 
> before my sluggish memory remembered this thread from six weeks ago.
> 
> So if anybody else hits this, do a make clean.

Actually, this is a different bug with the same symptom.

Applying the patch from Peter, and running it:

$ ./.build_test_kernel-x86_64/tools/objtool/objtool check -abdJsuld .build_test_kernel-x86_64/vmlinux.o
nr_sections: 15446
section_bits: 13
nr_symbols: 116448
symbol_bits: 16
max_reloc: 8031700
tot_reloc: 12477754
reloc_bits: 19
nr_insns: 2523443
.build_test_kernel-x86_64/vmlinux.o: warning: objtool: want_init_on_free()+0x0: jump-label unpatched

This is against a freshly built kernel -- i removed the build directory,
copied in a .config file and built a fresh kernel.
Peter Zijlstra June 30, 2021, 7:07 a.m. UTC | #7
On Tue, Jun 29, 2021 at 09:35:37PM +0100, Matthew Wilcox wrote:
> On Tue, Jun 29, 2021 at 09:01:25PM +0100, Matthew Wilcox wrote:
> > So this got merged without the corresponding Kbuild update being merged,
> > and my kernel failed to boot.  Bisect got as far as
> > 
> > $ git bisect good
> > Bisecting: 4 revisions left to test after this (roughly 2 steps)
> > [ab3257042c26d0cd44793c741e2f89bf38b21fe8] jump_label, x86: Allow short NOPs
> > 
> > before my sluggish memory remembered this thread from six weeks ago.
> > 
> > So if anybody else hits this, do a make clean.
> 
> Actually, this is a different bug with the same symptom.
> 
> Applying the patch from Peter, and running it:
> 
> $ ./.build_test_kernel-x86_64/tools/objtool/objtool check -abdJsuld .build_test_kernel-x86_64/vmlinux.o
> nr_sections: 15446
> section_bits: 13
> nr_symbols: 116448
> symbol_bits: 16
> max_reloc: 8031700
> tot_reloc: 12477754
> reloc_bits: 19
> nr_insns: 2523443
> .build_test_kernel-x86_64/vmlinux.o: warning: objtool: want_init_on_free()+0x0: jump-label unpatched
> 
> This is against a freshly built kernel -- i removed the build directory,
> copied in a .config file and built a fresh kernel.

You happen to have said .config for me?
Peter Zijlstra June 30, 2021, 7:38 a.m. UTC | #8
On Wed, Jun 30, 2021 at 09:07:05AM +0200, Peter Zijlstra wrote:
> On Tue, Jun 29, 2021 at 09:35:37PM +0100, Matthew Wilcox wrote:
> > On Tue, Jun 29, 2021 at 09:01:25PM +0100, Matthew Wilcox wrote:
> > > So this got merged without the corresponding Kbuild update being merged,
> > > and my kernel failed to boot.  Bisect got as far as
> > > 
> > > $ git bisect good
> > > Bisecting: 4 revisions left to test after this (roughly 2 steps)
> > > [ab3257042c26d0cd44793c741e2f89bf38b21fe8] jump_label, x86: Allow short NOPs
> > > 
> > > before my sluggish memory remembered this thread from six weeks ago.
> > > 
> > > So if anybody else hits this, do a make clean.
> > 
> > Actually, this is a different bug with the same symptom.
> > 
> > Applying the patch from Peter, and running it:
> > 
> > $ ./.build_test_kernel-x86_64/tools/objtool/objtool check -abdJsuld .build_test_kernel-x86_64/vmlinux.o
> > nr_sections: 15446
> > section_bits: 13
> > nr_symbols: 116448
> > symbol_bits: 16
> > max_reloc: 8031700
> > tot_reloc: 12477754
> > reloc_bits: 19
> > nr_insns: 2523443
> > .build_test_kernel-x86_64/vmlinux.o: warning: objtool: want_init_on_free()+0x0: jump-label unpatched
> > 
> > This is against a freshly built kernel -- i removed the build directory,
> > copied in a .config file and built a fresh kernel.
> 
> You happen to have said .config for me?

Also GCC version I suppose. The thing I'm wondering about in particular
is what translation unit is responsible for that symbol.

AFAICT the function itself is an inline from linux/mm.h, but I cannot
find any of the files or functions it's used in as being excluded from
objtool coverage :/

Patch
diff mbox series

diff --git a/arch/x86/include/asm/jump_label.h b/arch/x86/include/asm/jump_label.h
index ef819e3..0449b12 100644
--- a/arch/x86/include/asm/jump_label.h
+++ b/arch/x86/include/asm/jump_label.h
@@ -20,6 +20,22 @@ 
 	_ASM_PTR "%c0 + %c1 - .\n\t"			\
 	".popsection \n\t"
 
+#ifdef CONFIG_STACK_VALIDATION
+
+static __always_inline bool arch_static_branch(struct static_key *key, bool branch)
+{
+	asm_volatile_goto("1:"
+		"jmp %l[l_yes] # objtool NOPs this \n\t"
+		JUMP_TABLE_ENTRY
+		: :  "i" (key), "i" (2 | branch) : : l_yes);
+
+	return false;
+l_yes:
+	return true;
+}
+
+#else
+
 static __always_inline bool arch_static_branch(struct static_key * const key, const bool branch)
 {
 	asm_volatile_goto("1:"
@@ -32,6 +48,8 @@  l_yes:
 	return true;
 }
 
+#endif /* STACK_VALIDATION */
+
 static __always_inline bool arch_static_branch_jump(struct static_key * const key, const bool branch)
 {
 	asm_volatile_goto("1:"