| Message ID | 1417301173-23691-1-git-send-email-ast@plumgrid.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
Hi Alexei, On Sat, Nov 29, 2014 at 2:46 PM, Alexei Starovoitov <ast@plumgrid.com> 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. Just to confirm, in valid eBPF, BPF_EXIT *must* be present at least once, correct? Does an eBPF JIT implementation need to check for it? > Fix eBPF JIT to emit epilogue when first BPF_EXIT is seen > and all other BPF_EXIT instructions will be emitted as jump. > > Since jump offset to epilogue is computed as: > jmp_offset = ctx->cleanup_addr - addrs[i] > we need to change type of cleanup_addr to signed to compute the offset as: > (long long) ((int)20 - (int)30) > instead of: > (long long) ((unsigned int)20 - (int)30) > > Fixes: 622582786c9e ("net: filter: x86: internal BPF JIT") > Signed-off-by: Alexei Starovoitov <ast@plumgrid.com> > --- > Note, this bug is applicable only to native eBPF programs > which first were introduced in 3.18, so no need for stable. > > arm64 JIT has the same problem, but the fix is not as trivial, > so will be done as separate patch. I'll cook up a patch for arm64 if you haven't already done so. Any related test case I should run through? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Dec 2, 2014 at 10:38 PM, Z Lim <zlim.lnx@gmail.com> wrote: > Hi Alexei, > > On Sat, Nov 29, 2014 at 2:46 PM, Alexei Starovoitov <ast@plumgrid.com> 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. > > Just to confirm, in valid eBPF, BPF_EXIT *must* be present at least > once, correct? > Does an eBPF JIT implementation need to check for it? yes. of course. At least one bpf_exit is always there and there are no loops. verifier is checking for it. So no need for jit to check it again. > I'll cook up a patch for arm64 if you haven't already done so. > Any related test case I should run through? Pending socket samples are generating such code by llvm. I was planning to add an explicit test to test_bpf, but feel free to beat me to it. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Alexei Starovoitov <ast@plumgrid.com> Date: Sat, 29 Nov 2014 14:46:13 -0800 > 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. > > Since jump offset to epilogue is computed as: > jmp_offset = ctx->cleanup_addr - addrs[i] > we need to change type of cleanup_addr to signed to compute the offset as: > (long long) ((int)20 - (int)30) > instead of: > (long long) ((unsigned int)20 - (int)30) > > Fixes: 622582786c9e ("net: filter: x86: internal BPF JIT") > Signed-off-by: Alexei Starovoitov <ast@plumgrid.com> Applied, thanks. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 */ 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] */
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. Since jump offset to epilogue is computed as: jmp_offset = ctx->cleanup_addr - addrs[i] we need to change type of cleanup_addr to signed to compute the offset as: (long long) ((int)20 - (int)30) instead of: (long long) ((unsigned int)20 - (int)30) Fixes: 622582786c9e ("net: filter: x86: internal BPF JIT") Signed-off-by: Alexei Starovoitov <ast@plumgrid.com> --- Note, this bug is applicable only to native eBPF programs which first were introduced in 3.18, so no need for stable. 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 arch/x86/net/bpf_jit_comp.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)