[PATCH bpf-next 0/2] bpf, riscv: Add support for indirect jumps

[PATCH bpf-next 0/2] bpf, riscv: Add support for indirect jumps

[Chen Pei]

The indirect jump instruction (BPF_JMP | BPF_JA | BPF_X, "gotox") and the
BPF_MAP_TYPE_INSN_ARRAY jump tables it consumes are core features: the
verifier accepts them on every architecture, and the x86-64, arm64 and
powerpc JITs implement them. On riscv64 a program using gotox passes
verification and then fails to load, because the JIT does not know the
opcode and CONFIG_BPF_JIT_ALWAYS_ON leaves no interpreter to fall back
onto.

This series adds the riscv64 support and turns the existing selftests on
for that architecture.

Patch 1 emits "jalr zero, rd, 0" for gotox and publishes the xlated to
jitted offsets through bpf_prog_update_insn_ptrs(), which is what fills
in the jump table addresses. Patch 2 widens the arch guard in
verifier_gotox.c to riscv64, the same way arm64 and powerpc were enabled.

Testing
=======

Environment: QEMU virt rv64, with CONFIG_BPF_JIT=y,
CONFIG_BPF_JIT_ALWAYS_ON=y, CONFIG_DEBUG_INFO_BTF=y; selftests
cross-built with clang 22.

- test_progs -t verifier_gotox: 27/27 subtests pass on bpf-next, 13 of
  them executed through BPF_PROG_TEST_RUN.
- test_progs-cpuv4 -t bpf_gotox: 14/14 subtests pass, none skipped.
  The cpuv4 flavor is needed here because bpf_gotox gates its subtests
  on __BPF_FEATURE_GOTOX, which clang only defines for -mcpu=v4.

Chen Pei (2):
  bpf, riscv: Add support for indirect jumps
  selftests/bpf: Enable gotox tests for riscv64

 arch/riscv/net/bpf_jit_comp64.c                  |  5 +++++
 arch/riscv/net/bpf_jit_core.c                    | 16 ++++++++++++++--
 .../testing/selftests/bpf/progs/verifier_gotox.c |  8 ++++++--
 3 files changed, 25 insertions(+), 4 deletions(-)

-- 
2.50.1

[PATCH bpf-next 1/2] bpf, riscv: Add support for indirect jumps

[Chen Pei]

Implement JIT support for the indirect jump instruction (BPF_JMP |
BPF_JA | BPF_X), a.k.a. gotox, which lets a BPF program jump through a
BPF_MAP_TYPE_INSN_ARRAY jump table.

Emit "jalr zero, rd, 0" and hand the xlated to jitted offsets to
bpf_prog_update_insn_ptrs(), which is what fills in the jump table
entries; without that call the load fails with -EFAULT in
bpf_insn_array_ready(). ctx->offset[] holds the offset of the insn
*following* insn i, as bpf_prog_fill_jited_linfo() expects, so it is
shifted by one and offset[0] comes from the prologue length. build_body()
now records both halves of a multi-insn record, so no slot keeps a
fabricated offset.

Only the RV64 JIT is covered; RV32 keeps failing to load as before.

Signed-off-by: Chen Pei <cp0613@linux.alibaba.com>
---
 arch/riscv/net/bpf_jit_comp64.c |  5 +++++
 arch/riscv/net/bpf_jit_core.c   | 16 ++++++++++++++--
 2 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/arch/riscv/net/bpf_jit_comp64.c b/arch/riscv/net/bpf_jit_comp64.c
index ed0a6f871dea..9de3749fb268 100644
--- a/arch/riscv/net/bpf_jit_comp64.c
+++ b/arch/riscv/net/bpf_jit_comp64.c
@@ -1691,6 +1691,11 @@ int bpf_jit_emit_insn(const struct bpf_insn *insn, struct rv_jit_context *ctx,
 			emit_zextw(rd, rd, ctx);
 		break;
 
+	/* JUMP reg */
+	case BPF_JMP | BPF_JA | BPF_X:
+		emit_jalr(RV_REG_ZERO, rd, 0, ctx);
+		break;
+
 	/* JUMP off */
 	case BPF_JMP | BPF_JA:
 	case BPF_JMP32 | BPF_JA:
diff --git a/arch/riscv/net/bpf_jit_core.c b/arch/riscv/net/bpf_jit_core.c
index 2fb0b4e198b9..15acf249fed4 100644
--- a/arch/riscv/net/bpf_jit_core.c
+++ b/arch/riscv/net/bpf_jit_core.c
@@ -26,10 +26,13 @@ static int build_body(struct rv_jit_context *ctx, bool extra_pass, int *offset)
 		int ret;
 
 		ret = bpf_jit_emit_insn(insn, ctx, extra_pass);
-		if (ret > 0)
-			i++; /* skip the next instruction */
 		if (offset)
 			offset[i] = ctx->ninsns;
+		if (ret > 0) {
+			i++; /* skip the next instruction */
+			if (offset)
+				offset[i] = ctx->ninsns;
+		}
 		if (ret < 0)
 			return ret;
 	}
