From: Björn Töpel <[email protected]>
The RISC-V architecture does not expose sub-registers, and hold all
32-bit values in a sign-extended format [1] [2]:
| The compiler and calling convention maintain an invariant that all
| 32-bit values are held in a sign-extended format in 64-bit
| registers. Even 32-bit unsigned integers extend bit 31 into bits
| 63 through 32. Consequently, conversion between unsigned and
| signed 32-bit integers is a no-op, as is conversion from a signed
| 32-bit integer to a signed 64-bit integer.
While BPF, on the other hand, exposes sub-registers, and use
zero-extension (similar to arm64/x86).
This has led to some subtle bugs, where a BPF JITted program has not
sign-extended the a0 register (return value in RISC-V land), passed
the return value up the kernel, e.g.:
| int from_bpf(void);
|
| long foo(void)
| {
| return from_bpf();
| }
This series fixes this issue by keeping a pair of return value
registers; a0 (RISC-V ABI, sign-extended), a5 (BPF, zero-extended).
The following test_progs now pass, which were previously broken:
| 13 bpf_cookie
| 19 bpf_mod_race
| 68 deny_namespace
| 119 libbpf_get_fd_by_id_opts
| 135 lookup_key
| 137 lsm_cgroup
| 284 test_lsm
Björn
Björn Töpel (2):
riscv, bpf: Sign-extend return values
riscv, bpf: Track both a0 (RISC-V ABI) and a5 (BPF) return values
arch/riscv/net/bpf_jit_comp64.c | 18 ++++++++++++------
1 file changed, 12 insertions(+), 6 deletions(-)
base-commit: 9077fc228f09c9f975c498c55f5d2e882cd0da59
--
2.39.2
From: Björn Töpel <[email protected]>
The RISC-V BPF uses a5 for BPF return values, which are zero-extended,
whereas the RISC-V ABI uses a0 which is sign-extended. In other words,
a5 and a0 can differ, and are used in different context.
The BPF trampoline are used for both BPF programs, and regular kernel
functions.
Make sure that the RISC-V BPF trampoline saves, and restores both a0
and a5.
Fixes: 49b5e77ae3e2 ("riscv, bpf: Add bpf trampoline support for RV64")
Signed-off-by: Björn Töpel <[email protected]>
---
arch/riscv/net/bpf_jit_comp64.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/arch/riscv/net/bpf_jit_comp64.c b/arch/riscv/net/bpf_jit_comp64.c
index de4c9957d223..8581693e62d3 100644
--- a/arch/riscv/net/bpf_jit_comp64.c
+++ b/arch/riscv/net/bpf_jit_comp64.c
@@ -759,8 +759,10 @@ static int invoke_bpf_prog(struct bpf_tramp_link *l, int args_off, int retval_of
if (ret)
return ret;
- if (save_ret)
- emit_sd(RV_REG_FP, -retval_off, regmap[BPF_REG_0], ctx);
+ if (save_ret) {
+ emit_sd(RV_REG_FP, -retval_off, RV_REG_A0, ctx);
+ emit_sd(RV_REG_FP, -(retval_off - 8), regmap[BPF_REG_0], ctx);
+ }
/* update branch with beqz */
if (ctx->insns) {
@@ -853,7 +855,7 @@ static int __arch_prepare_bpf_trampoline(struct bpf_tramp_image *im,
save_ret = flags & (BPF_TRAMP_F_CALL_ORIG | BPF_TRAMP_F_RET_FENTRY_RET);
if (save_ret) {
- stack_size += 8;
+ stack_size += 16; /* Save both A5 (BPF R0) and A0 */
retval_off = stack_size;
}
@@ -957,6 +959,7 @@ static int __arch_prepare_bpf_trampoline(struct bpf_tramp_image *im,
if (ret)
goto out;
emit_sd(RV_REG_FP, -retval_off, RV_REG_A0, ctx);
+ emit_sd(RV_REG_FP, -(retval_off - 8), regmap[BPF_REG_0], ctx);
im->ip_after_call = ctx->insns + ctx->ninsns;
/* 2 nops reserved for auipc+jalr pair */
emit(rv_nop(), ctx);
@@ -988,8 +991,10 @@ static int __arch_prepare_bpf_trampoline(struct bpf_tramp_image *im,
if (flags & BPF_TRAMP_F_RESTORE_REGS)
restore_args(nregs, args_off, ctx);
- if (save_ret)
+ if (save_ret) {
emit_ld(RV_REG_A0, -retval_off, RV_REG_FP, ctx);
+ emit_ld(regmap[BPF_REG_0], -(retval_off - 8), RV_REG_FP, ctx);
+ }
emit_ld(RV_REG_S1, -sreg_off, RV_REG_FP, ctx);
--
2.39.2
From: Björn Töpel <[email protected]>
The RISC-V architecture does not expose sub-registers, and hold all
32-bit values in a sign-extended format [1] [2]:
| The compiler and calling convention maintain an invariant that all
| 32-bit values are held in a sign-extended format in 64-bit
| registers. Even 32-bit unsigned integers extend bit 31 into bits
| 63 through 32. Consequently, conversion between unsigned and
| signed 32-bit integers is a no-op, as is conversion from a signed
| 32-bit integer to a signed 64-bit integer.
While BPF, on the other hand, exposes sub-registers, and use
zero-extension (similar to arm64/x86).
This has led to some subtle bugs, where a BPF JITted program has not
sign-extended the a0 register (return value in RISC-V land), passed
the return value up the kernel, e.g.:
| int from_bpf(void);
|
| long foo(void)
| {
| return from_bpf();
| }
Here, a0 would be 0xffff_ffff, instead of the expected
0xffff_ffff_ffff_ffff.
Internally, the RISC-V JIT uses a5 as a dedicated register for BPF
return values.
Keep a5 zero-extended, but explicitly sign-extend a0 (which is used
outside BPF land). Now that a0 (RISC-V ABI) and a5 (BPF ABI) differs,
a0 is only moved to a5 for non-BPF native calls (BPF_PSEUDO_CALL).
Link: https://github.com/riscv/riscv-isa-manual/releases/download/riscv-isa-release-056b6ff-2023-10-02/unpriv-isa-asciidoc.pdf # [2]
Link: https://github.com/riscv-non-isa/riscv-elf-psabi-doc/releases/download/draft-20230929-e5c800e661a53efe3c2678d71a306323b60eb13b/riscv-abi.pdf # [2]
Fixes: 2353ecc6f91f ("bpf, riscv: add BPF JIT for RV64G")
Signed-off-by: Björn Töpel <[email protected]>
---
arch/riscv/net/bpf_jit_comp64.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/arch/riscv/net/bpf_jit_comp64.c b/arch/riscv/net/bpf_jit_comp64.c
index ecd3ae6f4116..de4c9957d223 100644
--- a/arch/riscv/net/bpf_jit_comp64.c
+++ b/arch/riscv/net/bpf_jit_comp64.c
@@ -245,7 +245,7 @@ static void __build_epilogue(bool is_tail_call, struct rv_jit_context *ctx)
emit_addi(RV_REG_SP, RV_REG_SP, stack_adjust, ctx);
/* Set return value. */
if (!is_tail_call)
- emit_mv(RV_REG_A0, RV_REG_A5, ctx);
+ emit_addiw(RV_REG_A0, RV_REG_A5, 0, ctx);
emit_jalr(RV_REG_ZERO, is_tail_call ? RV_REG_T3 : RV_REG_RA,
is_tail_call ? (RV_FENTRY_NINSNS + 1) * 4 : 0, /* skip reserved nops and TCC init */
ctx);
@@ -1515,7 +1515,8 @@ int bpf_jit_emit_insn(const struct bpf_insn *insn, struct rv_jit_context *ctx,
if (ret)
return ret;
- emit_mv(bpf_to_rv_reg(BPF_REG_0, ctx), RV_REG_A0, ctx);
+ if (insn->src_reg != BPF_PSEUDO_CALL)
+ emit_mv(bpf_to_rv_reg(BPF_REG_0, ctx), RV_REG_A0, ctx);
break;
}
/* tail call */
--
2.39.2
On 10/4/23 2:07 PM, Björn Töpel wrote:
> From: Björn Töpel <[email protected]>
[...]
> The following test_progs now pass, which were previously broken:
>
> | 13 bpf_cookie
> | 19 bpf_mod_race
> | 68 deny_namespace
> | 119 libbpf_get_fd_by_id_opts
> | 135 lookup_key
> | 137 lsm_cgroup
> | 284 test_lsm
Thanks for the fixes, took them into bpf tree. I was wondering whether this could be
backed by specific tests, but looks like the above list already takes care of it.
Thanks,
Daniel
Hello:
This series was applied to bpf/bpf.git (master)
by Daniel Borkmann <[email protected]>:
On Wed, 4 Oct 2023 14:07:04 +0200 you wrote:
> From: Björn Töpel <[email protected]>
>
> The RISC-V architecture does not expose sub-registers, and hold all
> 32-bit values in a sign-extended format [1] [2]:
>
> | The compiler and calling convention maintain an invariant that all
> | 32-bit values are held in a sign-extended format in 64-bit
> | registers. Even 32-bit unsigned integers extend bit 31 into bits
> | 63 through 32. Consequently, conversion between unsigned and
> | signed 32-bit integers is a no-op, as is conversion from a signed
> | 32-bit integer to a signed 64-bit integer.
>
> [...]
Here is the summary with links:
- [bpf,1/2] riscv, bpf: Sign-extend return values
https://git.kernel.org/bpf/bpf/c/2f1b0d3d7331
- [bpf,2/2] riscv, bpf: Track both a0 (RISC-V ABI) and a5 (BPF) return values
https://git.kernel.org/bpf/bpf/c/7112cd26e606
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html