linux-kernel - Re: [PATCH 2/9] objtool: Fix ORC unwinding in non-JIT BPF generated code

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 13 Jun 2019 20:20:30 -0500
From:   Josh Poimboeuf <jpoimboe@...hat.com>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc:     x86@...nel.org, linux-kernel@...r.kernel.org,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>, netdev@...r.kernel.org,
        bpf@...r.kernel.org, Peter Zijlstra <peterz@...radead.org>,
        Song Liu <songliubraving@...com>,
        Kairui Song <kasong@...hat.com>
Subject: Re: [PATCH 2/9] objtool: Fix ORC unwinding in non-JIT BPF generated
 code

On Thu, Jun 13, 2019 at 01:57:11PM -0700, Alexei Starovoitov wrote:
> On Thu, Jun 13, 2019 at 08:20:59AM -0500, Josh Poimboeuf wrote:
> > Objtool currently ignores ___bpf_prog_run() because it doesn't
> > understand the jump table.  This results in the ORC unwinder not being
> > able to unwind through non-JIT BPF code.
> > 
> > Luckily, the BPF jump table resembles a GCC switch jump table, which
> > objtool already knows how to read.
> > 
> > Add generic support for reading any static local jump table array named
> > "jump_table", and rename the BPF variable accordingly, so objtool can
> > generate ORC data for ___bpf_prog_run().
> > 
> > Fixes: d15d356887e7 ("perf/x86: Make perf callchains work without CONFIG_FRAME_POINTER")
> > Reported-by: Song Liu <songliubraving@...com>
> > Signed-off-by: Josh Poimboeuf <jpoimboe@...hat.com>
> > ---
> >  kernel/bpf/core.c     |  5 ++---
> >  tools/objtool/check.c | 16 ++++++++++++++--
> >  2 files changed, 16 insertions(+), 5 deletions(-)
> > 
> > diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> > index 7c473f208a10..aa546ef7dbdc 100644
> > --- a/kernel/bpf/core.c
> > +++ b/kernel/bpf/core.c
> > @@ -1299,7 +1299,7 @@ static u64 ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn, u64 *stack)
> >  {
> >  #define BPF_INSN_2_LBL(x, y)    [BPF_##x | BPF_##y] = &&x##_##y
> >  #define BPF_INSN_3_LBL(x, y, z) [BPF_##x | BPF_##y | BPF_##z] = &&x##_##y##_##z
> > -	static const void *jumptable[256] = {
> > +	static const void *jump_table[256] = {
> 
> Nack to the change like above

"jump table" is two words, so does it not make sense to separate them
with an underscore for readability?

I created a generic feature in objtool for this so that other code can
also use it.  So a generic name (and typical Linux naming convention --
separating words with an underscore) makes sense here.

> and to patches 8 and 9.

Well, it's your code, but ... can I ask why?  AT&T syntax is the
standard for Linux, which is in fact the OS we are developing for.

It makes the code extra confusing for it to be annotated differently
than all other Linux asm code.  And due to the inherent complexity of
generating code at runtime, I'd think we'd want to make the code as
readable as possible, for as many people as possible (i.e. other Linux
developers).

-- 
Josh