@@ -176,6 +179,15 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_verifier_env *env, struct bpf_pr
 		for (i = 0; i < prog->len; i++)
 			ctx->offset[i] = ninsns_rvoff(ctx->offset[i]);
 		bpf_prog_fill_jited_linfo(prog, ctx->offset);
+
+		/*
+		 * bpf_prog_update_insn_ptrs() wants the start of each insn, so
+		 * shift the linfo array by one and get insn 0 from the prologue.
+		 */
+		for (i = prog->len - 1; i > 0; i--)
+			ctx->offset[i] = ctx->offset[i - 1];
+		ctx->offset[0] = ninsns_rvoff(ctx->prologue_len);
+		bpf_prog_update_insn_ptrs(prog, ctx->offset, jit_data->ro_image);
 out_offset:
 		kvfree(ctx->offset);
 		kfree(jit_data);
-- 
2.50.1

Re: [PATCH bpf-next 1/2] bpf, riscv: Add support for indirect jumps

[bot+bpf-ci]

[-- Attachment #1: Type: text/plain, Size: 1608 bytes --]

> diff --git a/arch/riscv/net/bpf_jit_comp64.c b/arch/riscv/net/bpf_jit_comp64.c
> index ed0a6f871dea..9de3749fb268 100644
> --- a/arch/riscv/net/bpf_jit_comp64.c
> +++ b/arch/riscv/net/bpf_jit_comp64.c

[ ... ]

> diff --git a/arch/riscv/net/bpf_jit_core.c b/arch/riscv/net/bpf_jit_core.c
> index 2fb0b4e198b9..15acf249fed4 100644
> --- a/arch/riscv/net/bpf_jit_core.c
> +++ b/arch/riscv/net/bpf_jit_core.c

[ ... ]

> @@ -176,6 +179,15 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_verifier_env *env, struct bpf_pr
>  		for (i = 0; i < prog->len; i++)
>  			ctx->offset[i] = ninsns_rvoff(ctx->offset[i]);
>  		bpf_prog_fill_jited_linfo(prog, ctx->offset);
> +
> +		/*
> +		 * bpf_prog_update_insn_ptrs() wants the start of each insn, so
> +		 * shift the linfo array by one and get insn 0 from the prologue.
> +		 */
> +		for (i = prog->len - 1; i > 0; i--)
> +			ctx->offset[i] = ctx->offset[i - 1];
> +		ctx->offset[0] = ninsns_rvoff(ctx->prologue_len);
> +		bpf_prog_update_insn_ptrs(prog, ctx->offset, jit_data->ro_image);
>  out_offset:
>  		kvfree(ctx->offset);
>  		kfree(jit_data);

This isn't a bug, but could the comment say "shift ctx->offset[] by one"
rather than "shift the linfo array by one"? The jited_linfo was already
filled in by the bpf_prog_fill_jited_linfo() call above, and it is the
JIT's own offset table that is being rewritten here.


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/35866214591

[PATCH bpf-next 2/2] selftests/bpf: Enable gotox tests for riscv64

[Chen Pei]

The riscv64 JIT now supports the gotox instruction and jump tables, so
run the tests in verifier_gotox.c on riscv64 too, mirroring what was done
for arm64 and powerpc.

The guard is 64-bit only because the RV32 JIT does not implement gotox.

Signed-off-by: Chen Pei <cp0613@linux.alibaba.com>
---
 tools/testing/selftests/bpf/progs/verifier_gotox.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/bpf/progs/verifier_gotox.c b/tools/testing/selftests/bpf/progs/verifier_gotox.c
index 5b18c9a27717..4a38f7da720a 100644
--- a/tools/testing/selftests/bpf/progs/verifier_gotox.c
+++ b/tools/testing/selftests/bpf/progs/verifier_gotox.c
@@ -6,7 +6,9 @@
 #include "bpf_misc.h"
 #include "../../../include/linux/filter.h"
 
-#if defined(__TARGET_ARCH_x86) || defined(__TARGET_ARCH_arm64) || defined(__TARGET_ARCH_powerpc)
+#if defined(__TARGET_ARCH_x86) || defined(__TARGET_ARCH_arm64) || \
+	defined(__TARGET_ARCH_powerpc) || \
+	(defined(__TARGET_ARCH_riscv) && __riscv_xlen == 64)
 
 #define DEFINE_SIMPLE_JUMP_TABLE_PROG(NAME, SRC_REG, OFF, IMM, OUTCOME)	\
 									\
@@ -409,6 +411,8 @@ __naked void spill_fill_ptr_to_insn(void)
 	: __clobber_all);
 }
 
-#endif /* __TARGET_ARCH_x86 || __TARGET_ARCH_arm64 || __TARGET_ARCH_powerpc*/
+#endif /* __TARGET_ARCH_x86 || __TARGET_ARCH_arm64 ||
+	* __TARGET_ARCH_powerpc || __TARGET_ARCH_riscv
+	*/
 
 char _license[] SEC("license") = "GPL";
