linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 0/4] x86: bpf unwinder fixes
@ 2019-06-27  0:33 Josh Poimboeuf
  2019-06-27  0:33 ` [PATCH v3 1/4] perf/x86: Always store regs->ip in perf_callchain_kernel() Josh Poimboeuf
                   ` (3 more replies)
  0 siblings, 4 replies; 18+ messages in thread
From: Josh Poimboeuf @ 2019-06-27  0:33 UTC (permalink / raw)
  To: x86
  Cc: linux-kernel, Peter Zijlstra, Song Liu, Kairui Song,
	Steven Rostedt, Thomas Gleixner, Borislav Petkov, Ingo Molnar

v3:

- Drop even more NACKed BPF changes.

- Coincidentally, a separate fix has already been merged for JIT frame
  pointers:

    fe8d9571dc50 ("bpf, x64: fix stack layout of JITed bpf code")

- 32-bit JIT frame pointers are still broken.  I have a small patch
  which should fix it, if anybody wants to try interacting with the
  maintainer.

- Split the objtool jump table detection feature into a separate patch
  to clarify that it's a generic objtool feature.

Josh Poimboeuf (3):
  objtool: Add support for C jump tables
  bpf: Fix ORC unwinding in non-JIT BPF code
  x86/unwind/orc: Fall back to using frame pointers for generated code

Song Liu (1):
  perf/x86: Always store regs->ip in perf_callchain_kernel()

 arch/x86/events/core.c       | 10 +++++-----
 arch/x86/kernel/unwind_orc.c | 26 ++++++++++++++++++++++----
 kernel/bpf/core.c            |  5 ++---
 tools/objtool/check.c        | 16 ++++++++++++++--
 4 files changed, 43 insertions(+), 14 deletions(-)

-- 
2.20.1


^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH v3 1/4] perf/x86: Always store regs->ip in perf_callchain_kernel()
  2019-06-27  0:33 [PATCH v3 0/4] x86: bpf unwinder fixes Josh Poimboeuf
@ 2019-06-27  0:33 ` Josh Poimboeuf
  2019-06-27 22:22   ` [tip:x86/urgent] " tip-bot for Song Liu
  2019-06-27  0:33 ` [PATCH v3 2/4] objtool: Add support for C jump tables Josh Poimboeuf
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 18+ messages in thread
From: Josh Poimboeuf @ 2019-06-27  0:33 UTC (permalink / raw)
  To: x86
  Cc: linux-kernel, Peter Zijlstra, Song Liu, Kairui Song,
	Steven Rostedt, Thomas Gleixner, Borislav Petkov, Ingo Molnar

From: Song Liu <songliubraving@fb.com>

The stacktrace_map_raw_tp BPF selftest is failing because the RIP saved
by perf_arch_fetch_caller_regs() isn't getting saved by
perf_callchain_kernel().

This was broken by the following commit:

  d15d356887e7 ("perf/x86: Make perf callchains work without CONFIG_FRAME_POINTER")

With that change, when starting with non-HW regs, the unwinder starts
with the current stack frame and unwinds until it passes up the frame
which called perf_arch_fetch_caller_regs().  So regs->ip needs to be
saved deliberately.

Fixes: d15d356887e7 ("perf/x86: Make perf callchains work without CONFIG_FRAME_POINTER")
Signed-off-by: Song Liu <songliubraving@fb.com>
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/events/core.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
index b9ccfe8f7d87..8bc3a7203552 100644
--- a/arch/x86/events/core.c
+++ b/arch/x86/events/core.c
@@ -2328,13 +2328,13 @@ perf_callchain_kernel(struct perf_callchain_entry_ctx *entry, struct pt_regs *re
 		return;
 	}
 
-	if (perf_hw_regs(regs)) {
-		if (perf_callchain_store(entry, regs->ip))
-			return;
+	if (perf_callchain_store(entry, regs->ip))
+		return;
+
+	if (perf_hw_regs(regs))
 		unwind_start(&state, current, regs, NULL);
-	} else {
+	else
 		unwind_start(&state, current, NULL, (void *)regs->sp);
-	}
 
 	for (; !unwind_done(&state); unwind_next_frame(&state)) {
 		addr = unwind_get_return_address(&state);
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH v3 2/4] objtool: Add support for C jump tables
  2019-06-27  0:33 [PATCH v3 0/4] x86: bpf unwinder fixes Josh Poimboeuf
  2019-06-27  0:33 ` [PATCH v3 1/4] perf/x86: Always store regs->ip in perf_callchain_kernel() Josh Poimboeuf
@ 2019-06-27  0:33 ` Josh Poimboeuf
  2019-06-27  1:42   ` Alexei Starovoitov
  2019-06-27  0:33 ` [PATCH v3 3/4] bpf: Fix ORC unwinding in non-JIT BPF code Josh Poimboeuf
  2019-06-27  0:33 ` [PATCH v3 4/4] x86/unwind/orc: Fall back to using frame pointers for generated code Josh Poimboeuf
  3 siblings, 1 reply; 18+ messages in thread
From: Josh Poimboeuf @ 2019-06-27  0:33 UTC (permalink / raw)
  To: x86
  Cc: linux-kernel, Peter Zijlstra, Song Liu, Kairui Song,
	Steven Rostedt, Thomas Gleixner, Borislav Petkov, Ingo Molnar

