* [PATCH 01/18] objtool: Enable unreachable warnings for CLANG LTO
2022-04-13 23:19 [PATCH 00/18] objtool: Interface overhaul Josh Poimboeuf
@ 2022-04-13 23:19 ` Josh Poimboeuf
2022-04-13 23:19 ` [PATCH 02/18] objtool: Support data symbol printing Josh Poimboeuf
` (16 subsequent siblings)
17 siblings, 0 replies; 41+ messages in thread
From: Josh Poimboeuf @ 2022-04-13 23:19 UTC (permalink / raw)
To: x86; +Cc: Peter Zijlstra, linux-kernel, Miroslav Benes
With IBT support in, objtool is now fully capable of following vmlinux
code flow in LTO mode. Start reporting unreachable warnings for Clang
LTO as well.
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
scripts/Makefile.build | 2 +-
scripts/link-vmlinux.sh | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/scripts/Makefile.build b/scripts/Makefile.build
index 9717e6f6fb31..33c1ed581522 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -231,7 +231,7 @@ objtool_args = \
$(if $(part-of-module), --module) \
$(if $(CONFIG_X86_KERNEL_IBT), --lto --ibt) \
$(if $(CONFIG_FRAME_POINTER),, --no-fp) \
- $(if $(CONFIG_GCOV_KERNEL)$(CONFIG_LTO_CLANG), --no-unreachable)\
+ $(if $(CONFIG_GCOV_KERNEL), --no-unreachable) \
$(if $(CONFIG_RETPOLINE), --retpoline) \
$(if $(CONFIG_X86_SMAP), --uaccess) \
$(if $(CONFIG_FTRACE_MCOUNT_USE_OBJTOOL), --mcount) \
diff --git a/scripts/link-vmlinux.sh b/scripts/link-vmlinux.sh
index 20f44504a644..9361a1ef02c9 100755
--- a/scripts/link-vmlinux.sh
+++ b/scripts/link-vmlinux.sh
@@ -140,7 +140,7 @@ objtool_link()
if ! is_enabled CONFIG_FRAME_POINTER; then
objtoolopt="${objtoolopt} --no-fp"
fi
- if is_enabled CONFIG_GCOV_KERNEL || is_enabled CONFIG_LTO_CLANG; then
+ if is_enabled CONFIG_GCOV_KERNEL; then
objtoolopt="${objtoolopt} --no-unreachable"
fi
if is_enabled CONFIG_RETPOLINE; then
--
2.34.1
^ permalink raw reply related [flat|nested] 41+ messages in thread* [PATCH 02/18] objtool: Support data symbol printing
2022-04-13 23:19 [PATCH 00/18] objtool: Interface overhaul Josh Poimboeuf
2022-04-13 23:19 ` [PATCH 01/18] objtool: Enable unreachable warnings for CLANG LTO Josh Poimboeuf
@ 2022-04-13 23:19 ` Josh Poimboeuf
2022-04-14 7:05 ` Peter Zijlstra
2022-04-13 23:19 ` [PATCH 03/18] objtool: Add sec+offset to warnings Josh Poimboeuf
` (15 subsequent siblings)
17 siblings, 1 reply; 41+ messages in thread
From: Josh Poimboeuf @ 2022-04-13 23:19 UTC (permalink / raw)
To: x86; +Cc: Peter Zijlstra, linux-kernel, Miroslav Benes
For data relocations to missing ENDBR, it will be useful in some cases
to print the data symbol + offset. Add support for that.
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
tools/objtool/include/objtool/warn.h | 23 +++++++++++++----------
1 file changed, 13 insertions(+), 10 deletions(-)
diff --git a/tools/objtool/include/objtool/warn.h b/tools/objtool/include/objtool/warn.h
index 802cfda0a6f6..dab0dda7c617 100644
--- a/tools/objtool/include/objtool/warn.h
+++ b/tools/objtool/include/objtool/warn.h
@@ -17,16 +17,19 @@ extern const char *objname;
static inline char *offstr(struct section *sec, unsigned long offset)
{
- struct symbol *func;
- char *name, *str;
+ bool is_text = (sec->sh.sh_flags & SHF_EXECINSTR);
+ struct symbol *sym = NULL;
unsigned long name_off;
+ char *name, *str;
+
+ if (is_text)
+ sym = find_func_containing(sec, offset);
+ if (!sym)
+ sym = find_symbol_containing(sec, offset);
- func = find_func_containing(sec, offset);
- if (!func)
- func = find_symbol_containing(sec, offset);
- if (func) {
- name = func->name;
- name_off = offset - func->offset;
+ if (sym) {
+ name = sym->name;
+ name_off = offset - sym->offset;
} else {
name = sec->name;
name_off = offset;
@@ -34,8 +37,8 @@ static inline char *offstr(struct section *sec, unsigned long offset)
str = malloc(strlen(name) + 20);
- if (func)
- sprintf(str, "%s()+0x%lx", name, name_off);
+ if (sym)
+ sprintf(str, "%s%s+0x%lx", name, is_text ? "()" : "", name_off);
else
sprintf(str, "%s+0x%lx", name, name_off);
--
2.34.1
^ permalink raw reply related [flat|nested] 41+ messages in thread* Re: [PATCH 02/18] objtool: Support data symbol printing
2022-04-13 23:19 ` [PATCH 02/18] objtool: Support data symbol printing Josh Poimboeuf
@ 2022-04-14 7:05 ` Peter Zijlstra
2022-04-14 15:21 ` Josh Poimboeuf
0 siblings, 1 reply; 41+ messages in thread
From: Peter Zijlstra @ 2022-04-14 7:05 UTC (permalink / raw)
To: Josh Poimboeuf; +Cc: x86, linux-kernel, Miroslav Benes
On Wed, Apr 13, 2022 at 04:19:37PM -0700, Josh Poimboeuf wrote:
> @@ -34,8 +37,8 @@ static inline char *offstr(struct section *sec, unsigned long offset)
>
> str = malloc(strlen(name) + 20);
>
> - if (func)
> - sprintf(str, "%s()+0x%lx", name, name_off);
> + if (sym)
> + sprintf(str, "%s%s+0x%lx", name, is_text ? "()" : "", name_off);
> else
> sprintf(str, "%s+0x%lx", name, name_off);
So I like the patch, except that "()" thing is something where we differ
from the kernel's %ps format and I've cursed it a number of times
because I then have to manually edit (iow remove) things when pasting it
in various scripts etc..
That said, it totally makes sense to differentiate between a text and
data symbol this way :/
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 02/18] objtool: Support data symbol printing
2022-04-14 7:05 ` Peter Zijlstra
@ 2022-04-14 15:21 ` Josh Poimboeuf
2022-04-14 15:31 ` Peter Zijlstra
0 siblings, 1 reply; 41+ messages in thread
From: Josh Poimboeuf @ 2022-04-14 15:21 UTC (permalink / raw)
To: Peter Zijlstra; +Cc: x86, linux-kernel, Miroslav Benes
On Thu, Apr 14, 2022 at 09:05:47AM +0200, Peter Zijlstra wrote:
> On Wed, Apr 13, 2022 at 04:19:37PM -0700, Josh Poimboeuf wrote:
>
> > @@ -34,8 +37,8 @@ static inline char *offstr(struct section *sec, unsigned long offset)
> >
> > str = malloc(strlen(name) + 20);
> >
> > - if (func)
> > - sprintf(str, "%s()+0x%lx", name, name_off);
> > + if (sym)
> > + sprintf(str, "%s%s+0x%lx", name, is_text ? "()" : "", name_off);
> > else
> > sprintf(str, "%s+0x%lx", name, name_off);
>
> So I like the patch, except that "()" thing is something where we differ
> from the kernel's %ps format and I've cursed it a number of times
> because I then have to manually edit (iow remove) things when pasting it
> in various scripts etc..
Oh, hm, that's true. I can remove them if you prefer.
> That said, it totally makes sense to differentiate between a text and
> data symbol this way :/
Yes, but if we're keeping the "Add sec+offset to warnings" patch then
that distinction is already (kind of) being made by showing the data
section name. And the data symbol warnings should be rare.
--
Josh
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 02/18] objtool: Support data symbol printing
2022-04-14 15:21 ` Josh Poimboeuf
@ 2022-04-14 15:31 ` Peter Zijlstra
2022-04-14 15:38 ` Josh Poimboeuf
0 siblings, 1 reply; 41+ messages in thread
From: Peter Zijlstra @ 2022-04-14 15:31 UTC (permalink / raw)
To: Josh Poimboeuf; +Cc: x86, linux-kernel, Miroslav Benes
On Thu, Apr 14, 2022 at 08:21:48AM -0700, Josh Poimboeuf wrote:
> On Thu, Apr 14, 2022 at 09:05:47AM +0200, Peter Zijlstra wrote:
> > On Wed, Apr 13, 2022 at 04:19:37PM -0700, Josh Poimboeuf wrote:
> >
> > > @@ -34,8 +37,8 @@ static inline char *offstr(struct section *sec, unsigned long offset)
> > >
> > > str = malloc(strlen(name) + 20);
> > >
> > > - if (func)
> > > - sprintf(str, "%s()+0x%lx", name, name_off);
> > > + if (sym)
> > > + sprintf(str, "%s%s+0x%lx", name, is_text ? "()" : "", name_off);
> > > else
> > > sprintf(str, "%s+0x%lx", name, name_off);
> >
> > So I like the patch, except that "()" thing is something where we differ
> > from the kernel's %ps format and I've cursed it a number of times
> > because I then have to manually edit (iow remove) things when pasting it
> > in various scripts etc..
>
> Oh, hm, that's true. I can remove them if you prefer.
Yeah, I think taking it out is best, easier if we're consistent with %ps
for everybody.
> > That said, it totally makes sense to differentiate between a text and
> > data symbol this way :/
>
> Yes, but if we're keeping the "Add sec+offset to warnings" patch then
> that distinction is already (kind of) being made by showing the data
> section name. And the data symbol warnings should be rare.
Yes, I'd not seen that yet, what's that for? The Changelog alludes to
something, but I don't think it actually does get used later.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 02/18] objtool: Support data symbol printing
2022-04-14 15:31 ` Peter Zijlstra
@ 2022-04-14 15:38 ` Josh Poimboeuf
2022-04-14 16:36 ` Peter Zijlstra
0 siblings, 1 reply; 41+ messages in thread
From: Josh Poimboeuf @ 2022-04-14 15:38 UTC (permalink / raw)
To: Peter Zijlstra; +Cc: x86, linux-kernel, Miroslav Benes
On Thu, Apr 14, 2022 at 05:31:57PM +0200, Peter Zijlstra wrote:
> On Thu, Apr 14, 2022 at 08:21:48AM -0700, Josh Poimboeuf wrote:
> > On Thu, Apr 14, 2022 at 09:05:47AM +0200, Peter Zijlstra wrote:
> > > On Wed, Apr 13, 2022 at 04:19:37PM -0700, Josh Poimboeuf wrote:
> > >
> > > > @@ -34,8 +37,8 @@ static inline char *offstr(struct section *sec, unsigned long offset)
> > > >
> > > > str = malloc(strlen(name) + 20);
> > > >
> > > > - if (func)
> > > > - sprintf(str, "%s()+0x%lx", name, name_off);
> > > > + if (sym)
> > > > + sprintf(str, "%s%s+0x%lx", name, is_text ? "()" : "", name_off);
> > > > else
> > > > sprintf(str, "%s+0x%lx", name, name_off);
> > >
> > > So I like the patch, except that "()" thing is something where we differ
> > > from the kernel's %ps format and I've cursed it a number of times
> > > because I then have to manually edit (iow remove) things when pasting it
> > > in various scripts etc..
> >
> > Oh, hm, that's true. I can remove them if you prefer.
>
> Yeah, I think taking it out is best, easier if we're consistent with %ps
> for everybody.
Will do.
> > > That said, it totally makes sense to differentiate between a text and
> > > data symbol this way :/
> >
> > Yes, but if we're keeping the "Add sec+offset to warnings" patch then
> > that distinction is already (kind of) being made by showing the data
> > section name. And the data symbol warnings should be rare.
>
> Yes, I'd not seen that yet, what's that for? The Changelog alludes to
> something, but I don't think it actually does get used later.
Nick had asked for something like that, it's just a way to avoid doing
math every time we look at a warning, i.e. to convert func+offset to
sec+offset.
But it's kind of ugly and I'm not 100% happy with it.
Maybe it should be behind an option (--sec-offsets)?
--
Josh
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 02/18] objtool: Support data symbol printing
2022-04-14 15:38 ` Josh Poimboeuf
@ 2022-04-14 16:36 ` Peter Zijlstra
2022-04-14 17:01 ` Josh Poimboeuf
0 siblings, 1 reply; 41+ messages in thread
From: Peter Zijlstra @ 2022-04-14 16:36 UTC (permalink / raw)
To: Josh Poimboeuf; +Cc: x86, linux-kernel, Miroslav Benes
On Thu, Apr 14, 2022 at 08:38:54AM -0700, Josh Poimboeuf wrote:
> > Yes, I'd not seen that yet, what's that for? The Changelog alludes to
> > something, but I don't think it actually does get used later.
>
> Nick had asked for something like that, it's just a way to avoid doing
> math every time we look at a warning, i.e. to convert func+offset to
> sec+offset.
>
> But it's kind of ugly and I'm not 100% happy with it.
>
> Maybe it should be behind an option (--sec-offsets)?
Can do I suppose... Myself, I have this script:
$ cat objdump-func.sh
#!/bin/bash
OBJ=$1; shift
FUNC=$1; shift
objdump -wdr $@ $OBJ | awk "/^\$/ { P=0; } /$FUNC[^>]*>:\$/ { P=1; O=strtonum(\"0x\" \$1); } { if (P) { o=strtonum(\"0x\" \$1); printf(\"%04x \", o-O); print \$0; } }"
That prints a symbol relative offset next to the section, something
like:
$ ./objdump-func.sh defconfig-build/vmlinux.o pick_next_task_idle
0000 00000000000a9eb0 <pick_next_task_idle>:
0000 a9eb0: 41 54 push %r12
0002 a9eb2: 4c 8b a7 28 09 00 00 mov 0x928(%rdi),%r12
0009 a9eb9: 53 push %rbx
000a a9eba: 48 89 fb mov %rdi,%rbx
000d a9ebd: 66 90 xchg %ax,%ax
000f a9ebf: 66 90 xchg %ax,%ax
0011 a9ec1: 4c 89 e0 mov %r12,%rax
0014 a9ec4: 5b pop %rbx
0015 a9ec5: 41 5c pop %r12
0017 a9ec7: c3 ret
0018 a9ec8: e8 00 00 00 00 call a9ecd <pick_next_task_idle+0x1d> a9ec9: R_X86_64_PLT32 __update_idle_core-0x4
001d a9ecd: eb f0 jmp a9ebf <pick_next_task_idle+0xf>
001f a9ecf: 4c 89 e0 mov %r12,%rax
0022 a9ed2: 83 83 b8 0b 00 00 01 addl $0x1,0xbb8(%rbx)
0029 a9ed9: 5b pop %rbx
002a a9eda: 41 5c pop %r12
002c a9edc: c3 ret
002d a9edd: 0f 1f 00 nopl (%rax)
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [PATCH 02/18] objtool: Support data symbol printing
2022-04-14 16:36 ` Peter Zijlstra
@ 2022-04-14 17:01 ` Josh Poimboeuf
2022-04-14 17:21 ` Josh Poimboeuf
0 siblings, 1 reply; 41+ messages in thread
From: Josh Poimboeuf @ 2022-04-14 17:01 UTC (permalink / raw)
To: Peter Zijlstra; +Cc: x86, linux-kernel, Miroslav Benes, Nick Desaulniers
On Thu, Apr 14, 2022 at 06:36:51PM +0200, Peter Zijlstra wrote:
> On Thu, Apr 14, 2022 at 08:38:54AM -0700, Josh Poimboeuf wrote:
>
> > > Yes, I'd not seen that yet, what's that for? The Changelog alludes to
> > > something, but I don't think it actually does get used later.
> >
> > Nick had asked for something like that, it's just a way to avoid doing
> > math every time we look at a warning, i.e. to convert func+offset to
> > sec+offset.
> >
> > But it's kind of ugly and I'm not 100% happy with it.
> >
> > Maybe it should be behind an option (--sec-offsets)?
>
> Can do I suppose... Myself, I have this script:
>
> $ cat objdump-func.sh
> #!/bin/bash
>
> OBJ=$1; shift
> FUNC=$1; shift
>
> objdump -wdr $@ $OBJ | awk "/^\$/ { P=0; } /$FUNC[^>]*>:\$/ { P=1; O=strtonum(\"0x\" \$1); } { if (P) { o=strtonum(\"0x\" \$1); printf(\"%04x \", o-O); print \$0; } }"
That is nice, just added to my ~/bin.
And how am I just learning about objdump "-w" ?!?!
I wrote up a new version of that patch which adds a '--sec-address'
option (see below), but maybe I'll just drop it for now. It's not
really relevant to this set anyway.
From: Josh Poimboeuf <jpoimboe@redhat.com>
Subject: [PATCH] objtool: Add option to print section addresses
To help prevent objtool users from having to do math, add an option to
print the section address in addition to the function address.
Normal:
vmlinux.o: warning: objtool: fixup_exception()+0x2d1: unreachable instruction
With '--sec-address':
vmlinux.o: warning: objtool: fixup_exception()+0x2d1 (.text+0x76c51): unreachable instruction
Suggested-by: Nick Desaulniers <ndesaulniers@google.com>
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
tools/objtool/builtin-check.c | 1 +
tools/objtool/include/objtool/builtin.h | 1 +
tools/objtool/include/objtool/warn.h | 31 ++++++++++++++-----------
3 files changed, 19 insertions(+), 14 deletions(-)
diff --git a/tools/objtool/builtin-check.c b/tools/objtool/builtin-check.c
index 3df46e9b4b03..2c562e3dec55 100644
--- a/tools/objtool/builtin-check.c
+++ b/tools/objtool/builtin-check.c
@@ -55,6 +55,7 @@ const struct option check_options[] = {
OPT_BOOLEAN(0, "module", &opts.module, "object is part of a kernel module"),
OPT_BOOLEAN(0, "no-fp", &opts.no_fp, "skip frame pointer validation"),
OPT_BOOLEAN(0, "no-unreachable", &opts.no_unreachable, "skip 'unreachable instruction' warnings"),
+ OPT_BOOLEAN(0, "sec-address", &opts.sec_address, "print section addresses in warnings"),
OPT_BOOLEAN(0, "stats", &opts.stats, "print statistics"),
OPT_BOOLEAN(0, "vmlinux", &opts.vmlinux, "vmlinux.o validation"),
diff --git a/tools/objtool/include/objtool/builtin.h b/tools/objtool/include/objtool/builtin.h
index 0cac9bd6a97f..e6910a66317a 100644
--- a/tools/objtool/include/objtool/builtin.h
+++ b/tools/objtool/include/objtool/builtin.h
@@ -33,6 +33,7 @@ struct opts {
bool module;
bool no_fp;
bool no_unreachable;
+ bool sec_address;
bool stats;
bool vmlinux;
};
diff --git a/tools/objtool/include/objtool/warn.h b/tools/objtool/include/objtool/warn.h
index c4bde3e2a79c..a3e79ae75f2e 100644
--- a/tools/objtool/include/objtool/warn.h
+++ b/tools/objtool/include/objtool/warn.h
@@ -11,30 +11,33 @@
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
+#include <objtool/builtin.h>
#include <objtool/elf.h>
extern const char *objname;
static inline char *offstr(struct section *sec, unsigned long offset)
{
- struct symbol *func;
- char *name, *str;
- unsigned long name_off;
+ bool is_text = (sec->sh.sh_flags & SHF_EXECINSTR);
+ struct symbol *sym = NULL;
+ char *str;
+ int len;
- func = find_func_containing(sec, offset);
- if (!func)
- func = find_symbol_containing(sec, offset);
- if (func) {
- name = func->name;
- name_off = offset - func->offset;
+ if (is_text)
+ sym = find_func_containing(sec, offset);
+ if (!sym)
+ sym = find_symbol_containing(sec, offset);
+
+ if (sym) {
+ str = malloc(strlen(sym->name) + strlen(sec->name) + 40);
+ len = sprintf(str, "%s+0x%lx", sym->name, offset - sym->offset);
+ if (opts.sec_address)
+ sprintf(str+len, " (%s+0x%lx)", sec->name, offset);
} else {
- name = sec->name;
- name_off = offset;
+ str = malloc(strlen(sec->name) + 20);
+ sprintf(str, "%s+0x%lx", sec->name, offset);
}
- str = malloc(strlen(name) + 20);
- sprintf(str, "%s+0x%lx", name, name_off);
-
return str;
}
--
2.34.1
^ permalink raw reply related [flat|nested] 41+ messages in thread* Re: [PATCH 02/18] objtool: Support data symbol printing
2022-04-14 17:01 ` Josh Poimboeuf
@ 2022-04-14 17:21 ` Josh Poimboeuf
0 siblings, 0 replies; 41+ messages in thread
From: Josh Poimboeuf @ 2022-04-14 17:21 UTC (permalink / raw)
To: Peter Zijlstra; +Cc: x86, linux-kernel, Miroslav Benes, Nick Desaulniers
On Thu, Apr 14, 2022 at 10:01:04AM -0700, Josh Poimboeuf wrote:
> On Thu, Apr 14, 2022 at 06:36:51PM +0200, Peter Zijlstra wrote:
> > On Thu, Apr 14, 2022 at 08:38:54AM -0700, Josh Poimboeuf wrote:
> >
> > > > Yes, I'd not seen that yet, what's that for? The Changelog alludes to
> > > > something, but I don't think it actually does get used later.
> > >
> > > Nick had asked for something like that, it's just a way to avoid doing
> > > math every time we look at a warning, i.e. to convert func+offset to
> > > sec+offset.
> > >
> > > But it's kind of ugly and I'm not 100% happy with it.
> > >
> > > Maybe it should be behind an option (--sec-offsets)?
> >
> > Can do I suppose... Myself, I have this script:
> >
> > $ cat objdump-func.sh
> > #!/bin/bash
> >
> > OBJ=$1; shift
> > FUNC=$1; shift
> >
> > objdump -wdr $@ $OBJ | awk "/^\$/ { P=0; } /$FUNC[^>]*>:\$/ { P=1; O=strtonum(\"0x\" \$1); } { if (P) { o=strtonum(\"0x\" \$1); printf(\"%04x \", o-O); print \$0; } }"
>
> That is nice, just added to my ~/bin.
>
> And how am I just learning about objdump "-w" ?!?!
>
> I wrote up a new version of that patch which adds a '--sec-address'
> option (see below), but maybe I'll just drop it for now. It's not
> really relevant to this set anyway.
But now, testing the IBT code, I realize it would still be helpful for
data addresses. So maybe I'll keep it.
--
Josh
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH 03/18] objtool: Add sec+offset to warnings
2022-04-13 23:19 [PATCH 00/18] objtool: Interface overhaul Josh Poimboeuf
2022-04-13 23:19 ` [PATCH 01/18] objtool: Enable unreachable warnings for CLANG LTO Josh Poimboeuf
2022-04-13 23:19 ` [PATCH 02/18] objtool: Support data symbol printing Josh Poimboeuf
@ 2022-04-13 23:19 ` Josh Poimboeuf
2022-04-13 23:19 ` [PATCH 04/18] objtool: Print data address for "!ENDBR" data warnings Josh Poimboeuf
` (14 subsequent siblings)
17 siblings, 0 replies; 41+ messages in thread
From: Josh Poimboeuf @ 2022-04-13 23:19 UTC (permalink / raw)
To: x86; +Cc: Peter Zijlstra, linux-kernel, Miroslav Benes, Nick Desaulniers
Change this:
vmlinux.o: warning: objtool: fixup_exception()+0x2d1: unreachable instruction
to this:
vmlinux.o: warning: objtool: fixup_exception()+0x2d1: .text+0x76c51: unreachable instruction
It's noisier, but it makes our lives considerably easier.
Make this new 'verbose' mode optional, which will come in handy soon.
Suggested-by: Nick Desaulniers <ndesaulniers@google.com>
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
tools/objtool/include/objtool/warn.h | 39 +++++++++++++---------------
1 file changed, 18 insertions(+), 21 deletions(-)
diff --git a/tools/objtool/include/objtool/warn.h b/tools/objtool/include/objtool/warn.h
index dab0dda7c617..a4c6b52a58c9 100644
--- a/tools/objtool/include/objtool/warn.h
+++ b/tools/objtool/include/objtool/warn.h
@@ -15,12 +15,13 @@
extern const char *objname;
-static inline char *offstr(struct section *sec, unsigned long offset)
+static inline char *offstr(struct section *sec, unsigned long offset,
+ bool verbose)
{
bool is_text = (sec->sh.sh_flags & SHF_EXECINSTR);
struct symbol *sym = NULL;
- unsigned long name_off;
- char *name, *str;
+ char *str;
+ int len;
if (is_text)
sym = find_func_containing(sec, offset);
@@ -28,20 +29,16 @@ static inline char *offstr(struct section *sec, unsigned long offset)
sym = find_symbol_containing(sec, offset);
if (sym) {
- name = sym->name;
- name_off = offset - sym->offset;
+ str = malloc(strlen(sym->name) + strlen(sec->name) + 40);
+ len = sprintf(str, "%s%s+0x%lx",
+ sym->name, is_text ? "()" : "", offset - sym->offset);
+ if (verbose)
+ sprintf(str+len, ": %s+0x%lx", sec->name, offset);
} else {
- name = sec->name;
- name_off = offset;
+ str = malloc(strlen(sec->name) + 20);
+ sprintf(str, "%s+0x%lx", sec->name, offset);
}
- str = malloc(strlen(name) + 20);
-
- if (sym)
- sprintf(str, "%s%s+0x%lx", name, is_text ? "()" : "", name_off);
- else
- sprintf(str, "%s+0x%lx", name, name_off);
-
return str;
}
@@ -52,17 +49,17 @@ static inline char *offstr(struct section *sec, unsigned long offset)
#define WARN_FUNC(format, sec, offset, ...) \
({ \
- char *_str = offstr(sec, offset); \
+ char *_str = offstr(sec, offset, true); \
WARN("%s: " format, _str, ##__VA_ARGS__); \
free(_str); \
})
-#define BT_FUNC(format, insn, ...) \
-({ \
- struct instruction *_insn = (insn); \
- char *_str = offstr(_insn->sec, _insn->offset); \
- WARN(" %s: " format, _str, ##__VA_ARGS__); \
- free(_str); \
+#define BT_FUNC(format, insn, ...) \
+({ \
+ struct instruction *_insn = (insn); \
+ char *_str = offstr(_insn->sec, _insn->offset, true); \
+ WARN(" %s: " format, _str, ##__VA_ARGS__); \
+ free(_str); \
})
#define WARN_ELF(format, ...) \
--
2.34.1
^ permalink raw reply related [flat|nested] 41+ messages in thread* [PATCH 04/18] objtool: Print data address for "!ENDBR" data warnings
2022-04-13 23:19 [PATCH 00/18] objtool: Interface overhaul Josh Poimboeuf
` (2 preceding siblings ...)
2022-04-13 23:19 ` [PATCH 03/18] objtool: Add sec+offset to warnings Josh Poimboeuf
@ 2022-04-13 23:19 ` Josh Poimboeuf
2022-04-14 7:36 ` Peter Zijlstra
2022-04-13 23:19 ` [PATCH 05/18] objtool: Use offstr() to print address of missing ENDBR Josh Poimboeuf
` (13 subsequent siblings)
17 siblings, 1 reply; 41+ messages in thread
From: Josh Poimboeuf @ 2022-04-13 23:19 UTC (permalink / raw)
To: x86; +Cc: Peter Zijlstra, linux-kernel, Miroslav Benes
When a "!ENDBR" warning is reported for a data section, objtool just
prints the text address of the relocation target twice, without giving
any clues about the location of the original data reference:
vmlinux.o: warning: objtool: dcbnl_netdevice_event()+0x0: .text+0xb64680: data relocation to !ENDBR: dcbnl_netdevice_event+0x0
Instead, print the address of the data reference, in addition to the
address of the relocation target.
vmlinux.o: warning: objtool: dcbnl_nb+0x0: .data..read_mostly+0xe260: data relocation to !ENDBR: dcbnl_netdevice_event+0x0
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
tools/objtool/check.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 5bd3aa815d51..7a1a02dacb77 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -3825,11 +3825,8 @@ static int validate_ibt(struct objtool_file *file)
struct instruction *dest;
dest = validate_ibt_reloc(file, reloc);
- if (is_data && dest && !dest->noendbr) {
- warn_noendbr("data ", reloc->sym->sec,
- reloc->sym->offset + reloc->addend,
- dest);
- }
+ if (is_data && dest && !dest->noendbr)
+ warn_noendbr("data ", sec, reloc->offset, dest);
}
}
--
2.34.1
^ permalink raw reply related [flat|nested] 41+ messages in thread* Re: [PATCH 04/18] objtool: Print data address for "!ENDBR" data warnings
2022-04-13 23:19 ` [PATCH 04/18] objtool: Print data address for "!ENDBR" data warnings Josh Poimboeuf
@ 2022-04-14 7:36 ` Peter Zijlstra
0 siblings, 0 replies; 41+ messages in thread
From: Peter Zijlstra @ 2022-04-14 7:36 UTC (permalink / raw)
To: Josh Poimboeuf; +Cc: x86, linux-kernel, Miroslav Benes
On Wed, Apr 13, 2022 at 04:19:39PM -0700, Josh Poimboeuf wrote:
> When a "!ENDBR" warning is reported for a data section, objtool just
> prints the text address of the relocation target twice, without giving
> any clues about the location of the original data reference:
>
> vmlinux.o: warning: objtool: dcbnl_netdevice_event()+0x0: .text+0xb64680: data relocation to !ENDBR: dcbnl_netdevice_event+0x0
>
> Instead, print the address of the data reference, in addition to the
> address of the relocation target.
>
> vmlinux.o: warning: objtool: dcbnl_nb+0x0: .data..read_mostly+0xe260: data relocation to !ENDBR: dcbnl_netdevice_event+0x0
Duh yes!
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH 05/18] objtool: Use offstr() to print address of missing ENDBR
2022-04-13 23:19 [PATCH 00/18] objtool: Interface overhaul Josh Poimboeuf
` (3 preceding siblings ...)
2022-04-13 23:19 ` [PATCH 04/18] objtool: Print data address for "!ENDBR" data warnings Josh Poimboeuf
@ 2022-04-13 23:19 ` Josh Poimboeuf
2022-04-13 23:19 ` [PATCH 06/18] libsubcmd: Fix OPTION_GROUP sorting Josh Poimboeuf
` (12 subsequent siblings)
17 siblings, 0 replies; 41+ messages in thread
From: Josh Poimboeuf @ 2022-04-13 23:19 UTC (permalink / raw)
To: x86; +Cc: Peter Zijlstra, linux-kernel, Miroslav Benes
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
tools/objtool/check.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 7a1a02dacb77..b27c2ce5d79e 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -3219,9 +3219,8 @@ validate_ibt_reloc(struct objtool_file *file, struct reloc *reloc)
static void warn_noendbr(const char *msg, struct section *sec, unsigned long offset,
struct instruction *dest)
{
- WARN_FUNC("%srelocation to !ENDBR: %s+0x%lx", sec, offset, msg,
- dest->func ? dest->func->name : dest->sec->name,
- dest->func ? dest->offset - dest->func->offset : dest->offset);
+ WARN_FUNC("%srelocation to !ENDBR: %s", sec, offset, msg,
+ offstr(dest->sec, dest->offset, false));
}
static void validate_ibt_dest(struct objtool_file *file, struct instruction *insn,
--
2.34.1
^ permalink raw reply related [flat|nested] 41+ messages in thread* [PATCH 06/18] libsubcmd: Fix OPTION_GROUP sorting
2022-04-13 23:19 [PATCH 00/18] objtool: Interface overhaul Josh Poimboeuf
` (4 preceding siblings ...)
2022-04-13 23:19 ` [PATCH 05/18] objtool: Use offstr() to print address of missing ENDBR Josh Poimboeuf
@ 2022-04-13 23:19 ` Josh Poimboeuf
2022-04-13 23:19 ` [PATCH 07/18] objtool: Reorganize cmdline options Josh Poimboeuf
` (11 subsequent siblings)
17 siblings, 0 replies; 41+ messages in thread
From: Josh Poimboeuf @ 2022-04-13 23:19 UTC (permalink / raw)
To: x86; +Cc: Peter Zijlstra, linux-kernel, Miroslav Benes
The OPTION_GROUP option type is a way of grouping certain options
together in the printed usage text. It happens to be completely broken,
thanks to the fact that the subcmd option sorting just sorts everything,
without regard for grouping. Luckily, nobody uses this option anyway,
though that will change shortly.
Fix it by sorting each group individually.
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
tools/lib/subcmd/parse-options.c | 17 ++++++++++++++---
1 file changed, 14 insertions(+), 3 deletions(-)
diff --git a/tools/lib/subcmd/parse-options.c b/tools/lib/subcmd/parse-options.c
index 39ebf6192016..9fa75943f2ed 100644
--- a/tools/lib/subcmd/parse-options.c
+++ b/tools/lib/subcmd/parse-options.c
@@ -806,9 +806,9 @@ static int option__cmp(const void *va, const void *vb)
static struct option *options__order(const struct option *opts)
{
- int nr_opts = 0, len;
+ int nr_opts = 0, nr_group = 0, len;
const struct option *o = opts;
- struct option *ordered;
+ struct option *opt, *ordered, *group;
for (o = opts; o->type != OPTION_END; o++)
++nr_opts;
@@ -819,7 +819,18 @@ static struct option *options__order(const struct option *opts)
goto out;
memcpy(ordered, opts, len);
- qsort(ordered, nr_opts, sizeof(*o), option__cmp);
+ /* sort each option group individually */
+ for (opt = group = ordered; opt->type != OPTION_END; opt++) {
+ if (opt->type == OPTION_GROUP) {
+ qsort(group, nr_group, sizeof(*opt), option__cmp);
+ group = opt + 1;
+ nr_group = 0;
+ continue;
+ }
+ nr_group++;
+ }
+ qsort(group, nr_group, sizeof(*opt), option__cmp);
+
out:
return ordered;
}
--
2.34.1
^ permalink raw reply related [flat|nested] 41+ messages in thread* [PATCH 07/18] objtool: Reorganize cmdline options
2022-04-13 23:19 [PATCH 00/18] objtool: Interface overhaul Josh Poimboeuf
` (5 preceding siblings ...)
2022-04-13 23:19 ` [PATCH 06/18] libsubcmd: Fix OPTION_GROUP sorting Josh Poimboeuf
@ 2022-04-13 23:19 ` Josh Poimboeuf
2022-04-13 23:19 ` [PATCH 08/18] objtool: Ditch subcommands Josh Poimboeuf
` (10 subsequent siblings)
17 siblings, 0 replies; 41+ messages in thread
From: Josh Poimboeuf @ 2022-04-13 23:19 UTC (permalink / raw)
To: x86; +Cc: Peter Zijlstra, linux-kernel, Miroslav Benes
Split the existing options into two groups: actions, which actually do
something; and options, which modify the actions in some way.
Also there's no need to have short flags for all the non-action options.
Reserve short flags for the more important actions.
While at it:
- change a few of the short flags to be more intuitive
- make option descriptions more consistently descriptive
- sort options in the source like they are when printed
- move options to a global struct
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
scripts/Makefile.build | 10 ++--
scripts/link-vmlinux.sh | 30 +++++++-----
tools/objtool/arch/x86/decode.c | 2 +-
tools/objtool/arch/x86/special.c | 2 +-
tools/objtool/builtin-check.c | 40 ++++++++--------
tools/objtool/check.c | 62 ++++++++++++-------------
tools/objtool/elf.c | 8 ++--
tools/objtool/include/objtool/builtin.h | 26 +++++++++--
tools/objtool/objtool.c | 6 +--
9 files changed, 108 insertions(+), 78 deletions(-)
diff --git a/scripts/Makefile.build b/scripts/Makefile.build
index 33c1ed581522..dd9d582808d6 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -228,14 +228,14 @@ objtool := $(objtree)/tools/objtool/objtool
objtool_args = \
$(if $(CONFIG_UNWINDER_ORC),orc generate,check) \
- $(if $(part-of-module), --module) \
$(if $(CONFIG_X86_KERNEL_IBT), --lto --ibt) \
- $(if $(CONFIG_FRAME_POINTER),, --no-fp) \
- $(if $(CONFIG_GCOV_KERNEL), --no-unreachable) \
+ $(if $(CONFIG_FTRACE_MCOUNT_USE_OBJTOOL), --mcount) \
$(if $(CONFIG_RETPOLINE), --retpoline) \
+ $(if $(CONFIG_SLS), --sls) \
$(if $(CONFIG_X86_SMAP), --uaccess) \
- $(if $(CONFIG_FTRACE_MCOUNT_USE_OBJTOOL), --mcount) \
- $(if $(CONFIG_SLS), --sls)
+ $(if $(part-of-module), --module) \
+ $(if $(CONFIG_FRAME_POINTER),, --no-fp) \
+ $(if $(CONFIG_GCOV_KERNEL), --no-unreachable)
cmd_objtool = $(if $(objtool-enabled), ; $(objtool) $(objtool_args) $@)
cmd_gen_objtooldep = $(if $(objtool-enabled), { echo ; echo '$@: $$(wildcard $(objtool))' ; } >> $(dot-target).cmd)
diff --git a/scripts/link-vmlinux.sh b/scripts/link-vmlinux.sh
index 9361a1ef02c9..c6e9fef61b11 100755
--- a/scripts/link-vmlinux.sh
+++ b/scripts/link-vmlinux.sh
@@ -117,8 +117,6 @@ objtool_link()
objtoolcmd="orc generate"
fi
- objtoolopt="${objtoolopt} --lto"
-
if is_enabled CONFIG_X86_KERNEL_IBT; then
objtoolopt="${objtoolopt} --ibt"
fi
@@ -126,6 +124,8 @@ objtool_link()
if is_enabled CONFIG_FTRACE_MCOUNT_USE_OBJTOOL; then
objtoolopt="${objtoolopt} --mcount"
fi
+
+ objtoolopt="${objtoolopt} --lto"
fi
if is_enabled CONFIG_VMLINUX_VALIDATION; then
@@ -133,25 +133,33 @@ objtool_link()
fi
if [ -n "${objtoolopt}" ]; then
+
if [ -z "${objtoolcmd}" ]; then
objtoolcmd="check"
fi
- objtoolopt="${objtoolopt} --vmlinux"
- if ! is_enabled CONFIG_FRAME_POINTER; then
- objtoolopt="${objtoolopt} --no-fp"
- fi
- if is_enabled CONFIG_GCOV_KERNEL; then
- objtoolopt="${objtoolopt} --no-unreachable"
- fi
+
if is_enabled CONFIG_RETPOLINE; then
objtoolopt="${objtoolopt} --retpoline"
fi
+
+ if is_enabled CONFIG_SLS; then
+ objtoolopt="${objtoolopt} --sls"
+ fi
+
if is_enabled CONFIG_X86_SMAP; then
objtoolopt="${objtoolopt} --uaccess"
fi
- if is_enabled CONFIG_SLS; then
- objtoolopt="${objtoolopt} --sls"
+
+ if ! is_enabled CONFIG_FRAME_POINTER; then
+ objtoolopt="${objtoolopt} --no-fp"
fi
+
+ if is_enabled CONFIG_GCOV_KERNEL; then
+ objtoolopt="${objtoolopt} --no-unreachable"
+ fi
+
+ objtoolopt="${objtoolopt} --vmlinux"
+
info OBJTOOL ${1}
tools/objtool/objtool ${objtoolcmd} ${objtoolopt} ${1}
fi
diff --git a/tools/objtool/arch/x86/decode.c b/tools/objtool/arch/x86/decode.c
index 943cb41cddf7..8b990a52aada 100644
--- a/tools/objtool/arch/x86/decode.c
+++ b/tools/objtool/arch/x86/decode.c
@@ -581,7 +581,7 @@ int arch_decode_instruction(struct objtool_file *file, const struct section *sec
break;
case 0xc7: /* mov imm, r/m */
- if (!noinstr)
+ if (!opts.noinstr)
break;
if (insn.length == 3+4+4 && !strncmp(sec->name, ".init.text", 10)) {
diff --git a/tools/objtool/arch/x86/special.c b/tools/objtool/arch/x86/special.c
index e707d9bcd161..7c97b7391279 100644
--- a/tools/objtool/arch/x86/special.c
+++ b/tools/objtool/arch/x86/special.c
@@ -20,7 +20,7 @@ void arch_handle_alternative(unsigned short feature, struct special_alt *alt)
* find paths that see the STAC but take the NOP instead of
* CLAC and the other way around.
*/
- if (uaccess)
+ if (opts.uaccess)
alt->skip_orig = true;
else
alt->skip_alt = true;
diff --git a/tools/objtool/builtin-check.c b/tools/objtool/builtin-check.c
index fc6975ab8b06..4f4a087720b3 100644
--- a/tools/objtool/builtin-check.c
+++ b/tools/objtool/builtin-check.c
@@ -19,12 +19,10 @@
#include <objtool/builtin.h>
#include <objtool/objtool.h>
-bool no_fp, no_unreachable, retpoline, module, backtrace, uaccess, stats,
- lto, vmlinux, mcount, noinstr, backup, sls, dryrun,
- ibt;
+struct opts opts;
static const char * const check_usage[] = {
- "objtool check [<options>] file.o",
+ "objtool check <actions> [<options>] file.o",
NULL,
};
@@ -34,21 +32,25 @@ static const char * const env_usage[] = {
};
const struct option check_options[] = {
- OPT_BOOLEAN('f', "no-fp", &no_fp, "Skip frame pointer validation"),
- OPT_BOOLEAN('u', "no-unreachable", &no_unreachable, "Skip 'unreachable instruction' warnings"),
- OPT_BOOLEAN('r', "retpoline", &retpoline, "Validate retpoline assumptions"),
- OPT_BOOLEAN('m', "module", &module, "Indicates the object will be part of a kernel module"),
- OPT_BOOLEAN('b', "backtrace", &backtrace, "unwind on error"),
- OPT_BOOLEAN('a', "uaccess", &uaccess, "enable uaccess checking"),
- OPT_BOOLEAN('s', "stats", &stats, "print statistics"),
- OPT_BOOLEAN(0, "lto", <o, "whole-archive like runs"),
- OPT_BOOLEAN('n', "noinstr", &noinstr, "noinstr validation for vmlinux.o"),
- 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('S', "sls", &sls, "validate straight-line-speculation"),
- OPT_BOOLEAN(0, "dry-run", &dryrun, "don't write the modifications"),
- OPT_BOOLEAN(0, "ibt", &ibt, "validate ENDBR placement"),
+ OPT_GROUP("Actions:"),
+ OPT_BOOLEAN('i', "ibt", &opts.ibt, "validate and annotate IBT"),
+ OPT_BOOLEAN('m', "mcount", &opts.mcount, "annotate mcount/fentry calls for ftrace"),
+ OPT_BOOLEAN('n', "noinstr", &opts.noinstr, "validate noinstr rules"),
+ OPT_BOOLEAN('r', "retpoline", &opts.retpoline, "validate and annotate retpoline usage"),
+ OPT_BOOLEAN('S', "sls", &opts.sls, "validate straight-line-speculation mitigations"),
+ OPT_BOOLEAN('u', "uaccess", &opts.uaccess, "validate uaccess rules for SMAP"),
+
+ OPT_GROUP("Options:"),
+ OPT_BOOLEAN(0, "backtrace", &opts.backtrace, "unwind on error"),
+ OPT_BOOLEAN(0, "backup", &opts.backup, "create .orig files before modification"),
+ OPT_BOOLEAN(0, "dry-run", &opts.dryrun, "don't write modifications"),
+ OPT_BOOLEAN(0, "lto", &opts.lto, "whole-archive like runs"),
+ OPT_BOOLEAN(0, "module", &opts.module, "object is part of a kernel module"),
+ OPT_BOOLEAN(0, "no-fp", &opts.no_fp, "skip frame pointer validation"),
+ OPT_BOOLEAN(0, "no-unreachable", &opts.no_unreachable, "skip 'unreachable instruction' warnings"),
+ OPT_BOOLEAN(0, "stats", &opts.stats, "print statistics"),
+ OPT_BOOLEAN(0, "vmlinux", &opts.vmlinux, "vmlinux.o validation"),
+
OPT_END(),
};
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index b27c2ce5d79e..5059c097c563 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -274,7 +274,7 @@ static void init_insn_state(struct insn_state *state, struct section *sec)
* not correctly determine insn->call_dest->sec (external symbols do
* not have a section).
*/
- if (vmlinux && noinstr && sec)
+ if (opts.vmlinux && opts.noinstr && sec)
state->noinstr = sec->noinstr;
}
@@ -340,7 +340,7 @@ static void *cfi_hash_alloc(unsigned long size)
if (cfi_hash == (void *)-1L) {
WARN("mmap fail cfi_hash");
cfi_hash = NULL;
- } else if (stats) {
+ } else if (opts.stats) {
printf("cfi_bits: %d\n", cfi_bits);
}
@@ -435,7 +435,7 @@ static int decode_instructions(struct objtool_file *file)
}
}
- if (stats)
+ if (opts.stats)
printf("nr_insns: %lu\n", nr_insns);
return 0;
@@ -498,7 +498,7 @@ static int init_pv_ops(struct objtool_file *file)
struct symbol *sym;
int idx, nr;
- if (!noinstr)
+ if (!opts.noinstr)
return 0;
file->pv_ops = NULL;
@@ -669,7 +669,7 @@ static int create_static_call_sections(struct objtool_file *file)
key_sym = find_symbol_by_name(file->elf, tmp);
if (!key_sym) {
- if (!module) {
+ if (!opts.module) {
WARN("static_call: can't find static_call_key symbol: %s", tmp);
return -1;
}
@@ -762,7 +762,7 @@ static int create_ibt_endbr_seal_sections(struct objtool_file *file)
list_for_each_entry(insn, &file->endbr_list, call_node)
idx++;
- if (stats) {
+ if (opts.stats) {
printf("ibt: ENDBR at function start: %d\n", file->nr_endbr);
printf("ibt: ENDBR inside functions: %d\n", file->nr_endbr_int);
printf("ibt: superfluous ENDBR: %d\n", idx);
@@ -1029,7 +1029,7 @@ static void add_uaccess_safe(struct objtool_file *file)
struct symbol *func;
const char **name;
- if (!uaccess)
+ if (!opts.uaccess)
return;
for (name = uaccess_safe_builtin; *name; name++) {
@@ -1171,7 +1171,7 @@ static void annotate_call_site(struct objtool_file *file,
return;
}
- if (mcount && sym->fentry) {
+ if (opts.mcount && sym->fentry) {
if (sibling)
WARN_FUNC("Tail call to __fentry__ !?!?", insn->sec, insn->offset);
@@ -1257,7 +1257,7 @@ static bool is_first_func_insn(struct objtool_file *file, struct instruction *in
if (insn->offset == insn->func->offset)
return true;
- if (ibt) {
+ if (opts.ibt) {
struct instruction *prev = prev_insn_same_sym(file, insn);
if (prev && prev->type == INSN_ENDBR &&
@@ -1700,7 +1700,7 @@ static int add_special_section_alts(struct objtool_file *file)
free(special_alt);
}
- if (stats) {
+ if (opts.stats) {
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);
@@ -1946,7 +1946,7 @@ static int read_unwind_hints(struct objtool_file *file)
insn->hint = true;
- if (ibt && hint->type == UNWIND_HINT_TYPE_REGS_PARTIAL) {
+ if (opts.ibt && hint->type == UNWIND_HINT_TYPE_REGS_PARTIAL) {
struct symbol *sym = find_symbol_by_offset(insn->sec, insn->offset);
if (sym && sym->bind == STB_GLOBAL &&
@@ -2807,7 +2807,7 @@ static int update_cfi_state(struct instruction *insn,
}
/* detect when asm code uses rbp as a scratch register */
- if (!no_fp && insn->func && op->src.reg == CFI_BP &&
+ if (!opts.no_fp && insn->func && op->src.reg == CFI_BP &&
cfa->base != CFI_BP)
cfi->bp_scratch = true;
break;
@@ -3364,7 +3364,7 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
ret = validate_branch(file, func, alt->insn, state);
if (ret) {
- if (backtrace)
+ if (opts.backtrace)
BT_FUNC("(alt)", insn);
return ret;
}
@@ -3380,7 +3380,7 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
switch (insn->type) {
case INSN_RETURN:
- if (sls && !insn->retpoline_safe &&
+ if (opts.sls && !insn->retpoline_safe &&
next_insn && next_insn->type != INSN_TRAP) {
WARN_FUNC("missing int3 after ret",
insn->sec, insn->offset);
@@ -3393,7 +3393,7 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
if (ret)
return ret;
- if (!no_fp && func && !is_fentry_call(insn) &&
+ if (!opts.no_fp && func && !is_fentry_call(insn) &&
!has_valid_stack_frame(&state)) {
WARN_FUNC("call without frame pointer save/setup",
sec, insn->offset);
@@ -3416,7 +3416,7 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
ret = validate_branch(file, func,
insn->jump_dest, state);
if (ret) {
- if (backtrace)
+ if (opts.backtrace)
BT_FUNC("(branch)", insn);
return ret;
}
@@ -3428,7 +3428,7 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
break;
case INSN_JUMP_DYNAMIC:
- if (sls && !insn->retpoline_safe &&
+ if (opts.sls && !insn->retpoline_safe &&
next_insn && next_insn->type != INSN_TRAP) {
WARN_FUNC("missing int3 after indirect jump",
insn->sec, insn->offset);
@@ -3500,7 +3500,7 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
break;
}
- if (ibt)
+ if (opts.ibt)
validate_ibt_insn(file, insn);
if (insn->dead_end)
@@ -3542,7 +3542,7 @@ static int validate_unwind_hints(struct objtool_file *file, struct section *sec)
while (&insn->list != &file->insn_list && (!sec || insn->sec == sec)) {
if (insn->hint && !insn->visited && !insn->ignore) {
ret = validate_branch(file, insn->func, insn, state);
- if (ret && backtrace)
+ if (ret && opts.backtrace)
BT_FUNC("<=== (hint)", insn);
warnings += ret;
}
@@ -3572,7 +3572,7 @@ static int validate_retpoline(struct objtool_file *file)
* loaded late, they very much do need retpoline in their
* .init.text
*/
- if (!strcmp(insn->sec->name, ".init.text") && !module)
+ if (!strcmp(insn->sec->name, ".init.text") && !opts.module)
continue;
WARN_FUNC("indirect %s found in RETPOLINE build",
@@ -3622,7 +3622,7 @@ static bool ignore_unreachable_insn(struct objtool_file *file, struct instructio
* In this case we'll find a piece of code (whole function) that is not
* covered by a !section symbol. Ignore them.
*/
- if (!insn->func && lto) {
+ if (!insn->func && opts.lto) {
int size = find_symbol_hole_containing(insn->sec, insn->offset);
unsigned long end = insn->offset + size;
@@ -3729,7 +3729,7 @@ static int validate_symbol(struct objtool_file *file, struct section *sec,
state->uaccess = sym->uaccess_safe;
ret = validate_branch(file, insn->func, insn, *state);
- if (ret && backtrace)
+ if (ret && opts.backtrace)
BT_FUNC("<=== (sym)", insn);
return ret;
}
@@ -3854,12 +3854,12 @@ int check(struct objtool_file *file)
{
int ret, warnings = 0;
- if (lto && !(vmlinux || module)) {
+ if (opts.lto && !(opts.vmlinux || opts.module)) {
fprintf(stderr, "--lto requires: --vmlinux or --module\n");
return 1;
}
- if (ibt && !lto) {
+ if (opts.ibt && !opts.lto) {
fprintf(stderr, "--ibt requires: --lto\n");
return 1;
}
@@ -3884,7 +3884,7 @@ int check(struct objtool_file *file)
if (list_empty(&file->insn_list))
goto out;
- if (vmlinux && !lto) {
+ if (opts.vmlinux && !opts.lto) {
ret = validate_vmlinux_functions(file);
if (ret < 0)
goto out;
@@ -3893,7 +3893,7 @@ int check(struct objtool_file *file)
goto out;
}
- if (retpoline) {
+ if (opts.retpoline) {
ret = validate_retpoline(file);
if (ret < 0)
return ret;
@@ -3910,7 +3910,7 @@ int check(struct objtool_file *file)
goto out;
warnings += ret;
- if (ibt) {
+ if (opts.ibt) {
ret = validate_ibt(file);
if (ret < 0)
goto out;
@@ -3929,28 +3929,28 @@ int check(struct objtool_file *file)
goto out;
warnings += ret;
- if (retpoline) {
+ if (opts.retpoline) {
ret = create_retpoline_sites_sections(file);
if (ret < 0)
goto out;
warnings += ret;
}
- if (mcount) {
+ if (opts.mcount) {
ret = create_mcount_loc_sections(file);
if (ret < 0)
goto out;
warnings += ret;
}
- if (ibt) {
+ if (opts.ibt) {
ret = create_ibt_endbr_seal_sections(file);
if (ret < 0)
goto out;
warnings += ret;
}
- if (stats) {
+ if (opts.stats) {
printf("nr_insns_visited: %ld\n", nr_insns_visited);
printf("nr_cfi: %ld\n", nr_cfi);
printf("nr_cfi_reused: %ld\n", nr_cfi_reused);
diff --git a/tools/objtool/elf.c b/tools/objtool/elf.c
index d7b99a737496..f7b2ad27bb1c 100644
--- a/tools/objtool/elf.c
+++ b/tools/objtool/elf.c
@@ -355,7 +355,7 @@ static int read_sections(struct elf *elf)
elf_hash_add(section_name, &sec->name_hash, str_hash(sec->name));
}
- if (stats) {
+ if (opts.stats) {
printf("nr_sections: %lu\n", (unsigned long)sections_nr);
printf("section_bits: %d\n", elf->section_bits);
}
@@ -475,7 +475,7 @@ static int read_symbols(struct elf *elf)
elf_add_symbol(elf, sym);
}
- if (stats) {
+ if (opts.stats) {
printf("nr_symbols: %lu\n", (unsigned long)symbols_nr);
printf("symbol_bits: %d\n", elf->symbol_bits);
}
@@ -700,7 +700,7 @@ static int read_relocs(struct elf *elf)
tot_reloc += nr_reloc;
}
- if (stats) {
+ if (opts.stats) {
printf("max_reloc: %lu\n", max_reloc);
printf("tot_reloc: %lu\n", tot_reloc);
printf("reloc_bits: %d\n", elf->reloc_bits);
@@ -1079,7 +1079,7 @@ int elf_write(struct elf *elf)
struct section *sec;
Elf_Scn *s;
- if (dryrun)
+ if (opts.dryrun)
return 0;
/* Update changed relocation sections and section headers: */
diff --git a/tools/objtool/include/objtool/builtin.h b/tools/objtool/include/objtool/builtin.h
index c39dbfaef6dc..87c1a7351e3c 100644
--- a/tools/objtool/include/objtool/builtin.h
+++ b/tools/objtool/include/objtool/builtin.h
@@ -8,9 +8,29 @@
#include <subcmd/parse-options.h>
extern const struct option check_options[];
-extern bool no_fp, no_unreachable, retpoline, module, backtrace, uaccess, stats,
- lto, vmlinux, mcount, noinstr, backup, sls, dryrun,
- ibt;
+
+struct opts {
+ /* actions: */
+ bool ibt;
+ bool mcount;
+ bool noinstr;
+ bool retpoline;
+ bool sls;
+ bool uaccess;
+
+ /* options: */
+ bool backtrace;
+ bool backup;
+ bool dryrun;
+ bool lto;
+ bool module;
+ bool no_fp;
+ bool no_unreachable;
+ bool stats;
+ bool vmlinux;
+};
+
+extern struct opts opts;
extern int cmd_parse_options(int argc, const char **argv, const char * const usage[]);
diff --git a/tools/objtool/objtool.c b/tools/objtool/objtool.c
index 843ff3c2f28e..a0f3d3c9558d 100644
--- a/tools/objtool/objtool.c
+++ b/tools/objtool/objtool.c
@@ -118,7 +118,7 @@ struct objtool_file *objtool_open_read(const char *_objname)
if (!file.elf)
return NULL;
- if (backup && !objtool_create_backup(objname)) {
+ if (opts.backup && !objtool_create_backup(objname)) {
WARN("can't create backup file");
return NULL;
}
@@ -129,7 +129,7 @@ struct objtool_file *objtool_open_read(const char *_objname)
INIT_LIST_HEAD(&file.static_call_list);
INIT_LIST_HEAD(&file.mcount_loc_list);
INIT_LIST_HEAD(&file.endbr_list);
- file.ignore_unreachables = no_unreachable;
+ file.ignore_unreachables = opts.no_unreachable;
file.hints = false;
return &file;
@@ -137,7 +137,7 @@ struct objtool_file *objtool_open_read(const char *_objname)
void objtool_pv_add(struct objtool_file *f, int idx, struct symbol *func)
{
- if (!noinstr)
+ if (!opts.noinstr)
return;
if (!f->pv_ops) {
--
2.34.1
^ permalink raw reply related [flat|nested] 41+ messages in thread* [PATCH 08/18] objtool: Ditch subcommands
2022-04-13 23:19 [PATCH 00/18] objtool: Interface overhaul Josh Poimboeuf
` (6 preceding siblings ...)
2022-04-13 23:19 ` [PATCH 07/18] objtool: Reorganize cmdline options Josh Poimboeuf
@ 2022-04-13 23:19 ` Josh Poimboeuf
2022-04-13 23:19 ` [PATCH 09/18] objtool: Add stack validation cmdline option Josh Poimboeuf
` (9 subsequent siblings)
17 siblings, 0 replies; 41+ messages in thread
From: Josh Poimboeuf @ 2022-04-13 23:19 UTC (permalink / raw)
To: x86; +Cc: Peter Zijlstra, linux-kernel, Miroslav Benes
Objtool has a fairly singular focus. It runs on object files and does
validations and transformations which can be combined in various ways.
The subcommand model has never been a good fit, making it awkward to
combine and remove options.
Remove the "check" and "orc" subcommands in favor of a more traditional
cmdline option model. This makes it much more flexible to use, and
easier to port individual features to other arches.
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
scripts/Makefile.build | 2 +-
scripts/link-vmlinux.sh | 13 ++--
tools/objtool/Build | 12 ++-
tools/objtool/Makefile | 8 +-
tools/objtool/builtin-check.c | 61 +++++++++++++---
tools/objtool/builtin-orc.c | 73 -------------------
tools/objtool/check.c | 8 ++
tools/objtool/include/objtool/builtin.h | 10 ++-
tools/objtool/objtool.c | 97 +------------------------
tools/objtool/weak.c | 9 +--
10 files changed, 82 insertions(+), 211 deletions(-)
delete mode 100644 tools/objtool/builtin-orc.c
diff --git a/scripts/Makefile.build b/scripts/Makefile.build
index dd9d582808d6..116c7272b41c 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -227,9 +227,9 @@ ifdef CONFIG_STACK_VALIDATION
objtool := $(objtree)/tools/objtool/objtool
objtool_args = \
- $(if $(CONFIG_UNWINDER_ORC),orc generate,check) \
$(if $(CONFIG_X86_KERNEL_IBT), --lto --ibt) \
$(if $(CONFIG_FTRACE_MCOUNT_USE_OBJTOOL), --mcount) \
+ $(if $(CONFIG_UNWINDER_ORC), --orc) \
$(if $(CONFIG_RETPOLINE), --retpoline) \
$(if $(CONFIG_SLS), --sls) \
$(if $(CONFIG_X86_SMAP), --uaccess) \
diff --git a/scripts/link-vmlinux.sh b/scripts/link-vmlinux.sh
index c6e9fef61b11..f6db79b11573 100755
--- a/scripts/link-vmlinux.sh
+++ b/scripts/link-vmlinux.sh
@@ -113,9 +113,6 @@ objtool_link()
# Don't perform vmlinux validation unless explicitly requested,
# but run objtool on vmlinux.o now that we have an object file.
- if is_enabled CONFIG_UNWINDER_ORC; then
- objtoolcmd="orc generate"
- fi
if is_enabled CONFIG_X86_KERNEL_IBT; then
objtoolopt="${objtoolopt} --ibt"
@@ -125,6 +122,10 @@ objtool_link()
objtoolopt="${objtoolopt} --mcount"
fi
+ if is_enabled CONFIG_UNWINDER_ORC; then
+ objtoolopt="${objtoolopt} --orc"
+ fi
+
objtoolopt="${objtoolopt} --lto"
fi
@@ -134,10 +135,6 @@ objtool_link()
if [ -n "${objtoolopt}" ]; then
- if [ -z "${objtoolcmd}" ]; then
- objtoolcmd="check"
- fi
-
if is_enabled CONFIG_RETPOLINE; then
objtoolopt="${objtoolopt} --retpoline"
fi
@@ -161,7 +158,7 @@ objtool_link()
objtoolopt="${objtoolopt} --vmlinux"
info OBJTOOL ${1}
- tools/objtool/objtool ${objtoolcmd} ${objtoolopt} ${1}
+ tools/objtool/objtool ${objtoolopt} ${1}
fi
}
diff --git a/tools/objtool/Build b/tools/objtool/Build
index b7222d5cc7bc..33f2ee5a46d3 100644
--- a/tools/objtool/Build
+++ b/tools/objtool/Build
@@ -2,17 +2,15 @@ objtool-y += arch/$(SRCARCH)/
objtool-y += weak.o
-objtool-$(SUBCMD_CHECK) += check.o
-objtool-$(SUBCMD_CHECK) += special.o
-objtool-$(SUBCMD_ORC) += check.o
-objtool-$(SUBCMD_ORC) += orc_gen.o
-objtool-$(SUBCMD_ORC) += orc_dump.o
-
+objtool-y += check.o
+objtool-y += special.o
objtool-y += builtin-check.o
-objtool-y += builtin-orc.o
objtool-y += elf.o
objtool-y += objtool.o
+objtool-$(BUILD_ORC) += orc_gen.o
+objtool-$(BUILD_ORC) += orc_dump.o
+
objtool-y += libstring.o
objtool-y += libctype.o
objtool-y += str_error_r.o
diff --git a/tools/objtool/Makefile b/tools/objtool/Makefile
index 0dbd397f319d..061cf1cd42c4 100644
--- a/tools/objtool/Makefile
+++ b/tools/objtool/Makefile
@@ -39,15 +39,13 @@ CFLAGS += $(if $(elfshdr),,-DLIBELF_USE_DEPRECATED)
AWK = awk
-SUBCMD_CHECK := n
-SUBCMD_ORC := n
+BUILD_ORC := n
ifeq ($(SRCARCH),x86)
- SUBCMD_CHECK := y
- SUBCMD_ORC := y
+ BUILD_ORC := y
endif
-export SUBCMD_CHECK SUBCMD_ORC
+export BUILD_ORC
export srctree OUTPUT CFLAGS SRCARCH AWK
include $(srctree)/tools/build/Makefile.include
diff --git a/tools/objtool/builtin-check.c b/tools/objtool/builtin-check.c
index 4f4a087720b3..3df46e9b4b03 100644
--- a/tools/objtool/builtin-check.c
+++ b/tools/objtool/builtin-check.c
@@ -3,16 +3,6 @@
* Copyright (C) 2015-2017 Josh Poimboeuf <jpoimboe@redhat.com>
*/
-/*
- * objtool check:
- *
- * This command analyzes every .o file and ensures the validity of its stack
- * trace metadata. It enforces a set of rules on asm code and C inline
- * assembly code so that stack traces can be reliable.
- *
- * For more information, see tools/objtool/Documentation/stack-validation.txt.
- */
-
#include <subcmd/parse-options.h>
#include <string.h>
#include <stdlib.h>
@@ -22,7 +12,7 @@
struct opts opts;
static const char * const check_usage[] = {
- "objtool check <actions> [<options>] file.o",
+ "objtool <actions> [<options>] file.o",
NULL,
};
@@ -31,14 +21,31 @@ static const char * const env_usage[] = {
NULL,
};
+static int parse_dumpstr(const struct option *opt, const char *str, int unset)
+{
+ if (unset || !str) {
+ opts.dump = DUMP_NONE;
+ return 0;
+ }
+
+ if (!strcmp(str, "orc")) {
+ opts.dump = DUMP_ORC;
+ return 0;
+ }
+
+ return -1;
+}
+
const struct option check_options[] = {
OPT_GROUP("Actions:"),
OPT_BOOLEAN('i', "ibt", &opts.ibt, "validate and annotate IBT"),
OPT_BOOLEAN('m', "mcount", &opts.mcount, "annotate mcount/fentry calls for ftrace"),
OPT_BOOLEAN('n', "noinstr", &opts.noinstr, "validate noinstr rules"),
+ OPT_BOOLEAN('o', "orc", &opts.orc, "generate ORC metadata"),
OPT_BOOLEAN('r', "retpoline", &opts.retpoline, "validate and annotate retpoline usage"),
OPT_BOOLEAN('S', "sls", &opts.sls, "validate straight-line-speculation mitigations"),
OPT_BOOLEAN('u', "uaccess", &opts.uaccess, "validate uaccess rules for SMAP"),
+ OPT_CALLBACK(0, "dump", NULL, "orc", "dump object data", parse_dumpstr),
OPT_GROUP("Options:"),
OPT_BOOLEAN(0, "backtrace", &opts.backtrace, "unwind on error"),
@@ -81,7 +88,31 @@ int cmd_parse_options(int argc, const char **argv, const char * const usage[])
return argc;
}
-int cmd_check(int argc, const char **argv)
+static bool opts_valid(void)
+{
+ if (opts.ibt ||
+ opts.mcount ||
+ opts.noinstr ||
+ opts.orc ||
+ opts.retpoline ||
+ opts.sls ||
+ opts.uaccess) {
+ if (opts.dump) {
+ fprintf(stderr, "--dump can't be combined with other options\n");
+ return false;
+ }
+
+ return true;
+ }
+
+ if (opts.dump)
+ return true;
+
+ fprintf(stderr, "At least one command required\n");
+ return false;
+}
+
+int objtool_run(int argc, const char **argv)
{
const char *objname;
struct objtool_file *file;
@@ -90,6 +121,12 @@ int cmd_check(int argc, const char **argv)
argc = cmd_parse_options(argc, argv, check_usage);
objname = argv[0];
+ if (!opts_valid())
+ return 1;
+
+ if (opts.dump == DUMP_ORC)
+ return orc_dump(objname);
+
file = objtool_open_read(objname);
if (!file)
return 1;
diff --git a/tools/objtool/builtin-orc.c b/tools/objtool/builtin-orc.c
deleted file mode 100644
index 17f8b9307738..000000000000
--- a/tools/objtool/builtin-orc.c
+++ /dev/null
@@ -1,73 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0-or-later
-/*
- * Copyright (C) 2017 Josh Poimboeuf <jpoimboe@redhat.com>
- */
-
-/*
- * objtool orc:
- *
- * This command analyzes a .o file and adds .orc_unwind and .orc_unwind_ip
- * sections to it, which is used by the in-kernel ORC unwinder.
- *
- * This command is a superset of "objtool check".
- */
-
-#include <string.h>
-#include <objtool/builtin.h>
-#include <objtool/objtool.h>
-
-static const char *orc_usage[] = {
- "objtool orc generate [<options>] file.o",
- "objtool orc dump file.o",
- NULL,
-};
-
-int cmd_orc(int argc, const char **argv)
-{
- const char *objname;
-
- argc--; argv++;
- if (argc <= 0)
- usage_with_options(orc_usage, check_options);
-
- if (!strncmp(argv[0], "gen", 3)) {
- struct objtool_file *file;
- int ret;
-
- argc = cmd_parse_options(argc, argv, orc_usage);
- objname = argv[0];
-
- file = objtool_open_read(objname);
- if (!file)
- return 1;
-
- ret = check(file);
- if (ret)
- return ret;
-
- if (list_empty(&file->insn_list))
- return 0;
-
- ret = orc_create(file);
- if (ret)
- return ret;
-
- if (!file->elf->changed)
- return 0;
-
- return elf_write(file->elf);
- }
-
- if (!strcmp(argv[0], "dump")) {
- if (argc != 2)
- usage_with_options(orc_usage, check_options);
-
- objname = argv[1];
-
- return orc_dump(objname);
- }
-
- usage_with_options(orc_usage, check_options);
-
- return 0;
-}
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 5059c097c563..490ed3560d99 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -3950,6 +3950,14 @@ int check(struct objtool_file *file)
warnings += ret;
}
+ if (opts.orc && !list_empty(&file->insn_list)) {
+ ret = orc_create(file);
+ if (ret < 0)
+ goto out;
+ warnings += ret;
+ }
+
+
if (opts.stats) {
printf("nr_insns_visited: %ld\n", nr_insns_visited);
printf("nr_cfi: %ld\n", nr_cfi);
diff --git a/tools/objtool/include/objtool/builtin.h b/tools/objtool/include/objtool/builtin.h
index 87c1a7351e3c..0cac9bd6a97f 100644
--- a/tools/objtool/include/objtool/builtin.h
+++ b/tools/objtool/include/objtool/builtin.h
@@ -9,11 +9,18 @@
extern const struct option check_options[];
+enum dump {
+ DUMP_NONE,
+ DUMP_ORC,
+};
+
struct opts {
/* actions: */
+ enum dump dump;
bool ibt;
bool mcount;
bool noinstr;
+ bool orc;
bool retpoline;
bool sls;
bool uaccess;
@@ -34,7 +41,6 @@ extern struct opts opts;
extern int cmd_parse_options(int argc, const char **argv, const char * const usage[]);
-extern int cmd_check(int argc, const char **argv);
-extern int cmd_orc(int argc, const char **argv);
+extern int objtool_run(int argc, const char **argv);
#endif /* _BUILTIN_H */
diff --git a/tools/objtool/objtool.c b/tools/objtool/objtool.c
index a0f3d3c9558d..512669ce064c 100644
--- a/tools/objtool/objtool.c
+++ b/tools/objtool/objtool.c
@@ -3,16 +3,6 @@
* Copyright (C) 2015 Josh Poimboeuf <jpoimboe@redhat.com>
*/
-/*
- * objtool:
- *
- * The 'check' subcmd analyzes every .o file and ensures the validity of its
- * stack trace metadata. It enforces a set of rules on asm code and C inline
- * assembly code so that stack traces can be reliable.
- *
- * For more information, see tools/objtool/Documentation/stack-validation.txt.
- */
-
#include <stdio.h>
#include <stdbool.h>
#include <string.h>
@@ -26,20 +16,6 @@
#include <objtool/objtool.h>
#include <objtool/warn.h>
-struct cmd_struct {
- const char *name;
- int (*fn)(int, const char **);
- const char *help;
-};
-
-static const char objtool_usage_string[] =
- "objtool COMMAND [ARGS]";
-
-static struct cmd_struct objtool_cmds[] = {
- {"check", cmd_check, "Perform stack metadata validation on an object file" },
- {"orc", cmd_orc, "Generate in-place ORC unwind tables for an object file" },
-};
-
bool help;
const char *objname;
@@ -161,70 +137,6 @@ void objtool_pv_add(struct objtool_file *f, int idx, struct symbol *func)
f->pv_ops[idx].clean = false;
}
-static void cmd_usage(void)
-{
- unsigned int i, longest = 0;
-
- printf("\n usage: %s\n\n", objtool_usage_string);
-
- for (i = 0; i < ARRAY_SIZE(objtool_cmds); i++) {
- if (longest < strlen(objtool_cmds[i].name))
- longest = strlen(objtool_cmds[i].name);
- }
-
- puts(" Commands:");
- for (i = 0; i < ARRAY_SIZE(objtool_cmds); i++) {
- printf(" %-*s ", longest, objtool_cmds[i].name);
- puts(objtool_cmds[i].help);
- }
-
- printf("\n");
-
- if (!help)
- exit(129);
- exit(0);
-}
-
-static void handle_options(int *argc, const char ***argv)
-{
- while (*argc > 0) {
- const char *cmd = (*argv)[0];
-
- if (cmd[0] != '-')
- break;
-
- if (!strcmp(cmd, "--help") || !strcmp(cmd, "-h")) {
- help = true;
- break;
- } else {
- fprintf(stderr, "Unknown option: %s\n", cmd);
- cmd_usage();
- }
-
- (*argv)++;
- (*argc)--;
- }
-}
-
-static void handle_internal_command(int argc, const char **argv)
-{
- const char *cmd = argv[0];
- unsigned int i, ret;
-
- for (i = 0; i < ARRAY_SIZE(objtool_cmds); i++) {
- struct cmd_struct *p = objtool_cmds+i;
-
- if (strcmp(p->name, cmd))
- continue;
-
- ret = p->fn(argc, argv);
-
- exit(ret);
- }
-
- cmd_usage();
-}
-
int main(int argc, const char **argv)
{
static const char *UNUSED = "OBJTOOL_NOT_IMPLEMENTED";
@@ -233,14 +145,7 @@ int main(int argc, const char **argv)
exec_cmd_init("objtool", UNUSED, UNUSED, UNUSED);
pager_init(UNUSED);
- argv++;
- argc--;
- handle_options(&argc, &argv);
-
- if (!argc || help)
- cmd_usage();
-
- handle_internal_command(argc, argv);
+ objtool_run(argc, argv);
return 0;
}
diff --git a/tools/objtool/weak.c b/tools/objtool/weak.c
index 8314e824db4a..d83f607733b0 100644
--- a/tools/objtool/weak.c
+++ b/tools/objtool/weak.c
@@ -15,17 +15,12 @@
return ENOSYS; \
})
-int __weak check(struct objtool_file *file)
-{
- UNSUPPORTED("check subcommand");
-}
-
int __weak orc_dump(const char *_objname)
{
- UNSUPPORTED("orc");
+ UNSUPPORTED("ORC");
}
int __weak orc_create(struct objtool_file *file)
{
- UNSUPPORTED("orc");
+ UNSUPPORTED("ORC");
}
--
2.34.1
^ permalink raw reply related [flat|nested] 41+ messages in thread* [PATCH 09/18] objtool: Add stack validation cmdline option
2022-04-13 23:19 [PATCH 00/18] objtool: Interface overhaul Josh Poimboeuf
` (7 preceding siblings ...)
2022-04-13 23:19 ` [PATCH 08/18] objtool: Ditch subcommands Josh Poimboeuf
@ 2022-04-13 23:19 ` Josh Poimboeuf
2022-04-14 8:43 ` Peter Zijlstra
2022-04-13 23:19 ` [PATCH 10/18] objtool: Extricate ibt from stack validation Josh Poimboeuf
` (8 subsequent siblings)
17 siblings, 1 reply; 41+ messages in thread
From: Josh Poimboeuf @ 2022-04-13 23:19 UTC (permalink / raw)
To: x86; +Cc: Peter Zijlstra, linux-kernel, Miroslav Benes
Make stack validation an explicit cmdline option so that individual
objtool features can be enabled individually by other arches.
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
scripts/Makefile.build | 1 +
scripts/link-vmlinux.sh | 4 ++++
tools/objtool/builtin-check.c | 2 ++
tools/objtool/check.c | 28 +++++++++++++------------
tools/objtool/include/objtool/builtin.h | 1 +
5 files changed, 23 insertions(+), 13 deletions(-)
diff --git a/scripts/Makefile.build b/scripts/Makefile.build
index 116c7272b41c..d5e15ae29156 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -232,6 +232,7 @@ objtool_args = \
$(if $(CONFIG_UNWINDER_ORC), --orc) \
$(if $(CONFIG_RETPOLINE), --retpoline) \
$(if $(CONFIG_SLS), --sls) \
+ $(if $(CONFIG_STACK_VALIDATION), --stackval) \
$(if $(CONFIG_X86_SMAP), --uaccess) \
$(if $(part-of-module), --module) \
$(if $(CONFIG_FRAME_POINTER),, --no-fp) \
diff --git a/scripts/link-vmlinux.sh b/scripts/link-vmlinux.sh
index f6db79b11573..0140bfa32c0c 100755
--- a/scripts/link-vmlinux.sh
+++ b/scripts/link-vmlinux.sh
@@ -126,6 +126,10 @@ objtool_link()
objtoolopt="${objtoolopt} --orc"
fi
+ if is_enabled CONFIG_STACK_VALIDATION; then
+ objtoolopt="${objtoolopt} --stackval"
+ fi
+
objtoolopt="${objtoolopt} --lto"
fi
diff --git a/tools/objtool/builtin-check.c b/tools/objtool/builtin-check.c
index 3df46e9b4b03..a6a86e2d0598 100644
--- a/tools/objtool/builtin-check.c
+++ b/tools/objtool/builtin-check.c
@@ -44,6 +44,7 @@ const struct option check_options[] = {
OPT_BOOLEAN('o', "orc", &opts.orc, "generate ORC metadata"),
OPT_BOOLEAN('r', "retpoline", &opts.retpoline, "validate and annotate retpoline usage"),
OPT_BOOLEAN('S', "sls", &opts.sls, "validate straight-line-speculation mitigations"),
+ OPT_BOOLEAN('s', "stackval", &opts.stackval, "validate stack unwinding rules"),
OPT_BOOLEAN('u', "uaccess", &opts.uaccess, "validate uaccess rules for SMAP"),
OPT_CALLBACK(0, "dump", NULL, "orc", "dump object data", parse_dumpstr),
@@ -96,6 +97,7 @@ static bool opts_valid(void)
opts.orc ||
opts.retpoline ||
opts.sls ||
+ opts.stackval ||
opts.uaccess) {
if (opts.dump) {
fprintf(stderr, "--dump can't be combined with other options\n");
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 490ed3560d99..bb25937b2d1c 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -3900,25 +3900,27 @@ int check(struct objtool_file *file)
warnings += ret;
}
- ret = validate_functions(file);
- if (ret < 0)
- goto out;
- warnings += ret;
-
- ret = validate_unwind_hints(file, NULL);
- if (ret < 0)
- goto out;
- warnings += ret;
+ if (opts.stackval || opts.orc || opts.uaccess) {
+ ret = validate_functions(file);
+ if (ret < 0)
+ goto out;
+ warnings += ret;
- if (opts.ibt) {
- ret = validate_ibt(file);
+ ret = validate_unwind_hints(file, NULL);
if (ret < 0)
goto out;
warnings += ret;
+
+ if (!warnings) {
+ ret = validate_reachable_instructions(file);
+ if (ret < 0)
+ goto out;
+ warnings += ret;
+ }
}
- if (!warnings) {
- ret = validate_reachable_instructions(file);
+ if (opts.ibt) {
+ ret = validate_ibt(file);
if (ret < 0)
goto out;
warnings += ret;
diff --git a/tools/objtool/include/objtool/builtin.h b/tools/objtool/include/objtool/builtin.h
index 0cac9bd6a97f..edb0f550727b 100644
--- a/tools/objtool/include/objtool/builtin.h
+++ b/tools/objtool/include/objtool/builtin.h
@@ -23,6 +23,7 @@ struct opts {
bool orc;
bool retpoline;
bool sls;
+ bool stackval;
bool uaccess;
/* options: */
--
2.34.1
^ permalink raw reply related [flat|nested] 41+ messages in thread* Re: [PATCH 09/18] objtool: Add stack validation cmdline option
2022-04-13 23:19 ` [PATCH 09/18] objtool: Add stack validation cmdline option Josh Poimboeuf
@ 2022-04-14 8:43 ` Peter Zijlstra
2022-04-14 15:52 ` Josh Poimboeuf
0 siblings, 1 reply; 41+ messages in thread
From: Peter Zijlstra @ 2022-04-14 8:43 UTC (permalink / raw)
To: Josh Poimboeuf; +Cc: x86, linux-kernel, Miroslav Benes
On Wed, Apr 13, 2022 at 04:19:44PM -0700, Josh Poimboeuf wrote:
> + if (opts.stackval || opts.orc || opts.uaccess) {
> + ret = validate_functions(file);
> + if (ret < 0)
> + goto out;
> + warnings += ret;
>
> + ret = validate_unwind_hints(file, NULL);
> if (ret < 0)
> goto out;
> warnings += ret;
> +
> + if (!warnings) {
> + ret = validate_reachable_instructions(file);
> + if (ret < 0)
> + goto out;
> + warnings += ret;
> + }
> }
Doesn't SLS also depend on validate_functions() ?
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [PATCH 09/18] objtool: Add stack validation cmdline option
2022-04-14 8:43 ` Peter Zijlstra
@ 2022-04-14 15:52 ` Josh Poimboeuf
0 siblings, 0 replies; 41+ messages in thread
From: Josh Poimboeuf @ 2022-04-14 15:52 UTC (permalink / raw)
To: Peter Zijlstra; +Cc: x86, linux-kernel, Miroslav Benes
On Thu, Apr 14, 2022 at 10:43:25AM +0200, Peter Zijlstra wrote:
> On Wed, Apr 13, 2022 at 04:19:44PM -0700, Josh Poimboeuf wrote:
> > + if (opts.stackval || opts.orc || opts.uaccess) {
> > + ret = validate_functions(file);
> > + if (ret < 0)
> > + goto out;
> > + warnings += ret;
> >
> > + ret = validate_unwind_hints(file, NULL);
> > if (ret < 0)
> > goto out;
> > warnings += ret;
> > +
> > + if (!warnings) {
> > + ret = validate_reachable_instructions(file);
> > + if (ret < 0)
> > + goto out;
> > + warnings += ret;
> > + }
> > }
>
> Doesn't SLS also depend on validate_functions() ?
Doh, I had the intention of splitting that out from validate_branch()
like I did for ibt.
--
Josh
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH 10/18] objtool: Extricate ibt from stack validation
2022-04-13 23:19 [PATCH 00/18] objtool: Interface overhaul Josh Poimboeuf
` (8 preceding siblings ...)
2022-04-13 23:19 ` [PATCH 09/18] objtool: Add stack validation cmdline option Josh Poimboeuf
@ 2022-04-13 23:19 ` Josh Poimboeuf
2022-04-14 7:53 ` Peter Zijlstra
2022-04-13 23:19 ` [PATCH 11/18] objtool: Add CONFIG_OBJTOOL Josh Poimboeuf
` (7 subsequent siblings)
17 siblings, 1 reply; 41+ messages in thread
From: Josh Poimboeuf @ 2022-04-13 23:19 UTC (permalink / raw)
To: x86; +Cc: Peter Zijlstra, linux-kernel, Miroslav Benes
Extricate ibt from validate_branch() in preparation for making stack
validation optional.
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
tools/objtool/check.c | 253 ++++++++++++++++++++----------------------
1 file changed, 120 insertions(+), 133 deletions(-)
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index bb25937b2d1c..1b1e7a4ae18b 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -3183,114 +3183,6 @@ static struct instruction *next_insn_to_validate(struct objtool_file *file,
return next_insn_same_sec(file, insn);
}
-static struct instruction *
-validate_ibt_reloc(struct objtool_file *file, struct reloc *reloc)
-{
- struct instruction *dest;
- struct section *sec;
- unsigned long off;
-
- sec = reloc->sym->sec;
- off = reloc->sym->offset;
-
- if ((reloc->sec->base->sh.sh_flags & SHF_EXECINSTR) &&
- (reloc->type == R_X86_64_PC32 || reloc->type == R_X86_64_PLT32))
- off += arch_dest_reloc_offset(reloc->addend);
- else
- off += reloc->addend;
-
- dest = find_insn(file, sec, off);
- if (!dest)
- return NULL;
-
- if (dest->type == INSN_ENDBR) {
- if (!list_empty(&dest->call_node))
- list_del_init(&dest->call_node);
-
- return NULL;
- }
-
- if (reloc->sym->static_call_tramp)
- return NULL;
-
- return dest;
-}
-
-static void warn_noendbr(const char *msg, struct section *sec, unsigned long offset,
- struct instruction *dest)
-{
- WARN_FUNC("%srelocation to !ENDBR: %s", sec, offset, msg,
- offstr(dest->sec, dest->offset, false));
-}
-
-static void validate_ibt_dest(struct objtool_file *file, struct instruction *insn,
- struct instruction *dest)
-{
- if (dest->func && dest->func == insn->func) {
- /*
- * Anything from->to self is either _THIS_IP_ or IRET-to-self.
- *
- * There is no sane way to annotate _THIS_IP_ since the compiler treats the
- * relocation as a constant and is happy to fold in offsets, skewing any
- * annotation we do, leading to vast amounts of false-positives.
- *
- * There's also compiler generated _THIS_IP_ through KCOV and
- * such which we have no hope of annotating.
- *
- * As such, blanket accept self-references without issue.
- */
- return;
- }
-
- if (dest->noendbr)
- return;
-
- warn_noendbr("", insn->sec, insn->offset, dest);
-}
-
-static void validate_ibt_insn(struct objtool_file *file, struct instruction *insn)
-{
- struct instruction *dest;
- struct reloc *reloc;
-
- switch (insn->type) {
- case INSN_CALL:
- case INSN_CALL_DYNAMIC:
- case INSN_JUMP_CONDITIONAL:
- case INSN_JUMP_UNCONDITIONAL:
- case INSN_JUMP_DYNAMIC:
- case INSN_JUMP_DYNAMIC_CONDITIONAL:
- case INSN_RETURN:
- /*
- * We're looking for code references setting up indirect code
- * flow. As such, ignore direct code flow and the actual
- * dynamic branches.
- */
- return;
-
- case INSN_NOP:
- /*
- * handle_group_alt() will create INSN_NOP instruction that
- * don't belong to any section, ignore all NOP since they won't
- * carry a (useful) relocation anyway.
- */
- return;
-
- default:
- break;
- }
-
- for (reloc = insn_reloc(file, insn);
- reloc;
- reloc = find_reloc_by_dest_range(file->elf, insn->sec,
- reloc->offset + 1,
- (insn->offset + insn->len) - (reloc->offset + 1))) {
- dest = validate_ibt_reloc(file, reloc);
- if (dest)
- validate_ibt_dest(file, insn, dest);
- }
-}
-
/*
* Follow the branch starting at the given instruction, and recursively follow
* any other branches (jumps). Meanwhile, track the frame pointer state at
@@ -3500,9 +3392,6 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
break;
}
- if (opts.ibt)
- validate_ibt_insn(file, insn);
-
if (insn->dead_end)
return 0;
@@ -3788,48 +3677,146 @@ static int validate_functions(struct objtool_file *file)
return warnings;
}
-static int validate_ibt(struct objtool_file *file)
+static void mark_endbr_used(struct instruction *insn)
{
- struct section *sec;
+ if (!list_empty(&insn->call_node))
+ list_del_init(&insn->call_node);
+}
+
+static int validate_ibt_insn(struct objtool_file *file, struct instruction *insn)
+{
+ struct instruction *dest;
struct reloc *reloc;
+ unsigned long off;
+ int warnings = 0;
- for_each_sec(file, sec) {
- bool is_data;
+ /*
+ * Looking for function pointer load relocations. Ignore
+ * direct/indirect branches:
+ */
+ switch (insn->type) {
+ case INSN_CALL:
+ case INSN_CALL_DYNAMIC:
+ case INSN_JUMP_CONDITIONAL:
+ case INSN_JUMP_UNCONDITIONAL:
+ case INSN_JUMP_DYNAMIC:
+ case INSN_JUMP_DYNAMIC_CONDITIONAL:
+ case INSN_RETURN:
+ case INSN_NOP:
+ return 0;
+ default:
+ break;
+ }
- /* already done in validate_branch() */
- if (sec->sh.sh_flags & SHF_EXECINSTR)
- continue;
+ for (reloc = insn_reloc(file, insn);
+ reloc;
+ reloc = find_reloc_by_dest_range(file->elf, insn->sec,
+ reloc->offset + 1,
+ (insn->offset + insn->len) - (reloc->offset + 1))) {
- if (!sec->reloc)
+ /*
+ * static_call_update() references the trampoline, which
+ * doesn't have (or need) ENDBR. Skip warning in that case.
+ */
+ if (reloc->sym->static_call_tramp)
continue;
- if (!strncmp(sec->name, ".orc", 4))
- continue;
+ off = reloc->sym->offset;
+ if (reloc->type == R_X86_64_PC32 || reloc->type == R_X86_64_PLT32)
+ off += arch_dest_reloc_offset(reloc->addend);
+ else
+ off += reloc->addend;
- if (!strncmp(sec->name, ".discard", 8))
+ dest = find_insn(file, reloc->sym->sec, off);
+ if (!dest)
continue;
- if (!strncmp(sec->name, ".debug", 6))
+ if (dest->type == INSN_ENDBR) {
+ mark_endbr_used(dest);
continue;
+ }
- if (!strcmp(sec->name, "_error_injection_whitelist"))
+ if (dest->func && dest->func == insn->func) {
+ /*
+ * Anything from->to self is either _THIS_IP_ or
+ * IRET-to-self.
+ *
+ * There is no sane way to annotate _THIS_IP_ since the
+ * compiler treats the relocation as a constant and is
+ * happy to fold in offsets, skewing any annotation we
+ * do, leading to vast amounts of false-positives.
+ *
+ * There's also compiler generated _THIS_IP_ through
+ * KCOV and such which we have no hope of annotating.
+ *
+ * As such, blanket accept self-references without
+ * issue.
+ */
continue;
+ }
- if (!strcmp(sec->name, "_kprobe_blacklist"))
+ if (dest->noendbr)
continue;
- is_data = strstr(sec->name, ".data") || strstr(sec->name, ".rodata");
+ WARN_FUNC("relocation to !ENDBR: %s",
+ insn->sec, insn->offset,
+ offstr(dest->sec, dest->offset, false));
- list_for_each_entry(reloc, &sec->reloc->reloc_list, list) {
- struct instruction *dest;
+ warnings++;
+ }
- dest = validate_ibt_reloc(file, reloc);
- if (is_data && dest && !dest->noendbr)
- warn_noendbr("data ", sec, reloc->offset, dest);
- }
+ return warnings;
+}
+
+static int validate_ibt_data_reloc(struct objtool_file *file,
+ struct reloc *reloc)
+{
+ struct instruction *dest;
+
+ dest = find_insn(file, reloc->sym->sec,
+ reloc->sym->offset + reloc->addend);
+ if (!dest)
+ return 0;
+
+ if (dest->type == INSN_ENDBR) {
+ mark_endbr_used(dest);
+ return 0;
}
- return 0;
+ if (dest->noendbr)
+ return 0;
+
+ WARN_FUNC("data relocation to !ENDBR: %s",
+ reloc->sec->base, reloc->offset,
+ offstr(dest->sec, dest->offset, false));
+
+ return 1;
+}
+
+
+static int validate_ibt(struct objtool_file *file)
+{
+ struct section *sec;
+ struct reloc *reloc;
+ struct instruction *insn;
+ int warnings = 0;
+
+ for_each_insn(file, insn)
+ warnings += validate_ibt_insn(file, insn);
+
+ for_each_sec(file, sec) {
+
+ if (!strstr(sec->name, ".data") && !strstr(sec->name, ".rodata"))
+ continue;
+
+ if (!sec->reloc)
+ continue;
+
+ list_for_each_entry(reloc, &sec->reloc->reloc_list, list)
+ warnings += validate_ibt_data_reloc(file, reloc);
+ }
+
+ return warnings;
}
static int validate_reachable_instructions(struct objtool_file *file)
--
2.34.1
^ permalink raw reply related [flat|nested] 41+ messages in thread* Re: [PATCH 10/18] objtool: Extricate ibt from stack validation
2022-04-13 23:19 ` [PATCH 10/18] objtool: Extricate ibt from stack validation Josh Poimboeuf
@ 2022-04-14 7:53 ` Peter Zijlstra
2022-04-14 15:44 ` Josh Poimboeuf
0 siblings, 1 reply; 41+ messages in thread
From: Peter Zijlstra @ 2022-04-14 7:53 UTC (permalink / raw)
To: Josh Poimboeuf; +Cc: x86, linux-kernel, Miroslav Benes
On Wed, Apr 13, 2022 at 04:19:45PM -0700, Josh Poimboeuf wrote:
> Extricate ibt from validate_branch() in preparation for making stack
> validation optional.
It does a bit more..
> - /* already done in validate_branch() */
> - if (sec->sh.sh_flags & SHF_EXECINSTR)
> - continue;
>
> - if (!sec->reloc)
> continue;
>
> - if (!strncmp(sec->name, ".orc", 4))
> - continue;
>
> - if (!strncmp(sec->name, ".discard", 8))
> continue;
>
> - if (!strncmp(sec->name, ".debug", 6))
> continue;
>
> - if (!strcmp(sec->name, "_error_injection_whitelist"))
> continue;
>
> - if (!strcmp(sec->name, "_kprobe_blacklist"))
> continue;
>
> - is_data = strstr(sec->name, ".data") || strstr(sec->name, ".rodata");
>
> - list_for_each_entry(reloc, &sec->reloc->reloc_list, list) {
> - struct instruction *dest;
>
> - dest = validate_ibt_reloc(file, reloc);
> - if (is_data && dest && !dest->noendbr)
> - warn_noendbr("data ", sec, reloc->offset, dest);
> - }
So this iterates all sections and excludes a bunch, and only reports
fail for .data/.rodata.
> +static int validate_ibt(struct objtool_file *file)
> +{
> + struct section *sec;
> + struct reloc *reloc;
> + struct instruction *insn;
> + int warnings = 0;
> +
> + for_each_insn(file, insn)
> + warnings += validate_ibt_insn(file, insn);
So I specifically didn't do this because I wanted to reduce the amount
of loops we do over those instructions. But yeah, if you really want to
allow --ibt without --stack-validate (but why?) then I suppose so.
Esp. for the vmlinux.o case, iterating all insn can quickly add up to
significant time.
> + for_each_sec(file, sec) {
> +
> + if (!strstr(sec->name, ".data") && !strstr(sec->name, ".rodata"))
> + continue;
But this only iterates .data/.rodata.
That's not the same, specifically, it'll not iterate stuff like ksymtab
that contains the EXPORT_SYMBOL* crud. The result being that we can now
seal EXPORT'ed symbols, which will make modules really sad.
There's also the .initcall sections, sealing initcalls typcally ends really
badly.
And there might be a few others I forgot about.
> + if (!sec->reloc)
> + continue;
> +
> + list_for_each_entry(reloc, &sec->reloc->reloc_list, list)
> + warnings += validate_ibt_data_reloc(file, reloc);
> + }
> +
> + return warnings;
> }
>
> static int validate_reachable_instructions(struct objtool_file *file)
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [PATCH 10/18] objtool: Extricate ibt from stack validation
2022-04-14 7:53 ` Peter Zijlstra
@ 2022-04-14 15:44 ` Josh Poimboeuf
2022-04-14 16:38 ` Peter Zijlstra
0 siblings, 1 reply; 41+ messages in thread
From: Josh Poimboeuf @ 2022-04-14 15:44 UTC (permalink / raw)
To: Peter Zijlstra; +Cc: x86, linux-kernel, Miroslav Benes
On Thu, Apr 14, 2022 at 09:53:18AM +0200, Peter Zijlstra wrote:
> On Wed, Apr 13, 2022 at 04:19:45PM -0700, Josh Poimboeuf wrote:
> > Extricate ibt from validate_branch() in preparation for making stack
> > validation optional.
>
> It does a bit more..
Indeed.
> > - /* already done in validate_branch() */
> > - if (sec->sh.sh_flags & SHF_EXECINSTR)
> > - continue;
> >
> > - if (!sec->reloc)
> > continue;
> >
> > - if (!strncmp(sec->name, ".orc", 4))
> > - continue;
> >
> > - if (!strncmp(sec->name, ".discard", 8))
> > continue;
> >
> > - if (!strncmp(sec->name, ".debug", 6))
> > continue;
> >
> > - if (!strcmp(sec->name, "_error_injection_whitelist"))
> > continue;
> >
> > - if (!strcmp(sec->name, "_kprobe_blacklist"))
> > continue;
> >
> > - is_data = strstr(sec->name, ".data") || strstr(sec->name, ".rodata");
> >
> > - list_for_each_entry(reloc, &sec->reloc->reloc_list, list) {
> > - struct instruction *dest;
> >
> > - dest = validate_ibt_reloc(file, reloc);
> > - if (is_data && dest && !dest->noendbr)
> > - warn_noendbr("data ", sec, reloc->offset, dest);
> > - }
>
> So this iterates all sections and excludes a bunch, and only reports
> fail for .data/.rodata.
Oops.
> > +static int validate_ibt(struct objtool_file *file)
> > +{
> > + struct section *sec;
> > + struct reloc *reloc;
> > + struct instruction *insn;
> > + int warnings = 0;
> > +
> > + for_each_insn(file, insn)
> > + warnings += validate_ibt_insn(file, insn);
>
> So I specifically didn't do this because I wanted to reduce the amount
> of loops we do over those instructions. But yeah, if you really want to
> allow --ibt without --stack-validate (but why?) then I suppose so.
>
> Esp. for the vmlinux.o case, iterating all insn can quickly add up to
> significant time.
I didn't look at the performance, but if it's a problem then we can
eventually look at combining several of the for_each_insn() features
into a single loop: retpolines, ibt, reachability check, maybe even some
of decode_sections().
>
> > + for_each_sec(file, sec) {
> > +
> > + if (!strstr(sec->name, ".data") && !strstr(sec->name, ".rodata"))
> > + continue;
>
> But this only iterates .data/.rodata.
>
> That's not the same, specifically, it'll not iterate stuff like ksymtab
> that contains the EXPORT_SYMBOL* crud. The result being that we can now
> seal EXPORT'ed symbols, which will make modules really sad.
>
> There's also the .initcall sections, sealing initcalls typcally ends really
> badly.
>
> And there might be a few others I forgot about.
Ok. That was subtle, it needs a comment or two. I had the distinct
feeling I was introducing a bug, then I got distracted ;-)
Doesn't the compiler give those special cases ENDBR anyway? Just
wondering why we avoid the warning for those.
--
Josh
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [PATCH 10/18] objtool: Extricate ibt from stack validation
2022-04-14 15:44 ` Josh Poimboeuf
@ 2022-04-14 16:38 ` Peter Zijlstra
2022-04-14 17:05 ` Josh Poimboeuf
0 siblings, 1 reply; 41+ messages in thread
From: Peter Zijlstra @ 2022-04-14 16:38 UTC (permalink / raw)
To: Josh Poimboeuf; +Cc: x86, linux-kernel, Miroslav Benes
On Thu, Apr 14, 2022 at 08:44:49AM -0700, Josh Poimboeuf wrote:
> Ok. That was subtle, it needs a comment or two. I had the distinct
> feeling I was introducing a bug, then I got distracted ;-)
Right, lemme try and not forget to write one ;-)
> Doesn't the compiler give those special cases ENDBR anyway? Just
> wondering why we avoid the warning for those.
Sure, but this is about not scribbling that ENDBR with a NOP.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 10/18] objtool: Extricate ibt from stack validation
2022-04-14 16:38 ` Peter Zijlstra
@ 2022-04-14 17:05 ` Josh Poimboeuf
2022-04-14 18:25 ` Josh Poimboeuf
2022-04-14 18:49 ` Peter Zijlstra
0 siblings, 2 replies; 41+ messages in thread
From: Josh Poimboeuf @ 2022-04-14 17:05 UTC (permalink / raw)
To: Peter Zijlstra; +Cc: x86, linux-kernel, Miroslav Benes
On Thu, Apr 14, 2022 at 06:38:16PM +0200, Peter Zijlstra wrote:
> On Thu, Apr 14, 2022 at 08:44:49AM -0700, Josh Poimboeuf wrote:
>
> > Ok. That was subtle, it needs a comment or two. I had the distinct
> > feeling I was introducing a bug, then I got distracted ;-)
>
> Right, lemme try and not forget to write one ;-)
I'm rewriting the code anyway, I'll add some comments.
> > Doesn't the compiler give those special cases ENDBR anyway? Just
> > wondering why we avoid the warning for those.
>
> Sure, but this is about not scribbling that ENDBR with a NOP.
Right, but it only prints warnings for data sections, despite marking
others:
- dest = validate_ibt_reloc(file, reloc);
- if (is_data && dest && !dest->noendbr)
- warn_noendbr("data ", sec, reloc->offset, dest);
--
Josh
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [PATCH 10/18] objtool: Extricate ibt from stack validation
2022-04-14 17:05 ` Josh Poimboeuf
@ 2022-04-14 18:25 ` Josh Poimboeuf
2022-04-14 19:01 ` Peter Zijlstra
2022-04-14 18:49 ` Peter Zijlstra
1 sibling, 1 reply; 41+ messages in thread
From: Josh Poimboeuf @ 2022-04-14 18:25 UTC (permalink / raw)
To: Peter Zijlstra; +Cc: x86, linux-kernel, Miroslav Benes
On Thu, Apr 14, 2022 at 10:05:53AM -0700, Josh Poimboeuf wrote:
> On Thu, Apr 14, 2022 at 06:38:16PM +0200, Peter Zijlstra wrote:
> > On Thu, Apr 14, 2022 at 08:44:49AM -0700, Josh Poimboeuf wrote:
> >
> > > Ok. That was subtle, it needs a comment or two. I had the distinct
> > > feeling I was introducing a bug, then I got distracted ;-)
> >
> > Right, lemme try and not forget to write one ;-)
>
> I'm rewriting the code anyway, I'll add some comments.
>
> > > Doesn't the compiler give those special cases ENDBR anyway? Just
> > > wondering why we avoid the warning for those.
> >
> > Sure, but this is about not scribbling that ENDBR with a NOP.
>
> Right, but it only prints warnings for data sections, despite marking
> others:
>
> - dest = validate_ibt_reloc(file, reloc);
> - if (is_data && dest && !dest->noendbr)
> - warn_noendbr("data ", sec, reloc->offset, dest);
How about this? This way it doesn't silence warnings for non
.data/.rodata, as other sections (including ksymtab) can also have valid
function pointers. It also caught a bug(?) in putuser.S, as some of the
exported inner labels didn't have ENDBR.
diff --git a/arch/x86/include/asm/static_call.h b/arch/x86/include/asm/static_call.h
index 2455d721503e..2d8dacd02643 100644
--- a/arch/x86/include/asm/static_call.h
+++ b/arch/x86/include/asm/static_call.h
@@ -26,6 +26,7 @@
".align 4 \n" \
".globl " STATIC_CALL_TRAMP_STR(name) " \n" \
STATIC_CALL_TRAMP_STR(name) ": \n" \
+ ANNOTATE_NOENDBR \
insns " \n" \
".byte 0x53, 0x43, 0x54 \n" \
".type " STATIC_CALL_TRAMP_STR(name) ", @function \n" \
diff --git a/arch/x86/lib/putuser.S b/arch/x86/lib/putuser.S
index ecb2049c1273..b7dfd60243b7 100644
--- a/arch/x86/lib/putuser.S
+++ b/arch/x86/lib/putuser.S
@@ -48,6 +48,7 @@ SYM_FUNC_START(__put_user_1)
cmp %_ASM_BX,%_ASM_CX
jae .Lbad_put_user
SYM_INNER_LABEL(__put_user_nocheck_1, SYM_L_GLOBAL)
+ ENDBR
ASM_STAC
1: movb %al,(%_ASM_CX)
xor %ecx,%ecx
@@ -62,6 +63,7 @@ SYM_FUNC_START(__put_user_2)
cmp %_ASM_BX,%_ASM_CX
jae .Lbad_put_user
SYM_INNER_LABEL(__put_user_nocheck_2, SYM_L_GLOBAL)
+ ENDBR
ASM_STAC
2: movw %ax,(%_ASM_CX)
xor %ecx,%ecx
@@ -76,6 +78,7 @@ SYM_FUNC_START(__put_user_4)
cmp %_ASM_BX,%_ASM_CX
jae .Lbad_put_user
SYM_INNER_LABEL(__put_user_nocheck_4, SYM_L_GLOBAL)
+ ENDBR
ASM_STAC
3: movl %eax,(%_ASM_CX)
xor %ecx,%ecx
@@ -90,6 +93,7 @@ SYM_FUNC_START(__put_user_8)
cmp %_ASM_BX,%_ASM_CX
jae .Lbad_put_user
SYM_INNER_LABEL(__put_user_nocheck_8, SYM_L_GLOBAL)
+ ENDBR
ASM_STAC
4: mov %_ASM_AX,(%_ASM_CX)
#ifdef CONFIG_X86_32
diff --git a/arch/x86/lib/retpoline.S b/arch/x86/lib/retpoline.S
index 5f87bab4fb8d..b2b2366885a2 100644
--- a/arch/x86/lib/retpoline.S
+++ b/arch/x86/lib/retpoline.S
@@ -31,6 +31,7 @@
.align RETPOLINE_THUNK_SIZE
SYM_INNER_LABEL(__x86_indirect_thunk_\reg, SYM_L_GLOBAL)
UNWIND_HINT_EMPTY
+ ANNOTATE_NOENDBR
ALTERNATIVE_2 __stringify(ANNOTATE_RETPOLINE_SAFE; jmp *%\reg), \
__stringify(RETPOLINE \reg), X86_FEATURE_RETPOLINE, \
@@ -55,7 +56,6 @@ SYM_INNER_LABEL(__x86_indirect_thunk_\reg, SYM_L_GLOBAL)
.align RETPOLINE_THUNK_SIZE
SYM_CODE_START(__x86_indirect_thunk_array)
- ANNOTATE_NOENDBR // apply_retpolines
#define GEN(reg) THUNK reg
#include <asm/GEN-for-each-reg.h>
diff --git a/arch/x86/xen/xen-head.S b/arch/x86/xen/xen-head.S
index ac17196e2518..3a2cd93bf059 100644
--- a/arch/x86/xen/xen-head.S
+++ b/arch/x86/xen/xen-head.S
@@ -45,6 +45,7 @@ SYM_CODE_END(hypercall_page)
__INIT
SYM_CODE_START(startup_xen)
UNWIND_HINT_EMPTY
+ ANNOTATE_NOENDBR
cld
/* Clear .bss */
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index b09c416432b6..10e375c84088 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -3794,7 +3794,10 @@ static int validate_ibt_data_reloc(struct objtool_file *file,
return 1;
}
-
+/*
+ * Validate IBT rules and mark used ENDBR instructions so the non-used ones can
+ * be sealed later by create_ibt_endbr_seal_sections().
+ */
static int validate_ibt(struct objtool_file *file)
{
struct section *sec;
@@ -3807,12 +3810,36 @@ static int validate_ibt(struct objtool_file *file)
for_each_sec(file, sec) {
- if (!strstr(sec->name, ".data") && !strstr(sec->name, ".rodata"))
+ /* Already done by validate_ibt_insn() */
+ if (sec->sh.sh_flags & SHF_EXECINSTR)
continue;
if (!sec->reloc)
continue;
+ /*
+ * These sections can reference text addresses, but not with
+ * the intent to indirect branch to them.
+ */
+ if (!strncmp(sec->name, ".discard", 8) ||
+ !strncmp(sec->name, ".debug", 6) ||
+ !strcmp(sec->name, ".altinstructions") ||
+ !strcmp(sec->name, ".ibt_endbr_seal") ||
+ !strcmp(sec->name, ".orc_unwind_ip") ||
+ !strcmp(sec->name, ".parainstructions") ||
+ !strcmp(sec->name, ".retpoline_sites") ||
+ !strcmp(sec->name, ".smp_locks") ||
+ !strcmp(sec->name, ".static_call_sites") ||
+ !strcmp(sec->name, ".static_call_tramp_key") ||
+ !strcmp(sec->name, "_error_injection_whitelist") ||
+ !strcmp(sec->name, "_kprobe_blacklist") ||
+ !strcmp(sec->name, "__bug_table") ||
+ !strcmp(sec->name, "__ex_table") ||
+ !strcmp(sec->name, "__jump_table") ||
+ !strcmp(sec->name, "__mcount_loc") ||
+ !strcmp(sec->name, "__tracepoints"))
+ continue;
+
list_for_each_entry(reloc, &sec->reloc->reloc_list, list)
warnings += validate_ibt_data_reloc(file, reloc);
}
^ permalink raw reply related [flat|nested] 41+ messages in thread* Re: [PATCH 10/18] objtool: Extricate ibt from stack validation
2022-04-14 18:25 ` Josh Poimboeuf
@ 2022-04-14 19:01 ` Peter Zijlstra
2022-04-14 19:07 ` Josh Poimboeuf
0 siblings, 1 reply; 41+ messages in thread
From: Peter Zijlstra @ 2022-04-14 19:01 UTC (permalink / raw)
To: Josh Poimboeuf; +Cc: x86, linux-kernel, Miroslav Benes
On Thu, Apr 14, 2022 at 11:25:05AM -0700, Josh Poimboeuf wrote:
> On Thu, Apr 14, 2022 at 10:05:53AM -0700, Josh Poimboeuf wrote:
> > On Thu, Apr 14, 2022 at 06:38:16PM +0200, Peter Zijlstra wrote:
> > > On Thu, Apr 14, 2022 at 08:44:49AM -0700, Josh Poimboeuf wrote:
> > >
> > > > Ok. That was subtle, it needs a comment or two. I had the distinct
> > > > feeling I was introducing a bug, then I got distracted ;-)
> > >
> > > Right, lemme try and not forget to write one ;-)
> >
> > I'm rewriting the code anyway, I'll add some comments.
> >
> > > > Doesn't the compiler give those special cases ENDBR anyway? Just
> > > > wondering why we avoid the warning for those.
> > >
> > > Sure, but this is about not scribbling that ENDBR with a NOP.
> >
> > Right, but it only prints warnings for data sections, despite marking
> > others:
> >
> > - dest = validate_ibt_reloc(file, reloc);
> > - if (is_data && dest && !dest->noendbr)
> > - warn_noendbr("data ", sec, reloc->offset, dest);
>
>
> How about this? This way it doesn't silence warnings for non
> .data/.rodata, as other sections (including ksymtab) can also have valid
> function pointers. It also caught a bug(?) in putuser.S, as some of the
> exported inner labels didn't have ENDBR.
>
>
> diff --git a/arch/x86/include/asm/static_call.h b/arch/x86/include/asm/static_call.h
> index 2455d721503e..2d8dacd02643 100644
> --- a/arch/x86/include/asm/static_call.h
> +++ b/arch/x86/include/asm/static_call.h
> @@ -26,6 +26,7 @@
> ".align 4 \n" \
> ".globl " STATIC_CALL_TRAMP_STR(name) " \n" \
> STATIC_CALL_TRAMP_STR(name) ": \n" \
> + ANNOTATE_NOENDBR \
> insns " \n" \
> ".byte 0x53, 0x43, 0x54 \n" \
> ".type " STATIC_CALL_TRAMP_STR(name) ", @function \n" \
Right, that makes more sense than hard-coding that exclusion, no idea
what I was thinking ;-)
> diff --git a/arch/x86/lib/putuser.S b/arch/x86/lib/putuser.S
> index ecb2049c1273..b7dfd60243b7 100644
> --- a/arch/x86/lib/putuser.S
> +++ b/arch/x86/lib/putuser.S
> @@ -48,6 +48,7 @@ SYM_FUNC_START(__put_user_1)
> cmp %_ASM_BX,%_ASM_CX
> jae .Lbad_put_user
> SYM_INNER_LABEL(__put_user_nocheck_1, SYM_L_GLOBAL)
> + ENDBR
> ASM_STAC
> 1: movb %al,(%_ASM_CX)
> xor %ecx,%ecx
> @@ -62,6 +63,7 @@ SYM_FUNC_START(__put_user_2)
> cmp %_ASM_BX,%_ASM_CX
> jae .Lbad_put_user
> SYM_INNER_LABEL(__put_user_nocheck_2, SYM_L_GLOBAL)
> + ENDBR
> ASM_STAC
> 2: movw %ax,(%_ASM_CX)
> xor %ecx,%ecx
> @@ -76,6 +78,7 @@ SYM_FUNC_START(__put_user_4)
> cmp %_ASM_BX,%_ASM_CX
> jae .Lbad_put_user
> SYM_INNER_LABEL(__put_user_nocheck_4, SYM_L_GLOBAL)
> + ENDBR
> ASM_STAC
> 3: movl %eax,(%_ASM_CX)
> xor %ecx,%ecx
> @@ -90,6 +93,7 @@ SYM_FUNC_START(__put_user_8)
> cmp %_ASM_BX,%_ASM_CX
> jae .Lbad_put_user
> SYM_INNER_LABEL(__put_user_nocheck_8, SYM_L_GLOBAL)
> + ENDBR
> ASM_STAC
> 4: mov %_ASM_AX,(%_ASM_CX)
> #ifdef CONFIG_X86_32
Hmm, how did those go missing?
> diff --git a/arch/x86/lib/retpoline.S b/arch/x86/lib/retpoline.S
> index 5f87bab4fb8d..b2b2366885a2 100644
> --- a/arch/x86/lib/retpoline.S
> +++ b/arch/x86/lib/retpoline.S
> @@ -31,6 +31,7 @@
> .align RETPOLINE_THUNK_SIZE
> SYM_INNER_LABEL(__x86_indirect_thunk_\reg, SYM_L_GLOBAL)
> UNWIND_HINT_EMPTY
> + ANNOTATE_NOENDBR
>
> ALTERNATIVE_2 __stringify(ANNOTATE_RETPOLINE_SAFE; jmp *%\reg), \
> __stringify(RETPOLINE \reg), X86_FEATURE_RETPOLINE, \
> @@ -55,7 +56,6 @@ SYM_INNER_LABEL(__x86_indirect_thunk_\reg, SYM_L_GLOBAL)
>
> .align RETPOLINE_THUNK_SIZE
> SYM_CODE_START(__x86_indirect_thunk_array)
> - ANNOTATE_NOENDBR // apply_retpolines
>
> #define GEN(reg) THUNK reg
> #include <asm/GEN-for-each-reg.h>
Hmm, where does that come from? Do you have commit be8a096521ca
("x86,bpf: Avoid IBT objtool warning")?
> diff --git a/arch/x86/xen/xen-head.S b/arch/x86/xen/xen-head.S
> index ac17196e2518..3a2cd93bf059 100644
> --- a/arch/x86/xen/xen-head.S
> +++ b/arch/x86/xen/xen-head.S
> @@ -45,6 +45,7 @@ SYM_CODE_END(hypercall_page)
> __INIT
> SYM_CODE_START(startup_xen)
> UNWIND_HINT_EMPTY
> + ANNOTATE_NOENDBR
> cld
>
> /* Clear .bss */
> diff --git a/tools/objtool/check.c b/tools/objtool/check.c
> index b09c416432b6..10e375c84088 100644
> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
> @@ -3794,7 +3794,10 @@ static int validate_ibt_data_reloc(struct objtool_file *file,
> return 1;
> }
>
> -
> +/*
> + * Validate IBT rules and mark used ENDBR instructions so the non-used ones can
> + * be sealed later by create_ibt_endbr_seal_sections().
> + */
> static int validate_ibt(struct objtool_file *file)
> {
> struct section *sec;
> @@ -3807,12 +3810,36 @@ static int validate_ibt(struct objtool_file *file)
>
> for_each_sec(file, sec) {
>
> - if (!strstr(sec->name, ".data") && !strstr(sec->name, ".rodata"))
> + /* Already done by validate_ibt_insn() */
> + if (sec->sh.sh_flags & SHF_EXECINSTR)
> continue;
>
> if (!sec->reloc)
> continue;
>
> + /*
> + * These sections can reference text addresses, but not with
> + * the intent to indirect branch to them.
> + */
> + if (!strncmp(sec->name, ".discard", 8) ||
> + !strncmp(sec->name, ".debug", 6) ||
> + !strcmp(sec->name, ".altinstructions") ||
> + !strcmp(sec->name, ".ibt_endbr_seal") ||
> + !strcmp(sec->name, ".orc_unwind_ip") ||
> + !strcmp(sec->name, ".parainstructions") ||
> + !strcmp(sec->name, ".retpoline_sites") ||
> + !strcmp(sec->name, ".smp_locks") ||
> + !strcmp(sec->name, ".static_call_sites") ||
> + !strcmp(sec->name, ".static_call_tramp_key") ||
> + !strcmp(sec->name, "_error_injection_whitelist") ||
> + !strcmp(sec->name, "_kprobe_blacklist") ||
> + !strcmp(sec->name, "__bug_table") ||
> + !strcmp(sec->name, "__ex_table") ||
> + !strcmp(sec->name, "__jump_table") ||
> + !strcmp(sec->name, "__mcount_loc") ||
> + !strcmp(sec->name, "__tracepoints"))
> + continue;
> +
> list_for_each_entry(reloc, &sec->reloc->reloc_list, list)
> warnings += validate_ibt_data_reloc(file, reloc);
> }
>
Yes, looks good, Thanks!
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [PATCH 10/18] objtool: Extricate ibt from stack validation
2022-04-14 19:01 ` Peter Zijlstra
@ 2022-04-14 19:07 ` Josh Poimboeuf
0 siblings, 0 replies; 41+ messages in thread
From: Josh Poimboeuf @ 2022-04-14 19:07 UTC (permalink / raw)
To: Peter Zijlstra; +Cc: x86, linux-kernel, Miroslav Benes
On Thu, Apr 14, 2022 at 09:01:56PM +0200, Peter Zijlstra wrote:
> > diff --git a/arch/x86/lib/putuser.S b/arch/x86/lib/putuser.S
> > index ecb2049c1273..b7dfd60243b7 100644
> > --- a/arch/x86/lib/putuser.S
> > +++ b/arch/x86/lib/putuser.S
> > @@ -48,6 +48,7 @@ SYM_FUNC_START(__put_user_1)
> > cmp %_ASM_BX,%_ASM_CX
> > jae .Lbad_put_user
> > SYM_INNER_LABEL(__put_user_nocheck_1, SYM_L_GLOBAL)
> > + ENDBR
> > ASM_STAC
> > 1: movb %al,(%_ASM_CX)
> > xor %ecx,%ecx
> > @@ -62,6 +63,7 @@ SYM_FUNC_START(__put_user_2)
> > cmp %_ASM_BX,%_ASM_CX
> > jae .Lbad_put_user
> > SYM_INNER_LABEL(__put_user_nocheck_2, SYM_L_GLOBAL)
> > + ENDBR
> > ASM_STAC
> > 2: movw %ax,(%_ASM_CX)
> > xor %ecx,%ecx
> > @@ -76,6 +78,7 @@ SYM_FUNC_START(__put_user_4)
> > cmp %_ASM_BX,%_ASM_CX
> > jae .Lbad_put_user
> > SYM_INNER_LABEL(__put_user_nocheck_4, SYM_L_GLOBAL)
> > + ENDBR
> > ASM_STAC
> > 3: movl %eax,(%_ASM_CX)
> > xor %ecx,%ecx
> > @@ -90,6 +93,7 @@ SYM_FUNC_START(__put_user_8)
> > cmp %_ASM_BX,%_ASM_CX
> > jae .Lbad_put_user
> > SYM_INNER_LABEL(__put_user_nocheck_8, SYM_L_GLOBAL)
> > + ENDBR
> > ASM_STAC
> > 4: mov %_ASM_AX,(%_ASM_CX)
> > #ifdef CONFIG_X86_32
>
> Hmm, how did those go missing?
Because the current code only warns about references from .data/.rodata.
This patch changes that.
> > diff --git a/arch/x86/lib/retpoline.S b/arch/x86/lib/retpoline.S
> > index 5f87bab4fb8d..b2b2366885a2 100644
> > --- a/arch/x86/lib/retpoline.S
> > +++ b/arch/x86/lib/retpoline.S
> > @@ -31,6 +31,7 @@
> > .align RETPOLINE_THUNK_SIZE
> > SYM_INNER_LABEL(__x86_indirect_thunk_\reg, SYM_L_GLOBAL)
> > UNWIND_HINT_EMPTY
> > + ANNOTATE_NOENDBR
> >
> > ALTERNATIVE_2 __stringify(ANNOTATE_RETPOLINE_SAFE; jmp *%\reg), \
> > __stringify(RETPOLINE \reg), X86_FEATURE_RETPOLINE, \
> > @@ -55,7 +56,6 @@ SYM_INNER_LABEL(__x86_indirect_thunk_\reg, SYM_L_GLOBAL)
> >
> > .align RETPOLINE_THUNK_SIZE
> > SYM_CODE_START(__x86_indirect_thunk_array)
> > - ANNOTATE_NOENDBR // apply_retpolines
> >
> > #define GEN(reg) THUNK reg
> > #include <asm/GEN-for-each-reg.h>
>
> Hmm, where does that come from? Do you have commit be8a096521ca
> ("x86,bpf: Avoid IBT objtool warning")?
Same answer as above, this patch now warns about exported symbols with
missing ENDBR.
--
Josh
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 10/18] objtool: Extricate ibt from stack validation
2022-04-14 17:05 ` Josh Poimboeuf
2022-04-14 18:25 ` Josh Poimboeuf
@ 2022-04-14 18:49 ` Peter Zijlstra
1 sibling, 0 replies; 41+ messages in thread
From: Peter Zijlstra @ 2022-04-14 18:49 UTC (permalink / raw)
To: Josh Poimboeuf; +Cc: x86, linux-kernel, Miroslav Benes
On Thu, Apr 14, 2022 at 10:05:50AM -0700, Josh Poimboeuf wrote:
> On Thu, Apr 14, 2022 at 06:38:16PM +0200, Peter Zijlstra wrote:
> > On Thu, Apr 14, 2022 at 08:44:49AM -0700, Josh Poimboeuf wrote:
> >
> > > Ok. That was subtle, it needs a comment or two. I had the distinct
> > > feeling I was introducing a bug, then I got distracted ;-)
> >
> > Right, lemme try and not forget to write one ;-)
>
> I'm rewriting the code anyway, I'll add some comments.
>
> > > Doesn't the compiler give those special cases ENDBR anyway? Just
> > > wondering why we avoid the warning for those.
> >
> > Sure, but this is about not scribbling that ENDBR with a NOP.
>
> Right, but it only prints warnings for data sections, despite marking
> others:
>
> - dest = validate_ibt_reloc(file, reloc);
> - if (is_data && dest && !dest->noendbr)
> - warn_noendbr("data ", sec, reloc->offset, dest);
>
Right, so validate_ibt_reloc() does two things:
- it removes the instruction from the seal list (if it was still on it)
- it returns the dest instruction if it isn't ENDBR (or a static call
trampoline, which we know will never be indirectly called).
So that first thing is always important; we should not seal too many
things (.initcall and .ksymtab would be bad etc..), that second thing is
only useful if there is potential control flow.
That is, .ksymtab is not used for control flow (directly), it's the
symbol table for the module linker. OTOH .initcall does have control
flow, but it is 100% control flow.
In neither case does it really make sense to warn (then again, they also
should not trigger warns I suppose).
.data/.rodata otoh is more questionable, typically a code reference
there is a function pointer and we really want an ENDBR there, but it's
not always the case, hence the warns from there.
Now, I have some vague memories of getting a lot of noise from some
other sections, but I can't really remember now :-/
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH 11/18] objtool: Add CONFIG_OBJTOOL
2022-04-13 23:19 [PATCH 00/18] objtool: Interface overhaul Josh Poimboeuf
` (9 preceding siblings ...)
2022-04-13 23:19 ` [PATCH 10/18] objtool: Extricate ibt from stack validation Josh Poimboeuf
@ 2022-04-13 23:19 ` Josh Poimboeuf
2022-04-13 23:19 ` [PATCH 12/18] objtool: Make stack validation frame-pointer-specific Josh Poimboeuf
` (6 subsequent siblings)
17 siblings, 0 replies; 41+ messages in thread
From: Josh Poimboeuf @ 2022-04-13 23:19 UTC (permalink / raw)
To: x86; +Cc: Peter Zijlstra, linux-kernel, Miroslav Benes
Now that stack validation is an optional feature of objtool, add
CONFIG_OBJTOOL and replace most usages of CONFIG_STACK_VALIDATION with
it.
CONFIG_STACK_VALIDATION can now be considered to be frame-pointer
specific. CONFIG_UNWINDER_ORC is already inherently valid for live
patching, so no need to "validate" it.
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
Makefile | 2 +-
arch/Kconfig | 8 ++++++--
arch/x86/Kconfig | 18 ++++++++++-------
arch/x86/Kconfig.debug | 2 +-
arch/x86/include/asm/jump_label.h | 6 +++---
arch/x86/kernel/alternative.c | 6 +++---
include/linux/compiler.h | 6 +++---
include/linux/instrumentation.h | 6 +++---
include/linux/objtool.h | 6 +++---
kernel/trace/Kconfig | 1 +
lib/Kconfig.debug | 20 ++++++++++---------
lib/Kconfig.kcsan | 3 ++-
lib/Kconfig.ubsan | 2 +-
scripts/Makefile.build | 4 ++--
scripts/link-vmlinux.sh | 32 +++++++++++++++++--------------
scripts/package/builddeb | 2 +-
tools/include/linux/objtool.h | 6 +++---
17 files changed, 73 insertions(+), 57 deletions(-)
diff --git a/Makefile b/Makefile
index 29e273d3f8cc..707dfbf643a2 100644
--- a/Makefile
+++ b/Makefile
@@ -1302,7 +1302,7 @@ install: sub_make_done :=
# ---------------------------------------------------------------------------
# Tools
-ifdef CONFIG_STACK_VALIDATION
+ifdef CONFIG_OBJTOOL
prepare: tools/objtool
endif
diff --git a/arch/Kconfig b/arch/Kconfig
index 29b0167c088b..04cdef16db24 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -1028,11 +1028,14 @@ config ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT
depends on MMU
select ARCH_HAS_ELF_RANDOMIZE
+config HAVE_OBJTOOL
+ bool
+
config HAVE_STACK_VALIDATION
bool
help
- Architecture supports the 'objtool check' host tool command, which
- performs compile-time stack metadata validation.
+ Architecture supports objtool compile-time frame pointer rule
+ validation.
config HAVE_RELIABLE_STACKTRACE
bool
@@ -1302,6 +1305,7 @@ config HAVE_STATIC_CALL
config HAVE_STATIC_CALL_INLINE
bool
depends on HAVE_STATIC_CALL
+ select OBJTOOL
config HAVE_PREEMPT_DYNAMIC
bool
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index b0142e01002e..bce0c23f3550 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -188,7 +188,7 @@ config X86
select HAVE_CONTEXT_TRACKING if X86_64
select HAVE_CONTEXT_TRACKING_OFFSTACK if HAVE_CONTEXT_TRACKING
select HAVE_C_RECORDMCOUNT
- select HAVE_OBJTOOL_MCOUNT if STACK_VALIDATION
+ select HAVE_OBJTOOL_MCOUNT if HAVE_OBJTOOL
select HAVE_BUILDTIME_MCOUNT_SORT
select HAVE_DEBUG_KMEMLEAK
select HAVE_DMA_CONTIGUOUS
@@ -231,6 +231,7 @@ config X86
select HAVE_MOVE_PMD
select HAVE_MOVE_PUD
select HAVE_NMI
+ select HAVE_OBJTOOL if X86_64
select HAVE_OPTPROBES
select HAVE_PCSPKR_PLATFORM
select HAVE_PERF_EVENTS
@@ -239,17 +240,17 @@ config X86
select HAVE_PCI
select HAVE_PERF_REGS
select HAVE_PERF_USER_STACK_DUMP
- select MMU_GATHER_RCU_TABLE_FREE if PARAVIRT
+ select MMU_GATHER_RCU_TABLE_FREE if PARAVIRT
select HAVE_POSIX_CPU_TIMERS_TASK_WORK
select HAVE_REGS_AND_STACK_ACCESS_API
- select HAVE_RELIABLE_STACKTRACE if X86_64 && (UNWINDER_FRAME_POINTER || UNWINDER_ORC) && STACK_VALIDATION
+ select HAVE_RELIABLE_STACKTRACE if UNWINDER_ORC || STACK_VALIDATION
select HAVE_FUNCTION_ARG_ACCESS_API
select HAVE_SETUP_PER_CPU_AREA
select HAVE_SOFTIRQ_ON_OWN_STACK
select HAVE_STACKPROTECTOR if CC_HAS_SANE_STACKPROTECTOR
- select HAVE_STACK_VALIDATION if X86_64
+ select HAVE_STACK_VALIDATION if HAVE_OBJTOOL
select HAVE_STATIC_CALL
- select HAVE_STATIC_CALL_INLINE if HAVE_STACK_VALIDATION
+ select HAVE_STATIC_CALL_INLINE if HAVE_OBJTOOL
select HAVE_PREEMPT_DYNAMIC_CALL
select HAVE_RSEQ
select HAVE_SYSCALL_TRACEPOINTS
@@ -268,7 +269,6 @@ config X86
select RTC_MC146818_LIB
select SPARSE_IRQ
select SRCU
- select STACK_VALIDATION if HAVE_STACK_VALIDATION && (HAVE_STATIC_CALL_INLINE || RETPOLINE)
select SYSCTL_EXCEPTION_TRACE
select THREAD_INFO_IN_TASK
select TRACE_IRQFLAGS_SUPPORT
@@ -459,6 +459,7 @@ config GOLDFISH
config RETPOLINE
bool "Avoid speculative indirect branches in kernel"
+ select OBJTOOL if HAVE_OBJTOOL
default y
help
Compile kernel with the retpoline compiler options to guard against
@@ -472,6 +473,7 @@ config CC_HAS_SLS
config SLS
bool "Mitigate Straight-Line-Speculation"
depends on CC_HAS_SLS && X86_64
+ select OBJTOOL if HAVE_OBJTOOL
default n
help
Compile the kernel with straight-line-speculation options to guard
@@ -1819,6 +1821,7 @@ config ARCH_RANDOM
config X86_SMAP
def_bool y
prompt "Supervisor Mode Access Prevention" if EXPERT
+ select OBJTOOL if HAVE_OBJTOOL
help
Supervisor Mode Access Prevention (SMAP) is a security
feature in newer Intel processors. There is a small
@@ -1855,9 +1858,10 @@ config CC_HAS_IBT
config X86_KERNEL_IBT
prompt "Indirect Branch Tracking"
bool
- depends on X86_64 && CC_HAS_IBT && STACK_VALIDATION
+ depends on X86_64 && CC_HAS_IBT && HAVE_OBJTOOL
# https://github.com/llvm/llvm-project/commit/9d7001eba9c4cb311e03cd8cdc231f9e579f2d0f
depends on !LD_IS_LLD || LLD_VERSION >= 140000
+ select OBJTOOL
help
Build the kernel with support for Indirect Branch Tracking, a
hardware support course-grain forward-edge Control Flow Integrity
diff --git a/arch/x86/Kconfig.debug b/arch/x86/Kconfig.debug
index d3a6f74a94bd..d872a7522e55 100644
--- a/arch/x86/Kconfig.debug
+++ b/arch/x86/Kconfig.debug
@@ -237,7 +237,7 @@ choice
config UNWINDER_ORC
bool "ORC unwinder"
depends on X86_64
- select STACK_VALIDATION
+ select OBJTOOL
help
This option enables the ORC (Oops Rewind Capability) unwinder for
unwinding kernel stack traces. It uses a custom data format which is
diff --git a/arch/x86/include/asm/jump_label.h b/arch/x86/include/asm/jump_label.h
index 0449b125d27f..3ce0e67c579c 100644
--- a/arch/x86/include/asm/jump_label.h
+++ b/arch/x86/include/asm/jump_label.h
@@ -20,7 +20,7 @@
_ASM_PTR "%c0 + %c1 - .\n\t" \
".popsection \n\t"
-#ifdef CONFIG_STACK_VALIDATION
+#ifdef CONFIG_OBJTOOL
static __always_inline bool arch_static_branch(struct static_key *key, bool branch)
{
@@ -34,7 +34,7 @@ static __always_inline bool arch_static_branch(struct static_key *key, bool bran
return true;
}
-#else
+#else /* !CONFIG_OBJTOOL */
static __always_inline bool arch_static_branch(struct static_key * const key, const bool branch)
{
@@ -48,7 +48,7 @@ static __always_inline bool arch_static_branch(struct static_key * const key, co
return true;
}
-#endif /* STACK_VALIDATION */
+#endif /* CONFIG_OBJTOOL */
static __always_inline bool arch_static_branch_jump(struct static_key * const key, const bool branch)
{
diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
index d374cb3cf024..3c66073e7645 100644
--- a/arch/x86/kernel/alternative.c
+++ b/arch/x86/kernel/alternative.c
@@ -338,7 +338,7 @@ void __init_or_module noinline apply_alternatives(struct alt_instr *start,
}
}
-#if defined(CONFIG_RETPOLINE) && defined(CONFIG_STACK_VALIDATION)
+#if defined(CONFIG_RETPOLINE) && defined(CONFIG_OBJTOOL)
/*
* CALL/JMP *%\reg
@@ -507,11 +507,11 @@ void __init_or_module noinline apply_retpolines(s32 *start, s32 *end)
}
}
-#else /* !RETPOLINES || !CONFIG_STACK_VALIDATION */
+#else /* !CONFIG_RETPOLINE || !CONFIG_OBJTOOL */
void __init_or_module noinline apply_retpolines(s32 *start, s32 *end) { }
-#endif /* CONFIG_RETPOLINE && CONFIG_STACK_VALIDATION */
+#endif /* CONFIG_RETPOLINE && CONFIG_OBJTOOL */
#ifdef CONFIG_X86_KERNEL_IBT
diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index 219aa5ddbc73..01ce94b58b42 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -109,7 +109,7 @@ void ftrace_likely_update(struct ftrace_likely_data *f, int val,
#endif
/* Unreachable code */
-#ifdef CONFIG_STACK_VALIDATION
+#ifdef CONFIG_OBJTOOL
/*
* These macros help objtool understand GCC code flow for unreachable code.
* The __COUNTER__ based labels are a hack to make each instance of the macros
@@ -128,10 +128,10 @@ void ftrace_likely_update(struct ftrace_likely_data *f, int val,
/* Annotate a C jump table to allow objtool to follow the code flow */
#define __annotate_jump_table __section(".rodata..c_jump_table")
-#else
+#else /* !CONFIG_OBJTOOL */
#define annotate_unreachable()
#define __annotate_jump_table
-#endif
+#endif /* CONFIG_OBJTOOL */
#ifndef unreachable
# define unreachable() do { \
diff --git a/include/linux/instrumentation.h b/include/linux/instrumentation.h
index 24359b4a9605..9111a3704072 100644
--- a/include/linux/instrumentation.h
+++ b/include/linux/instrumentation.h
@@ -2,7 +2,7 @@
#ifndef __LINUX_INSTRUMENTATION_H
#define __LINUX_INSTRUMENTATION_H
-#if defined(CONFIG_DEBUG_ENTRY) && defined(CONFIG_STACK_VALIDATION)
+#ifdef CONFIG_VMLINUX_VALIDATION
#include <linux/stringify.h>
@@ -53,9 +53,9 @@
".popsection\n\t" : : "i" (c)); \
})
#define instrumentation_end() __instrumentation_end(__COUNTER__)
-#else
+#else /* !CONFIG_VMLINUX_VALIDATION */
# define instrumentation_begin() do { } while(0)
# define instrumentation_end() do { } while(0)
-#endif
+#endif /* CONFIG_VMLINUX_VALIDATION */
#endif /* __LINUX_INSTRUMENTATION_H */
diff --git a/include/linux/objtool.h b/include/linux/objtool.h
index 586d35720f13..977d90ba642d 100644
--- a/include/linux/objtool.h
+++ b/include/linux/objtool.h
@@ -38,7 +38,7 @@ struct unwind_hint {
#define UNWIND_HINT_TYPE_REGS_PARTIAL 2
#define UNWIND_HINT_TYPE_FUNC 3
-#ifdef CONFIG_STACK_VALIDATION
+#ifdef CONFIG_OBJTOOL
#ifndef __ASSEMBLY__
@@ -157,7 +157,7 @@ struct unwind_hint {
#endif /* __ASSEMBLY__ */
-#else /* !CONFIG_STACK_VALIDATION */
+#else /* !CONFIG_OBJTOOL */
#ifndef __ASSEMBLY__
@@ -179,6 +179,6 @@ struct unwind_hint {
.endm
#endif
-#endif /* CONFIG_STACK_VALIDATION */
+#endif /* CONFIG_OBJTOOL */
#endif /* _LINUX_OBJTOOL_H */
diff --git a/kernel/trace/Kconfig b/kernel/trace/Kconfig
index 2c43e327a619..2956bc277150 100644
--- a/kernel/trace/Kconfig
+++ b/kernel/trace/Kconfig
@@ -728,6 +728,7 @@ config FTRACE_MCOUNT_USE_OBJTOOL
depends on !FTRACE_MCOUNT_USE_PATCHABLE_FUNCTION_ENTRY
depends on !FTRACE_MCOUNT_USE_CC
depends on FTRACE_MCOUNT_RECORD
+ select OBJTOOL
config FTRACE_MCOUNT_USE_RECORDMCOUNT
def_bool y
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 075cd25363ac..c0e4e47f3ce3 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -485,24 +485,25 @@ config FRAME_POINTER
larger and slower, but it gives very useful debugging information
in case of kernel bugs. (precise oopses/stacktraces/warnings)
+config OBJTOOL
+ bool
+
config STACK_VALIDATION
bool "Compile-time stack metadata validation"
- depends on HAVE_STACK_VALIDATION
+ depends on HAVE_STACK_VALIDATION && UNWINDER_FRAME_POINTER
+ select OBJTOOL
default n
help
- Add compile-time checks to validate stack metadata, including frame
- pointers (if CONFIG_FRAME_POINTER is enabled). This helps ensure
- that runtime stack traces are more reliable.
-
- This is also a prerequisite for generation of ORC unwind data, which
- is needed for CONFIG_UNWINDER_ORC.
+ Validate frame pointer rules at compile-time. This helps ensure that
+ runtime stack traces are more reliable.
For more information, see
tools/objtool/Documentation/stack-validation.txt.
config VMLINUX_VALIDATION
bool
- depends on STACK_VALIDATION && DEBUG_ENTRY
+ depends on HAVE_OBJTOOL && DEBUG_ENTRY
+ select OBJTOOL
default y
config VMLINUX_MAP
@@ -2035,10 +2036,11 @@ config KCOV
bool "Code coverage for fuzzing"
depends on ARCH_HAS_KCOV
depends on CC_HAS_SANCOV_TRACE_PC || GCC_PLUGINS
- depends on !ARCH_WANTS_NO_INSTR || STACK_VALIDATION || \
+ depends on !ARCH_WANTS_NO_INSTR || HAVE_OBJTOOL || \
GCC_VERSION >= 120000 || CLANG_VERSION >= 130000
select DEBUG_FS
select GCC_PLUGIN_SANCOV if !CC_HAS_SANCOV_TRACE_PC
+ select OBJTOOL if HAVE_OBJTOOL
help
KCOV exposes kernel code coverage information in a form suitable
for coverage-guided fuzzing (randomized testing).
diff --git a/lib/Kconfig.kcsan b/lib/Kconfig.kcsan
index de022445fbba..901c3b509aca 100644
--- a/lib/Kconfig.kcsan
+++ b/lib/Kconfig.kcsan
@@ -187,7 +187,8 @@ config KCSAN_WEAK_MEMORY
# We can either let objtool nop __tsan_func_{entry,exit}() and builtin
# atomics instrumentation in .noinstr.text, or use a compiler that can
# implement __no_kcsan to really remove all instrumentation.
- depends on STACK_VALIDATION || CC_IS_GCC || CLANG_VERSION >= 140000
+ depends on HAVE_OBJTOOL || CC_IS_GCC || CLANG_VERSION >= 140000
+ select OBJTOOL if HAVE_OBJTOOL
help
Enable support for modeling a subset of weak memory, which allows
detecting a subset of data races due to missing memory barriers.
diff --git a/lib/Kconfig.ubsan b/lib/Kconfig.ubsan
index f3c57ed51838..c4fe15d38b60 100644
--- a/lib/Kconfig.ubsan
+++ b/lib/Kconfig.ubsan
@@ -94,7 +94,7 @@ config UBSAN_UNREACHABLE
bool "Perform checking for unreachable code"
# objtool already handles unreachable checking and gets angry about
# seeing UBSan instrumentation located in unreachable places.
- depends on !STACK_VALIDATION
+ depends on !(OBJTOOL && (STACK_VALIDATION || UNWINDER_ORC || X86_SMAP))
depends on $(cc-option,-fsanitize=unreachable)
help
This option enables -fsanitize=unreachable which checks for control
diff --git a/scripts/Makefile.build b/scripts/Makefile.build
index d5e15ae29156..0f73e02b7cf1 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -222,7 +222,7 @@ cmd_record_mcount = $(if $(findstring $(strip $(CC_FLAGS_FTRACE)),$(_c_flags)),
$(sub_cmd_record_mcount))
endif # CONFIG_FTRACE_MCOUNT_USE_RECORDMCOUNT
-ifdef CONFIG_STACK_VALIDATION
+ifdef CONFIG_OBJTOOL
objtool := $(objtree)/tools/objtool/objtool
@@ -241,7 +241,7 @@ objtool_args = \
cmd_objtool = $(if $(objtool-enabled), ; $(objtool) $(objtool_args) $@)
cmd_gen_objtooldep = $(if $(objtool-enabled), { echo ; echo '$@: $$(wildcard $(objtool))' ; } >> $(dot-target).cmd)
-endif # CONFIG_STACK_VALIDATION
+endif # CONFIG_OBJTOOL
ifneq ($(CONFIG_LTO_CLANG)$(CONFIG_X86_KERNEL_IBT),)
diff --git a/scripts/link-vmlinux.sh b/scripts/link-vmlinux.sh
index 0140bfa32c0c..5101a7fbfaaf 100755
--- a/scripts/link-vmlinux.sh
+++ b/scripts/link-vmlinux.sh
@@ -108,8 +108,11 @@ objtool_link()
local objtoolcmd;
local objtoolopt;
- if is_enabled CONFIG_STACK_VALIDATION && \
- ( is_enabled CONFIG_LTO_CLANG || is_enabled CONFIG_X86_KERNEL_IBT ); then
+ if ! is_enabled CONFIG_OBJTOOL; then
+ return;
+ fi
+
+ if is_enabled CONFIG_LTO_CLANG || is_enabled CONFIG_X86_KERNEL_IBT; then
# Don't perform vmlinux validation unless explicitly requested,
# but run objtool on vmlinux.o now that we have an object file.
@@ -126,10 +129,23 @@ objtool_link()
objtoolopt="${objtoolopt} --orc"
fi
+ if is_enabled CONFIG_RETPOLINE; then
+ objtoolopt="${objtoolopt} --retpoline"
+ fi
+
+ if is_enabled CONFIG_SLS; then
+ objtoolopt="${objtoolopt} --sls"
+ fi
+
if is_enabled CONFIG_STACK_VALIDATION; then
objtoolopt="${objtoolopt} --stackval"
fi
+ if is_enabled CONFIG_X86_SMAP; then
+ objtoolopt="${objtoolopt} --uaccess"
+ fi
+
+
objtoolopt="${objtoolopt} --lto"
fi
@@ -139,18 +155,6 @@ objtool_link()
if [ -n "${objtoolopt}" ]; then
- if is_enabled CONFIG_RETPOLINE; then
- objtoolopt="${objtoolopt} --retpoline"
- fi
-
- if is_enabled CONFIG_SLS; then
- objtoolopt="${objtoolopt} --sls"
- fi
-
- if is_enabled CONFIG_X86_SMAP; then
- objtoolopt="${objtoolopt} --uaccess"
- fi
-
if ! is_enabled CONFIG_FRAME_POINTER; then
objtoolopt="${objtoolopt} --no-fp"
fi
diff --git a/scripts/package/builddeb b/scripts/package/builddeb
index 91a502bb97e8..67cd420dcf89 100755
--- a/scripts/package/builddeb
+++ b/scripts/package/builddeb
@@ -67,7 +67,7 @@ deploy_kernel_headers () {
) > debian/hdrsrcfiles
{
- if is_enabled CONFIG_STACK_VALIDATION; then
+ if is_enabled CONFIG_OBJTOOL; then
echo tools/objtool/objtool
fi
diff --git a/tools/include/linux/objtool.h b/tools/include/linux/objtool.h
index 586d35720f13..977d90ba642d 100644
--- a/tools/include/linux/objtool.h
+++ b/tools/include/linux/objtool.h
@@ -38,7 +38,7 @@ struct unwind_hint {
#define UNWIND_HINT_TYPE_REGS_PARTIAL 2
#define UNWIND_HINT_TYPE_FUNC 3
-#ifdef CONFIG_STACK_VALIDATION
+#ifdef CONFIG_OBJTOOL
#ifndef __ASSEMBLY__
@@ -157,7 +157,7 @@ struct unwind_hint {
#endif /* __ASSEMBLY__ */
-#else /* !CONFIG_STACK_VALIDATION */
+#else /* !CONFIG_OBJTOOL */
#ifndef __ASSEMBLY__
@@ -179,6 +179,6 @@ struct unwind_hint {
.endm
#endif
-#endif /* CONFIG_STACK_VALIDATION */
+#endif /* CONFIG_OBJTOOL */
#endif /* _LINUX_OBJTOOL_H */
--
2.34.1
^ permalink raw reply related [flat|nested] 41+ messages in thread* [PATCH 12/18] objtool: Make stack validation frame-pointer-specific
2022-04-13 23:19 [PATCH 00/18] objtool: Interface overhaul Josh Poimboeuf
` (10 preceding siblings ...)
2022-04-13 23:19 ` [PATCH 11/18] objtool: Add CONFIG_OBJTOOL Josh Poimboeuf
@ 2022-04-13 23:19 ` Josh Poimboeuf
2022-04-13 23:19 ` [PATCH 13/18] objtool: Add static call cmdline option Josh Poimboeuf
` (5 subsequent siblings)
17 siblings, 0 replies; 41+ messages in thread
From: Josh Poimboeuf @ 2022-04-13 23:19 UTC (permalink / raw)
To: x86; +Cc: Peter Zijlstra, linux-kernel, Miroslav Benes
Now that CONFIG_STACK_VALIDATION is frame-pointer specific, do the same
for the '--stackval' option. Now the '--no-fp' option is redundant and
can be removed.
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
scripts/Makefile.build | 1 -
scripts/link-vmlinux.sh | 4 ----
tools/objtool/builtin-check.c | 3 +--
tools/objtool/check.c | 4 ++--
tools/objtool/include/objtool/builtin.h | 1 -
5 files changed, 3 insertions(+), 10 deletions(-)
diff --git a/scripts/Makefile.build b/scripts/Makefile.build
index 0f73e02b7cf1..6eb99cb08821 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -235,7 +235,6 @@ objtool_args = \
$(if $(CONFIG_STACK_VALIDATION), --stackval) \
$(if $(CONFIG_X86_SMAP), --uaccess) \
$(if $(part-of-module), --module) \
- $(if $(CONFIG_FRAME_POINTER),, --no-fp) \
$(if $(CONFIG_GCOV_KERNEL), --no-unreachable)
cmd_objtool = $(if $(objtool-enabled), ; $(objtool) $(objtool_args) $@)
diff --git a/scripts/link-vmlinux.sh b/scripts/link-vmlinux.sh
index 5101a7fbfaaf..1be01163a9c5 100755
--- a/scripts/link-vmlinux.sh
+++ b/scripts/link-vmlinux.sh
@@ -155,10 +155,6 @@ objtool_link()
if [ -n "${objtoolopt}" ]; then
- if ! is_enabled CONFIG_FRAME_POINTER; then
- objtoolopt="${objtoolopt} --no-fp"
- fi
-
if is_enabled CONFIG_GCOV_KERNEL; then
objtoolopt="${objtoolopt} --no-unreachable"
fi
diff --git a/tools/objtool/builtin-check.c b/tools/objtool/builtin-check.c
index a6a86e2d0598..28bdcffb4267 100644
--- a/tools/objtool/builtin-check.c
+++ b/tools/objtool/builtin-check.c
@@ -44,7 +44,7 @@ const struct option check_options[] = {
OPT_BOOLEAN('o', "orc", &opts.orc, "generate ORC metadata"),
OPT_BOOLEAN('r', "retpoline", &opts.retpoline, "validate and annotate retpoline usage"),
OPT_BOOLEAN('S', "sls", &opts.sls, "validate straight-line-speculation mitigations"),
- OPT_BOOLEAN('s', "stackval", &opts.stackval, "validate stack unwinding rules"),
+ OPT_BOOLEAN('s', "stackval", &opts.stackval, "validate frame pointer rules"),
OPT_BOOLEAN('u', "uaccess", &opts.uaccess, "validate uaccess rules for SMAP"),
OPT_CALLBACK(0, "dump", NULL, "orc", "dump object data", parse_dumpstr),
@@ -54,7 +54,6 @@ const struct option check_options[] = {
OPT_BOOLEAN(0, "dry-run", &opts.dryrun, "don't write modifications"),
OPT_BOOLEAN(0, "lto", &opts.lto, "whole-archive like runs"),
OPT_BOOLEAN(0, "module", &opts.module, "object is part of a kernel module"),
- OPT_BOOLEAN(0, "no-fp", &opts.no_fp, "skip frame pointer validation"),
OPT_BOOLEAN(0, "no-unreachable", &opts.no_unreachable, "skip 'unreachable instruction' warnings"),
OPT_BOOLEAN(0, "stats", &opts.stats, "print statistics"),
OPT_BOOLEAN(0, "vmlinux", &opts.vmlinux, "vmlinux.o validation"),
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 1b1e7a4ae18b..9a6d77a3c5d4 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -2807,7 +2807,7 @@ static int update_cfi_state(struct instruction *insn,
}
/* detect when asm code uses rbp as a scratch register */
- if (!opts.no_fp && insn->func && op->src.reg == CFI_BP &&
+ if (opts.stackval && insn->func && op->src.reg == CFI_BP &&
cfa->base != CFI_BP)
cfi->bp_scratch = true;
break;
@@ -3285,7 +3285,7 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
if (ret)
return ret;
- if (!opts.no_fp && func && !is_fentry_call(insn) &&
+ if (opts.stackval && func && !is_fentry_call(insn) &&
!has_valid_stack_frame(&state)) {
WARN_FUNC("call without frame pointer save/setup",
sec, insn->offset);
diff --git a/tools/objtool/include/objtool/builtin.h b/tools/objtool/include/objtool/builtin.h
index edb0f550727b..ac94db3470d2 100644
--- a/tools/objtool/include/objtool/builtin.h
+++ b/tools/objtool/include/objtool/builtin.h
@@ -32,7 +32,6 @@ struct opts {
bool dryrun;
bool lto;
bool module;
- bool no_fp;
bool no_unreachable;
bool stats;
bool vmlinux;
--
2.34.1
^ permalink raw reply related [flat|nested] 41+ messages in thread* [PATCH 13/18] objtool: Add static call cmdline option
2022-04-13 23:19 [PATCH 00/18] objtool: Interface overhaul Josh Poimboeuf
` (11 preceding siblings ...)
2022-04-13 23:19 ` [PATCH 12/18] objtool: Make stack validation frame-pointer-specific Josh Poimboeuf
@ 2022-04-13 23:19 ` Josh Poimboeuf
2022-04-13 23:19 ` [PATCH 14/18] objtool: Add toolchain hacks " Josh Poimboeuf
` (4 subsequent siblings)
17 siblings, 0 replies; 41+ messages in thread
From: Josh Poimboeuf @ 2022-04-13 23:19 UTC (permalink / raw)
To: x86; +Cc: Peter Zijlstra, linux-kernel, Miroslav Benes
As part of making objtool more modular, put the existing static call
code behind a new '--static-call' option.
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
scripts/Makefile.build | 1 +
scripts/link-vmlinux.sh | 5 ++++-
tools/objtool/builtin-check.c | 2 ++
tools/objtool/check.c | 10 ++++++----
tools/objtool/include/objtool/builtin.h | 1 +
5 files changed, 14 insertions(+), 5 deletions(-)
diff --git a/scripts/Makefile.build b/scripts/Makefile.build
index 6eb99cb08821..3f20d565733c 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -233,6 +233,7 @@ objtool_args = \
$(if $(CONFIG_RETPOLINE), --retpoline) \
$(if $(CONFIG_SLS), --sls) \
$(if $(CONFIG_STACK_VALIDATION), --stackval) \
+ $(if $(CONFIG_HAVE_STATIC_CALL_INLINE), --static-call) \
$(if $(CONFIG_X86_SMAP), --uaccess) \
$(if $(part-of-module), --module) \
$(if $(CONFIG_GCOV_KERNEL), --no-unreachable)
diff --git a/scripts/link-vmlinux.sh b/scripts/link-vmlinux.sh
index 1be01163a9c5..33f14fe1ddde 100755
--- a/scripts/link-vmlinux.sh
+++ b/scripts/link-vmlinux.sh
@@ -141,11 +141,14 @@ objtool_link()
objtoolopt="${objtoolopt} --stackval"
fi
+ if is_enabled CONFIG_HAVE_STATIC_CALL_INLINE; then
+ objtoolopt="${objtoolopt} --static-call"
+ fi
+
if is_enabled CONFIG_X86_SMAP; then
objtoolopt="${objtoolopt} --uaccess"
fi
-
objtoolopt="${objtoolopt} --lto"
fi
diff --git a/tools/objtool/builtin-check.c b/tools/objtool/builtin-check.c
index 28bdcffb4267..c663828834e1 100644
--- a/tools/objtool/builtin-check.c
+++ b/tools/objtool/builtin-check.c
@@ -45,6 +45,7 @@ const struct option check_options[] = {
OPT_BOOLEAN('r', "retpoline", &opts.retpoline, "validate and annotate retpoline usage"),
OPT_BOOLEAN('S', "sls", &opts.sls, "validate straight-line-speculation mitigations"),
OPT_BOOLEAN('s', "stackval", &opts.stackval, "validate frame pointer rules"),
+ OPT_BOOLEAN('t', "static-call", &opts.static_call, "annotate static calls"),
OPT_BOOLEAN('u', "uaccess", &opts.uaccess, "validate uaccess rules for SMAP"),
OPT_CALLBACK(0, "dump", NULL, "orc", "dump object data", parse_dumpstr),
@@ -97,6 +98,7 @@ static bool opts_valid(void)
opts.retpoline ||
opts.sls ||
opts.stackval ||
+ opts.static_call ||
opts.uaccess) {
if (opts.dump) {
fprintf(stderr, "--dump can't be combined with other options\n");
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 9a6d77a3c5d4..511b76aaa6de 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -3913,10 +3913,12 @@ int check(struct objtool_file *file)
warnings += ret;
}
- ret = create_static_call_sections(file);
- if (ret < 0)
- goto out;
- warnings += ret;
+ if (opts.static_call) {
+ ret = create_static_call_sections(file);
+ if (ret < 0)
+ goto out;
+ warnings += ret;
+ }
if (opts.retpoline) {
ret = create_retpoline_sites_sections(file);
diff --git a/tools/objtool/include/objtool/builtin.h b/tools/objtool/include/objtool/builtin.h
index ac94db3470d2..6ffa6b5dc276 100644
--- a/tools/objtool/include/objtool/builtin.h
+++ b/tools/objtool/include/objtool/builtin.h
@@ -24,6 +24,7 @@ struct opts {
bool retpoline;
bool sls;
bool stackval;
+ bool static_call;
bool uaccess;
/* options: */
--
2.34.1
^ permalink raw reply related [flat|nested] 41+ messages in thread* [PATCH 14/18] objtool: Add toolchain hacks cmdline option
2022-04-13 23:19 [PATCH 00/18] objtool: Interface overhaul Josh Poimboeuf
` (12 preceding siblings ...)
2022-04-13 23:19 ` [PATCH 13/18] objtool: Add static call cmdline option Josh Poimboeuf
@ 2022-04-13 23:19 ` Josh Poimboeuf
2022-04-14 8:09 ` Peter Zijlstra
2022-04-13 23:19 ` [PATCH 15/18] objtool: Rename "VMLINUX_VALIDATION" -> "NOINSTR_VALIDATION" Josh Poimboeuf
` (3 subsequent siblings)
17 siblings, 1 reply; 41+ messages in thread
From: Josh Poimboeuf @ 2022-04-13 23:19 UTC (permalink / raw)
To: x86; +Cc: Peter Zijlstra, linux-kernel, Miroslav Benes
Objtool secretly does a few awful hacks to overcome toolchain
limitations. Make those hacks explicit (and optional for other arches)
by associating them with a new '--hacks' cmdline option and
corresponding CONFIG_HAVE_TOOLCHAIN_HACKS.
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
arch/Kconfig | 4 ++++
arch/x86/Kconfig | 1 +
arch/x86/include/asm/jump_label.h | 6 +++---
lib/Kconfig.debug | 4 ++--
lib/Kconfig.kcsan | 4 ++--
scripts/Makefile.build | 1 +
scripts/link-vmlinux.sh | 4 ++++
tools/objtool/builtin-check.c | 4 +++-
tools/objtool/check.c | 4 ++--
tools/objtool/include/objtool/builtin.h | 1 +
10 files changed, 23 insertions(+), 10 deletions(-)
diff --git a/arch/Kconfig b/arch/Kconfig
index 04cdef16db24..cb5fc88cb996 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -1031,6 +1031,10 @@ config ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT
config HAVE_OBJTOOL
bool
+config HAVE_TOOLCHAIN_HACKS
+ bool
+ select OBJTOOL
+
config HAVE_STACK_VALIDATION
bool
help
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index bce0c23f3550..410c4e2c7390 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -232,6 +232,7 @@ config X86
select HAVE_MOVE_PUD
select HAVE_NMI
select HAVE_OBJTOOL if X86_64
+ select HAVE_TOOLCHAIN_HACKS if HAVE_OBJTOOL
select HAVE_OPTPROBES
select HAVE_PCSPKR_PLATFORM
select HAVE_PERF_EVENTS
diff --git a/arch/x86/include/asm/jump_label.h b/arch/x86/include/asm/jump_label.h
index 3ce0e67c579c..032434791bf1 100644
--- a/arch/x86/include/asm/jump_label.h
+++ b/arch/x86/include/asm/jump_label.h
@@ -20,7 +20,7 @@
_ASM_PTR "%c0 + %c1 - .\n\t" \
".popsection \n\t"
-#ifdef CONFIG_OBJTOOL
+#ifdef CONFIG_HAVE_TOOLCHAIN_HACKS
static __always_inline bool arch_static_branch(struct static_key *key, bool branch)
{
@@ -34,7 +34,7 @@ static __always_inline bool arch_static_branch(struct static_key *key, bool bran
return true;
}
-#else /* !CONFIG_OBJTOOL */
+#else /* !CONFIG_HAVE_TOOLCHAIN_HACKS */
static __always_inline bool arch_static_branch(struct static_key * const key, const bool branch)
{
@@ -48,7 +48,7 @@ static __always_inline bool arch_static_branch(struct static_key * const key, co
return true;
}
-#endif /* CONFIG_OBJTOOL */
+#endif /* CONFIG_HAVE_TOOLCHAIN_HACKS */
static __always_inline bool arch_static_branch_jump(struct static_key * const key, const bool branch)
{
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index c0e4e47f3ce3..616b93d5db6d 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -2036,11 +2036,11 @@ config KCOV
bool "Code coverage for fuzzing"
depends on ARCH_HAS_KCOV
depends on CC_HAS_SANCOV_TRACE_PC || GCC_PLUGINS
- depends on !ARCH_WANTS_NO_INSTR || HAVE_OBJTOOL || \
+ depends on !ARCH_WANTS_NO_INSTR || HAVE_TOOLCHAIN_HACKS || \
GCC_VERSION >= 120000 || CLANG_VERSION >= 130000
select DEBUG_FS
select GCC_PLUGIN_SANCOV if !CC_HAS_SANCOV_TRACE_PC
- select OBJTOOL if HAVE_OBJTOOL
+ select OBJTOOL if HAVE_TOOLCHAIN_HACKS
help
KCOV exposes kernel code coverage information in a form suitable
for coverage-guided fuzzing (randomized testing).
diff --git a/lib/Kconfig.kcsan b/lib/Kconfig.kcsan
index 901c3b509aca..2bdb46354d6a 100644
--- a/lib/Kconfig.kcsan
+++ b/lib/Kconfig.kcsan
@@ -187,8 +187,8 @@ config KCSAN_WEAK_MEMORY
# We can either let objtool nop __tsan_func_{entry,exit}() and builtin
# atomics instrumentation in .noinstr.text, or use a compiler that can
# implement __no_kcsan to really remove all instrumentation.
- depends on HAVE_OBJTOOL || CC_IS_GCC || CLANG_VERSION >= 140000
- select OBJTOOL if HAVE_OBJTOOL
+ depends on HAVE_TOOLCHAIN_HACKS || CC_IS_GCC || CLANG_VERSION >= 140000
+ select OBJTOOL if HAVE_TOOLCHAIN_HACKS
help
Enable support for modeling a subset of weak memory, which allows
detecting a subset of data races due to missing memory barriers.
diff --git a/scripts/Makefile.build b/scripts/Makefile.build
index 3f20d565733c..3b53de3dec67 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -227,6 +227,7 @@ ifdef CONFIG_OBJTOOL
objtool := $(objtree)/tools/objtool/objtool
objtool_args = \
+ $(if $(CONFIG_HAVE_TOOLCHAIN_HACKS), --hacks) \
$(if $(CONFIG_X86_KERNEL_IBT), --lto --ibt) \
$(if $(CONFIG_FTRACE_MCOUNT_USE_OBJTOOL), --mcount) \
$(if $(CONFIG_UNWINDER_ORC), --orc) \
diff --git a/scripts/link-vmlinux.sh b/scripts/link-vmlinux.sh
index 33f14fe1ddde..480a49e47fbc 100755
--- a/scripts/link-vmlinux.sh
+++ b/scripts/link-vmlinux.sh
@@ -117,6 +117,10 @@ objtool_link()
# Don't perform vmlinux validation unless explicitly requested,
# but run objtool on vmlinux.o now that we have an object file.
+ if is_enabled CONFIG_HAVE_TOOLCHAIN_HACKS; then
+ objtoolopt="${objtoolopt} --hacks"
+ fi
+
if is_enabled CONFIG_X86_KERNEL_IBT; then
objtoolopt="${objtoolopt} --ibt"
fi
diff --git a/tools/objtool/builtin-check.c b/tools/objtool/builtin-check.c
index c663828834e1..13e1c46f155a 100644
--- a/tools/objtool/builtin-check.c
+++ b/tools/objtool/builtin-check.c
@@ -38,6 +38,7 @@ static int parse_dumpstr(const struct option *opt, const char *str, int unset)
const struct option check_options[] = {
OPT_GROUP("Actions:"),
+ OPT_BOOLEAN('h', "hacks", &opts.hacks, "patch some toolchain bugs/limitations"),
OPT_BOOLEAN('i', "ibt", &opts.ibt, "validate and annotate IBT"),
OPT_BOOLEAN('m', "mcount", &opts.mcount, "annotate mcount/fentry calls for ftrace"),
OPT_BOOLEAN('n', "noinstr", &opts.noinstr, "validate noinstr rules"),
@@ -91,7 +92,8 @@ int cmd_parse_options(int argc, const char **argv, const char * const usage[])
static bool opts_valid(void)
{
- if (opts.ibt ||
+ if (opts.hacks ||
+ opts.ibt ||
opts.mcount ||
opts.noinstr ||
opts.orc ||
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 511b76aaa6de..273ba6840ed2 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -1145,7 +1145,7 @@ static void annotate_call_site(struct objtool_file *file,
* attribute so they need a little help, NOP out any such calls from
* noinstr text.
*/
- if (insn->sec->noinstr && sym->profiling_func) {
+ if (opts.hacks && insn->sec->noinstr && sym->profiling_func) {
if (reloc) {
reloc->type = R_NONE;
elf_write_reloc(file->elf, reloc);
@@ -1593,7 +1593,7 @@ static int handle_jump_alt(struct objtool_file *file,
return -1;
}
- if (special_alt->key_addend & 2) {
+ if (opts.hacks && special_alt->key_addend & 2) {
struct reloc *reloc = insn_reloc(file, orig_insn);
if (reloc) {
diff --git a/tools/objtool/include/objtool/builtin.h b/tools/objtool/include/objtool/builtin.h
index 6ffa6b5dc276..7bc76edb0a85 100644
--- a/tools/objtool/include/objtool/builtin.h
+++ b/tools/objtool/include/objtool/builtin.h
@@ -17,6 +17,7 @@ enum dump {
struct opts {
/* actions: */
enum dump dump;
+ bool hacks;
bool ibt;
bool mcount;
bool noinstr;
--
2.34.1
^ permalink raw reply related [flat|nested] 41+ messages in thread* Re: [PATCH 14/18] objtool: Add toolchain hacks cmdline option
2022-04-13 23:19 ` [PATCH 14/18] objtool: Add toolchain hacks " Josh Poimboeuf
@ 2022-04-14 8:09 ` Peter Zijlstra
2022-04-14 15:49 ` Josh Poimboeuf
0 siblings, 1 reply; 41+ messages in thread
From: Peter Zijlstra @ 2022-04-14 8:09 UTC (permalink / raw)
To: Josh Poimboeuf; +Cc: x86, linux-kernel, Miroslav Benes
On Wed, Apr 13, 2022 at 04:19:49PM -0700, Josh Poimboeuf wrote:
> Objtool secretly does a few awful hacks to overcome toolchain
> limitations. Make those hacks explicit (and optional for other arches)
> by associating them with a new '--hacks' cmdline option and
> corresponding CONFIG_HAVE_TOOLCHAIN_HACKS.
Should we either clarify the specific hacks done, or split this in two
options?
--hack-jump_label
--hack-noinstr
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 14/18] objtool: Add toolchain hacks cmdline option
2022-04-14 8:09 ` Peter Zijlstra
@ 2022-04-14 15:49 ` Josh Poimboeuf
0 siblings, 0 replies; 41+ messages in thread
From: Josh Poimboeuf @ 2022-04-14 15:49 UTC (permalink / raw)
To: Peter Zijlstra; +Cc: x86, linux-kernel, Miroslav Benes
On Thu, Apr 14, 2022 at 10:09:46AM +0200, Peter Zijlstra wrote:
> On Wed, Apr 13, 2022 at 04:19:49PM -0700, Josh Poimboeuf wrote:
> > Objtool secretly does a few awful hacks to overcome toolchain
> > limitations. Make those hacks explicit (and optional for other arches)
> > by associating them with a new '--hacks' cmdline option and
> > corresponding CONFIG_HAVE_TOOLCHAIN_HACKS.
>
> Should we either clarify the specific hacks done, or split this in two
> options?
>
> --hack-jump_label
> --hack-noinstr
Yeah, maybe we should split them out somehow.
--
Josh
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH 15/18] objtool: Rename "VMLINUX_VALIDATION" -> "NOINSTR_VALIDATION"
2022-04-13 23:19 [PATCH 00/18] objtool: Interface overhaul Josh Poimboeuf
` (13 preceding siblings ...)
2022-04-13 23:19 ` [PATCH 14/18] objtool: Add toolchain hacks " Josh Poimboeuf
@ 2022-04-13 23:19 ` Josh Poimboeuf
2022-04-13 23:19 ` [PATCH 16/18] objtool: Add HAVE_NOINSTR_VALIDATION Josh Poimboeuf
` (2 subsequent siblings)
17 siblings, 0 replies; 41+ messages in thread
From: Josh Poimboeuf @ 2022-04-13 23:19 UTC (permalink / raw)
To: x86; +Cc: Peter Zijlstra, linux-kernel, Miroslav Benes
CONFIG_VMLINUX_VALIDATION is just the validation of the "noinstr" rules.
That name is a misnomer, because now objtool actually does vmlinux
validation for other reasons.
Rename CONFIG_VMLINUX_VALIDATION to CONFIG_NOINSTR_VALIDATION.
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
include/linux/instrumentation.h | 6 +++---
lib/Kconfig.debug | 2 +-
scripts/link-vmlinux.sh | 2 +-
3 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/include/linux/instrumentation.h b/include/linux/instrumentation.h
index 9111a3704072..bc7babe91b2e 100644
--- a/include/linux/instrumentation.h
+++ b/include/linux/instrumentation.h
@@ -2,7 +2,7 @@
#ifndef __LINUX_INSTRUMENTATION_H
#define __LINUX_INSTRUMENTATION_H
-#ifdef CONFIG_VMLINUX_VALIDATION
+#ifdef CONFIG_NOINSTR_VALIDATION
#include <linux/stringify.h>
@@ -53,9 +53,9 @@
".popsection\n\t" : : "i" (c)); \
})
#define instrumentation_end() __instrumentation_end(__COUNTER__)
-#else /* !CONFIG_VMLINUX_VALIDATION */
+#else /* !CONFIG_NOINSTR_VALIDATION */
# define instrumentation_begin() do { } while(0)
# define instrumentation_end() do { } while(0)
-#endif /* CONFIG_VMLINUX_VALIDATION */
+#endif /* CONFIG_NOINSTR_VALIDATION */
#endif /* __LINUX_INSTRUMENTATION_H */
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 616b93d5db6d..be6ca70d558a 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -500,7 +500,7 @@ config STACK_VALIDATION
For more information, see
tools/objtool/Documentation/stack-validation.txt.
-config VMLINUX_VALIDATION
+config NOINSTR_VALIDATION
bool
depends on HAVE_OBJTOOL && DEBUG_ENTRY
select OBJTOOL
diff --git a/scripts/link-vmlinux.sh b/scripts/link-vmlinux.sh
index 480a49e47fbc..96dbaaeaecd1 100755
--- a/scripts/link-vmlinux.sh
+++ b/scripts/link-vmlinux.sh
@@ -156,7 +156,7 @@ objtool_link()
objtoolopt="${objtoolopt} --lto"
fi
- if is_enabled CONFIG_VMLINUX_VALIDATION; then
+ if is_enabled CONFIG_NOINSTR_VALIDATION; then
objtoolopt="${objtoolopt} --noinstr"
fi
--
2.34.1
^ permalink raw reply related [flat|nested] 41+ messages in thread* [PATCH 16/18] objtool: Add HAVE_NOINSTR_VALIDATION
2022-04-13 23:19 [PATCH 00/18] objtool: Interface overhaul Josh Poimboeuf
` (14 preceding siblings ...)
2022-04-13 23:19 ` [PATCH 15/18] objtool: Rename "VMLINUX_VALIDATION" -> "NOINSTR_VALIDATION" Josh Poimboeuf
@ 2022-04-13 23:19 ` Josh Poimboeuf
2022-04-13 23:19 ` [PATCH 17/18] objtool: Remove --lto and --vmlinux Josh Poimboeuf
2022-04-13 23:19 ` [PATCH 18/18] objtool: Update documentation Josh Poimboeuf
17 siblings, 0 replies; 41+ messages in thread
From: Josh Poimboeuf @ 2022-04-13 23:19 UTC (permalink / raw)
To: x86; +Cc: Peter Zijlstra, linux-kernel, Miroslav Benes
Remove CONFIG_NOINSTR_VALIDATION's dependency on HAVE_OBJTOOL, since
other arches might want to implement objtool without it.
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
arch/Kconfig | 3 +++
arch/x86/Kconfig | 1 +
lib/Kconfig.debug | 2 +-
3 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/arch/Kconfig b/arch/Kconfig
index cb5fc88cb996..d0c00564bbf8 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -1031,6 +1031,9 @@ config ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT
config HAVE_OBJTOOL
bool
+config HAVE_NOINSTR_VALIDATION
+ bool
+
config HAVE_TOOLCHAIN_HACKS
bool
select OBJTOOL
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 410c4e2c7390..c73ca72f5d98 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -231,6 +231,7 @@ config X86
select HAVE_MOVE_PMD
select HAVE_MOVE_PUD
select HAVE_NMI
+ select HAVE_NOINSTR_VALIDATION if HAVE_OBJTOOL
select HAVE_OBJTOOL if X86_64
select HAVE_TOOLCHAIN_HACKS if HAVE_OBJTOOL
select HAVE_OPTPROBES
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index be6ca70d558a..b5fa86994fd3 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -502,7 +502,7 @@ config STACK_VALIDATION
config NOINSTR_VALIDATION
bool
- depends on HAVE_OBJTOOL && DEBUG_ENTRY
+ depends on HAVE_NOINSTR_VALIDATION && DEBUG_ENTRY
select OBJTOOL
default y
--
2.34.1
^ permalink raw reply related [flat|nested] 41+ messages in thread* [PATCH 17/18] objtool: Remove --lto and --vmlinux
2022-04-13 23:19 [PATCH 00/18] objtool: Interface overhaul Josh Poimboeuf
` (15 preceding siblings ...)
2022-04-13 23:19 ` [PATCH 16/18] objtool: Add HAVE_NOINSTR_VALIDATION Josh Poimboeuf
@ 2022-04-13 23:19 ` Josh Poimboeuf
2022-04-14 8:13 ` Peter Zijlstra
2022-04-13 23:19 ` [PATCH 18/18] objtool: Update documentation Josh Poimboeuf
17 siblings, 1 reply; 41+ messages in thread
From: Josh Poimboeuf @ 2022-04-13 23:19 UTC (permalink / raw)
To: x86; +Cc: Peter Zijlstra, linux-kernel, Miroslav Benes
The '--lto' option is a confusing way of telling objtool to do stack
validation despite it being a linked object. That's no longer needed
now that an explicit '--stackval' option exists.
The '--vmlinux' option can also be made redundant by adding detection of
a multi-object archive.
Remove both options.
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
scripts/Makefile.build | 2 +-
scripts/link-vmlinux.sh | 8 ++---
tools/objtool/builtin-check.c | 23 ++++++++++++--
tools/objtool/check.c | 40 +++++++++----------------
tools/objtool/elf.c | 3 ++
tools/objtool/include/objtool/builtin.h | 2 --
tools/objtool/include/objtool/elf.h | 8 ++++-
7 files changed, 48 insertions(+), 38 deletions(-)
diff --git a/scripts/Makefile.build b/scripts/Makefile.build
index 3b53de3dec67..f4b44d77e8a0 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -228,7 +228,7 @@ objtool := $(objtree)/tools/objtool/objtool
objtool_args = \
$(if $(CONFIG_HAVE_TOOLCHAIN_HACKS), --hacks) \
- $(if $(CONFIG_X86_KERNEL_IBT), --lto --ibt) \
+ $(if $(CONFIG_X86_KERNEL_IBT), --ibt) \
$(if $(CONFIG_FTRACE_MCOUNT_USE_OBJTOOL), --mcount) \
$(if $(CONFIG_UNWINDER_ORC), --orc) \
$(if $(CONFIG_RETPOLINE), --retpoline) \
diff --git a/scripts/link-vmlinux.sh b/scripts/link-vmlinux.sh
index 96dbaaeaecd1..0a79f80bd789 100755
--- a/scripts/link-vmlinux.sh
+++ b/scripts/link-vmlinux.sh
@@ -114,8 +114,8 @@ objtool_link()
if is_enabled CONFIG_LTO_CLANG || is_enabled CONFIG_X86_KERNEL_IBT; then
- # Don't perform vmlinux validation unless explicitly requested,
- # but run objtool on vmlinux.o now that we have an object file.
+ # For LTO and IBT, objtool doesn't run on individual
+ # translation units. Run everything on vmlinux instead.
if is_enabled CONFIG_HAVE_TOOLCHAIN_HACKS; then
objtoolopt="${objtoolopt} --hacks"
@@ -152,8 +152,6 @@ objtool_link()
if is_enabled CONFIG_X86_SMAP; then
objtoolopt="${objtoolopt} --uaccess"
fi
-
- objtoolopt="${objtoolopt} --lto"
fi
if is_enabled CONFIG_NOINSTR_VALIDATION; then
@@ -166,8 +164,6 @@ objtool_link()
objtoolopt="${objtoolopt} --no-unreachable"
fi
- objtoolopt="${objtoolopt} --vmlinux"
-
info OBJTOOL ${1}
tools/objtool/objtool ${objtoolopt} ${1}
fi
diff --git a/tools/objtool/builtin-check.c b/tools/objtool/builtin-check.c
index 13e1c46f155a..80fc0d1c0a53 100644
--- a/tools/objtool/builtin-check.c
+++ b/tools/objtool/builtin-check.c
@@ -54,11 +54,9 @@ const struct option check_options[] = {
OPT_BOOLEAN(0, "backtrace", &opts.backtrace, "unwind on error"),
OPT_BOOLEAN(0, "backup", &opts.backup, "create .orig files before modification"),
OPT_BOOLEAN(0, "dry-run", &opts.dryrun, "don't write modifications"),
- OPT_BOOLEAN(0, "lto", &opts.lto, "whole-archive like runs"),
OPT_BOOLEAN(0, "module", &opts.module, "object is part of a kernel module"),
OPT_BOOLEAN(0, "no-unreachable", &opts.no_unreachable, "skip 'unreachable instruction' warnings"),
OPT_BOOLEAN(0, "stats", &opts.stats, "print statistics"),
- OPT_BOOLEAN(0, "vmlinux", &opts.vmlinux, "vmlinux.o validation"),
OPT_END(),
};
@@ -117,6 +115,24 @@ static bool opts_valid(void)
return false;
}
+static bool link_opts_valid(struct objtool_file *file)
+{
+ if (is_linked_object(file->elf))
+ return true;
+
+ if (opts.noinstr) {
+ fprintf(stderr, "--noinstr requires linked object\n");
+ return false;
+ }
+
+ if (opts.ibt) {
+ fprintf(stderr, "--ibt requires linked object\n");
+ return false;
+ }
+
+ return true;
+}
+
int objtool_run(int argc, const char **argv)
{
const char *objname;
@@ -136,6 +152,9 @@ int objtool_run(int argc, const char **argv)
if (!file)
return 1;
+ if (!link_opts_valid(file))
+ return 1;
+
ret = check(file);
if (ret)
return ret;
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 273ba6840ed2..2d18f23a895b 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -264,7 +264,8 @@ static void init_cfi_state(struct cfi_state *cfi)
cfi->drap_offset = -1;
}
-static void init_insn_state(struct insn_state *state, struct section *sec)
+static void init_insn_state(struct objtool_file *file, struct insn_state *state,
+ struct section *sec)
{
memset(state, 0, sizeof(*state));
init_cfi_state(&state->cfi);
@@ -274,7 +275,7 @@ static void init_insn_state(struct insn_state *state, struct section *sec)
* not correctly determine insn->call_dest->sec (external symbols do
* not have a section).
*/
- if (opts.vmlinux && opts.noinstr && sec)
+ if (is_linked_object(file->elf) && opts.noinstr && sec)
state->noinstr = sec->noinstr;
}
@@ -3418,7 +3419,7 @@ static int validate_unwind_hints(struct objtool_file *file, struct section *sec)
if (!file->hints)
return 0;
- init_insn_state(&state, sec);
+ init_insn_state(file, &state, sec);
if (sec) {
insn = find_insn(file, sec, 0);
@@ -3504,14 +3505,14 @@ static bool ignore_unreachable_insn(struct objtool_file *file, struct instructio
return true;
/*
- * Whole archive runs might encounder dead code from weak symbols.
+ * Whole archive runs might encounter dead code from weak symbols.
* This is where the linker will have dropped the weak symbol in
* favour of a regular symbol, but leaves the code in place.
*
* In this case we'll find a piece of code (whole function) that is not
* covered by a !section symbol. Ignore them.
*/
- if (!insn->func && opts.lto) {
+ if (!insn->func && is_linked_object(file->elf)) {
int size = find_symbol_hole_containing(insn->sec, insn->offset);
unsigned long end = insn->offset + size;
@@ -3633,7 +3634,7 @@ static int validate_section(struct objtool_file *file, struct section *sec)
if (func->type != STT_FUNC)
continue;
- init_insn_state(&state, sec);
+ init_insn_state(file, &state, sec);
set_func_state(&state.cfi);
warnings += validate_symbol(file, sec, func, &state);
@@ -3642,7 +3643,7 @@ static int validate_section(struct objtool_file *file, struct section *sec)
return warnings;
}
-static int validate_vmlinux_functions(struct objtool_file *file)
+static int validate_noinstr_sections(struct objtool_file *file)
{
struct section *sec;
int warnings = 0;
@@ -3841,16 +3842,6 @@ int check(struct objtool_file *file)
{
int ret, warnings = 0;
- if (opts.lto && !(opts.vmlinux || opts.module)) {
- fprintf(stderr, "--lto requires: --vmlinux or --module\n");
- return 1;
- }
-
- if (opts.ibt && !opts.lto) {
- fprintf(stderr, "--ibt requires: --lto\n");
- return 1;
- }
-
arch_initial_func_cfi_state(&initial_func_cfi);
init_cfi_state(&init_cfi);
init_cfi_state(&func_cfi);
@@ -3871,15 +3862,6 @@ int check(struct objtool_file *file)
if (list_empty(&file->insn_list))
goto out;
- if (opts.vmlinux && !opts.lto) {
- ret = validate_vmlinux_functions(file);
- if (ret < 0)
- goto out;
-
- warnings += ret;
- goto out;
- }
-
if (opts.retpoline) {
ret = validate_retpoline(file);
if (ret < 0)
@@ -3904,6 +3886,12 @@ int check(struct objtool_file *file)
goto out;
warnings += ret;
}
+
+ } else if (opts.noinstr) {
+ ret = validate_noinstr_sections(file);
+ if (ret < 0)
+ goto out;
+ warnings += ret;
}
if (opts.ibt) {
diff --git a/tools/objtool/elf.c b/tools/objtool/elf.c
index f7b2ad27bb1c..41fea838aeba 100644
--- a/tools/objtool/elf.c
+++ b/tools/objtool/elf.c
@@ -377,6 +377,9 @@ static void elf_add_symbol(struct elf *elf, struct symbol *sym)
sym->type = GELF_ST_TYPE(sym->sym.st_info);
sym->bind = GELF_ST_BIND(sym->sym.st_info);
+ if (sym->type == STT_FILE)
+ elf->num_files++;
+
sym->offset = sym->sym.st_value;
sym->len = sym->sym.st_size;
diff --git a/tools/objtool/include/objtool/builtin.h b/tools/objtool/include/objtool/builtin.h
index 7bc76edb0a85..5c698fb33eca 100644
--- a/tools/objtool/include/objtool/builtin.h
+++ b/tools/objtool/include/objtool/builtin.h
@@ -32,11 +32,9 @@ struct opts {
bool backtrace;
bool backup;
bool dryrun;
- bool lto;
bool module;
bool no_unreachable;
bool stats;
- bool vmlinux;
};
extern struct opts opts;
diff --git a/tools/objtool/include/objtool/elf.h b/tools/objtool/include/objtool/elf.h
index 22ba7e2b816e..1b03a270c58c 100644
--- a/tools/objtool/include/objtool/elf.h
+++ b/tools/objtool/include/objtool/elf.h
@@ -86,7 +86,7 @@ struct elf {
int fd;
bool changed;
char *name;
- unsigned int text_size;
+ unsigned int text_size, num_files;
struct list_head sections;
int symbol_bits;
@@ -131,6 +131,12 @@ static inline u32 reloc_hash(struct reloc *reloc)
return sec_offset_hash(reloc->sec, reloc->offset);
}
+/* is it a whole archive (vmlinux.o or module)? */
+static inline bool is_linked_object(struct elf *elf)
+{
+ return elf->num_files > 1;
+}
+
struct elf *elf_open_read(const char *name, int flags);
struct section *elf_create_section(struct elf *elf, const char *name, unsigned int sh_flags, size_t entsize, int nr);
--
2.34.1
^ permalink raw reply related [flat|nested] 41+ messages in thread* Re: [PATCH 17/18] objtool: Remove --lto and --vmlinux
2022-04-13 23:19 ` [PATCH 17/18] objtool: Remove --lto and --vmlinux Josh Poimboeuf
@ 2022-04-14 8:13 ` Peter Zijlstra
2022-04-15 2:18 ` Josh Poimboeuf
0 siblings, 1 reply; 41+ messages in thread
From: Peter Zijlstra @ 2022-04-14 8:13 UTC (permalink / raw)
To: Josh Poimboeuf; +Cc: x86, linux-kernel, Miroslav Benes
On Wed, Apr 13, 2022 at 04:19:52PM -0700, Josh Poimboeuf wrote:
> @@ -131,6 +131,12 @@ static inline u32 reloc_hash(struct reloc *reloc)
> return sec_offset_hash(reloc->sec, reloc->offset);
> }
>
> +/* is it a whole archive (vmlinux.o or module)? */
> +static inline bool is_linked_object(struct elf *elf)
> +{
> + return elf->num_files > 1;
> +}
> +
> struct elf *elf_open_read(const char *name, int flags);
> struct section *elf_create_section(struct elf *elf, const char *name, unsigned int sh_flags, size_t entsize, int nr);
Ooh, nice... yes, that was the entire point of --lto.
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [PATCH 17/18] objtool: Remove --lto and --vmlinux
2022-04-14 8:13 ` Peter Zijlstra
@ 2022-04-15 2:18 ` Josh Poimboeuf
0 siblings, 0 replies; 41+ messages in thread
From: Josh Poimboeuf @ 2022-04-15 2:18 UTC (permalink / raw)
To: Peter Zijlstra; +Cc: x86, linux-kernel, Miroslav Benes
On Thu, Apr 14, 2022 at 10:13:50AM +0200, Peter Zijlstra wrote:
> On Wed, Apr 13, 2022 at 04:19:52PM -0700, Josh Poimboeuf wrote:
>
> > @@ -131,6 +131,12 @@ static inline u32 reloc_hash(struct reloc *reloc)
> > return sec_offset_hash(reloc->sec, reloc->offset);
> > }
> >
> > +/* is it a whole archive (vmlinux.o or module)? */
> > +static inline bool is_linked_object(struct elf *elf)
> > +{
> > + return elf->num_files > 1;
> > +}
> > +
> > struct elf *elf_open_read(const char *name, int flags);
> > struct section *elf_create_section(struct elf *elf, const char *name, unsigned int sh_flags, size_t entsize, int nr);
>
> Ooh, nice... yes, that was the entire point of --lto.
Urgh, some kernel modules only have a single source file, which
obviously breaks this.
We might need an explicit --link option after all. But with different
semantics from --lto. It would just mean "is a linked object".
--
Josh
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH 18/18] objtool: Update documentation
2022-04-13 23:19 [PATCH 00/18] objtool: Interface overhaul Josh Poimboeuf
` (16 preceding siblings ...)
2022-04-13 23:19 ` [PATCH 17/18] objtool: Remove --lto and --vmlinux Josh Poimboeuf
@ 2022-04-13 23:19 ` Josh Poimboeuf
17 siblings, 0 replies; 41+ messages in thread
From: Josh Poimboeuf @ 2022-04-13 23:19 UTC (permalink / raw)
To: x86; +Cc: Peter Zijlstra, linux-kernel, Miroslav Benes
The objtool documentation is very stack validation centric. Broaden the
documentation and describe all the features objtool supports.
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
.../{stack-validation.txt => objtool.txt} | 122 +++++++++++++++---
1 file changed, 102 insertions(+), 20 deletions(-)
rename tools/objtool/Documentation/{stack-validation.txt => objtool.txt} (80%)
diff --git a/tools/objtool/Documentation/stack-validation.txt b/tools/objtool/Documentation/objtool.txt
similarity index 80%
rename from tools/objtool/Documentation/stack-validation.txt
rename to tools/objtool/Documentation/objtool.txt
index 30f38fdc0d56..8a671902a187 100644
--- a/tools/objtool/Documentation/stack-validation.txt
+++ b/tools/objtool/Documentation/objtool.txt
@@ -1,15 +1,103 @@
-Compile-time stack metadata validation
-======================================
+Objtool
+=======
+The kernel CONFIG_OBJTOOL option enables a host tool named 'objtool'
+which runs at compile time. It can do various validations and
+transformations on .o files.
-Overview
+Objtool has become an integral part of the x86-64 kernel toolchain. The
+kernel depends on it for a variety of security and performance features
+(and other types of features as well).
+
+
+Features
--------
-The kernel CONFIG_STACK_VALIDATION option enables a host tool named
-objtool which runs at compile time. It has a "check" subcommand which
-analyzes every .o file and ensures the validity of its stack metadata.
-It enforces a set of rules on asm code and C inline assembly code so
-that stack traces can be reliable.
+Objtool has the following features:
+
+- Stack unwinding metadata validation -- useful for helping to ensure
+ stack traces are reliable for live patching
+
+- ORC unwinder metadata generation -- a faster and more precise
+ alternative to frame pointer based unwinding
+
+- Retpoline validation -- ensures that all indirect calls go through
+ retpoline thunks, for Spectre v2 mitigations
+
+- Retpoline call site annotation -- annotates all retpoline thunk call
+ sites, enabling the kernel to patch them inline, to prevent "thunk
+ funneling" for both security and performance reasons
+
+- Non-instrumentation validation -- validates non-instrumentable
+ ("noinstr") code rules, preventing instrumentation in low-level C
+ entry code
+
+- Static call annotation -- annotates static call sites, enabling the
+ kernel to implement inline static calls, a faster alternative to some
+ indirect branches
+
+- Uaccess validation -- validates uaccess rules for a proper
+ implementation of Supervisor Mode Access Protection (SMAP)
+
+- Straight Line Speculation validation -- validates certain SLS
+ mitigations
+
+- Indirect Branch Tracking validation -- validates Intel CET IBT rules
+ to ensure that all functions referenced by function pointers have
+ corresponding ENDBR instructions
+
+- Indirect Branch Tracking annotation -- annotates unused ENDBR
+ instruction sites, enabling the kernel to "seal" them (replace them
+ with NOPs) to further harden IBT
+
+- Function entry annotation -- annotates function entries, enabling
+ kernel function tracing
+
+- Other toolchain hacks which will go unmentioned at this time...
+
+Each feature can be enabled individually or in combination using the
+objtool cmdline.
+
+
+Objects
+-------
+
+Typically, objtool runs on every translation unit (TU, aka ".o file") in
+the kernel. If a TU is part of a kernel module, the '--module' option
+is added.
+
+However:
+
+- If noinstr validation is enabled, it also runs on vmlinux.o, with all
+ options removed and '--noinstr' added.
+
+- If IBT or LTO is enabled, it doesn't run on TUs at all. Instead it
+ runs on vmlinux.o and linked modules, with all options.
+
+In summary:
+
+ A) Legacy mode:
+ TU: objtool [--module] <options>
+ vmlinux: N/A
+ module: N/A
+
+ B) CONFIG_NOINSTR_VALIDATION=y && !(CONFIG_X86_KERNEL_IBT=y || CONFIG_LTO=y):
+ TU: objtool [--module] <options> // no --noinstr
+ vmlinux: objtool --noinstr // other options removed
+ module: N/A
+
+ C) CONFIG_X86_KERNEL_IBT=y || CONFIG_LTO=y:
+ TU: N/A
+ vmlinux: objtool --noinstr <options>
+ module: objtool --module --noinstr <options>
+
+
+Stack validation
+----------------
+
+Objtool's stack validation feature analyzes every .o file and ensures
+the validity of its stack metadata. It enforces a set of rules on asm
+code and C inline assembly code so that stack traces can be reliable.
For each function, it recursively follows all possible code paths and
validates the correct frame pointer state at each instruction.
@@ -20,14 +108,6 @@ alternative execution paths to a given instruction (or set of
instructions). Similarly, it knows how to follow switch statements, for
which gcc sometimes uses jump tables.
-(Objtool also has an 'orc generate' subcommand which generates debuginfo
-for the ORC unwinder. See Documentation/x86/orc-unwinder.rst in the
-kernel tree for more details.)
-
-
-Why do we need stack metadata validation?
------------------------------------------
-
Here are some of the benefits of validating stack metadata:
a) More reliable stack traces for frame pointer enabled kernels
@@ -113,9 +193,6 @@ c) Higher live patching compatibility rate
For more details, see the livepatch documentation in the Linux kernel
source tree at Documentation/livepatch/livepatch.rst.
-Rules
------
-
To achieve the validation, objtool enforces the following rules:
1. Each callable function must be annotated as such with the ELF
@@ -177,7 +254,8 @@ Another possible cause for errors in C code is if the Makefile removes
-fno-omit-frame-pointer or adds -fomit-frame-pointer to the gcc options.
Here are some examples of common warnings reported by objtool, what
-they mean, and suggestions for how to fix them.
+they mean, and suggestions for how to fix them. When in doubt, ping
+the objtool maintainers.
1. file.o: warning: objtool: func()+0x128: call without frame pointer save/setup
@@ -358,3 +436,7 @@ ignore it:
OBJECT_FILES_NON_STANDARD := y
to the Makefile.
+
+NOTE: OBJECT_FILES_NON_STANDARD doesn't work for link time validation of
+vmlinux.o or a linked module. So it should only be used for files which
+aren't linked into vmlinux or a module.
--
2.34.1
^ permalink raw reply related [flat|nested] 41+ messages in thread