[PATCH v2 0/5] x86/bpf: unwinder fixes

[PATCH v2 0/5] x86/bpf: unwinder fixes

[Josh Poimboeuf]

v2:

- Simplified the frame pointer fixes - instead of using R12, just
  continue to use RBP for BPF_REG_FP, but use nested frames so the
  unwinder can understand (suggested by David Laight).

- Dropped the AT&T syntax patches for now.  I'm about to disappear for a
  week and I don't have time to argue about whether code readability is
  a good thing.

- I can do the 32-bit version of the fix when I get back.  It should be
  easy enough.

v1 is here:
https://lkml.kernel.org/r/cover.1560431531.git.jpoimboe@redhat.com


Josh Poimboeuf (4):
  objtool: Fix ORC unwinding in non-JIT BPF generated code
  x86/bpf: Move epilogue generation to a dedicated function
  x86/bpf: Fix 64-bit JIT frame pointer usage
  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 ++++++--
 arch/x86/net/bpf_jit_comp.c  | 115 +++++++++++++++++++----------------
 kernel/bpf/core.c            |   5 +-
 tools/objtool/check.c        |  16 ++++-
 5 files changed, 107 insertions(+), 65 deletions(-)

-- 
2.20.1

[PATCH v2 1/5] perf/x86: Always store regs->ip in perf_callchain_kernel()

[Josh Poimboeuf]

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: 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 f0e4804515d8..6a7cfcadfc1e 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

[PATCH v2 2/5] objtool: Fix ORC unwinding in non-JIT BPF generated code

[Josh Poimboeuf]

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>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 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] = {
 		[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);
-- 
2.20.1

[PATCH v2 3/5] x86/bpf: Move epilogue generation to a dedicated function

[Josh Poimboeuf]

Improve code readability by moving the BPF JIT function epilogue
generation code to a dedicated emit_epilogue() function, analagous to
the existing emit_prologue() function.

Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/net/bpf_jit_comp.c | 37 ++++++++++++++++++++++++-------------
 1 file changed, 24 insertions(+), 13 deletions(-)

diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
index 32bfab4e21eb..da8c988b0f0f 100644
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c
@@ -240,6 +240,28 @@ static void emit_prologue(u8 **pprog, u32 stack_depth, bool ebpf_from_cbpf)
 	*pprog = prog;
 }
 
+static void emit_epilogue(u8 **pprog)
+{
+	u8 *prog = *pprog;
+	int cnt = 0;
+
+	/* mov rbx, qword ptr [rbp+0] */
+	EMIT4(0x48, 0x8B, 0x5D, 0);
+	/* mov r13, qword ptr [rbp+8] */
+	EMIT4(0x4C, 0x8B, 0x6D, 8);
+	/* mov r14, qword ptr [rbp+16] */
+	EMIT4(0x4C, 0x8B, 0x75, 16);
+	/* mov r15, qword ptr [rbp+24] */
+	EMIT4(0x4C, 0x8B, 0x7D, 24);
+
+	/* add rbp, AUX_STACK_SPACE */
+	EMIT4(0x48, 0x83, 0xC5, AUX_STACK_SPACE);
+	EMIT1(0xC9); /* leave */
+	EMIT1(0xC3); /* ret */
+
+	*pprog = prog;
+}
+
 /*
  * Generate the following code:
  *
@@ -1036,19 +1058,8 @@ xadd:			if (is_imm8(insn->off))
 			seen_exit = true;
 			/* Update cleanup_addr */
 			ctx->cleanup_addr = proglen;
-			/* mov rbx, qword ptr [rbp+0] */
-			EMIT4(0x48, 0x8B, 0x5D, 0);
-			/* mov r13, qword ptr [rbp+8] */
-			EMIT4(0x4C, 0x8B, 0x6D, 8);
-			/* mov r14, qword ptr [rbp+16] */
-			EMIT4(0x4C, 0x8B, 0x75, 16);
-			/* mov r15, qword ptr [rbp+24] */
-			EMIT4(0x4C, 0x8B, 0x7D, 24);
-
-			/* add rbp, AUX_STACK_SPACE */
-			EMIT4(0x48, 0x83, 0xC5, AUX_STACK_SPACE);
-			EMIT1(0xC9); /* leave */
-			EMIT1(0xC3); /* ret */
+
+			emit_epilogue(&prog);
 			break;
 
 		default:
-- 
2.20.1

[PATCH v2 4/5] x86/bpf: Fix 64-bit JIT frame pointer usage

[Josh Poimboeuf]

The BPF JIT code clobbers RBP.  This breaks frame pointer convention and
thus prevents the FP unwinder from unwinding through JIT generated code.

Fix it by saving the new RBP value on the stack before updating it.
This effectively creates a new stack frame which the unwinder can
understand.

Also, simplify the BPF JIT prologue such that it more closely resembles
a typical compiler-generated prologue.  This also reduces the prologue
size quite a bit overall.

Suggested-by: David Laight <David.Laight@ACULAB.COM>
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 arch/x86/net/bpf_jit_comp.c | 106 ++++++++++++++++++------------------
 1 file changed, 54 insertions(+), 52 deletions(-)

diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
index da8c988b0f0f..fa1fe65c4cb4 100644
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c
@@ -186,56 +186,54 @@ struct jit_context {
 #define BPF_MAX_INSN_SIZE	128
 #define BPF_INSN_SAFETY		64
 
-#define AUX_STACK_SPACE		40 /* Space for RBX, R13, R14, R15, tailcnt */
-
-#define PROLOGUE_SIZE		37
+#define PROLOGUE_SIZE		24
 
 /*
  * Emit x86-64 prologue code for BPF program and check its size.
  * bpf_tail_call helper will skip it while jumping into another program
  */
-static void emit_prologue(u8 **pprog, u32 stack_depth, bool ebpf_from_cbpf)
+static void emit_prologue(u8 **pprog, u32 stack_depth)
 {
 	u8 *prog = *pprog;
 	int cnt = 0;
 
 	/* push rbp */
 	EMIT1(0x55);
-
-	/* mov rbp,rsp */
+	/* mov rbp, rsp */
 	EMIT3(0x48, 0x89, 0xE5);
 
-	/* sub rsp, rounded_stack_depth + AUX_STACK_SPACE */
-	EMIT3_off32(0x48, 0x81, 0xEC,
-		    round_up(stack_depth, 8) + AUX_STACK_SPACE);
+	/* push r15 */
+	EMIT2(0x41, 0x57);
+	/* push r14 */
+	EMIT2(0x41, 0x56);
+	/* push r13 */
+	EMIT2(0x41, 0x55);
+	/* push rbx */
+	EMIT1(0x53);
 
-	/* sub rbp, AUX_STACK_SPACE */
-	EMIT4(0x48, 0x83, 0xED, AUX_STACK_SPACE);
-
-	/* mov qword ptr [rbp+0],rbx */
-	EMIT4(0x48, 0x89, 0x5D, 0);
-	/* mov qword ptr [rbp+8],r13 */
-	EMIT4(0x4C, 0x89, 0x6D, 8);
-	/* mov qword ptr [rbp+16],r14 */
-	EMIT4(0x4C, 0x89, 0x75, 16);
-	/* mov qword ptr [rbp+24],r15 */
-	EMIT4(0x4C, 0x89, 0x7D, 24);
+	/*
+	 * Push the tail call counter (tail_call_cnt) for eBPF tail calls.
+	 * Initialized to zero.
+	 *
+	 * push $0
+	 */
+	EMIT2(0x6a, 0x00);
 
-	if (!ebpf_from_cbpf) {
-		/*
-		 * Clear the tail call counter (tail_call_cnt): for eBPF tail
-		 * calls we need to reset the counter to 0. It's done in two
-		 * instructions, resetting RAX register to 0, and moving it
-		 * to the counter location.
-		 */
+	/*
+	 * RBP is used for the BPF program's FP register.  It points to the end
+	 * of the program's stack area.  Create another stack frame so the
+	 * unwinder can unwind through the generated code.  The tail_call_cnt
+	 * value doubles as an (invalid) RIP address.
+	 */
+	/* push rbp */
+	EMIT1(0x55);
+	/* mov rbp, rsp */
+	EMIT3(0x48, 0x89, 0xE5);
 
-		/* xor eax, eax */
-		EMIT2(0x31, 0xc0);
-		/* mov qword ptr [rbp+32], rax */
-		EMIT4(0x48, 0x89, 0x45, 32);
+	/* sub rsp, rounded_stack_depth */
+	EMIT3_off32(0x48, 0x81, 0xEC, round_up(stack_depth, 8));
 
-		BUILD_BUG_ON(cnt != PROLOGUE_SIZE);
-	}
+	BUILD_BUG_ON(cnt != PROLOGUE_SIZE);
 
 	*pprog = prog;
 }
@@ -245,19 +243,24 @@ static void emit_epilogue(u8 **pprog)
 	u8 *prog = *pprog;
 	int cnt = 0;
 
-	/* mov rbx, qword ptr [rbp+0] */
-	EMIT4(0x48, 0x8B, 0x5D, 0);
-	/* mov r13, qword ptr [rbp+8] */
-	EMIT4(0x4C, 0x8B, 0x6D, 8);
-	/* mov r14, qword ptr [rbp+16] */
-	EMIT4(0x4C, 0x8B, 0x75, 16);
-	/* mov r15, qword ptr [rbp+24] */
-	EMIT4(0x4C, 0x8B, 0x7D, 24);
-
-	/* add rbp, AUX_STACK_SPACE */
-	EMIT4(0x48, 0x83, 0xC5, AUX_STACK_SPACE);
-	EMIT1(0xC9); /* leave */
-	EMIT1(0xC3); /* ret */
+	/* leave (restore rsp and rbp) */
+	EMIT1(0xC9);
+	/* pop rbx (skip over tail_call_cnt) */
+	EMIT1(0x5B);
+
+	/* pop rbx */
+	EMIT1(0x5B);
+	/* pop r13 */
+	EMIT2(0x41, 0x5D);
+	/* pop r14 */
+	EMIT2(0x41, 0x5E);
+	/* pop r15 */
+	EMIT2(0x41, 0x5F);
+	/* pop rbp */
+	EMIT1(0x5D);
+
+	/* ret */
+	EMIT1(0xC3);
 
 	*pprog = prog;
 }
@@ -295,7 +298,7 @@ static void emit_bpf_tail_call(u8 **pprog)
 	EMIT2(0x89, 0xD2);                        /* mov edx, edx */
 	EMIT3(0x39, 0x56,                         /* cmp dword ptr [rsi + 16], edx */
 	      offsetof(struct bpf_array, map.max_entries));
-#define OFFSET1 (41 + RETPOLINE_RAX_BPF_JIT_SIZE) /* Number of bytes to jump */
+#define OFFSET1 (35 + RETPOLINE_RAX_BPF_JIT_SIZE) /* Number of bytes to jump */
 	EMIT2(X86_JBE, OFFSET1);                  /* jbe out */
 	label1 = cnt;
 
@@ -303,13 +306,13 @@ static void emit_bpf_tail_call(u8 **pprog)
 	 * if (tail_call_cnt > MAX_TAIL_CALL_CNT)
 	 *	goto out;
 	 */
-	EMIT2_off32(0x8B, 0x85, 36);              /* mov eax, dword ptr [rbp + 36] */
+	EMIT3(0x8B, 0x45, 0x0C);                  /* mov eax, dword ptr [rbp + 12] */
 	EMIT3(0x83, 0xF8, MAX_TAIL_CALL_CNT);     /* cmp eax, MAX_TAIL_CALL_CNT */
-#define OFFSET2 (30 + RETPOLINE_RAX_BPF_JIT_SIZE)
+#define OFFSET2 (27 + RETPOLINE_RAX_BPF_JIT_SIZE)
 	EMIT2(X86_JA, OFFSET2);                   /* ja out */
 	label2 = cnt;
 	EMIT3(0x83, 0xC0, 0x01);                  /* add eax, 1 */