-- 
2.50.1

Re: [PATCH bpf-next 0/2] bpf, riscv: Add support for indirect jumps

[Chen Pei]

Hi all,

Thanks for the reviews. Patch 1 is a plain wording fix and will go into
v2. For patch 2 I need your input on one thing before I respin: which
form of the #endif comment you would prefer.

Patch 1, on the comment above bpf_prog_update_insn_ptrs():

> This isn't a bug, but could the comment say "shift ctx->offset[] by one"
> rather than "shift the linfo array by one"? The jited_linfo was already
> filled in by the bpf_prog_fill_jited_linfo() call above, and it is the
> JIT's own offset table that is being rewritten here.

Agreed. Fixed in v2.

Patch 2, on the #endif marker in verifier_gotox.c:

> [Severity: Low]
> This isn't a bug, but does this newly introduced multi-line comment
> follow the BPF subsystem style guide? The subsystem guidelines
> explicitly require that multi-line comments have the opening '/*' on
> its own line, rather than beginning text on the same line as the
> opening marker.

I would rather collapse it to a single line, but there is more than one
way to spell it:

  a) 89 columns, clean under scripts/checkpatch.pl --strict:

#endif /* __TARGET_ARCH_x86 || __TARGET_ARCH_arm64 || __TARGET_ARCH_powerpc || riscv64 */

  b) 101 columns, keeps every macro name verbatim, but checkpatch then
     reports "WARNING: line length of 101 exceeds 100 columns":

#endif /* __TARGET_ARCH_x86 || __TARGET_ARCH_arm64 || __TARGET_ARCH_powerpc || __TARGET_ARCH_riscv */

  c) or a short marker that does not repeat the condition at all:

#endif /* gotox-capable arch */

I lean towards (a): tools/testing/selftests/bpf/progs/ otherwise only
uses single-line #endif markers, and "riscv64" matches the guard, which
is __TARGET_ARCH_riscv && __riscv_xlen == 64.

I will send v2 with the patch 1 fix and whichever form you pick, together
with any other feedback, so please let me know if you would like anything
else changed. If nobody has a preference, I will go with (a) in a few
days.

Thanks,
Pei

Re: [PATCH bpf-next 0/2] bpf, riscv: Add support for indirect jumps

[Björn Töpel]

Hey!

Chen Pei <cp0613@linux.alibaba.com> writes:

> Hi all,
>
> Thanks for the reviews. Patch 1 is a plain wording fix and will go into
> v2. For patch 2 I need your input on one thing before I respin: which
> form of the #endif comment you would prefer.
>
> Patch 1, on the comment above bpf_prog_update_insn_ptrs():
>
>> This isn't a bug, but could the comment say "shift ctx->offset[] by one"
>> rather than "shift the linfo array by one"? The jited_linfo was already
>> filled in by the bpf_prog_fill_jited_linfo() call above, and it is the
>> JIT's own offset table that is being rewritten here.
>
> Agreed. Fixed in v2.
>
> Patch 2, on the #endif marker in verifier_gotox.c:
>
>> [Severity: Low]
>> This isn't a bug, but does this newly introduced multi-line comment
>> follow the BPF subsystem style guide? The subsystem guidelines
>> explicitly require that multi-line comments have the opening '/*' on
>> its own line, rather than beginning text on the same line as the
>> opening marker.
>
> I would rather collapse it to a single line, but there is more than one
> way to spell it:
>
>   a) 89 columns, clean under scripts/checkpatch.pl --strict:
>
> #endif /* __TARGET_ARCH_x86 || __TARGET_ARCH_arm64 || __TARGET_ARCH_powerpc || riscv64 */
>
>   b) 101 columns, keeps every macro name verbatim, but checkpatch then
>      reports "WARNING: line length of 101 exceeds 100 columns":
>
> #endif /* __TARGET_ARCH_x86 || __TARGET_ARCH_arm64 || __TARGET_ARCH_powerpc || __TARGET_ARCH_riscv */
>
>   c) or a short marker that does not repeat the condition at all:
>
> #endif /* gotox-capable arch */
>
> I lean towards (a): tools/testing/selftests/bpf/progs/ otherwise only
> uses single-line #endif markers, and "riscv64" matches the guard, which
> is __TARGET_ARCH_riscv && __riscv_xlen == 64.
>
> I will send v2 with the patch 1 fix and whichever form you pick, together
> with any other feedback, so please let me know if you would like anything
> else changed. If nobody has a preference, I will go with (a) in a few
> days.

Maybe just (d):
  | #endif /* gotox: x86, arm64, powerpc, riscv64 */

but (c) or (a) is fine as well, IMO.

Feel free to add for the series for your v2:

Reviewed-by: Björn Töpel <bjorn@kernel.org>
Acked-by: Björn Töpel <bjorn@kernel.org>