Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754757AbaK0Jxl (ORCPT ); Thu, 27 Nov 2014 04:53:41 -0500 Received: from mx1.redhat.com ([209.132.183.28]:60175 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751105AbaK0Jxi (ORCPT ); Thu, 27 Nov 2014 04:53:38 -0500 Message-ID: <5476F471.80500@redhat.com> Date: Thu, 27 Nov 2014 10:52:49 +0100 From: Daniel Borkmann User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/17.0 Thunderbird/17.0 MIME-Version: 1.0 To: Alexei Starovoitov CC: "David S. Miller" , Zi Shen Lim , Eric Dumazet , "H. Peter Anvin" , Thomas Gleixner , Ingo Molnar , netdev@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH net] bpf: x86: fix epilogue generation for eBPF programs References: <1417064546-4129-1-git-send-email-ast@plumgrid.com> In-Reply-To: <1417064546-4129-1-git-send-email-ast@plumgrid.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 11/27/2014 06:02 AM, Alexei Starovoitov wrote: > classic BPF has a restriction that last insn is always BPF_RET. > eBPF doesn't have BPF_RET instruction and this restriction. > It has BPF_EXIT insn which can appear anywhere in the program > one or more times and it doesn't have to be last insn. > Fix eBPF JIT to emit epilogue when first BPF_EXIT is seen > and all other BPF_EXIT instructions will be emitted as jump. > > Signed-off-by: Alexei Starovoitov > --- > Note, this bug is applicable only to native eBPF programs > which first were introduced in 3.18, so no need to send it > to stable and therefore no 'Fixes' tag. Btw, even if it's not sent to -stable, a 'Fixes:' tag is useful information for backporting and regression tracking, preferably always mentioned where it can clearly be identified. > arm64 JIT has the same problem, but the fix is not as trivial, > so will be done as separate patch. > > Since 3.18 can only load eBPF programs and cannot execute them, > this patch can even be done in net-next only, but I think it's worth > to apply it to 3.18(net), so that JITed output for native eBPF > programs is correct when bpf syscall loads it with net.core.bpf_jit_enable=2 Yes, sounds good to me, the condition insn_cnt - 1 is still held with BPF to eBPF transformations. > arch/x86/net/bpf_jit_comp.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c > index 3f62734..7e90244 100644 > --- a/arch/x86/net/bpf_jit_comp.c > +++ b/arch/x86/net/bpf_jit_comp.c > @@ -178,7 +178,7 @@ static void jit_fill_hole(void *area, unsigned int size) > } > > struct jit_context { > - unsigned int cleanup_addr; /* epilogue code offset */ > + int cleanup_addr; /* epilogue code offset */ Why this type change here? This seems a bit out of context (I would have expected a mention of this in the commit message, otherwise). > bool seen_ld_abs; > }; > > @@ -192,6 +192,7 @@ static int do_jit(struct bpf_prog *bpf_prog, int *addrs, u8 *image, > struct bpf_insn *insn = bpf_prog->insnsi; > int insn_cnt = bpf_prog->len; > bool seen_ld_abs = ctx->seen_ld_abs | (oldproglen == 0); > + bool seen_exit = false; > u8 temp[BPF_MAX_INSN_SIZE + BPF_INSN_SAFETY]; > int i; > int proglen = 0; > @@ -854,10 +855,11 @@ common_load: > goto common_load; > > case BPF_JMP | BPF_EXIT: > - if (i != insn_cnt - 1) { > + if (seen_exit) { > jmp_offset = ctx->cleanup_addr - addrs[i]; > goto emit_jmp; > } > + seen_exit = true; > /* update cleanup_addr */ > ctx->cleanup_addr = proglen; > /* mov rbx, qword ptr [rbp-X] */ > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/