Objtool doesn't know how to read C jump tables, so it has to whitelist
functions which use them, causing missing ORC unwinder data for such
functions, e.g. ___bpf_prog_run().

C jump tables are very similar to GCC switch jump tables, which objtool
already knows how to read.  So adding support for C jump tables is easy.
It just needs to be able to find the tables and distinguish them from
other data.

To allow the jump tables to be found, create a standard: objtool will
automatically recognize any static local jump table named "jump_table".

Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 tools/objtool/check.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

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);
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH v3 3/4] bpf: Fix ORC unwinding in non-JIT BPF code
  2019-06-27  0:33 [PATCH v3 0/4] x86: bpf unwinder fixes Josh Poimboeuf
  2019-06-27  0:33 ` [PATCH v3 1/4] perf/x86: Always store regs->ip in perf_callchain_kernel() Josh Poimboeuf
  2019-06-27  0:33 ` [PATCH v3 2/4] objtool: Add support for C jump tables Josh Poimboeuf
@ 2019-06-27  0:33 ` Josh Poimboeuf
  2019-06-27  0:57   ` Alexei Starovoitov
  2019-06-27  0:33 ` [PATCH v3 4/4] x86/unwind/orc: Fall back to using frame pointers for generated code Josh Poimboeuf
  3 siblings, 1 reply; 18+ messages in thread
From: Josh Poimboeuf @ 2019-06-27  0:33 UTC (permalink / raw)
  To: x86
  Cc: linux-kernel, Peter Zijlstra, Song Liu, Kairui Song,
	Steven Rostedt, Thomas Gleixner, Borislav Petkov, Ingo Molnar

Objtool previously ignored ___bpf_prog_run() because it didn't
understand the jump table.  This resulted in the ORC unwinder not being
able to unwind through non-JIT BPF code.

Now that objtool knows how to read jump tables, remove the whitelist and
rename the variable to "jump_table" so objtool can recognize it.

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 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index 080e2bb644cc..ff66294882f8 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) \
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH v3 4/4] x86/unwind/orc: Fall back to using frame pointers for generated code
  2019-06-27  0:33 [PATCH v3 0/4] x86: bpf unwinder fixes Josh Poimboeuf
                   ` (2 preceding siblings ...)
  2019-06-27  0:33 ` [PATCH v3 3/4] bpf: Fix ORC unwinding in non-JIT BPF code Josh Poimboeuf
@ 2019-06-27  0:33 ` Josh Poimboeuf
  2019-06-27 22:22   ` [tip:x86/urgent] " tip-bot for Josh Poimboeuf
  3 siblings, 1 reply; 18+ messages in thread
From: Josh Poimboeuf @ 2019-06-27  0:33 UTC (permalink / raw)
  To: x86
  Cc: linux-kernel, Peter Zijlstra, Song Liu, Kairui Song,
	Steven Rostedt, Thomas Gleixner, Borislav Petkov, Ingo Molnar

The ORC unwinder can't unwind through BPF JIT generated code because
there are no ORC entries associated with the code.

If an ORC entry isn't available, try to fall back to frame pointers.  If
BPF and other generated code always do frame pointer setup (even with
CONFIG_FRAME_POINTERS=n) then this will allow ORC to unwind through most
generated code despite there being no corresponding ORC entries.

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>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/kernel/unwind_orc.c | 26 ++++++++++++++++++++++----
 1 file changed, 22 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kernel/unwind_orc.c b/arch/x86/kernel/unwind_orc.c
index 33b66b5c5aec..72b997eaa1fc 100644
--- a/arch/x86/kernel/unwind_orc.c
+++ b/arch/x86/kernel/unwind_orc.c
@@ -82,9 +82,9 @@ static struct orc_entry *orc_find(unsigned long ip);
  * But they are copies of the ftrace entries that are static and
  * defined in ftrace_*.S, which do have orc entries.
  *
- * If the undwinder comes across a ftrace trampoline, then find the
+ * If the unwinder comes across a ftrace trampoline, then find the
  * ftrace function that was used to create it, and use that ftrace
- * function's orc entrie, as the placement of the return code in
+ * function's orc entry, as the placement of the return code in
  * the stack will be identical.
  */
 static struct orc_entry *orc_ftrace_find(unsigned long ip)
@@ -128,6 +128,16 @@ static struct orc_entry null_orc_entry = {
 	.type = ORC_TYPE_CALL
 };
 
+/* Fake frame pointer entry -- used as a fallback for generated code */
+static struct orc_entry orc_fp_entry = {
+	.type		= ORC_TYPE_CALL,
+	.sp_reg		= ORC_REG_BP,
+	.sp_offset	= 16,
+	.bp_reg		= ORC_REG_PREV_SP,
+	.bp_offset	= -16,
+	.end		= 0,
+};
+
 static struct orc_entry *orc_find(unsigned long ip)
 {
 	static struct orc_entry *orc;
@@ -392,8 +402,16 @@ bool unwind_next_frame(struct unwind_state *state)
 	 * calls and calls to noreturn functions.
 	 */
 	orc = orc_find(state->signal ? state->ip : state->ip - 1);
-	if (!orc)
-		goto err;
+	if (!orc) {
+		/*
+		 * As a fallback, try to assume this code uses a frame pointer.
+		 * This is useful for generated code, like BPF, which ORC
+		 * doesn't know about.  This is just a guess, so the rest of
+		 * the unwind is no longer considered reliable.
+		 */
+		orc = &orc_fp_entry;
+		state->error = true;
+	}
 
 	/* End-of-stack check for kernel threads: */
 	if (orc->sp_reg == ORC_REG_UNDEFINED) {
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [PATCH v3 3/4] bpf: Fix ORC unwinding in non-JIT BPF code
  2019-06-27  0:33 ` [PATCH v3 3/4] bpf: Fix ORC unwinding in non-JIT BPF code Josh Poimboeuf