-	EMIT2_off32(0x89, 0x85, 36);              /* mov dword ptr [rbp + 36], eax */
+	EMIT3(0x89, 0x45, 0x0C);                  /* mov dword ptr [rbp + 12], eax */
 
 	/* prog = array->ptrs[index]; */
 	EMIT4_off32(0x48, 0x8B, 0x84, 0xD6,       /* mov rax, [rsi + rdx * 8 + offsetof(...)] */
@@ -437,8 +440,7 @@ static int do_jit(struct bpf_prog *bpf_prog, int *addrs, u8 *image,
 	int proglen = 0;
 	u8 *prog = temp;
 
-	emit_prologue(&prog, bpf_prog->aux->stack_depth,
-		      bpf_prog_was_classic(bpf_prog));
+	emit_prologue(&prog, bpf_prog->aux->stack_depth);
 
 	for (i = 0; i < insn_cnt; i++, insn++) {
 		const s32 imm32 = insn->imm;
-- 
2.20.1

[PATCH v2 5/5] x86/unwind/orc: Fall back to using frame pointers for generated code

[Josh Poimboeuf]

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

Re: [PATCH v2 1/5] perf/x86: Always store regs->ip in perf_callchain_kernel()

[Alexei Starovoitov]

On Fri, Jun 14, 2019 at 12:56:40PM -0500, Josh Poimboeuf wrote:
> 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: Josh Poimboeuf <jpoimboe@redhat.com>
> Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>

It's not cool to remove people's SOB.
It's Song's patch. His should be first and your second.

Re: [PATCH v2 2/5] objtool: Fix ORC unwinding in non-JIT BPF generated code

[Alexei Starovoitov]

On Fri, Jun 14, 2019 at 12:56:41PM -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>
> Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  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] = {
>  		[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."

since external tool will be looking at it should it be named 
"bpf_jump_table." to avoid potential name conflicts?
Or even more unique name?
Like "bpf_interpreter_jump_table." ?

Re: [PATCH v2 4/5] x86/bpf: Fix 64-bit JIT frame pointer usage

[Alexei Starovoitov]

On Fri, Jun 14, 2019 at 12:56:43PM -0500, Josh Poimboeuf wrote:
> The BPF JIT code clobbers RBP.  This breaks frame pointer convention and
> thus prevents the FP unwinder from unwinding through JIT generated code.
> 
> Fix it by saving the new RBP value on the stack before updating it.
> This effectively creates a new stack frame which the unwinder can
> understand.
> 
> Also, simplify the BPF JIT prologue such that it more closely resembles
> a typical compiler-generated prologue.  This also reduces the prologue
> size quite a bit overall.
> 
> Suggested-by: David Laight <David.Laight@ACULAB.COM>
> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> ---
>  arch/x86/net/bpf_jit_comp.c | 106 ++++++++++++++++++------------------
>  1 file changed, 54 insertions(+), 52 deletions(-)
> 
> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> index da8c988b0f0f..fa1fe65c4cb4 100644
> --- a/arch/x86/net/bpf_jit_comp.c
> +++ b/arch/x86/net/bpf_jit_comp.c
> @@ -186,56 +186,54 @@ struct jit_context {
>  #define BPF_MAX_INSN_SIZE	128
>  #define BPF_INSN_SAFETY		64
>  
> -#define AUX_STACK_SPACE		40 /* Space for RBX, R13, R14, R15, tailcnt */
> -
> -#define PROLOGUE_SIZE		37
> +#define PROLOGUE_SIZE		24
>  
>  /*
>   * Emit x86-64 prologue code for BPF program and check its size.
>   * bpf_tail_call helper will skip it while jumping into another program
>   */
> -static void emit_prologue(u8 **pprog, u32 stack_depth, bool ebpf_from_cbpf)
> +static void emit_prologue(u8 **pprog, u32 stack_depth)
>  {
>  	u8 *prog = *pprog;
>  	int cnt = 0;
>  
>  	/* push rbp */
>  	EMIT1(0x55);
> -
> -	/* mov rbp,rsp */
> +	/* mov rbp, rsp */
>  	EMIT3(0x48, 0x89, 0xE5);
>  
> -	/* sub rsp, rounded_stack_depth + AUX_STACK_SPACE */
> -	EMIT3_off32(0x48, 0x81, 0xEC,
> -		    round_up(stack_depth, 8) + AUX_STACK_SPACE);
> +	/* push r15 */
> +	EMIT2(0x41, 0x57);
> +	/* push r14 */
> +	EMIT2(0x41, 0x56);
> +	/* push r13 */
> +	EMIT2(0x41, 0x55);
> +	/* push rbx */
> +	EMIT1(0x53);
>  
> -	/* sub rbp, AUX_STACK_SPACE */
> -	EMIT4(0x48, 0x83, 0xED, AUX_STACK_SPACE);
> -
> -	/* mov qword ptr [rbp+0],rbx */
> -	EMIT4(0x48, 0x89, 0x5D, 0);
> -	/* mov qword ptr [rbp+8],r13 */
> -	EMIT4(0x4C, 0x89, 0x6D, 8);
> -	/* mov qword ptr [rbp+16],r14 */
> -	EMIT4(0x4C, 0x89, 0x75, 16);
> -	/* mov qword ptr [rbp+24],r15 */
> -	EMIT4(0x4C, 0x89, 0x7D, 24);
> +	/*
> +	 * Push the tail call counter (tail_call_cnt) for eBPF tail calls.
> +	 * Initialized to zero.
> +	 *
> +	 * push $0
> +	 */
> +	EMIT2(0x6a, 0x00);
>  
> -	if (!ebpf_from_cbpf) {
> -		/*
> -		 * Clear the tail call counter (tail_call_cnt): for eBPF tail
> -		 * calls we need to reset the counter to 0. It's done in two
> -		 * instructions, resetting RAX register to 0, and moving it
> -		 * to the counter location.
> -		 */
> +	/*
> +	 * RBP is used for the BPF program's FP register.  It points to the end
> +	 * of the program's stack area.  Create another stack frame so the
> +	 * unwinder can unwind through the generated code.  The tail_call_cnt
> +	 * value doubles as an (invalid) RIP address.
> +	 */
> +	/* push rbp */
> +	EMIT1(0x55);
> +	/* mov rbp, rsp */
> +	EMIT3(0x48, 0x89, 0xE5);

Have you tested it ?
I really doubt, since in my test both CONFIG_UNWINDER_ORC and
CONFIG_UNWINDER_FRAME_POINTER failed to unwind through such odd frame.

Here is much simple patch that I mentioned in the email yesterday,
but you failed to listen instead of focusing on perceived 'code readability'.

It makes one proper frame and both frame and orc unwinders are happy.

From 442d91571a7f7f92a5cd3bd4a1b139390befbee3 Mon Sep 17 00:00:00 2001
From: Alexei Starovoitov <ast@kernel.org>
Date: Fri, 14 Jun 2019 12:56:43 -0500
Subject: [PATCH bpf] x86/bpf: Fix 64-bit JIT frame pointer usage

The BPF JIT code clobbers RBP.  This breaks frame pointer convention and
thus prevents the FP unwinder from unwinding through JIT generated code.
Fix it by moving callee saved space to the bottom.
Similar to how it was before commit 177366bf7ceb.

Fixes: 177366bf7ceb ("bpf: change x86 JITed program stack layout")
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 arch/x86/net/bpf_jit_comp.c | 84 +++++++++++++++----------------------
 1 file changed, 34 insertions(+), 50 deletions(-)

diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
index a858d9f331b0..4259593b6935 100644
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c
@@ -190,56 +190,38 @@ struct jit_context {
 #define BPF_MAX_INSN_SIZE	128
 #define BPF_INSN_SAFETY		64
 
-#define AUX_STACK_SPACE		40 /* Space for RBX, R13, R14, R15, tailcnt */
-
-#define PROLOGUE_SIZE		37
+#define PROLOGUE_SIZE		20
 
 /*
  * Emit x86-64 prologue code for BPF program and check its size.
  * bpf_tail_call helper will skip it while jumping into another program
  */
-static void emit_prologue(u8 **pprog, u32 stack_depth, bool ebpf_from_cbpf)
+static void emit_prologue(u8 **pprog, u32 stack_depth)
 {
 	u8 *prog = *pprog;
 	int cnt = 0;
 
 	/* push rbp */
 	EMIT1(0x55);
-
-	/* mov rbp,rsp */
+	/* mov rbp, rsp */
 	EMIT3(0x48, 0x89, 0xE5);
 
-	/* sub rsp, rounded_stack_depth + AUX_STACK_SPACE */
-	EMIT3_off32(0x48, 0x81, 0xEC,
-		    round_up(stack_depth, 8) + AUX_STACK_SPACE);
-
-	/* sub rbp, AUX_STACK_SPACE */
-	EMIT4(0x48, 0x83, 0xED, AUX_STACK_SPACE);
-
-	/* mov qword ptr [rbp+0],rbx */
-	EMIT4(0x48, 0x89, 0x5D, 0);
-	/* mov qword ptr [rbp+8],r13 */
-	EMIT4(0x4C, 0x89, 0x6D, 8);
-	/* mov qword ptr [rbp+16],r14 */
-	EMIT4(0x4C, 0x89, 0x75, 16);
-	/* mov qword ptr [rbp+24],r15 */
-	EMIT4(0x4C, 0x89, 0x7D, 24);
+	/* sub rsp, rounded_stack_depth */
+	EMIT3_off32(0x48, 0x81, 0xEC, round_up(stack_depth, 8));
 
-	if (!ebpf_from_cbpf) {
-		/*
-		 * Clear the tail call counter (tail_call_cnt): for eBPF tail
-		 * calls we need to reset the counter to 0. It's done in two
-		 * instructions, resetting RAX register to 0, and moving it
-		 * to the counter location.
-		 */
+	/* push r15 */
+	EMIT2(0x41, 0x57);
+	/* push r14 */
+	EMIT2(0x41, 0x56);
+	/* push r13 */
+	EMIT2(0x41, 0x55);
+	/* push rbx */
+	EMIT1(0x53);
 
-		/* xor eax, eax */
-		EMIT2(0x31, 0xc0);
-		/* mov qword ptr [rbp+32], rax */
-		EMIT4(0x48, 0x89, 0x45, 32);
+	/* zero init tail_call_cnt */
+	EMIT2(0x6a, 0x00);
 
-		BUILD_BUG_ON(cnt != PROLOGUE_SIZE);
-	}
+	BUILD_BUG_ON(cnt != PROLOGUE_SIZE);
 
 	*pprog = prog;
 }
@@ -249,19 +231,22 @@ static void emit_epilogue(u8 **pprog)
 	u8 *prog = *pprog;
 	int cnt = 0;
 
-	/* mov rbx, qword ptr [rbp+0] */
-	EMIT4(0x48, 0x8B, 0x5D, 0);
-	/* mov r13, qword ptr [rbp+8] */
-	EMIT4(0x4C, 0x8B, 0x6D, 8);
-	/* mov r14, qword ptr [rbp+16] */
-	EMIT4(0x4C, 0x8B, 0x75, 16);
-	/* mov r15, qword ptr [rbp+24] */
-	EMIT4(0x4C, 0x8B, 0x7D, 24);
+	/* pop rbx (skip over tail_call_cnt) */
+	EMIT1(0x5B);
+
+	/* pop rbx */
+	EMIT1(0x5B);
+	/* pop r13 */
+	EMIT2(0x41, 0x5D);
+	/* pop r14 */
+	EMIT2(0x41, 0x5E);
+	/* pop r15 */
+	EMIT2(0x41, 0x5F);
+	/* leave (restore rsp and rbp) */
+	EMIT1(0xC9);
 
-	/* add rbp, AUX_STACK_SPACE */
-	EMIT4(0x48, 0x83, 0xC5, AUX_STACK_SPACE);
-	EMIT1(0xC9); /* leave */
-	EMIT1(0xC3); /* ret */
+	/* ret */
+	EMIT1(0xC3);
 
 	*pprog = prog;
 }
@@ -307,13 +292,13 @@ static void emit_bpf_tail_call(u8 **pprog)
 	 * if (tail_call_cnt > MAX_TAIL_CALL_CNT)
 	 *	goto out;
 	 */
-	EMIT2_off32(0x8B, 0x85, 36);              /* mov eax, dword ptr [rbp + 36] */
+	EMIT2_off32(0x8B, 0x85, -36 - MAX_BPF_STACK); /* mov eax, dword ptr [rbp - 548] */
 	EMIT3(0x83, 0xF8, MAX_TAIL_CALL_CNT);     /* cmp eax, MAX_TAIL_CALL_CNT */
 #define OFFSET2 (30 + RETPOLINE_RAX_BPF_JIT_SIZE)
 	EMIT2(X86_JA, OFFSET2);                   /* ja out */
 	label2 = cnt;
 	EMIT3(0x83, 0xC0, 0x01);                  /* add eax, 1 */
-	EMIT2_off32(0x89, 0x85, 36);              /* mov dword ptr [rbp + 36], eax */
+	EMIT2_off32(0x89, 0x85, -36 - MAX_BPF_STACK); /* mov dword ptr [rbp -548], eax */
 
 	/* prog = array->ptrs[index]; */
 	EMIT4_off32(0x48, 0x8B, 0x84, 0xD6,       /* mov rax, [rsi + rdx * 8 + offsetof(...)] */
@@ -441,8 +426,7 @@ static int do_jit(struct bpf_prog *bpf_prog, int *addrs, u8 *image,
 	int proglen = 0;
 	u8 *prog = temp;
 
-	emit_prologue(&prog, bpf_prog->aux->stack_depth,
-		      bpf_prog_was_classic(bpf_prog));
+	emit_prologue(&prog, bpf_prog->aux->stack_depth);
 
 	for (i = 0; i < insn_cnt; i++, insn++) {
 		const s32 imm32 = insn->imm;
-- 
2.20.0

Re: [PATCH v2 1/5] perf/x86: Always store regs->ip in perf_callchain_kernel()

[Josh Poimboeuf]

On Fri, Jun 14, 2019 at 01:56:15PM -0700, Alexei Starovoitov wrote:
> On Fri, Jun 14, 2019 at 12:56:40PM -0500, Josh Poimboeuf wrote:
> > 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: Josh Poimboeuf <jpoimboe@redhat.com>
> > Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> 
> It's not cool to remove people's SOB.
> It's Song's patch. His should be first and your second.

His original patch didn't have an SOB.  I preserved the "From" field.

-- 
Josh

Re: [PATCH v2 2/5] objtool: Fix ORC unwinding in non-JIT BPF generated code

[Josh Poimboeuf]

On Fri, Jun 14, 2019 at 01:58:42PM -0700, Alexei Starovoitov wrote:
> On Fri, Jun 14, 2019 at 12:56:41PM -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>
> > Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > ---
> >  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] = {
> >  		[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."
> 
> since external tool will be looking at it should it be named 
> "bpf_jump_table." to avoid potential name conflicts?
> Or even more unique name?
> Like "bpf_interpreter_jump_table." ?

No, the point is that it's a generic feature which can also be used any
non-BPF code which might also have a jump table.

-- 
Josh

Re: [PATCH v2 2/5] objtool: Fix ORC unwinding in non-JIT BPF generated code

[Alexei Starovoitov]

On Fri, Jun 14, 2019 at 2:07 PM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>
> On Fri, Jun 14, 2019 at 01:58:42PM -0700, Alexei Starovoitov wrote:
> > On Fri, Jun 14, 2019 at 12:56:41PM -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>
> > > Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > > ---
> > >  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] = {
> > >             [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."
> >
> > since external tool will be looking at it should it be named
> > "bpf_jump_table." to avoid potential name conflicts?
> > Or even more unique name?
> > Like "bpf_interpreter_jump_table." ?
>
> No, the point is that it's a generic feature which can also be used any
> non-BPF code which might also have a jump table.

and you're proposing to name all such jump tables in the kernel
as static foo jump_table[] ?

Re: [PATCH v2 1/5] perf/x86: Always store regs->ip in perf_callchain_kernel()

[Steven Rostedt]

On Fri, 14 Jun 2019 16:06:19 -0500
Josh Poimboeuf <jpoimboe@redhat.com> wrote:

> > It's not cool to remove people's SOB.
> > It's Song's patch. His should be first and your second.  
> 
> His original patch didn't have an SOB.  I preserved the "From" field.

Then it can't be accepted. It needs an SOB from the original author.

Song, Please reply with a Signed-off-by tag.

Thanks!

-- Steve

Re: [PATCH v2 4/5] x86/bpf: Fix 64-bit JIT frame pointer usage

[Josh Poimboeuf]

On Fri, Jun 14, 2019 at 02:05:56PM -0700, Alexei Starovoitov wrote:
> Have you tested it ?
> I really doubt, since in my test both CONFIG_UNWINDER_ORC and
> CONFIG_UNWINDER_FRAME_POINTER failed to unwind through such odd frame.

Hm, are you seeing selftest failures?  They seem to work for me.

> Here is much simple patch that I mentioned in the email yesterday,
> but you failed to listen instead of focusing on perceived 'code readability'.
> 
> It makes one proper frame and both frame and orc unwinders are happy.

I'm on my way out the door and I just skimmed it, but it looks fine.

Some of the code and patch description look familiar, please be sure to
give me proper credit.

-- 
Josh

Re: [PATCH v2 2/5] objtool: Fix ORC unwinding in non-JIT BPF generated code

[Josh Poimboeuf]

On Fri, Jun 14, 2019 at 02:09:25PM -0700, Alexei Starovoitov wrote:
> On Fri, Jun 14, 2019 at 2:07 PM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> >
> > On Fri, Jun 14, 2019 at 01:58:42PM -0700, Alexei Starovoitov wrote:
> > > On Fri, Jun 14, 2019 at 12:56:41PM -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>
> > > > Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > > > ---
> > > >  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] = {
> > > >             [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."
> > >
> > > since external tool will be looking at it should it be named
> > > "bpf_jump_table." to avoid potential name conflicts?
> > > Or even more unique name?
> > > Like "bpf_interpreter_jump_table." ?
> >
> > No, the point is that it's a generic feature which can also be used any
> > non-BPF code which might also have a jump table.
> 
> and you're proposing to name all such jump tables in the kernel
> as static foo jump_table[] ?

That's the idea.

-- 
Josh

Re: [PATCH v2 1/5] perf/x86: Always store regs->ip in perf_callchain_kernel()

[Song Liu]

> On Jun 14, 2019, at 2:16 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> On Fri, 14 Jun 2019 16:06:19 -0500
> Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> 
>>> It's not cool to remove people's SOB.
>>> It's Song's patch. His should be first and your second.  
>> 
>> His original patch didn't have an SOB.  I preserved the "From" field.
> 
> Then it can't be accepted. It needs an SOB from the original author.
> 
> Song, Please reply with a Signed-off-by tag.
> 
> Thanks!
> 
> -- Steve

Yes, Sir!

Signed-off-by: Song Liu <songliubraving@fb.com>

It was my fault to forget it in the first place. Sorry for the confusion.

Song

Re: [PATCH v2 2/5] objtool: Fix ORC unwinding in non-JIT BPF generated code

[Alexei Starovoitov]

On Fri, Jun 14, 2019 at 2:19 PM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > > > >
> > > > > +#define JUMP_TABLE_SYM_PREFIX "jump_table."
> > > >
> > > > since external tool will be looking at it should it be named
> > > > "bpf_jump_table." to avoid potential name conflicts?
> > > > Or even more unique name?
> > > > Like "bpf_interpreter_jump_table." ?
> > >
> > > No, the point is that it's a generic feature which can also be used any
> > > non-BPF code which might also have a jump table.
> >
> > and you're proposing to name all such jump tables in the kernel
> > as static foo jump_table[] ?
>
> That's the idea.

Then it needs much wider discussion.
I suggest to rename it to "bpf_interpreter_jump_table."
so it can be resolved now for this specific issue.
While bigger kernel-wide naming convention get resolved.
Later we can rename it to whatever the "standard name for jump table"
will be.

Re: [PATCH v2 4/5] x86/bpf: Fix 64-bit JIT frame pointer usage

[Alexei Starovoitov]

On Fri, Jun 14, 2019 at 2:19 PM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>
> On Fri, Jun 14, 2019 at 02:05:56PM -0700, Alexei Starovoitov wrote:
> > Have you tested it ?
> > I really doubt, since in my test both CONFIG_UNWINDER_ORC and
> > CONFIG_UNWINDER_FRAME_POINTER failed to unwind through such odd frame.
>
> Hm, are you seeing selftest failures?  They seem to work for me.
>
> > Here is much simple patch that I mentioned in the email yesterday,
> > but you failed to listen instead of focusing on perceived 'code readability'.
> >
> > It makes one proper frame and both frame and orc unwinders are happy.
>
> I'm on my way out the door and I just skimmed it, but it looks fine.
>
> Some of the code and patch description look familiar, please be sure to
> give me proper credit.

credit means something positive.
your contribution to bpf jit fix was negative.
I'm going to rewrite the fix from relying on patch 3.

Re: [PATCH v2 4/5] x86/bpf: Fix 64-bit JIT frame pointer usage

[Josh Poimboeuf]

On Fri, Jun 14, 2019 at 02:27:30PM -0700, Alexei Starovoitov wrote:
> On Fri, Jun 14, 2019 at 2:19 PM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> >
> > On Fri, Jun 14, 2019 at 02:05:56PM -0700, Alexei Starovoitov wrote:
> > > Have you tested it ?
> > > I really doubt, since in my test both CONFIG_UNWINDER_ORC and
> > > CONFIG_UNWINDER_FRAME_POINTER failed to unwind through such odd frame.
> >
> > Hm, are you seeing selftest failures?  They seem to work for me.
> >
> > > Here is much simple patch that I mentioned in the email yesterday,
> > > but you failed to listen instead of focusing on perceived 'code readability'.
> > >
> > > It makes one proper frame and both frame and orc unwinders are happy.
> >
> > I'm on my way out the door and I just skimmed it, but it looks fine.
> >
> > Some of the code and patch description look familiar, please be sure to
> > give me proper credit.
> 
> credit means something positive.

So you only give credit for *good* stolen code.  I must have missed that
section of the kernel patch guidelines.

Ironically you posted this patch right after you accused me of removing
Song's SOB (which I didn't).

> your contribution to bpf jit fix was negative.

You have a delightful personality.

-- 
Josh

Re: [PATCH v2 2/5] objtool: Fix ORC unwinding in non-JIT BPF generated code

[Josh Poimboeuf]

On Fri, Jun 14, 2019 at 02:22:59PM -0700, Alexei Starovoitov wrote:
> On Fri, Jun 14, 2019 at 2:19 PM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > > > > >
> > > > > > +#define JUMP_TABLE_SYM_PREFIX "jump_table."
> > > > >
> > > > > since external tool will be looking at it should it be named
> > > > > "bpf_jump_table." to avoid potential name conflicts?
> > > > > Or even more unique name?
> > > > > Like "bpf_interpreter_jump_table." ?
> > > >
> > > > No, the point is that it's a generic feature which can also be used any
> > > > non-BPF code which might also have a jump table.
> > >
> > > and you're proposing to name all such jump tables in the kernel
> > > as static foo jump_table[] ?
> >
> > That's the idea.
> 
> Then it needs much wider discussion.

Why would it need wider discussion?  It only has one user.  If you
honestly believe that it will be controversial to require future users
to call a static jump table "jump_table" then we can have that
discussion when it comes up.

-- 
Josh

Re: [PATCH v2 4/5] x86/bpf: Fix 64-bit JIT frame pointer usage

[Alexei Starovoitov]

On Fri, Jun 14, 2019 at 4:13 PM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>
> On Fri, Jun 14, 2019 at 02:27:30PM -0700, Alexei Starovoitov wrote:
> > On Fri, Jun 14, 2019 at 2:19 PM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > >
> > > On Fri, Jun 14, 2019 at 02:05:56PM -0700, Alexei Starovoitov wrote:
> > > > Have you tested it ?
> > > > I really doubt, since in my test both CONFIG_UNWINDER_ORC and
> > > > CONFIG_UNWINDER_FRAME_POINTER failed to unwind through such odd frame.
> > >
> > > Hm, are you seeing selftest failures?  They seem to work for me.
> > >
> > > > Here is much simple patch that I mentioned in the email yesterday,
> > > > but you failed to listen instead of focusing on perceived 'code readability'.
> > > >
> > > > It makes one proper frame and both frame and orc unwinders are happy.
> > >
> > > I'm on my way out the door and I just skimmed it, but it looks fine.
> > >
> > > Some of the code and patch description look familiar, please be sure to
> > > give me proper credit.
> >
> > credit means something positive.
>
> So you only give credit for *good* stolen code.  I must have missed that
> section of the kernel patch guidelines.

what are you talking about?
you've posted one bad patch. I pointed out multiple issues in it.
Then proposed another bad idea. I pointed out another set of issues.
Than David proposed yet another idea that you've implemented
and claimed that it's working when it was not.
Then I got fed up with this thread and fix it for real by reverting
that old commit that I mentioned way earlier.
https://patchwork.ozlabs.org/patch/1116307/
Where do you see your code or ideas being used?
I see none.

Re: [PATCH v2 2/5] objtool: Fix ORC unwinding in non-JIT BPF generated code

[Alexei Starovoitov]

On Fri, Jun 14, 2019 at 4:17 PM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>
> On Fri, Jun 14, 2019 at 02:22:59PM -0700, Alexei Starovoitov wrote:
> > On Fri, Jun 14, 2019 at 2:19 PM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > > > > > >
> > > > > > > +#define JUMP_TABLE_SYM_PREFIX "jump_table."
> > > > > >
> > > > > > since external tool will be looking at it should it be named
> > > > > > "bpf_jump_table." to avoid potential name conflicts?
> > > > > > Or even more unique name?
> > > > > > Like "bpf_interpreter_jump_table." ?
> > > > >
> > > > > No, the point is that it's a generic feature which can also be used any
> > > > > non-BPF code which might also have a jump table.
> > > >
> > > > and you're proposing to name all such jump tables in the kernel
> > > > as static foo jump_table[] ?
> > >
> > > That's the idea.
> >
> > Then it needs much wider discussion.
>
> Why would it need wider discussion?  It only has one user.  If you
> honestly believe that it will be controversial to require future users
> to call a static jump table "jump_table" then we can have that
> discussion when it comes up.

It's clearly controversial.
I nacked it already on pointless name change
from "jumptable" to "jump_table" and now you're saying
that no one will complain about "jump_table" name
for all jump tables in the kernel that will ever appear?

Re: [PATCH v2 4/5] x86/bpf: Fix 64-bit JIT frame pointer usage

[Josh Poimboeuf]

On Fri, Jun 14, 2019 at 04:23:41PM -0700, Alexei Starovoitov wrote:
> On Fri, Jun 14, 2019 at 4:13 PM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> >
> > On Fri, Jun 14, 2019 at 02:27:30PM -0700, Alexei Starovoitov wrote:
> > > On Fri, Jun 14, 2019 at 2:19 PM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > > >
> > > > On Fri, Jun 14, 2019 at 02:05:56PM -0700, Alexei Starovoitov wrote:
> > > > > Have you tested it ?
> > > > > I really doubt, since in my test both CONFIG_UNWINDER_ORC and
> > > > > CONFIG_UNWINDER_FRAME_POINTER failed to unwind through such odd frame.
> > > >
> > > > Hm, are you seeing selftest failures?  They seem to work for me.
> > > >
> > > > > Here is much simple patch that I mentioned in the email yesterday,
> > > > > but you failed to listen instead of focusing on perceived 'code readability'.
> > > > >
> > > > > It makes one proper frame and both frame and orc unwinders are happy.
> > > >
> > > > I'm on my way out the door and I just skimmed it, but it looks fine.
> > > >
> > > > Some of the code and patch description look familiar, please be sure to
> > > > give me proper credit.
> > >
> > > credit means something positive.
> >
> > So you only give credit for *good* stolen code.  I must have missed that
> > section of the kernel patch guidelines.
> 
> what are you talking about?
> you've posted one bad patch. I pointed out multiple issues in it.
> Then proposed another bad idea. I pointed out another set of issues.
> Than David proposed yet another idea that you've implemented
> and claimed that it's working when it was not.
> Then I got fed up with this thread and fix it for real by reverting
> that old commit that I mentioned way earlier.
> https://patchwork.ozlabs.org/patch/1116307/
> Where do you see your code or ideas being used?
> I see none.

Obviously I wasn't referring to this new whitewashed patch for which I
wasn't even on Cc, despite being one of the people (along with Peter Z)
who convinced you that there was a problem to begin with.

The previous patch you posted has my patch description, push/pop and
comment changes, with no credit:

https://lkml.kernel.org/r/20190614210555.q4ictql3tzzjio4r@ast-mbp.dhcp.thefacebook.com

-- 
Josh

Re: [PATCH v2 4/5] x86/bpf: Fix 64-bit JIT frame pointer usage

[Alexei Starovoitov]

On Fri, Jun 14, 2019 at 4:54 PM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>
> On Fri, Jun 14, 2019 at 04:23:41PM -0700, Alexei Starovoitov wrote:
> > On Fri, Jun 14, 2019 at 4:13 PM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > >
> > > On Fri, Jun 14, 2019 at 02:27:30PM -0700, Alexei Starovoitov wrote:
> > > > On Fri, Jun 14, 2019 at 2:19 PM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > > > >
> > > > > On Fri, Jun 14, 2019 at 02:05:56PM -0700, Alexei Starovoitov wrote:
> > > > > > Have you tested it ?
> > > > > > I really doubt, since in my test both CONFIG_UNWINDER_ORC and
> > > > > > CONFIG_UNWINDER_FRAME_POINTER failed to unwind through such odd frame.
> > > > >
> > > > > Hm, are you seeing selftest failures?  They seem to work for me.
> > > > >
> > > > > > Here is much simple patch that I mentioned in the email yesterday,
> > > > > > but you failed to listen instead of focusing on perceived 'code readability'.
> > > > > >
> > > > > > It makes one proper frame and both frame and orc unwinders are happy.
> > > > >
> > > > > I'm on my way out the door and I just skimmed it, but it looks fine.
> > > > >
> > > > > Some of the code and patch description look familiar, please be sure to
> > > > > give me proper credit.
> > > >
> > > > credit means something positive.
> > >
> > > So you only give credit for *good* stolen code.  I must have missed that
> > > section of the kernel patch guidelines.
> >
> > what are you talking about?
> > you've posted one bad patch. I pointed out multiple issues in it.
> > Then proposed another bad idea. I pointed out another set of issues.
> > Than David proposed yet another idea that you've implemented
> > and claimed that it's working when it was not.
> > Then I got fed up with this thread and fix it for real by reverting
> > that old commit that I mentioned way earlier.
> > https://patchwork.ozlabs.org/patch/1116307/
> > Where do you see your code or ideas being used?
> > I see none.
>
> Obviously I wasn't referring to this new whitewashed patch for which I
> wasn't even on Cc, despite being one of the people (along with Peter Z)
> who convinced you that there was a problem to begin with.

vger has a small limit on cc list. I always trim it to the minimum.

> The previous patch you posted has my patch description, push/pop and
> comment changes, with no credit:

I'm sorry for reusing one sentence from your commit log and
not realizing you want credit for that.
Will not happen again.
I also suggest you never touch anything bpf related.
Just to avoid this credit claims and threads like this one.

Re: [PATCH v2 2/5] objtool: Fix ORC unwinding in non-JIT BPF generated code

[Josh Poimboeuf]

On Fri, Jun 14, 2019 at 04:30:15PM -0700, Alexei Starovoitov wrote:
> On Fri, Jun 14, 2019 at 4:17 PM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> >
> > On Fri, Jun 14, 2019 at 02:22:59PM -0700, Alexei Starovoitov wrote:
> > > On Fri, Jun 14, 2019 at 2:19 PM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > > > > > > >
> > > > > > > > +#define JUMP_TABLE_SYM_PREFIX "jump_table."
> > > > > > >
> > > > > > > since external tool will be looking at it should it be named
> > > > > > > "bpf_jump_table." to avoid potential name conflicts?
> > > > > > > Or even more unique name?
> > > > > > > Like "bpf_interpreter_jump_table." ?
> > > > > >
> > > > > > No, the point is that it's a generic feature which can also be used any
> > > > > > non-BPF code which might also have a jump table.
> > > > >
> > > > > and you're proposing to name all such jump tables in the kernel
> > > > > as static foo jump_table[] ?
> > > >
> > > > That's the idea.
> > >
> > > Then it needs much wider discussion.
> >
> > Why would it need wider discussion?  It only has one user.  If you
> > honestly believe that it will be controversial to require future users
> > to call a static jump table "jump_table" then we can have that
> > discussion when it comes up.
> 
> It's clearly controversial.
> I nacked it already on pointless name change
> from "jumptable" to "jump_table" and now you're saying
> that no one will complain about "jump_table" name
> for all jump tables in the kernel that will ever appear?

Let me get this straight.  You're saying that "jumptable" and
"bpf_interpreter_jump_table" are both acceptable.

But NACK to "jump_table".

Ok...

-- 
Josh

Re: [PATCH v2 2/5] objtool: Fix ORC unwinding in non-JIT BPF generated code

[abhja kaanlani]

Maybe add more multidimensional arrays? 

Sent from my iPhone

>> On Jun 14, 2019, at 5:02 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>> 
>>> On Fri, Jun 14, 2019 at 04:30:15PM -0700, Alexei Starovoitov wrote:
>>>> On Fri, Jun 14, 2019 at 4:17 PM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>>>> 
>>>> On Fri, Jun 14, 2019 at 02:22:59PM -0700, Alexei Starovoitov wrote:
>>>> On Fri, Jun 14, 2019 at 2:19 PM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>>>>>>>>> 
>>>>>>>>> +#define JUMP_TABLE_SYM_PREFIX "jump_table."
>>>>>>>> 
>>>>>>>> since external tool will be looking at it should it be named
>>>>>>>> "bpf_jump_table." to avoid potential name conflicts?
>>>>>>>> Or even more unique name?
>>>>>>>> Like "bpf_interpreter_jump_table." ?
>>>>>>> 
>>>>>>> No, the point is that it's a generic feature which can also be used any
>>>>>>> non-BPF code which might also have a jump table.
>>>>>> 
>>>>>> and you're proposing to name all such jump tables in the kernel
>>>>>> as static foo jump_table[] ?
>>>>> 
>>>>> That's the idea.
>>>> 
>>>> Then it needs much wider discussion.
>>> 
>>> Why would it need wider discussion?  It only has one user.  If you
>>> honestly believe that it will be controversial to require future users
>>> to call a static jump table "jump_table" then we can have that
>>> discussion when it comes up.
>> 
>> It's clearly controversial.
>> I nacked it already on pointless name change
>> from "jumptable" to "jump_table" and now you're saying
>> that no one will complain about "jump_table" name
>> for all jump tables in the kernel that will ever appear?
> 
> Let me get this straight.  You're saying that "jumptable" and
> "bpf_interpreter_jump_table" are both acceptable.
> 
> But NACK to "jump_table".
> 
> Ok...
> 
> -- 
> Josh

Re: [PATCH v2 2/5] objtool: Fix ORC unwinding in non-JIT BPF generated code

[Alexei Starovoitov]

On Fri, Jun 14, 2019 at 5:02 PM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>
> On Fri, Jun 14, 2019 at 04:30:15PM -0700, Alexei Starovoitov wrote:
> > On Fri, Jun 14, 2019 at 4:17 PM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > >
> > > On Fri, Jun 14, 2019 at 02:22:59PM -0700, Alexei Starovoitov wrote:
> > > > On Fri, Jun 14, 2019 at 2:19 PM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > > > > > > > >
> > > > > > > > > +#define JUMP_TABLE_SYM_PREFIX "jump_table."
> > > > > > > >
> > > > > > > > since external tool will be looking at it should it be named
> > > > > > > > "bpf_jump_table." to avoid potential name conflicts?
> > > > > > > > Or even more unique name?
> > > > > > > > Like "bpf_interpreter_jump_table." ?
> > > > > > >
> > > > > > > No, the point is that it's a generic feature which can also be used any
> > > > > > > non-BPF code which might also have a jump table.
> > > > > >
> > > > > > and you're proposing to name all such jump tables in the kernel
> > > > > > as static foo jump_table[] ?
> > > > >
> > > > > That's the idea.
> > > >
> > > > Then it needs much wider discussion.
> > >
> > > Why would it need wider discussion?  It only has one user.  If you
> > > honestly believe that it will be controversial to require future users
> > > to call a static jump table "jump_table" then we can have that
> > > discussion when it comes up.
> >
> > It's clearly controversial.
> > I nacked it already on pointless name change
> > from "jumptable" to "jump_table" and now you're saying
> > that no one will complain about "jump_table" name
> > for all jump tables in the kernel that will ever appear?
>
> Let me get this straight.  You're saying that "jumptable" and
> "bpf_interpreter_jump_table" are both acceptable.
>
> But NACK to "jump_table".
>
> Ok...

Correct. I think I explained the reasons behind, right?

Re: [PATCH v2 4/5] x86/bpf: Fix 64-bit JIT frame pointer usage

[Josh Poimboeuf]

On Fri, Jun 14, 2019 at 05:02:36PM -0700, Alexei Starovoitov wrote:
> On Fri, Jun 14, 2019 at 4:54 PM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > The previous patch you posted has my patch description, push/pop and
> > comment changes, with no credit:
> >
> > https://lkml.kernel.org/r/20190614210555.q4ictql3tzzjio4r@ast-mbp.dhcp.thefacebook.com
> 
> I'm sorry for reusing one sentence from your commit log and
> not realizing you want credit for that.
> Will not happen again.

Um.  What are you talking about?  The entire patch was clearly derived
from mine.  Not just "one sentence from your commit log".  The title,
the pushes/pops in the prologue/epilogue, the removal of the
"ebpf_from_cbpf" argument, the code spacing, and some of the non trivial
comment changes were the same.

> I also suggest you never touch anything bpf related.
> Just to avoid this credit claims and threads like this one.

Wth.  I made a simple request for credit.  Anybody can see the patch was
derived from mine.  It's not like I really care.  It's just basic human
decency.

-- 
Josh

Re: [PATCH v2 4/5] x86/bpf: Fix 64-bit JIT frame pointer usage

[Alexei Starovoitov]

On Fri, Jun 14, 2019 at 9:27 PM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>
> On Fri, Jun 14, 2019 at 05:02:36PM -0700, Alexei Starovoitov wrote:
> > On Fri, Jun 14, 2019 at 4:54 PM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > > The previous patch you posted has my patch description, push/pop and
> > > comment changes, with no credit:
> > >
> > > https://lkml.kernel.org/r/20190614210555.q4ictql3tzzjio4r@ast-mbp.dhcp.thefacebook.com
> >
> > I'm sorry for reusing one sentence from your commit log and
> > not realizing you want credit for that.
> > Will not happen again.
>
> Um.  What are you talking about?  The entire patch was clearly derived
> from mine.  Not just "one sentence from your commit log".  The title,
> the pushes/pops in the prologue/epilogue, the removal of the
> "ebpf_from_cbpf" argument, the code spacing, and some of the non trivial
> comment changes were the same.
>
> > I also suggest you never touch anything bpf related.
> > Just to avoid this credit claims and threads like this one.
>
> Wth.  I made a simple request for credit.  Anybody can see the patch was
> derived from mine.  It's not like I really care.  It's just basic human
> decency.

derived? do you really think so ?
Please fix your orc stuff that is still broken.
Human decency is fixing stuff that you're responsible for.
Your commit d15d356887e7 on April 23 broke stack traces.
And we reported it 3 weeks ago.
Yet instead of fixing it you kept arguing about JIT frame pointers
that is orthogonal issue and was in this state for the last 2 years.

Re: [PATCH v2 4/5] x86/bpf: Fix 64-bit JIT frame pointer usage

[Josh Poimboeuf]

On Fri, Jun 14, 2019 at 10:16:53PM -0700, Alexei Starovoitov wrote:
> On Fri, Jun 14, 2019 at 9:27 PM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> >
> > On Fri, Jun 14, 2019 at 05:02:36PM -0700, Alexei Starovoitov wrote:
> > > On Fri, Jun 14, 2019 at 4:54 PM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > > > The previous patch you posted has my patch description, push/pop and
> > > > comment changes, with no credit:
> > > >
> > > > https://lkml.kernel.org/r/20190614210555.q4ictql3tzzjio4r@ast-mbp.dhcp.thefacebook.com
> > >
> > > I'm sorry for reusing one sentence from your commit log and
> > > not realizing you want credit for that.
> > > Will not happen again.
> >
> > Um.  What are you talking about?  The entire patch was clearly derived
> > from mine.  Not just "one sentence from your commit log".  The title,
> > the pushes/pops in the prologue/epilogue, the removal of the
> > "ebpf_from_cbpf" argument, the code spacing, and some of the non trivial
> > comment changes were the same.
> >
> > > I also suggest you never touch anything bpf related.
> > > Just to avoid this credit claims and threads like this one.
> >
> > Wth.  I made a simple request for credit.  Anybody can see the patch was
> > derived from mine.  It's not like I really care.  It's just basic human
> > decency.
> 
> derived? do you really think so ?
> Please fix your orc stuff that is still broken.
> Human decency is fixing stuff that you're responsible for.
> Your commit d15d356887e7 on April 23 broke stack traces.
> And we reported it 3 weeks ago.
> Yet instead of fixing it you kept arguing about JIT frame pointers
> that is orthogonal issue and was in this state for the last 2 years.

Again you're not making sense.  The fix has already been posted.  That
was the point of this patch set.

It's your call if you want to cherry pick the FP fix (which is a
dependency of the ORC fix) without taking the others.

-- 
Josh

RE: [PATCH v2 2/5] objtool: Fix ORC unwinding in non-JIT BPF generated code

[David Laight]

From: Alexei Starovoitov
> Sent: 14 June 2019 21:59
> 
> On Fri, Jun 14, 2019 at 12:56:41PM -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>
> > Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > ---
> >  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] = {
> >  		[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."
> 
> since external tool will be looking at it should it be named
> "bpf_jump_table." to avoid potential name conflicts?
> Or even more unique name?
> Like "bpf_interpreter_jump_table." ?

If external code might need to process such symbols then
jump_table_bpf_interpreter would (probably) make the symbols
easier to locate.

Oh, and blue is a good colour :-)

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)