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

[Josh Poimboeuf]

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@xxxxxxxxxxxxx>

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