Re: [PATCH 6/9] x86/bpf: Fix JIT frame pointer usage

[Josh Poimboeuf <jpoimboe@redhat.com>]

On Fri, Jun 14, 2019 at 01:58:21PM +0000, David Laight wrote:
> From: Josh Poimboeuf
> > Sent: 14 June 2019 14:44
> > 
> > On Fri, Jun 14, 2019 at 10:50:23AM +0000, David Laight wrote:
> > > On Thu, Jun 13, 2019 at 08:21:03AM -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.
> > > >
> > > > RBP is currently used as the BPF stack frame pointer register.  The
> > > > actual register used is opaque to the user, as long as it's a
> > > > callee-saved register.  Change it to use R12 instead.
> > >
> > > Could you maintain the system %rbp chain through the BPF stack?
> > 
> > Do you mean to save RBP again before changing it again, so that we
> > create another stack frame inside the BPF stack?  That might work.
> 
> The unwinder will (IIRC) expect *%rbp to be the previous %rbp value.
> If you maintain that it will probably all work.
> 
> > > It might even be possible to put something relevant in the %rip
> > > location.
> > 
> > I'm not sure what you mean here.
> 
> The return address is (again IIRC) %rbp[-8] so the unwinder will
> expect that address to be a symbol.

Ah, gotcha.  We don't necessarily need the real rip on the stack as the
unwinder can handle bad text addresses ok.  Though the real one would be
better.

> I do remember a stack trace printer for x86 this didn't need
> any annotation of the object code and didn't need frame pointers.
> The only downside was that it had to 'guess' (ie scan the stack)
> to get out of functions that couldn't return.
> Basically it followed the control flow forwards tracking the
> values of %sp and %bp until it found a return instuction.
> All it has to do is detect loops and retry from the other
> target of conditional branches.

That actually sounds kind of cool, though I don't think we need that for
the kernel.

Anyway here's a patch with your suggestion.  I think it's the best idea
so far because it doesn't require the use of R12, nor does it require
abstracting BPF_REG_FP with an offset.  And the diffstat is pretty
small and self-contained.

It seems to work, though I didn't put a real RIP on the stack yet.  This
is based on top of the "x86/bpf: Simplify prologue generation" patch.

diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
index 485692d4b163..fa1fe65c4cb4 100644
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c
@@ -186,7 +186,7 @@ struct jit_context {
 #define BPF_MAX_INSN_SIZE	128
 #define BPF_INSN_SAFETY		64
 
-#define PROLOGUE_SIZE		20
+#define PROLOGUE_SIZE		24
 
 /*
  * Emit x86-64 prologue code for BPF program and check its size.
@@ -197,14 +197,17 @@ static void emit_prologue(u8 **pprog, u32 stack_depth)
 	u8 *prog = *pprog;
 	int cnt = 0;
 
+	/* push rbp */
+	EMIT1(0x55);
+	/* mov rbp, rsp */
+	EMIT3(0x48, 0x89, 0xE5);
+
 	/* push r15 */
 	EMIT2(0x41, 0x57);
 	/* push r14 */
 	EMIT2(0x41, 0x56);
 	/* push r13 */
 	EMIT2(0x41, 0x55);
-	/* push rbp */
-	EMIT1(0x55);
 	/* push rbx */
 	EMIT1(0x53);
 
@@ -218,10 +221,13 @@ static void emit_prologue(u8 **pprog, u32 stack_depth)
 
 	/*
 	 * RBP is used for the BPF program's FP register.  It points to the end
-	 * of the program's stack area.
-	 *
-	 * mov rbp, rsp
+	 * 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);
 
 	/* sub rsp, rounded_stack_depth */
@@ -237,19 +243,21 @@ static void emit_epilogue(u8 **pprog)
 	u8 *prog = *pprog;
 	int cnt = 0;
 
-	/* lea rsp, [rbp+0x8] */
-	EMIT4(0x48, 0x8D, 0x65, 0x08);
+	/* leave (restore rsp and rbp) */
+	EMIT1(0xC9);
+	/* pop rbx (skip over tail_call_cnt) */
+	EMIT1(0x5B);
 
 	/* pop rbx */
 	EMIT1(0x5B);
-	/* pop rbp */
-	EMIT1(0x5D);
 	/* pop r13 */
 	EMIT2(0x41, 0x5D);
 	/* pop r14 */
 	EMIT2(0x41, 0x5E);
 	/* pop r15 */
 	EMIT2(0x41, 0x5F);
+	/* pop rbp */
+	EMIT1(0x5D);
 
 	/* ret */
 	EMIT1(0xC3);
@@ -298,13 +306,13 @@ static void emit_bpf_tail_call(u8 **pprog)
 	 * if (tail_call_cnt > MAX_TAIL_CALL_CNT)
 	 *	goto out;
 	 */
-	EMIT3(0x8B, 0x45, 0x04);                  /* mov eax, dword ptr [rbp + 4] */
+	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 (27 + RETPOLINE_RAX_BPF_JIT_SIZE)
 	EMIT2(X86_JA, OFFSET2);                   /* ja out */
 	label2 = cnt;
 	EMIT3(0x83, 0xC0, 0x01);                  /* add eax, 1 */
-	EMIT3(0x89, 0x45, 0x04);                  /* mov dword ptr [rbp + 4], 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(...)] */