Date:   Sun, 7 Jul 2019 00:52:09 -0500
From:   Josh Poimboeuf <jpoimboe@...hat.com>
To:     Ingo Molnar <mingo@...nel.org>
Cc:     bp@...en8.de, hpa@...or.com, songliubraving@...com,
        tglx@...utronix.de, rostedt@...dmis.org, kasong@...hat.com,
        daniel@...earbox.net, ast@...nel.org, peterz@...radead.org,
        linux-kernel@...r.kernel.org, linux-tip-commits@...r.kernel.org
Subject: Re: [tip:x86/urgent] bpf: Fix ORC unwinding in non-JIT BPF code

On Sat, Jul 06, 2019 at 08:32:06PM -0500, Josh Poimboeuf wrote:
> On Sat, Jul 06, 2019 at 10:29:42PM +0200, Ingo Molnar wrote:
> > Hm, I get this new build warning on x86-64 defconfig-ish kernels plus 
> > these enabled:
> > 
> >  CONFIG_BPF=y
> >  CONFIG_BPF_JIT=y
> > 
> > kernel/bpf/core.o: warning: objtool: ___bpf_prog_run()+0x8da: sibling call from callable instruction with modified stack frame
> 
> I assume you have CONFIG_RETPOLINE disabled?  For some reason that
> causes GCC to add 166 indirect jumps to that function, which is giving
> objtool trouble.  Looking into it.

Alexei, do you have any objections to setting -fno-gcse for
___bpf_prog_run()?  Either for the function or the file?  Doing so seems
to be recommended by the GCC manual for computed gotos.  It would also
"fix" one of the issues.  More details below.

Details:

With CONFIG_RETPOLINE=n, there are a couple of GCC optimizations in
___bpf_prog_run() which objtool is having trouble with.

1)

  The function has:

	select_insn:
		goto *jumptable[insn->code];

  And then a bunch of "goto select_insn" statements.

  GCC is basically replacing

	select_insn:
		jmp *jumptable(,%rax,8)
		...
	ALU64_ADD_X:
		...
		jmp select_insn
	ALU_ADD_X:
		...
		jmp select_insn

  with

	select_insn:
		jmp *jumptable(, %rax, 8)
		...
	ALU64_ADD_X:
		...
		jmp *jumptable(, %rax, 8)
	ALU_ADD_X:
		...
		jmp *jumptable(, %rax, 8)


  It does that 166 times.

  For some reason, it doesn't do the optimization with retpolines
  enabled.

  Objtool has never seen multiple indirect jump sites which use the same
  jump table.  This is relatively trivial to fix (I already have a
  working patch).

2)

  After doing the first optimization, GCC then does another one which is
  a little trickier.  It replaces:

	select_insn:
		jmp *jumptable(, %rax, 8)
		...
	ALU64_ADD_X:
		...
		jmp *jumptable(, %rax, 8)
	ALU_ADD_X:
		...
		jmp *jumptable(, %rax, 8)

  with

	select_insn:
		mov jumptable, %r12
		jmp *(%r12, %rax, 8)
		...
	ALU64_ADD_X:
		...
		jmp *(%r12, %rax, 8)
	ALU_ADD_X:
		...
		jmp *(%r12, %rax, 8)

  The problem is that it only moves the jumptable address into %r12
  once, for the entire function, then it goes through multiple recursive
  indirect jumps which rely on that %r12 value.  But objtool isn't yet
  smart enough to be able to track the value across multiple recursive
  indirect jumps through the jump table.

  After some digging I found that the quick and easy fix is to disable
  -fgcse.  In fact, this seems to be recommended by the GCC manual, for
  code like this:

    -fgcse
	Perform a global common subexpression elimination pass.  This
	pass also performs global constant and copy propagation.

	Note: When compiling a program using computed gotos, a GCC
	extension, you may get better run-time performance if you
	disable the global common subexpression elimination pass by
	adding -fno-gcse to the command line.

        Enabled at levels -O2, -O3, -Os.

  This code indeed relies extensively on computed gotos.  I don't know
  *why* disabling this optimization would improve performance.  In fact
  I really don't see how it could make much of a difference either way.

  Anyway, using -fno-gcse makes optimization #2 go away and makes
  objtool happy, with only a fix for #1 needed.

  If -fno-gcse isn't an option, we might be able to fix objtool by using
  the "first_jump_src" thing which Peter added, improving it such that
  it also takes table jumps into account.

-- 
Josh

Powered by blists - more mailing lists