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@xxxxxxxxxx>
> Signed-off-by: Josh Poimboeuf <jpoimboe@xxxxxxxxxx>
> ---
>  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.