| Message ID | 20230710074131.19596-1-bjorn@kernel.org (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | [bpf] riscv, bpf: Fix inconsistent JIT image generation | expand |
| Context | Check | Description |
|---|---|---|
| conchuod/cover_letter | success | Single patches do not need cover letters |
| conchuod/tree_selection | success | Guessed tree name to be fixes at HEAD 533925cb7604 |
| conchuod/fixes_present | success | Fixes tag present in non-next series |
| conchuod/maintainers_pattern | success | MAINTAINERS pattern errors before the patch: 4 and now 4 |
| conchuod/verify_signedoff | success | Signed-off-by tag matches author and committer |
| conchuod/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| conchuod/build_rv64_clang_allmodconfig | success | Errors and warnings before: 8 this patch: 8 |
| conchuod/module_param | success | Was 0 now: 0 |
| conchuod/build_rv64_gcc_allmodconfig | success | Errors and warnings before: 9 this patch: 9 |
| conchuod/build_rv32_defconfig | success | Build OK |
| conchuod/dtb_warn_rv64 | success | Errors and warnings before: 3 this patch: 3 |
| conchuod/header_inline | success | No static functions without inline keyword in header files |
| conchuod/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 66 lines checked |
| conchuod/build_rv64_nommu_k210_defconfig | success | Build OK |
| conchuod/verify_fixes | success | Fixes tag looks correct |
| conchuod/build_rv64_nommu_virt_defconfig | success | Build OK |
On Mon, 10 Jul 2023 00:41:31 PDT (-0700), bjorn@kernel.org wrote: > From: Björn Töpel <bjorn@rivosinc.com> > > In order to generate the prologue and epilogue, the BPF JIT needs to > know which registers that are clobbered. Therefore, the during > pre-final passes, the prologue is generated after the body of the > program body-prologue-epilogue. Then, in the final pass, a proper > prologue-body-epilogue JITted image is generated. > > This scheme has worked most of the time. However, for some large > programs with many jumps, e.g. the test_kmod.sh BPF selftest with > hardening enabled (blinding constants), this has shown to be > incorrect. For the final pass, when the proper prologue-body-epilogue > is generated, the image has not converged. This will lead to that the > final image will have incorrect jump offsets. The following is an > excerpt from an incorrect image: > > | ... > | 3b8: 00c50663 beq a0,a2,3c4 <.text+0x3c4> > | 3bc: 0020e317 auipc t1,0x20e > | 3c0: 49630067 jalr zero,1174(t1) # 20e852 <.text+0x20e852> > | ... > | 20e84c: 8796 c.mv a5,t0 > | 20e84e: 6422 c.ldsp s0,8(sp) # Epilogue start > | 20e850: 6141 c.addi16sp sp,16 > | 20e852: 853e c.mv a0,a5 # Incorrect jump target > | 20e854: 8082 c.jr ra > > The image has shrunk, and the epilogue offset is incorrect in the > final pass. > > Correct the problem by always generating proper prologue-body-epilogue > outputs, which means that the first pass will only generate the body > to track what registers that are touched. > > Fixes: 2353ecc6f91f ("bpf, riscv: add BPF JIT for RV64G") > Signed-off-by: Björn Töpel <bjorn@rivosinc.com> > --- > arch/riscv/net/bpf_jit.h | 6 +++--- > arch/riscv/net/bpf_jit_core.c | 19 +++++++++++++------ > 2 files changed, 16 insertions(+), 9 deletions(-) > > diff --git a/arch/riscv/net/bpf_jit.h b/arch/riscv/net/bpf_jit.h > index bf9802a63061..2717f5490428 100644 > --- a/arch/riscv/net/bpf_jit.h > +++ b/arch/riscv/net/bpf_jit.h > @@ -69,7 +69,7 @@ struct rv_jit_context { > struct bpf_prog *prog; > u16 *insns; /* RV insns */ > int ninsns; > - int body_len; > + int prologue_len; > int epilogue_offset; > int *offset; /* BPF to RV */ > int nexentries; > @@ -216,8 +216,8 @@ static inline int rv_offset(int insn, int off, struct rv_jit_context *ctx) > int from, to; > > off++; /* BPF branch is from PC+1, RV is from PC */ > - from = (insn > 0) ? ctx->offset[insn - 1] : 0; > - to = (insn + off > 0) ? ctx->offset[insn + off - 1] : 0; > + from = (insn > 0) ? ctx->offset[insn - 1] : ctx->prologue_len; > + to = (insn + off > 0) ? ctx->offset[insn + off - 1] : ctx->prologue_len; > return ninsns_rvoff(to - from); > } > > diff --git a/arch/riscv/net/bpf_jit_core.c b/arch/riscv/net/bpf_jit_core.c > index 737baf8715da..7a26a3e1c73c 100644 > --- a/arch/riscv/net/bpf_jit_core.c > +++ b/arch/riscv/net/bpf_jit_core.c > @@ -44,7 +44,7 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog) > unsigned int prog_size = 0, extable_size = 0; > bool tmp_blinded = false, extra_pass = false; > struct bpf_prog *tmp, *orig_prog = prog; > - int pass = 0, prev_ninsns = 0, prologue_len, i; > + int pass = 0, prev_ninsns = 0, i; > struct rv_jit_data *jit_data; > struct rv_jit_context *ctx; > > @@ -83,6 +83,12 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog) > prog = orig_prog; > goto out_offset; > } > + > + if (build_body(ctx, extra_pass, NULL)) { > + prog = orig_prog; > + goto out_offset; > + } > + > for (i = 0; i < prog->len; i++) { > prev_ninsns += 32; > ctx->offset[i] = prev_ninsns; > @@ -91,12 +97,15 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog) > for (i = 0; i < NR_JIT_ITERATIONS; i++) { > pass++; > ctx->ninsns = 0; > + > + bpf_jit_build_prologue(ctx); > + ctx->prologue_len = ctx->ninsns; > + > if (build_body(ctx, extra_pass, ctx->offset)) { > prog = orig_prog; > goto out_offset; > } > - ctx->body_len = ctx->ninsns; > - bpf_jit_build_prologue(ctx); > + > ctx->epilogue_offset = ctx->ninsns; > bpf_jit_build_epilogue(ctx); > > @@ -162,10 +171,8 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog) > > if (!prog->is_func || extra_pass) { > bpf_jit_binary_lock_ro(jit_data->header); > - prologue_len = ctx->epilogue_offset - ctx->body_len; > for (i = 0; i < prog->len; i++) > - ctx->offset[i] = ninsns_rvoff(prologue_len + > - ctx->offset[i]); > + ctx->offset[i] = ninsns_rvoff(ctx->offset[i]); > bpf_prog_fill_jited_linfo(prog, ctx->offset); > out_offset: > kfree(ctx->offset); > > base-commit: 496720b7cfb6574a8f6f4d434f23e3d1e6cfaeb9 Acked-by: Palmer Dabbelt <palmer@rivosinc.com> Reviewed-by: Palmer Dabbelt <palmer@rivosinc.com> I'm assuming this is aimed at the BPF tree, but LMK if you guys want me to pick it up -- I've already got something for this week, so it's easy on my end. I'm dropping it from my queue and patchwork for now, though. Thanks for the fix!
On 7/11/23 7:50 PM, Palmer Dabbelt wrote: > On Mon, 10 Jul 2023 00:41:31 PDT (-0700), bjorn@kernel.org wrote: >> From: Björn Töpel <bjorn@rivosinc.com> >> >> In order to generate the prologue and epilogue, the BPF JIT needs to >> know which registers that are clobbered. Therefore, the during >> pre-final passes, the prologue is generated after the body of the >> program body-prologue-epilogue. Then, in the final pass, a proper >> prologue-body-epilogue JITted image is generated. >> >> This scheme has worked most of the time. However, for some large >> programs with many jumps, e.g. the test_kmod.sh BPF selftest with >> hardening enabled (blinding constants), this has shown to be >> incorrect. For the final pass, when the proper prologue-body-epilogue >> is generated, the image has not converged. This will lead to that the >> final image will have incorrect jump offsets. The following is an >> excerpt from an incorrect image: >> >> | ... >> | 3b8: 00c50663 beq a0,a2,3c4 <.text+0x3c4> >> | 3bc: 0020e317 auipc t1,0x20e >> | 3c0: 49630067 jalr zero,1174(t1) # 20e852 <.text+0x20e852> >> | ... >> | 20e84c: 8796 c.mv a5,t0 >> | 20e84e: 6422 c.ldsp s0,8(sp) # Epilogue start >> | 20e850: 6141 c.addi16sp sp,16 >> | 20e852: 853e c.mv a0,a5 # Incorrect jump target >> | 20e854: 8082 c.jr ra >> >> The image has shrunk, and the epilogue offset is incorrect in the >> final pass. >> >> Correct the problem by always generating proper prologue-body-epilogue >> outputs, which means that the first pass will only generate the body >> to track what registers that are touched. >> >> Fixes: 2353ecc6f91f ("bpf, riscv: add BPF JIT for RV64G") >> Signed-off-by: Björn Töpel <bjorn@rivosinc.com> >> --- >> arch/riscv/net/bpf_jit.h | 6 +++--- >> arch/riscv/net/bpf_jit_core.c | 19 +++++++++++++------ >> 2 files changed, 16 insertions(+), 9 deletions(-) >> >> diff --git a/arch/riscv/net/bpf_jit.h b/arch/riscv/net/bpf_jit.h >> index bf9802a63061..2717f5490428 100644 >> --- a/arch/riscv/net/bpf_jit.h >> +++ b/arch/riscv/net/bpf_jit.h >> @@ -69,7 +69,7 @@ struct rv_jit_context { >> struct bpf_prog *prog; >> u16 *insns; /* RV insns */ >> int ninsns; >> - int body_len; >> + int prologue_len; >> int epilogue_offset; >> int *offset; /* BPF to RV */ >> int nexentries; >> @@ -216,8 +216,8 @@ static inline int rv_offset(int insn, int off, struct rv_jit_context *ctx) >> int from, to; >> >> off++; /* BPF branch is from PC+1, RV is from PC */ >> - from = (insn > 0) ? ctx->offset[insn - 1] : 0; >> - to = (insn + off > 0) ? ctx->offset[insn + off - 1] : 0; >> + from = (insn > 0) ? ctx->offset[insn - 1] : ctx->prologue_len; >> + to = (insn + off > 0) ? ctx->offset[insn + off - 1] : ctx->prologue_len; >> return ninsns_rvoff(to - from); >> } >> >> diff --git a/arch/riscv/net/bpf_jit_core.c b/arch/riscv/net/bpf_jit_core.c >> index 737baf8715da..7a26a3e1c73c 100644 >> --- a/arch/riscv/net/bpf_jit_core.c >> +++ b/arch/riscv/net/bpf_jit_core.c >> @@ -44,7 +44,7 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog) >> unsigned int prog_size = 0, extable_size = 0; >> bool tmp_blinded = false, extra_pass = false; >> struct bpf_prog *tmp, *orig_prog = prog; >> - int pass = 0, prev_ninsns = 0, prologue_len, i; >> + int pass = 0, prev_ninsns = 0, i; >> struct rv_jit_data *jit_data; >> struct rv_jit_context *ctx; >> >> @@ -83,6 +83,12 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog) >> prog = orig_prog; >> goto out_offset; >> } >> + >> + if (build_body(ctx, extra_pass, NULL)) { >> + prog = orig_prog; >> + goto out_offset; >> + } >> + >> for (i = 0; i < prog->len; i++) { >> prev_ninsns += 32; >> ctx->offset[i] = prev_ninsns; >> @@ -91,12 +97,15 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog) >> for (i = 0; i < NR_JIT_ITERATIONS; i++) { >> pass++; >> ctx->ninsns = 0; >> + >> + bpf_jit_build_prologue(ctx); >> + ctx->prologue_len = ctx->ninsns; >> + >> if (build_body(ctx, extra_pass, ctx->offset)) { >> prog = orig_prog; >> goto out_offset; >> } >> - ctx->body_len = ctx->ninsns; >> - bpf_jit_build_prologue(ctx); >> + >> ctx->epilogue_offset = ctx->ninsns; >> bpf_jit_build_epilogue(ctx); >> >> @@ -162,10 +171,8 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog) >> >> if (!prog->is_func || extra_pass) { >> bpf_jit_binary_lock_ro(jit_data->header); >> - prologue_len = ctx->epilogue_offset - ctx->body_len; >> for (i = 0; i < prog->len; i++) >> - ctx->offset[i] = ninsns_rvoff(prologue_len + >> - ctx->offset[i]); >> + ctx->offset[i] = ninsns_rvoff(ctx->offset[i]); >> bpf_prog_fill_jited_linfo(prog, ctx->offset); >> out_offset: >> kfree(ctx->offset); >> >> base-commit: 496720b7cfb6574a8f6f4d434f23e3d1e6cfaeb9 > > Acked-by: Palmer Dabbelt <palmer@rivosinc.com> > Reviewed-by: Palmer Dabbelt <palmer@rivosinc.com> > > I'm assuming this is aimed at the BPF tree, but LMK if you guys want me > to pick it up -- I've already got something for this week, so it's easy > on my end. I'm dropping it from my queue and patchwork for now, though. Sounds good, we applied it to bpf already. Thanks, Daniel
On Tue, 11 Jul 2023 13:03:47 PDT (-0700), daniel@iogearbox.net wrote: > On 7/11/23 7:50 PM, Palmer Dabbelt wrote: >> On Mon, 10 Jul 2023 00:41:31 PDT (-0700), bjorn@kernel.org wrote: >>> From: Björn Töpel <bjorn@rivosinc.com> >>> >>> In order to generate the prologue and epilogue, the BPF JIT needs to >>> know which registers that are clobbered. Therefore, the during >>> pre-final passes, the prologue is generated after the body of the >>> program body-prologue-epilogue. Then, in the final pass, a proper >>> prologue-body-epilogue JITted image is generated. >>> >>> This scheme has worked most of the time. However, for some large >>> programs with many jumps, e.g. the test_kmod.sh BPF selftest with >>> hardening enabled (blinding constants), this has shown to be >>> incorrect. For the final pass, when the proper prologue-body-epilogue >>> is generated, the image has not converged. This will lead to that the >>> final image will have incorrect jump offsets. The following is an >>> excerpt from an incorrect image: >>> >>> | ... >>> | 3b8: 00c50663 beq a0,a2,3c4 <.text+0x3c4> >>> | 3bc: 0020e317 auipc t1,0x20e >>> | 3c0: 49630067 jalr zero,1174(t1) # 20e852 <.text+0x20e852> >>> | ... >>> | 20e84c: 8796 c.mv a5,t0 >>> | 20e84e: 6422 c.ldsp s0,8(sp) # Epilogue start >>> | 20e850: 6141 c.addi16sp sp,16 >>> | 20e852: 853e c.mv a0,a5 # Incorrect jump target >>> | 20e854: 8082 c.jr ra >>> >>> The image has shrunk, and the epilogue offset is incorrect in the >>> final pass. >>> >>> Correct the problem by always generating proper prologue-body-epilogue >>> outputs, which means that the first pass will only generate the body >>> to track what registers that are touched. >>> >>> Fixes: 2353ecc6f91f ("bpf, riscv: add BPF JIT for RV64G") >>> Signed-off-by: Björn Töpel <bjorn@rivosinc.com> >>> --- >>> arch/riscv/net/bpf_jit.h | 6 +++--- >>> arch/riscv/net/bpf_jit_core.c | 19 +++++++++++++------ >>> 2 files changed, 16 insertions(+), 9 deletions(-) >>> >>> diff --git a/arch/riscv/net/bpf_jit.h b/arch/riscv/net/bpf_jit.h >>> index bf9802a63061..2717f5490428 100644 >>> --- a/arch/riscv/net/bpf_jit.h >>> +++ b/arch/riscv/net/bpf_jit.h >>> @@ -69,7 +69,7 @@ struct rv_jit_context { >>> struct bpf_prog *prog; >>> u16 *insns; /* RV insns */ >>> int ninsns; >>> - int body_len; >>> + int prologue_len; >>> int epilogue_offset; >>> int *offset; /* BPF to RV */ >>> int nexentries; >>> @@ -216,8 +216,8 @@ static inline int rv_offset(int insn, int off, struct rv_jit_context *ctx) >>> int from, to; >>> >>> off++; /* BPF branch is from PC+1, RV is from PC */ >>> - from = (insn > 0) ? ctx->offset[insn - 1] : 0; >>> - to = (insn + off > 0) ? ctx->offset[insn + off - 1] : 0; >>> + from = (insn > 0) ? ctx->offset[insn - 1] : ctx->prologue_len; >>> + to = (insn + off > 0) ? ctx->offset[insn + off - 1] : ctx->prologue_len; >>> return ninsns_rvoff(to - from); >>> } >>> >>> diff --git a/arch/riscv/net/bpf_jit_core.c b/arch/riscv/net/bpf_jit_core.c >>> index 737baf8715da..7a26a3e1c73c 100644 >>> --- a/arch/riscv/net/bpf_jit_core.c >>> +++ b/arch/riscv/net/bpf_jit_core.c >>> @@ -44,7 +44,7 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog) >>> unsigned int prog_size = 0, extable_size = 0; >>> bool tmp_blinded = false, extra_pass = false; >>> struct bpf_prog *tmp, *orig_prog = prog; >>> - int pass = 0, prev_ninsns = 0, prologue_len, i; >>> + int pass = 0, prev_ninsns = 0, i; >>> struct rv_jit_data *jit_data; >>> struct rv_jit_context *ctx; >>> >>> @@ -83,6 +83,12 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog) >>> prog = orig_prog; >>> goto out_offset; >>> } >>> + >>> + if (build_body(ctx, extra_pass, NULL)) { >>> + prog = orig_prog; >>> + goto out_offset; >>> + } >>> + >>> for (i = 0; i < prog->len; i++) { >>> prev_ninsns += 32; >>> ctx->offset[i] = prev_ninsns; >>> @@ -91,12 +97,15 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog) >>> for (i = 0; i < NR_JIT_ITERATIONS; i++) { >>> pass++; >>> ctx->ninsns = 0; >>> + >>> + bpf_jit_build_prologue(ctx); >>> + ctx->prologue_len = ctx->ninsns; >>> + >>> if (build_body(ctx, extra_pass, ctx->offset)) { >>> prog = orig_prog; >>> goto out_offset; >>> } >>> - ctx->body_len = ctx->ninsns; >>> - bpf_jit_build_prologue(ctx); >>> + >>> ctx->epilogue_offset = ctx->ninsns; >>> bpf_jit_build_epilogue(ctx); >>> >>> @@ -162,10 +171,8 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog) >>> >>> if (!prog->is_func || extra_pass) { >>> bpf_jit_binary_lock_ro(jit_data->header); >>> - prologue_len = ctx->epilogue_offset - ctx->body_len; >>> for (i = 0; i < prog->len; i++) >>> - ctx->offset[i] = ninsns_rvoff(prologue_len + >>> - ctx->offset[i]); >>> + ctx->offset[i] = ninsns_rvoff(ctx->offset[i]); >>> bpf_prog_fill_jited_linfo(prog, ctx->offset); >>> out_offset: >>> kfree(ctx->offset); >>> >>> base-commit: 496720b7cfb6574a8f6f4d434f23e3d1e6cfaeb9 >> >> Acked-by: Palmer Dabbelt <palmer@rivosinc.com> >> Reviewed-by: Palmer Dabbelt <palmer@rivosinc.com> >> >> I'm assuming this is aimed at the BPF tree, but LMK if you guys want me >> to pick it up -- I've already got something for this week, so it's easy >> on my end. I'm dropping it from my queue and patchwork for now, though. > > Sounds good, we applied it to bpf already. Thanks! > > Thanks, > Daniel
diff --git a/arch/riscv/net/bpf_jit.h b/arch/riscv/net/bpf_jit.h index bf9802a63061..2717f5490428 100644 --- a/arch/riscv/net/bpf_jit.h +++ b/arch/riscv/net/bpf_jit.h @@ -69,7 +69,7 @@ struct rv_jit_context { struct bpf_prog *prog; u16 *insns; /* RV insns */ int ninsns; - int body_len; + int prologue_len; int epilogue_offset; int *offset; /* BPF to RV */ int nexentries; @@ -216,8 +216,8 @@ static inline int rv_offset(int insn, int off, struct rv_jit_context *ctx) int from, to; off++; /* BPF branch is from PC+1, RV is from PC */ - from = (insn > 0) ? ctx->offset[insn - 1] : 0; - to = (insn + off > 0) ? ctx->offset[insn + off - 1] : 0; + from = (insn > 0) ? ctx->offset[insn - 1] : ctx->prologue_len; + to = (insn + off > 0) ? ctx->offset[insn + off - 1] : ctx->prologue_len; return ninsns_rvoff(to - from); } diff --git a/arch/riscv/net/bpf_jit_core.c b/arch/riscv/net/bpf_jit_core.c index 737baf8715da..7a26a3e1c73c 100644 --- a/arch/riscv/net/bpf_jit_core.c +++ b/arch/riscv/net/bpf_jit_core.c @@ -44,7 +44,7 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog) unsigned int prog_size = 0, extable_size = 0; bool tmp_blinded = false, extra_pass = false; struct bpf_prog *tmp, *orig_prog = prog; - int pass = 0, prev_ninsns = 0, prologue_len, i; + int pass = 0, prev_ninsns = 0, i; struct rv_jit_data *jit_data; struct rv_jit_context *ctx; @@ -83,6 +83,12 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog) prog = orig_prog; goto out_offset; } + + if (build_body(ctx, extra_pass, NULL)) { + prog = orig_prog; + goto out_offset; + } + for (i = 0; i < prog->len; i++) { prev_ninsns += 32; ctx->offset[i] = prev_ninsns; @@ -91,12 +97,15 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog) for (i = 0; i < NR_JIT_ITERATIONS; i++) { pass++; ctx->ninsns = 0; + + bpf_jit_build_prologue(ctx); + ctx->prologue_len = ctx->ninsns; + if (build_body(ctx, extra_pass, ctx->offset)) { prog = orig_prog; goto out_offset; } - ctx->body_len = ctx->ninsns; - bpf_jit_build_prologue(ctx); + ctx->epilogue_offset = ctx->ninsns; bpf_jit_build_epilogue(ctx); @@ -162,10 +171,8 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog) if (!prog->is_func || extra_pass) { bpf_jit_binary_lock_ro(jit_data->header); - prologue_len = ctx->epilogue_offset - ctx->body_len; for (i = 0; i < prog->len; i++) - ctx->offset[i] = ninsns_rvoff(prologue_len + - ctx->offset[i]); + ctx->offset[i] = ninsns_rvoff(ctx->offset[i]); bpf_prog_fill_jited_linfo(prog, ctx->offset); out_offset: kfree(ctx->offset);