Re: [PATCH 00/33] Compile-time stack metadata validation

[Ingo Molnar <mingo@kernel.org>]

So I tried out this latest stacktool series and it looks mostly good for an 
upstream merge.

To help this effort move forward I've applied the preparatory/fix patches that are 
part of this series to tip:x86/debug - that's 26 out of 31 patches. (I've 
propagated all the acks that the latest submission got into the changelogs.)

I have 5 (easy to address) observations that need to be addressed before we can 
move on with the rest of the merge:

1)

Due to recent changes to x86 exception handling, I get a lot of bogus warnings 
about exception table sizes:

  stacktool: arch/x86/kernel/cpu/mcheck/mce.o: __ex_table size not a multiple of 24
  stacktool: arch/x86/kernel/cpu/mtrr/generic.o: __ex_table size not a multiple of 24
  stacktool: arch/x86/kernel/cpu/mtrr/cleanup.o: __ex_table size not a multiple of 24

this is due to:

  548acf19234d x86/mm: Expand the exception table logic to allow new handling options

2)

The fact that 'stacktool' already checks about assembly details like __ex_table[] 
shows that my review feedback early iterations of this series, that the 
'stacktool' name is too specific, was correct.

We really need to rename it before it gets upstream and the situation gets worse. 
__ex_table[] has obviously nothing to do with the 'stack layout' of binaries.

Another suitable name would be 'asmtool' or 'objtool'. For example the following 
would naturally express what it does:

  objtool check kernel/built-in.o

the name expresses that the tool checks object files, independently of the 
existing toolchain. Its primary purpose right now is the checking of stack layout 
details, but the tool is not limited to that at all.

3)

There's quite a bit of overhead when running the tool on larger object files, most 
prominently in cmd_check():

    62.06%  stacktool        stacktool                              [.] cmd_check
     6.72%  stacktool        stacktool                              [.] find_rela_by_dest_range

I added -g to the CFLAGS, which made it visible to annotated output of perf top:

    0.00 :        40430d:       lea    0x4(%rdx,%rax,1),%r13
         :      find_instruction():
         :                                                  struct section *sec,
         :                                                  unsigned long offset)
         :      {
         :              struct instruction *insn;
         :
         :              list_for_each_entry(insn, &file->insns, list)
    0.03 :        404312:       mov    0x38(%rsp),%rax
    0.00 :        404317:       cmp    %rbp,%rax
    0.00 :        40431a:       jne    404334 <cmd_check+0x5b4>
    0.00 :        40431c:       jmpq   4045ba <cmd_check+0x83a>
    0.00 :        404321:       nopl   0x0(%rax)
    6.14 :        404328:       mov    (%rax),%rax
    0.00 :        40432b:       cmp    %rbp,%rax
    2.02 :        40432e:       je     4045ba <cmd_check+0x83a>
         :                      if (insn->sec == sec && insn->offset == offset)
    0.55 :        404334:       cmp    %r12,0x10(%rax)
   87.91 :        404338:       jne    404328 <cmd_check+0x5a8>
    0.00 :        40433a:       cmp    %r13,0x18(%rax)
    3.36 :        40433e:       jne    404328 <cmd_check+0x5a8>
         :      get_jump_destinations():
         :                               * later in validate_functions().
         :                               */
         :                              continue;
         :                      }
         :
         :                      insn->jump_dest = find_instruction(file, dest_sec, dest_off);
    0.00 :        404340:       mov    %rax,0x48(%rbx)
    0.00 :        404344:       jmpq   4042b0 <cmd_check+0x530>
    0.00 :        404349:       nopl   0x0(%rax)
         :      fprintf():
         :
         :      # ifdef __va_arg_pack
         :      __fortify_function int
         :      fprintf (FILE *__restrict __stream, const char *__restrict __fmt, ...)
         :      {
         :        return __fprintf_chk (__stream, __USE_FORTIFY_LEVEL - 1, __fmt,

that looks like a linear list search? That would suck with thousands of entries.

(If this is non-trivial to improve then we can delay this optimization to a later 
patch.)

4)

I think the various 'STACKTOOL' flags in the kernel source are a bit of a misnomer 
- it's not the tool we want to name but the actual property of the code.

So instead of:

  STACKTOOL_IGNORE_FUNC(__bpf_prog_run);

we should do something like:

  STACK_FRAME_NON_STANDARD(__bpf_prog_run);

see how much more expressive it is? It becomes a function attribute independent of 
the tooling that makes use of it.

Similarly, for the highest level 'don't check these directories' makefile flags, 
I'd suggest, instead of using this rather opaque, tool dependent naming:

  STACKTOOL := n

something more specific, like:

  OBJECT_FILES_NON_STANDARD := y

which makes it clearer what's going on: these are special object files that are 
not the typical kernel object files.

Stacktool (or objtool) would be one consumer of this annotation.

I think Boris made similar observations in past reviews.

5)

Likewise, I think the CONFIG_STACK_VALIDATION=y Kconfig flag does not express that 
we do exception table checks as well - and it does not express all the other 
things we may check in object files in the future.

Something like CONFIG_CHECK_OBJECT_FILES=y would express it, and the help text 
would list all the things the tool is able to checks for at the moment.

-------------------

Please send followup iterations of the series against the tip:x86/debug tree (or 
tip:master), to keep the size of the series down.

Thanks,

	Ingo