Messages in this thread | | | Date | Sun, 15 Mar 2020 13:03:20 -0500 | From | Josh Poimboeuf <> | Subject | Re: [RFC][PATCH 15/16] objtool: Implement noinstr validation |
| |
On Thu, Mar 12, 2020 at 02:41:22PM +0100, Peter Zijlstra wrote: > Validate that any call out of .noinstr.text is in between > instr_begin() and instr_end() annotations. > > This annotation is useful to ensure correct behaviour wrt tracing > sensitive code like entry/exit and idle code. When we run code in a > sensitive context we want a guarantee no unknown code is ran. > > Since this validation relies on knowing the section of call > destination symbols, we must run it on vmlinux.o instead of on > individual object files. > > Add the -i "noinstr validation only" option because: > > - vmlinux.o isn't 'clean' vs the existing validations > - skipping the other validations (which have already been done > earlier in the build) saves around a second of runtime. > > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
I find the phrase "noinstr" to be WAY too ambiguous. To my brain it clearly stands for "no instructions" and I have to do a double take every time I read it.
And "read_instr_hints" reads as "read_instruction_hints()".
Can we come up with a more readable name? Why not just "notrace"?
The trace begin/end annotations could be
trace_allow_begin() trace_allow_end()
Also -- what happens when a function belongs in both .notrace.text and in one of the other special-purpose sections like .sched.text, .meminit.text or .entry.text?
Maybe storing pointers to the functions, like NOKPROBE_SYMBOL does, would be better than putting the functions in a separate section.
> --- > tools/objtool/builtin-check.c | 4 - > tools/objtool/builtin.h | 2 > tools/objtool/check.c | 155 ++++++++++++++++++++++++++++++++++++------ > tools/objtool/check.h | 3 > 4 files changed, 140 insertions(+), 24 deletions(-) > > --- a/tools/objtool/builtin-check.c > +++ b/tools/objtool/builtin-check.c > @@ -17,7 +17,7 @@ > #include "builtin.h" > #include "check.h" > > -bool no_fp, no_unreachable, retpoline, module, backtrace, uaccess, stats; > +bool no_fp, no_unreachable, retpoline, module, backtrace, uaccess, stats, noinstr, vmlinux; > > static const char * const check_usage[] = { > "objtool check [<options>] file.o", > @@ -32,6 +32,8 @@ const struct option check_options[] = { > 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('i', "noinstr", &noinstr, "noinstr validation only"), > + OPT_BOOLEAN('l', "vmlinux", &vmlinux, "vmlinux.o validation"), > OPT_END(), > };
It seems like there should be an easy way to auto-detect vmlinux.o, without needing a cmdline option.
For example, if the file name is "vmlinux.o" :-)
Also, maybe we can just hard-code the fact that vmlinux.o is always noinstr-only. Over time we'll probably need to move more per-.o functionalities to vmlinux.o and I think we should avoid creating a bunch of cmdline options.
-- Josh
| |