@ 2019-06-27  0:57   ` Alexei Starovoitov
  2019-06-27  1:06     ` Josh Poimboeuf
  0 siblings, 1 reply; 18+ messages in thread
From: Alexei Starovoitov @ 2019-06-27  0:57 UTC (permalink / raw)
  To: Josh Poimboeuf
  Cc: X86 ML, LKML, Peter Zijlstra, Song Liu, Kairui Song,
	Steven Rostedt, Thomas Gleixner, Borislav Petkov, Ingo Molnar

On Wed, Jun 26, 2019 at 5:36 PM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>
> Objtool previously ignored ___bpf_prog_run() because it didn't
> understand the jump table.  This resulted in the ORC unwinder not being
> able to unwind through non-JIT BPF code.
>
> Now that objtool knows how to read jump tables, remove the whitelist and
> rename the variable to "jump_table" so objtool can recognize it.
>
> 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 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> index 080e2bb644cc..ff66294882f8 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];

I thought we were clear that it is a nack?
Either live it alone or rename to something like jump_table_bpf_interpreter
or bpf_interpreter_jump_table.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v3 3/4] bpf: Fix ORC unwinding in non-JIT BPF code
  2019-06-27  0:57   ` Alexei Starovoitov
@ 2019-06-27  1:06     ` Josh Poimboeuf
  2019-06-27  1:22       ` Alexei Starovoitov
  0 siblings, 1 reply; 18+ messages in thread
From: Josh Poimboeuf @ 2019-06-27  1:06 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: X86 ML, LKML, Peter Zijlstra, Song Liu, Kairui Song,
	Steven Rostedt, Thomas Gleixner, Borislav Petkov, Ingo Molnar

On Wed, Jun 26, 2019 at 05:57:08PM -0700, Alexei Starovoitov wrote:
> On Wed, Jun 26, 2019 at 5:36 PM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> >
> > Objtool previously ignored ___bpf_prog_run() because it didn't
> > understand the jump table.  This resulted in the ORC unwinder not being
> > able to unwind through non-JIT BPF code.
> >
> > Now that objtool knows how to read jump tables, remove the whitelist and
> > rename the variable to "jump_table" so objtool can recognize it.
> >
> > 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 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> >
> > diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> > index 080e2bb644cc..ff66294882f8 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];
> 
> I thought we were clear that it is a nack?
> Either live it alone or rename to something like jump_table_bpf_interpreter
> or bpf_interpreter_jump_table.

As I have said many times:

The jump table detection is a generic objtool feature.  It makes no
sense to give a bpf-specific name to a generic objtool feature which can
be used by other components.

-- 
Josh

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v3 3/4] bpf: Fix ORC unwinding in non-JIT BPF code
  2019-06-27  1:06     ` Josh Poimboeuf
@ 2019-06-27  1:22       ` Alexei Starovoitov
  2019-06-27  1:30         ` Josh Poimboeuf
  0 siblings, 1 reply; 18+ messages in thread
From: Alexei Starovoitov @ 2019-06-27  1:22 UTC (permalink / raw)
  To: Josh Poimboeuf
  Cc: X86 ML, LKML, Peter Zijlstra, Song Liu, Kairui Song,
	Steven Rostedt, Thomas Gleixner, Borislav Petkov, Ingo Molnar

On Wed, Jun 26, 2019 at 6:07 PM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>
> On Wed, Jun 26, 2019 at 05:57:08PM -0700, Alexei Starovoitov wrote:
> > On Wed, Jun 26, 2019 at 5:36 PM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > >
> > > Objtool previously ignored ___bpf_prog_run() because it didn't
> > > understand the jump table.  This resulted in the ORC unwinder not being
> > > able to unwind through non-JIT BPF code.
> > >
> > > Now that objtool knows how to read jump tables, remove the whitelist and
> > > rename the variable to "jump_table" so objtool can recognize it.
> > >
> > > 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 ++---
> > >  1 file changed, 2 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> > > index 080e2bb644cc..ff66294882f8 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];
> >
> > I thought we were clear that it is a nack?
> > Either live it alone or rename to something like jump_table_bpf_interpreter
> > or bpf_interpreter_jump_table.
>
> As I have said many times:
>
> The jump table detection is a generic objtool feature.  It makes no
> sense to give a bpf-specific name to a generic objtool feature which can
> be used by other components.

Nacked-by: Alexei Starovoitov <ast@kernel.org>

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v3 3/4] bpf: Fix ORC unwinding in non-JIT BPF code
  2019-06-27  1:22       ` Alexei Starovoitov
@ 2019-06-27  1:30         ` Josh Poimboeuf
  0 siblings, 0 replies; 18+ messages in thread
From: Josh Poimboeuf @ 2019-06-27  1:30 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: X86 ML, LKML, Peter Zijlstra, Song Liu, Kairui Song,
	Steven Rostedt, Thomas Gleixner, Borislav Petkov, Ingo Molnar

