Re: [RFC][PATCH 15/16] objtool: Implement noinstr validation

Josh Poimboeuf
2020-03-15T18:03:31Z

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