| Message ID | 20190530190800.7633-1-luke.r.nels@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] bpf, riscv: fix bugs in JIT for 32-bit ALU operations | expand |
On Thu, May 30, 2019 at 12:09 PM Luke Nelson <luke.r.nels@gmail.com> wrote: > > In BPF, 32-bit ALU operations should zero-extend their results into > the 64-bit registers. The current BPF JIT on RISC-V emits incorrect > instructions that perform either sign extension only (e.g., addw/subw) > or no extension on 32-bit add, sub, and, or, xor, lsh, rsh, arsh, > and neg. This behavior diverges from the interpreter and JITs for > other architectures. > > This patch fixes the bugs by performing zero extension on the destination > register of 32-bit ALU operations. > > Fixes: 2353ecc6f91f ("bpf, riscv: add BPF JIT for RV64G") > Cc: Xi Wang <xi.wang@gmail.com> > Signed-off-by: Luke Nelson <luke.r.nels@gmail.com> This is a little messy. How about we introduce some helper function like: /* please find a better name... */ emit_32_or_64(bool is64, const u32 insn_32, const u32 inst_64, struct rv_jit_context *ctx) { if (is64) emit(insn_64, ctx); else { emit(insn_32, ctx); rd = xxxx; emit_zext_32(rd, ctx); } } Thanks, Song > --- > arch/riscv/net/bpf_jit_comp.c | 24 ++++++++++++++++++++++++ > 1 file changed, 24 insertions(+) > > diff --git a/arch/riscv/net/bpf_jit_comp.c b/arch/riscv/net/bpf_jit_comp.c > index 80b12aa5e10d..426d5c33ea90 100644 > --- a/arch/riscv/net/bpf_jit_comp.c > +++ b/arch/riscv/net/bpf_jit_comp.c > @@ -751,22 +751,32 @@ static int emit_insn(const struct bpf_insn *insn, struct rv_jit_context *ctx, > case BPF_ALU | BPF_ADD | BPF_X: > case BPF_ALU64 | BPF_ADD | BPF_X: > emit(is64 ? rv_add(rd, rd, rs) : rv_addw(rd, rd, rs), ctx); > + if (!is64) > + emit_zext_32(rd, ctx); > break; > case BPF_ALU | BPF_SUB | BPF_X: > case BPF_ALU64 | BPF_SUB | BPF_X: > emit(is64 ? rv_sub(rd, rd, rs) : rv_subw(rd, rd, rs), ctx); > + if (!is64) > + emit_zext_32(rd, ctx); > break; > case BPF_ALU | BPF_AND | BPF_X: > case BPF_ALU64 | BPF_AND | BPF_X: > emit(rv_and(rd, rd, rs), ctx); > + if (!is64) > + emit_zext_32(rd, ctx); > break; > case BPF_ALU | BPF_OR | BPF_X: > case BPF_ALU64 | BPF_OR | BPF_X: > emit(rv_or(rd, rd, rs), ctx); > + if (!is64) > + emit_zext_32(rd, ctx); > break; > case BPF_ALU | BPF_XOR | BPF_X: > case BPF_ALU64 | BPF_XOR | BPF_X: > emit(rv_xor(rd, rd, rs), ctx); > + if (!is64) > + emit_zext_32(rd, ctx); > break; > case BPF_ALU | BPF_MUL | BPF_X: > case BPF_ALU64 | BPF_MUL | BPF_X: > @@ -789,14 +799,20 @@ static int emit_insn(const struct bpf_insn *insn, struct rv_jit_context *ctx, > case BPF_ALU | BPF_LSH | BPF_X: > case BPF_ALU64 | BPF_LSH | BPF_X: > emit(is64 ? rv_sll(rd, rd, rs) : rv_sllw(rd, rd, rs), ctx); > + if (!is64) > + emit_zext_32(rd, ctx); > break; > case BPF_ALU | BPF_RSH | BPF_X: > case BPF_ALU64 | BPF_RSH | BPF_X: > emit(is64 ? rv_srl(rd, rd, rs) : rv_srlw(rd, rd, rs), ctx); > + if (!is64) > + emit_zext_32(rd, ctx); > break; > case BPF_ALU | BPF_ARSH | BPF_X: > case BPF_ALU64 | BPF_ARSH | BPF_X: > emit(is64 ? rv_sra(rd, rd, rs) : rv_sraw(rd, rd, rs), ctx); > + if (!is64) > + emit_zext_32(rd, ctx); > break; > > /* dst = -dst */ > @@ -804,6 +820,8 @@ static int emit_insn(const struct bpf_insn *insn, struct rv_jit_context *ctx, > case BPF_ALU64 | BPF_NEG: > emit(is64 ? rv_sub(rd, RV_REG_ZERO, rd) : > rv_subw(rd, RV_REG_ZERO, rd), ctx); > + if (!is64) > + emit_zext_32(rd, ctx); > break; > > /* dst = BSWAP##imm(dst) */ > @@ -958,14 +976,20 @@ static int emit_insn(const struct bpf_insn *insn, struct rv_jit_context *ctx, > case BPF_ALU | BPF_LSH | BPF_K: > case BPF_ALU64 | BPF_LSH | BPF_K: > emit(is64 ? rv_slli(rd, rd, imm) : rv_slliw(rd, rd, imm), ctx); > + if (!is64) > + emit_zext_32(rd, ctx); > break; > case BPF_ALU | BPF_RSH | BPF_K: > case BPF_ALU64 | BPF_RSH | BPF_K: > emit(is64 ? rv_srli(rd, rd, imm) : rv_srliw(rd, rd, imm), ctx); > + if (!is64) > + emit_zext_32(rd, ctx); > break; > case BPF_ALU | BPF_ARSH | BPF_K: > case BPF_ALU64 | BPF_ARSH | BPF_K: > emit(is64 ? rv_srai(rd, rd, imm) : rv_sraiw(rd, rd, imm), ctx); > + if (!is64) > + emit_zext_32(rd, ctx); > break; > > /* JUMP off */ > -- > 2.19.1 >
On Thu, May 30, 2019 at 1:53 PM Song Liu <liu.song.a23@gmail.com> wrote: > > This is a little messy. How about we introduce some helper function > like: > > /* please find a better name... */ > emit_32_or_64(bool is64, const u32 insn_32, const u32 inst_64, struct > rv_jit_context *ctx) > { > if (is64) > emit(insn_64, ctx); > else { > emit(insn_32, ctx); > rd = xxxx; > emit_zext_32(rd, ctx); > } > } This same check is used throughout the file, maybe clean it up in a separate patch?
On Thu, May 30, 2019 at 3:34 PM Luke Nelson <luke.r.nels@gmail.com> wrote: > > On Thu, May 30, 2019 at 1:53 PM Song Liu <liu.song.a23@gmail.com> wrote: > > > > This is a little messy. How about we introduce some helper function > > like: > > > > /* please find a better name... */ > > emit_32_or_64(bool is64, const u32 insn_32, const u32 inst_64, struct > > rv_jit_context *ctx) > > { > > if (is64) > > emit(insn_64, ctx); > > else { > > emit(insn_32, ctx); > > rd = xxxx; > > emit_zext_32(rd, ctx); > > } > > } > > This same check is used throughout the file, maybe clean it up in a > separate patch? Yes, let's do follow up patch. Thanks, Song
Song Liu writes: > On Thu, May 30, 2019 at 3:34 PM Luke Nelson <luke.r.nels@gmail.com> wrote: >> >> On Thu, May 30, 2019 at 1:53 PM Song Liu <liu.song.a23@gmail.com> wrote: >> > >> > This is a little messy. How about we introduce some helper function >> > like: >> > >> > /* please find a better name... */ >> > emit_32_or_64(bool is64, const u32 insn_32, const u32 inst_64, struct >> > rv_jit_context *ctx) >> > { >> > if (is64) >> > emit(insn_64, ctx); >> > else { >> > emit(insn_32, ctx); >> > rd = xxxx; >> > emit_zext_32(rd, ctx); >> > } >> > } >> >> This same check is used throughout the file, maybe clean it up in a >> separate patch? We also need to enable the recent 32-bit opt (on bpf-next) on these missing insns, like what has been done at: https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git/commit/?id=66d0d5a854a6625974e7de4b874e7934988b0ef8 Perhaps the best way is to wait this patch merged back to bpf-next, then we do two patches, the first one to enable the opt, the second one then do the re-factor. I guess this could avoid some code conflict. Regards, Jiong > > Yes, let's do follow up patch. > > Thanks, > Song
diff --git a/arch/riscv/net/bpf_jit_comp.c b/arch/riscv/net/bpf_jit_comp.c index 80b12aa5e10d..426d5c33ea90 100644 --- a/arch/riscv/net/bpf_jit_comp.c +++ b/arch/riscv/net/bpf_jit_comp.c @@ -751,22 +751,32 @@ static int emit_insn(const struct bpf_insn *insn, struct rv_jit_context *ctx, case BPF_ALU | BPF_ADD | BPF_X: case BPF_ALU64 | BPF_ADD | BPF_X: emit(is64 ? rv_add(rd, rd, rs) : rv_addw(rd, rd, rs), ctx); + if (!is64) + emit_zext_32(rd, ctx); break; case BPF_ALU | BPF_SUB | BPF_X: case BPF_ALU64 | BPF_SUB | BPF_X: emit(is64 ? rv_sub(rd, rd, rs) : rv_subw(rd, rd, rs), ctx); + if (!is64) + emit_zext_32(rd, ctx); break; case BPF_ALU | BPF_AND | BPF_X: case BPF_ALU64 | BPF_AND | BPF_X: emit(rv_and(rd, rd, rs), ctx); + if (!is64) + emit_zext_32(rd, ctx); break; case BPF_ALU | BPF_OR | BPF_X: case BPF_ALU64 | BPF_OR | BPF_X: emit(rv_or(rd, rd, rs), ctx); + if (!is64) + emit_zext_32(rd, ctx); break; case BPF_ALU | BPF_XOR | BPF_X: case BPF_ALU64 | BPF_XOR | BPF_X: emit(rv_xor(rd, rd, rs), ctx); + if (!is64) + emit_zext_32(rd, ctx); break; case BPF_ALU | BPF_MUL | BPF_X: case BPF_ALU64 | BPF_MUL | BPF_X: @@ -789,14 +799,20 @@ static int emit_insn(const struct bpf_insn *insn, struct rv_jit_context *ctx, case BPF_ALU | BPF_LSH | BPF_X: case BPF_ALU64 | BPF_LSH | BPF_X: emit(is64 ? rv_sll(rd, rd, rs) : rv_sllw(rd, rd, rs), ctx); + if (!is64) + emit_zext_32(rd, ctx); break; case BPF_ALU | BPF_RSH | BPF_X: case BPF_ALU64 | BPF_RSH | BPF_X: emit(is64 ? rv_srl(rd, rd, rs) : rv_srlw(rd, rd, rs), ctx); + if (!is64) + emit_zext_32(rd, ctx); break; case BPF_ALU | BPF_ARSH | BPF_X: case BPF_ALU64 | BPF_ARSH | BPF_X: emit(is64 ? rv_sra(rd, rd, rs) : rv_sraw(rd, rd, rs), ctx); + if (!is64) + emit_zext_32(rd, ctx); break; /* dst = -dst */ @@ -804,6 +820,8 @@ static int emit_insn(const struct bpf_insn *insn, struct rv_jit_context *ctx, case BPF_ALU64 | BPF_NEG: emit(is64 ? rv_sub(rd, RV_REG_ZERO, rd) : rv_subw(rd, RV_REG_ZERO, rd), ctx); + if (!is64) + emit_zext_32(rd, ctx); break; /* dst = BSWAP##imm(dst) */ @@ -958,14 +976,20 @@ static int emit_insn(const struct bpf_insn *insn, struct rv_jit_context *ctx, case BPF_ALU | BPF_LSH | BPF_K: case BPF_ALU64 | BPF_LSH | BPF_K: emit(is64 ? rv_slli(rd, rd, imm) : rv_slliw(rd, rd, imm), ctx); + if (!is64) + emit_zext_32(rd, ctx); break; case BPF_ALU | BPF_RSH | BPF_K: case BPF_ALU64 | BPF_RSH | BPF_K: emit(is64 ? rv_srli(rd, rd, imm) : rv_srliw(rd, rd, imm), ctx); + if (!is64) + emit_zext_32(rd, ctx); break; case BPF_ALU | BPF_ARSH | BPF_K: case BPF_ALU64 | BPF_ARSH | BPF_K: emit(is64 ? rv_srai(rd, rd, imm) : rv_sraiw(rd, rd, imm), ctx); + if (!is64) + emit_zext_32(rd, ctx); break; /* JUMP off */
In BPF, 32-bit ALU operations should zero-extend their results into the 64-bit registers. The current BPF JIT on RISC-V emits incorrect instructions that perform either sign extension only (e.g., addw/subw) or no extension on 32-bit add, sub, and, or, xor, lsh, rsh, arsh, and neg. This behavior diverges from the interpreter and JITs for other architectures. This patch fixes the bugs by performing zero extension on the destination register of 32-bit ALU operations. Fixes: 2353ecc6f91f ("bpf, riscv: add BPF JIT for RV64G") Cc: Xi Wang <xi.wang@gmail.com> Signed-off-by: Luke Nelson <luke.r.nels@gmail.com> --- arch/riscv/net/bpf_jit_comp.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)