On Wed, Jun 26, 2019 at 06:22:48PM -0700, Alexei Starovoitov wrote:
> On Wed, Jun 26, 2019 at 6:07 PM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> >
> > On Wed, Jun 26, 2019 at 05:57:08PM -0700, Alexei Starovoitov wrote:
> > > On Wed, Jun 26, 2019 at 5:36 PM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > > >
> > > > Objtool previously ignored ___bpf_prog_run() because it didn't
> > > > understand the jump table.  This resulted in the ORC unwinder not being
> > > > able to unwind through non-JIT BPF code.
> > > >
> > > > Now that objtool knows how to read jump tables, remove the whitelist and
> > > > rename the variable to "jump_table" so objtool can recognize it.
> > > >
> > > > 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 ++---
> > > >  1 file changed, 2 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> > > > index 080e2bb644cc..ff66294882f8 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];
> > >
> > > I thought we were clear that it is a nack?
> > > Either live it alone or rename to something like jump_table_bpf_interpreter
> > > or bpf_interpreter_jump_table.
> >
> > As I have said many times:
> >
> > The jump table detection is a generic objtool feature.  It makes no
> > sense to give a bpf-specific name to a generic objtool feature which can
> > be used by other components.
> 
> Nacked-by: Alexei Starovoitov <ast@kernel.org>

Whatever, it's your code...

To the -tip maintainers: patches 1 and 4 are independent of this change
and can be merged regardless.

-- 
Josh

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v3 2/4] objtool: Add support for C jump tables
  2019-06-27  0:33 ` [PATCH v3 2/4] objtool: Add support for C jump tables Josh Poimboeuf
@ 2019-06-27  1:42   ` Alexei Starovoitov
  2019-06-27  2:47     ` Josh Poimboeuf
  0 siblings, 1 reply; 18+ messages in thread
From: Alexei Starovoitov @ 2019-06-27  1:42 UTC (permalink / raw)
  To: Josh Poimboeuf
  Cc: X86 ML, LKML, Peter Zijlstra, Song Liu, Kairui Song,
	Steven Rostedt, Thomas Gleixner, Borislav Petkov, Ingo Molnar

On Wed, Jun 26, 2019 at 5:38 PM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>
> Objtool doesn't know how to read C jump tables, so it has to whitelist
> functions which use them, causing missing ORC unwinder data for such
> functions, e.g. ___bpf_prog_run().
>
> C jump tables are very similar to GCC switch jump tables, which objtool
> already knows how to read.  So adding support for C jump tables is easy.
> It just needs to be able to find the tables and distinguish them from
> other data.
>
> To allow the jump tables to be found, create a standard: objtool will
> automatically recognize any static local jump table named "jump_table".
>
> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  tools/objtool/check.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
>
> 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.

Nacked-by: Alexei Starovoitov <ast@kernel.org>

It's not acceptable for objtool to dictate kernel naming convention.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v3 2/4] objtool: Add support for C jump tables
  2019-06-27  1:42   ` Alexei Starovoitov
@ 2019-06-27  2:47     ` Josh Poimboeuf
  2019-06-27  2:54       ` Alexei Starovoitov
  2019-06-27 12:58       ` Steven Rostedt
  0 siblings, 2 replies; 18+ messages in thread
From: Josh Poimboeuf @ 2019-06-27  2:47 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: X86 ML, LKML, Peter Zijlstra, Song Liu, Kairui Song,
	Steven Rostedt, Thomas Gleixner, Borislav Petkov, Ingo Molnar

On Wed, Jun 26, 2019 at 06:42:40PM -0700, Alexei Starovoitov wrote:
> > @@ -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.
> 
> Nacked-by: Alexei Starovoitov <ast@kernel.org>
> 
> It's not acceptable for objtool to dictate kernel naming convention.

Abrasive nack notwithstanding, I agree it's not ideal.

How about the following approach instead?  This is the only other way I
can think of to annotate a jump table so that objtool can distinguish
it:

#define __annotate_jump_table __section(".jump_table.rodata")

Then bpf would just need the following:

-	static const void *jumptable[256] = {
+	static const void __annotate_jump_table *jumptable[256] = {

This would be less magical and fragile than my original approach.

I think the jump table would still be placed with all the other rodata,
like before, because the vmlinux linker script recognizes the section
".rodata" suffix and bundles them all together.

-- 
Josh

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v3 2/4] objtool: Add support for C jump tables
  2019-06-27  2:47     ` Josh Poimboeuf
@ 2019-06-27  2:54       ` Alexei Starovoitov
  2019-06-27  3:44         ` Josh Poimboeuf
  2019-06-27 12:58       ` Steven Rostedt
  1 sibling, 1 reply; 18+ messages in thread
From: Alexei Starovoitov @ 2019-06-27  2:54 UTC (permalink / raw)
  To: Josh Poimboeuf
  Cc: X86 ML, LKML, Peter Zijlstra, Song Liu, Kairui Song,
	Steven Rostedt, Thomas Gleixner, Borislav Petkov, Ingo Molnar

On Wed, Jun 26, 2019 at 7:47 PM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>
> On Wed, Jun 26, 2019 at 06:42:40PM -0700, Alexei Starovoitov wrote:
> > > @@ -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.
> >
> > Nacked-by: Alexei Starovoitov <ast@kernel.org>
> >
> > It's not acceptable for objtool to dictate kernel naming convention.
>
> Abrasive nack notwithstanding, I agree it's not ideal.
>
> How about the following approach instead?  This is the only other way I
> can think of to annotate a jump table so that objtool can distinguish
> it:
>
> #define __annotate_jump_table __section(".jump_table.rodata")
>
> Then bpf would just need the following:
>
> -       static const void *jumptable[256] = {
> +       static const void __annotate_jump_table *jumptable[256] = {
>
> This would be less magical and fragile than my original approach.
>
> I think the jump table would still be placed with all the other rodata,
> like before, because the vmlinux linker script recognizes the section
> ".rodata" suffix and bundles them all together.

I like it if that works :)
Definitely cleaner.
May be a bit more linker script magic would be necessary,
but hopefully not.
And no need to rely on gcc style of mangling static vars.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v3 2/4] objtool: Add support for C jump tables
  2019-06-27  2:54       ` Alexei Starovoitov
