| Message ID | 99c22bbd79e72855f4bc9049981602d537a54e70.1560431531.git.jpoimboe@redhat.com |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | x86/bpf: unwinder fixes | expand |
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@fb.com> > Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.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 and to patches 8 and 9. Everyone has different stylistic preferences. My preference is to keep things as they are. Please respin the rest. We'll take it via bpf tree.
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@fb.com> > > Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.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).
On Thu, Jun 13, 2019 at 08:20:30PM -0500, Josh Poimboeuf wrote: > 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@fb.com> > > > Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.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). imo your changes make it less readable. please do not randomly change names and style based on your own preferences. dst=src mov(dst,src) memcpy(dst,src) if people want to have more bugs in assembler code. it's their call. bpf_jit_comp.c is C code. dest is on the left.
On Thu, Jun 13, 2019 at 06:37:21PM -0700, Alexei Starovoitov wrote: > On Thu, Jun 13, 2019 at 08:20:30PM -0500, Josh Poimboeuf wrote: > > 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@fb.com> > > > > Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.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). > > imo your changes make it less readable. How does introducing an underscore between two words make them less readable? > please do not randomly change names and style based on your own preferences. These are Linux standards, not my own preferences. > dst=src > mov(dst,src) > memcpy(dst,src) > if people want to have more bugs in assembler code. it's their call. > bpf_jit_comp.c is C code. dest is on the left. So you don't like the ordering of the src,dst function arguments? Ok. But what do you think about the AT&T syntax comments?
On Thu, Jun 13, 2019 at 08:20:30PM -0500, Josh Poimboeuf wrote: > On Thu, Jun 13, 2019 at 01:57:11PM -0700, Alexei Starovoitov wrote: > > 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. I agree, all assembly in Linux is AT&T, adding Intel notation only serves to cause confusion.
On Fri, Jun 14, 2019 at 09:08:52AM +0200, Peter Zijlstra wrote: > On Thu, Jun 13, 2019 at 08:20:30PM -0500, Josh Poimboeuf wrote: > > On Thu, Jun 13, 2019 at 01:57:11PM -0700, Alexei Starovoitov wrote: > > > > 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. > > I agree, all assembly in Linux is AT&T, adding Intel notation only > serves to cause confusion. It's not assembly. It's C code that generates binary and here we're talking about comments. I'm sure you're not proposing to do: /* mov src, dst */ #define EMIT_mov(DST, SRC) \ right? bpf_jit_comp.c stays as-is. Enough of it.
On Fri, Jun 14, 2019 at 12:35:38AM -0700, Alexei Starovoitov wrote: > On Fri, Jun 14, 2019 at 09:08:52AM +0200, Peter Zijlstra wrote: > > On Thu, Jun 13, 2019 at 08:20:30PM -0500, Josh Poimboeuf wrote: > > > On Thu, Jun 13, 2019 at 01:57:11PM -0700, Alexei Starovoitov wrote: > > > > > > 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. > > > > I agree, all assembly in Linux is AT&T, adding Intel notation only > > serves to cause confusion. > > It's not assembly. It's C code that generates binary and here > we're talking about comments. And comments are useless if they don't clarify. Intel syntax confuses. > I'm sure you're not proposing to do: > /* mov src, dst */ > #define EMIT_mov(DST, SRC) \ > right? Which is why Josh reversed both of them. The current Intel order is just terribly confusing. And I don't see any of the other JITs having confusing comments like this. > bpf_jit_comp.c stays as-is. Enough of it. I think you're forgetting this is also arch/x86 code, and no, it needs changes because its broken wrt unwinding.
On Fri, Jun 14, 2019 at 1:11 AM Peter Zijlstra <peterz@infradead.org> wrote: > > On Fri, Jun 14, 2019 at 12:35:38AM -0700, Alexei Starovoitov wrote: > > On Fri, Jun 14, 2019 at 09:08:52AM +0200, Peter Zijlstra wrote: > > > On Thu, Jun 13, 2019 at 08:20:30PM -0500, Josh Poimboeuf wrote: > > > > On Thu, Jun 13, 2019 at 01:57:11PM -0700, Alexei Starovoitov wrote: > > > > > > > > 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. > > > > > > I agree, all assembly in Linux is AT&T, adding Intel notation only > > > serves to cause confusion. > > > > It's not assembly. It's C code that generates binary and here > > we're talking about comments. > > And comments are useless if they don't clarify. Intel syntax confuses. > > > I'm sure you're not proposing to do: > > /* mov src, dst */ > > #define EMIT_mov(DST, SRC) \ > > right? > > Which is why Josh reversed both of them. The current Intel order is just > terribly confusing. And I don't see any of the other JITs having > confusing comments like this. > > > bpf_jit_comp.c stays as-is. Enough of it. > > I think you're forgetting this is also arch/x86 code, and no, it needs > changes because its broken wrt unwinding. See MAINTAINERS file. If you guys keep insisting on pointless churn like this we'll move arch/x86/net/ into net/ where it probably belongs. netdev has its own comment style too. And it is also probably confusing to some folks.
On Fri, Jun 14, 2019 at 08:13:49AM -0700, Alexei Starovoitov wrote: > On Fri, Jun 14, 2019 at 1:11 AM Peter Zijlstra <peterz@infradead.org> wrote: > > > > On Fri, Jun 14, 2019 at 12:35:38AM -0700, Alexei Starovoitov wrote: > > > On Fri, Jun 14, 2019 at 09:08:52AM +0200, Peter Zijlstra wrote: > > > > On Thu, Jun 13, 2019 at 08:20:30PM -0500, Josh Poimboeuf wrote: > > > > > On Thu, Jun 13, 2019 at 01:57:11PM -0700, Alexei Starovoitov wrote: > > > > > > > > > > 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. > > > > > > > > I agree, all assembly in Linux is AT&T, adding Intel notation only > > > > serves to cause confusion. > > > > > > It's not assembly. It's C code that generates binary and here > > > we're talking about comments. > > > > And comments are useless if they don't clarify. Intel syntax confuses. > > > > > I'm sure you're not proposing to do: > > > /* mov src, dst */ > > > #define EMIT_mov(DST, SRC) \ > > > right? > > > > Which is why Josh reversed both of them. The current Intel order is just > > terribly confusing. And I don't see any of the other JITs having > > confusing comments like this. > > > > > bpf_jit_comp.c stays as-is. Enough of it. > > > > I think you're forgetting this is also arch/x86 code, and no, it needs > > changes because its broken wrt unwinding. > > See MAINTAINERS file. > If you guys keep insisting on pointless churn like this > we'll move arch/x86/net/ into net/ where it probably belongs. > netdev has its own comment style too. > And it is also probably confusing to some folks. So if I understand correctly, you're proposing that we move x86-specific code to net/arch/x86 so you don't have to make your code readable to others and adhere to kernel style guidelines?
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] = { [0 ... 255] = &&default_label, /* Now overwrite non-defaults ... */ BPF_INSN_MAP(BPF_INSN_2_LBL, BPF_INSN_3_LBL), @@ -1315,7 +1315,7 @@ static u64 ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn, u64 *stack) #define CONT_JMP ({ insn++; goto select_insn; }) select_insn: - goto *jumptable[insn->code]; + goto *jump_table[insn->code]; /* ALU */ #define ALU(OPCODE, OP) \ @@ -1558,7 +1558,6 @@ static u64 ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn, u64 *stack) BUG_ON(1); return 0; } -STACK_FRAME_NON_STANDARD(___bpf_prog_run); /* jump table */ #define PROG_NAME(stack_size) __bpf_prog_run##stack_size #define DEFINE_BPF_PROG_RUN(stack_size) \ diff --git a/tools/objtool/check.c b/tools/objtool/check.c index 172f99195726..8341c2fff14f 100644 --- a/tools/objtool/check.c +++ b/tools/objtool/check.c @@ -18,6 +18,8 @@ #define FAKE_JUMP_OFFSET -1 +#define JUMP_TABLE_SYM_PREFIX "jump_table." + struct alternative { struct list_head list; struct instruction *insn; @@ -997,6 +999,7 @@ static struct rela *find_switch_table(struct objtool_file *file, struct instruction *orig_insn = insn; struct section *rodata_sec; unsigned long table_offset; + struct symbol *sym; /* * Backward search using the @first_jump_src links, these help avoid @@ -1035,9 +1038,18 @@ static struct rela *find_switch_table(struct objtool_file *file, /* * Make sure the .rodata address isn't associated with a - * symbol. gcc jump tables are anonymous data. + * symbol. GCC jump tables are anonymous data. + * + * Also support C jump tables which are in the same format as + * switch jump tables. Each jump table should be a static + * local const array named "jump_table" for objtool to + * recognize it. Note: GCC will add a numbered suffix to the + * ELF symbol name, like "jump_table.12345", which it does for + * all static local variables. */ - if (find_symbol_containing(rodata_sec, table_offset)) + sym = find_symbol_containing(rodata_sec, table_offset); + if (sym && strncmp(sym->name, JUMP_TABLE_SYM_PREFIX, + strlen(JUMP_TABLE_SYM_PREFIX))) continue; rodata_rela = find_rela_by_dest(rodata_sec, table_offset);
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@fb.com> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com> --- kernel/bpf/core.c | 5 ++--- tools/objtool/check.c | 16 ++++++++++++++-- 2 files changed, 16 insertions(+), 5 deletions(-)