@ 2019-06-27  3:44         ` Josh Poimboeuf
  2019-06-27  3:56           ` Josh Poimboeuf
  0 siblings, 1 reply; 18+ messages in thread
From: Josh Poimboeuf @ 2019-06-27  3:44 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: X86 ML, LKML, Peter Zijlstra, Song Liu, Kairui Song,
	Steven Rostedt, Thomas Gleixner, Borislav Petkov, Ingo Molnar

On Wed, Jun 26, 2019 at 07:54:08PM -0700, Alexei Starovoitov wrote:
> On Wed, Jun 26, 2019 at 7:47 PM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> >
> > On Wed, Jun 26, 2019 at 06:42:40PM -0700, Alexei Starovoitov wrote:
> > > > @@ -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.
> > >
> > > Nacked-by: Alexei Starovoitov <ast@kernel.org>
> > >
> > > It's not acceptable for objtool to dictate kernel naming convention.
> >
> > Abrasive nack notwithstanding, I agree it's not ideal.
> >
> > How about the following approach instead?  This is the only other way I
> > can think of to annotate a jump table so that objtool can distinguish
> > it:
> >
> > #define __annotate_jump_table __section(".jump_table.rodata")
> >
> > Then bpf would just need the following:
> >
> > -       static const void *jumptable[256] = {
> > +       static const void __annotate_jump_table *jumptable[256] = {
> >
> > This would be less magical and fragile than my original approach.
> >
> > I think the jump table would still be placed with all the other rodata,
> > like before, because the vmlinux linker script recognizes the section
> > ".rodata" suffix and bundles them all together.
> 
> I like it if that works :)
> Definitely cleaner.
> May be a bit more linker script magic would be necessary,
> but hopefully not.
> And no need to rely on gcc style of mangling static vars.

Completely untested swag:

diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index 8aaf7cd026b0..84212bcb5015 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -116,9 +116,14 @@ void ftrace_likely_update(struct ftrace_likely_data *f, int val,
 	".pushsection .discard.unreachable\n\t"				\
 	".long 999b - .\n\t"						\
 	".popsection\n\t"
+
+/* Annotate a C jump table to enable objtool to follow the code flow */
+#define __annotate_jump_table __section(".jump_table.rodata")
+
 #else
 #define annotate_reachable()
 #define annotate_unreachable()
+#define __annotate_jump_table
 #endif
 
 #ifndef ASM_UNREACHABLE
diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index 080e2bb644cc..e67977e22967 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 __annotate_jump_table *jumptable[256] = {
 		[0 ... 255] = &&default_label,
 		/* Now overwrite non-defaults ... */
 		BPF_INSN_MAP(BPF_INSN_2_LBL, BPF_INSN_3_LBL),
@@ -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 8341c2fff14f..d6d7dd65a195 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -18,7 +18,7 @@
 
 #define FAKE_JUMP_OFFSET -1
 
-#define JUMP_TABLE_SYM_PREFIX "jump_table."
+#define C_JUMP_TABLE_SECTION ".jump_table.rodata"
 
 struct alternative {
 	struct list_head list;
@@ -999,7 +999,6 @@ 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
@@ -1041,15 +1040,12 @@ static struct rela *find_switch_table(struct objtool_file *file,
 		 * 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.
+		 * switch jump tables.  For objtool to recognize them, they
+		 * need to be placed in the C_JUMP_TABLE_SECTION section.  They
+		 * have symbols associated with them.
 		 */
-		sym = find_symbol_containing(rodata_sec, table_offset);
-		if (sym && strncmp(sym->name, JUMP_TABLE_SYM_PREFIX,
-				   strlen(JUMP_TABLE_SYM_PREFIX)))
+		if (find_symbol_containing(rodata_sec, table_offset) &&
+		    strcmp(rodata_sec->name, C_JUMP_TABLE_SECTION))
 			continue;
 
 		rodata_rela = find_rela_by_dest(rodata_sec, table_offset);
@@ -1289,13 +1285,19 @@ static void mark_rodata(struct objtool_file *file)
 	bool found = false;
 
 	/*
-	 * This searches for the .rodata section or multiple .rodata.func_name
-	 * sections if -fdata-sections is being used. The .str.1.1 and .str.1.8
-	 * rodata sections are ignored as they don't contain jump tables.
+	 * This searches for the following rodata sections, each of which can
+	 * potentially contain jump tables:
+	 *
+	 * - .rodata - can contain GCC switch tables
+	 * - .rodata.func_name - if -fdata-sections * is being used
+	 * - .jump_table.rodata - contains C annotated jump tables
+	 *
+	 * The .rodata.str.* sections are ignored because they don't contain
+	 * jump tables.
 	 */
 	for_each_sec(file, sec) {
-		if (!strncmp(sec->name, ".rodata", 7) &&
-		    !strstr(sec->name, ".str1.")) {
+		if ((!strncmp(sec->name, ".rodata", 7) && !strstr(sec->name, ".str1.")) ||
+		    !strcmp(sec->name, C_JUMP_TABLE_SECTION)) {
 			sec->rodata = true;
 			found = true;
 		}

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [PATCH v3 2/4] objtool: Add support for C jump tables
  2019-06-27  3:44         ` Josh Poimboeuf
@ 2019-06-27  3:56           ` Josh Poimboeuf
  2019-06-27  4:38             ` Alexei Starovoitov
  0 siblings, 1 reply; 18+ messages in thread
From: Josh Poimboeuf @ 2019-06-27  3:56 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: X86 ML, LKML, Peter Zijlstra, Song Liu, Kairui Song,
	Steven Rostedt, Thomas Gleixner, Borislav Petkov, Ingo Molnar

On Wed, Jun 26, 2019 at 10:44:47PM -0500, Josh Poimboeuf wrote:
> > > How about the following approach instead?  This is the only other way I
> > > can think of to annotate a jump table so that objtool can distinguish
> > > it:
> > >
> > > #define __annotate_jump_table __section(".jump_table.rodata")
> > >
> > > Then bpf would just need the following:
> > >
> > > -       static const void *jumptable[256] = {
> > > +       static const void __annotate_jump_table *jumptable[256] = {
> > >
> > > This would be less magical and fragile than my original approach.
> > >
> > > I think the jump table would still be placed with all the other rodata,
> > > like before, because the vmlinux linker script recognizes the section
> > > ".rodata" suffix and bundles them all together.
> > 
> > I like it if that works :)
> > Definitely cleaner.
> > May be a bit more linker script magic would be necessary,
> > but hopefully not.
> > And no need to rely on gcc style of mangling static vars.

The last patch was based weird, this one's based on upstream.  Will test
tomorrow.

diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index 8aaf7cd026b0..84212bcb5015 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -116,9 +116,14 @@ void ftrace_likely_update(struct ftrace_likely_data *f, int val,
 	".pushsection .discard.unreachable\n\t"				\
 	".long 999b - .\n\t"						\
 	".popsection\n\t"
+
+/* Annotate a C jump table to enable objtool to follow the code flow */
+#define __annotate_jump_table __section(".jump_table.rodata")
+
 #else
 #define annotate_reachable()
 #define annotate_unreachable()
+#define __annotate_jump_table
 #endif
 
 #ifndef ASM_UNREACHABLE
diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index 080e2bb644cc..e67977e22967 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 __annotate_jump_table *jumptable[256] = {
 		[0 ... 255] = &&default_label,
 		/* Now overwrite non-defaults ... */
 		BPF_INSN_MAP(BPF_INSN_2_LBL, BPF_INSN_3_LBL),
@@ -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..6ade2b32f484 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -18,6 +18,8 @@
 
 #define FAKE_JUMP_OFFSET -1
 
+#define C_JUMP_TABLE_SECTION ".jump_table.rodata"
+
 struct alternative {
 	struct list_head list;
 	struct instruction *insn;
@@ -1035,9 +1037,15 @@ 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.  For objtool to recognize them, they
+		 * need to be placed in the C_JUMP_TABLE_SECTION section.  They
+		 * have symbols associated with them.
 		 */
-		if (find_symbol_containing(rodata_sec, table_offset))
+		if (find_symbol_containing(rodata_sec, table_offset) &&
+		    strcmp(rodata_sec->name, C_JUMP_TABLE_SECTION))
 			continue;
 
 		rodata_rela = find_rela_by_dest(rodata_sec, table_offset);
@@ -1277,13 +1285,18 @@ static void mark_rodata(struct objtool_file *file)
 	bool found = false;
 
 	/*
-	 * This searches for the .rodata section or multiple .rodata.func_name
-	 * sections if -fdata-sections is being used. The .str.1.1 and .str.1.8
-	 * rodata sections are ignored as they don't contain jump tables.
+	 * Search for the following rodata sections, each of which can
+	 * potentially contain jump tables:
+	 *
+	 * - .rodata: can contain GCC switch tables
+	 * - .rodata.<func>: same, if -fdata-sections is being used
+	 * - .jump_table.rodata: contains C annotated jump tables
+	 *
+	 * .rodata.str1.* sections are ignored; they don't contain jump tables.
 	 */
 	for_each_sec(file, sec) {
-		if (!strncmp(sec->name, ".rodata", 7) &&
-		    !strstr(sec->name, ".str1.")) {
+		if ((!strncmp(sec->name, ".rodata", 7) && !strstr(sec->name, ".str1.")) ||
+		    !strcmp(sec->name, C_JUMP_TABLE_SECTION)) {
 			sec->rodata = true;
 			found = true;
 		}

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [PATCH v3 2/4] objtool: Add support for C jump tables
  2019-06-27  3:56           ` Josh Poimboeuf
@ 2019-06-27  4:38             ` Alexei Starovoitov
  0 siblings, 0 replies; 18+ messages in thread
From: Alexei Starovoitov @ 2019-06-27  4:38 UTC (permalink / raw)
  To: Josh Poimboeuf
  Cc: X86 ML, LKML, Peter Zijlstra, Song Liu, Kairui Song,
	Steven Rostedt, Thomas Gleixner, Borislav Petkov, Ingo Molnar

On Wed, Jun 26, 2019 at 8:56 PM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>
> The last patch was based weird, this one's based on upstream.  Will test
> tomorrow.

Great. Once it passes your tests I'll be happy to test it on my side.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v3 2/4] objtool: Add support for C jump tables
  2019-06-27  2:47     ` Josh Poimboeuf
  2019-06-27  2:54       ` Alexei Starovoitov
@ 2019-06-27 12:58       ` Steven Rostedt
  1 sibling, 0 replies; 18+ messages in thread
From: Steven Rostedt @ 2019-06-27 12:58 UTC (permalink / raw)
  To: Josh Poimboeuf
  Cc: Alexei Starovoitov, X86 ML, LKML, Peter Zijlstra, Song Liu,
	Kairui Song, Thomas Gleixner, Borislav Petkov, Ingo Molnar

On Wed, 26 Jun 2019 21:47:00 -0500
Josh Poimboeuf <jpoimboe@redhat.com> wrote:

> On Wed, Jun 26, 2019 at 06:42:40PM -0700, Alexei Starovoitov wrote:
> > > @@ -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.  
> > 
> > Nacked-by: Alexei Starovoitov <ast@kernel.org>
> > 
> > It's not acceptable for objtool to dictate kernel naming convention.  
> 
> Abrasive nack notwithstanding, I agree it's not ideal.
> 
> How about the following approach instead?  This is the only other way I
> can think of to annotate a jump table so that objtool can distinguish
> it:
> 
> #define __annotate_jump_table __section(".jump_table.rodata")
> 
> Then bpf would just need the following:
> 
> -	static const void *jumptable[256] = {
> +	static const void __annotate_jump_table *jumptable[256] = {
> 
> This would be less magical and fragile than my original approach.
> 
> I think the jump table would still be placed with all the other rodata,
> like before, because the vmlinux linker script recognizes the section
> ".rodata" suffix and bundles them all together.
> 

After finally getting a chance to skim through this lovely thread, I
was going to suggest exactly this. This is the way we usually handle
"special" data.

As it appears that Alexei is good with this approach, please go this
route.

Thanks!

-- Steve



^ permalink raw reply	[flat|nested] 18+ messages in thread

* [tip:x86/urgent] perf/x86: Always store regs->ip in perf_callchain_kernel()
  2019-06-27  0:33 ` [PATCH v3 1/4] perf/x86: Always store regs->ip in perf_callchain_kernel() Josh Poimboeuf
@ 2019-06-27 22:22   ` tip-bot for Song Liu
  0 siblings, 0 replies; 18+ messages in thread
From: tip-bot for Song Liu @ 2019-06-27 22:22 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: jpoimboe, tglx, songliubraving, kasong, peterz, linux-kernel,
	rostedt, hpa, bp, mingo

Commit-ID:  83f44ae0f8afcc9da659799db8693f74847e66b3
Gitweb:     https://git.kernel.org/tip/83f44ae0f8afcc9da659799db8693f74847e66b3
Author:     Song Liu <songliubraving@fb.com>
AuthorDate: Wed, 26 Jun 2019 19:33:52 -0500
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Fri, 28 Jun 2019 00:11:20 +0200

perf/x86: Always store regs->ip in perf_callchain_kernel()

The stacktrace_map_raw_tp BPF selftest is failing because the RIP saved by
perf_arch_fetch_caller_regs() isn't getting saved by perf_callchain_kernel().

This was broken by the following commit:

  d15d356887e7 ("perf/x86: Make perf callchains work without CONFIG_FRAME_POINTER")

With that change, when starting with non-HW regs, the unwinder starts
with the current stack frame and unwinds until it passes up the frame
which called perf_arch_fetch_caller_regs().  So regs->ip needs to be
saved deliberately.

Fixes: d15d356887e7 ("perf/x86: Make perf callchains work without CONFIG_FRAME_POINTER")
Signed-off-by: Song Liu <songliubraving@fb.com>
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Kairui Song <kasong@redhat.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Borislav Petkov <bp@alien8.de>
Link: https://lkml.kernel.org/r/3975a298fa52b506fea32666d8ff6a13467eee6d.1561595111.git.jpoimboe@redhat.com

---
 arch/x86/events/core.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
index f315425d8468..4fb3ca1e699d 100644
--- a/arch/x86/events/core.c
+++ b/arch/x86/events/core.c
@@ -2402,13 +2402,13 @@ perf_callchain_kernel(struct perf_callchain_entry_ctx *entry, struct pt_regs *re
 		return;
 	}
 
-	if (perf_hw_regs(regs)) {
-		if (perf_callchain_store(entry, regs->ip))
-			return;
+	if (perf_callchain_store(entry, regs->ip))
+		return;
+
+	if (perf_hw_regs(regs))
 		unwind_start(&state, current, regs, NULL);
-	} else {
+	else
 		unwind_start(&state, current, NULL, (void *)regs->sp);
-	}
 
 	for (; !unwind_done(&state); unwind_next_frame(&state)) {
 		addr = unwind_get_return_address(&state);

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [tip:x86/urgent] x86/unwind/orc: Fall back to using frame pointers for generated code
  2019-06-27  0:33 ` [PATCH v3 4/4] x86/unwind/orc: Fall back to using frame pointers for generated code Josh Poimboeuf
@ 2019-06-27 22:22   ` tip-bot for Josh Poimboeuf
  0 siblings, 0 replies; 18+ messages in thread
From: tip-bot for Josh Poimboeuf @ 2019-06-27 22:22 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: rostedt, kasong, bp, linux-kernel, mingo, hpa, peterz, jpoimboe,
	songliubraving, tglx

Commit-ID:  ae6a45a0868986f69039a2150d3b2b9ca294c378
Gitweb:     https://git.kernel.org/tip/ae6a45a0868986f69039a2150d3b2b9ca294c378
Author:     Josh Poimboeuf <jpoimboe@redhat.com>
AuthorDate: Wed, 26 Jun 2019 19:33:55 -0500
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Fri, 28 Jun 2019 00:11:21 +0200

x86/unwind/orc: Fall back to using frame pointers for generated code

The ORC unwinder can't unwind through BPF JIT generated code because
there are no ORC entries associated with the code.

If an ORC entry isn't available, try to fall back to frame pointers.  If
BPF and other generated code always do frame pointer setup (even with
CONFIG_FRAME_POINTERS=n) then this will allow ORC to unwind through most
generated code despite there being no corresponding ORC entries.

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>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Kairui Song <kasong@redhat.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Borislav Petkov <bp@alien8.de>
Link: https://lkml.kernel.org/r/b6f69208ddff4343d56b7bfac1fc7cfcd62689e8.1561595111.git.jpoimboe@redhat.com

---
 arch/x86/kernel/unwind_orc.c | 26 ++++++++++++++++++++++----
 1 file changed, 22 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kernel/unwind_orc.c b/arch/x86/kernel/unwind_orc.c
index 33b66b5c5aec..72b997eaa1fc 100644
--- a/arch/x86/kernel/unwind_orc.c
+++ b/arch/x86/kernel/unwind_orc.c
@@ -82,9 +82,9 @@ static struct orc_entry *orc_find(unsigned long ip);
  * But they are copies of the ftrace entries that are static and
  * defined in ftrace_*.S, which do have orc entries.
  *
- * If the undwinder comes across a ftrace trampoline, then find the
+ * If the unwinder comes across a ftrace trampoline, then find the
  * ftrace function that was used to create it, and use that ftrace
- * function's orc entrie, as the placement of the return code in
+ * function's orc entry, as the placement of the return code in
  * the stack will be identical.
  */
 static struct orc_entry *orc_ftrace_find(unsigned long ip)
@@ -128,6 +128,16 @@ static struct orc_entry null_orc_entry = {
 	.type = ORC_TYPE_CALL
 };
 
+/* Fake frame pointer entry -- used as a fallback for generated code */
+static struct orc_entry orc_fp_entry = {
+	.type		= ORC_TYPE_CALL,
+	.sp_reg		= ORC_REG_BP,
+	.sp_offset	= 16,
+	.bp_reg		= ORC_REG_PREV_SP,
+	.bp_offset	= -16,
+	.end		= 0,
+};
+
 static struct orc_entry *orc_find(unsigned long ip)
 {
 	static struct orc_entry *orc;
@@ -392,8 +402,16 @@ bool unwind_next_frame(struct unwind_state *state)
 	 * calls and calls to noreturn functions.
 	 */
 	orc = orc_find(state->signal ? state->ip : state->ip - 1);
-	if (!orc)
-		goto err;
+	if (!orc) {
+		/*
+		 * As a fallback, try to assume this code uses a frame pointer.
+		 * This is useful for generated code, like BPF, which ORC
+		 * doesn't know about.  This is just a guess, so the rest of
+		 * the unwind is no longer considered reliable.
+		 */
+		orc = &orc_fp_entry;
+		state->error = true;
+	}
 
 	/* End-of-stack check for kernel threads: */
 	if (orc->sp_reg == ORC_REG_UNDEFINED) {

^ permalink raw reply related	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2019-06-27 22:23 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-27  0:33 [PATCH v3 0/4] x86: bpf unwinder fixes Josh Poimboeuf
2019-06-27  0:33 ` [PATCH v3 1/4] perf/x86: Always store regs->ip in perf_callchain_kernel() Josh Poimboeuf
2019-06-27 22:22   ` [tip:x86/urgent] " tip-bot for Song Liu
2019-06-27  0:33 ` [PATCH v3 2/4] objtool: Add support for C jump tables Josh Poimboeuf
2019-06-27  1:42   ` Alexei Starovoitov
2019-06-27  2:47     ` Josh Poimboeuf
2019-06-27  2:54       ` Alexei Starovoitov
2019-06-27  3:44         ` Josh Poimboeuf
2019-06-27  3:56           ` Josh Poimboeuf
2019-06-27  4:38             ` Alexei Starovoitov
2019-06-27 12:58       ` Steven Rostedt
2019-06-27  0:33 ` [PATCH v3 3/4] bpf: Fix ORC unwinding in non-JIT BPF code Josh Poimboeuf
2019-06-27  0:57   ` Alexei Starovoitov
2019-06-27  1:06     ` Josh Poimboeuf
2019-06-27  1:22       ` Alexei Starovoitov
2019-06-27  1:30         ` Josh Poimboeuf
2019-06-27  0:33 ` [PATCH v3 4/4] x86/unwind/orc: Fall back to using frame pointers for generated code Josh Poimboeuf
2019-06-27 22:22   ` [tip:x86/urgent] " tip-bot for Josh Poimboeuf

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).