[PATCH bpf-next] bpf, riscv: Register extable entries at probe insn call sites

[PATCH bpf-next] bpf, riscv: Register extable entries at probe insn call sites

[Chen Pei]

add_exception_handler() filters insns by matching BPF_MODE() against
the probe mode values. These values occupy unused opcode slots and
collide with legitimate encodings: BPF_PROBE_MEM32SX (0xc0) shares
its mode value with BPF_ATOMIC. Relying on the mode alone therefore
risks registering exception table entries for the wrong instructions,
as would happen for plain atomics without the LDX class check.

Make the decision explicit at the call sites instead: register an
entry only when emitting a probe load, a PROBE_MEM32 store, or a
PROBE_ATOMIC atomic, and drop the fragile mode gate from
add_exception_handler() along with the now unused insn argument. No
functional change intended.

Signed-off-by: Chen Pei <cp0613@linux.alibaba.com>
---
base-commit: d761934c9483ecde93fe99d8705282f716dfee50

 arch/riscv/net/bpf_jit_comp64.c | 50 ++++++++++++++++++---------------
 1 file changed, 28 insertions(+), 22 deletions(-)

diff --git a/arch/riscv/net/bpf_jit_comp64.c b/arch/riscv/net/bpf_jit_comp64.c
index e7378be171a9..726c0169fc60 100644
--- a/arch/riscv/net/bpf_jit_comp64.c
+++ b/arch/riscv/net/bpf_jit_comp64.c
@@ -731,9 +731,8 @@ bool ex_handler_bpf(const struct exception_table_entry *ex,
 	return true;
 }
 
-/* For accesses to BTF pointers, add an entry to the exception table */
-static int add_exception_handler(const struct bpf_insn *insn, int dst_reg,
-				 struct rv_jit_context *ctx)
+/* Add an entry to the exception table for a faulting probe insn */
+static int add_exception_handler(int dst_reg, struct rv_jit_context *ctx)
 {
 	struct exception_table_entry *ex;
 	unsigned long pc;
@@ -744,14 +743,6 @@ static int add_exception_handler(const struct bpf_insn *insn, int dst_reg,
 	    ctx->ex_insn_off <= 0 || ctx->ex_jmp_off <= 0)
 		return 0;
 
-	if (BPF_MODE(insn->code) != BPF_PROBE_MEM &&
-	    BPF_MODE(insn->code) != BPF_PROBE_MEMSX &&
-	    BPF_MODE(insn->code) != BPF_PROBE_MEM32 &&
-	    !(BPF_MODE(insn->code) == BPF_PROBE_MEM32SX &&
-	      BPF_CLASS(insn->code) == BPF_LDX) &&
-	    BPF_MODE(insn->code) != BPF_PROBE_ATOMIC)
-		return 0;
-
 	if (WARN_ON_ONCE(ctx->nexentries >= ctx->prog->aux->num_exentries))
 		return -EINVAL;
 
@@ -1360,6 +1351,15 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *ro_image,
 	return ret < 0 ? ret : size;
 }
 
+/* Probe loads fault on unmapped memory and need an exception table entry */
+static bool insn_is_probe_ldx(const struct bpf_insn *insn)
+{
+	return BPF_MODE(insn->code) == BPF_PROBE_MEM ||
+	       BPF_MODE(insn->code) == BPF_PROBE_MEMSX ||
+	       BPF_MODE(insn->code) == BPF_PROBE_MEM32 ||
+	       BPF_MODE(insn->code) == BPF_PROBE_MEM32SX;
+}
+
 int bpf_jit_emit_insn(const struct bpf_insn *insn, struct rv_jit_context *ctx,
 		      bool extra_pass)
 {
@@ -1929,9 +1929,11 @@ int bpf_jit_emit_insn(const struct bpf_insn *insn, struct rv_jit_context *ctx,
 
 		emit_ldx(rd, off, rs, BPF_SIZE(code), sign_ext, ctx);
 
-		ret = add_exception_handler(insn, rd, ctx);
-		if (ret)
-			return ret;
+		if (insn_is_probe_ldx(insn)) {
+			ret = add_exception_handler(rd, ctx);
+			if (ret)
+				return ret;
+		}
 
 		if (BPF_SIZE(code) != BPF_DW && insn_is_zext(&insn[1]))
 			return 1;
@@ -1959,9 +1961,11 @@ int bpf_jit_emit_insn(const struct bpf_insn *insn, struct rv_jit_context *ctx,
 
 		emit_st(rd, off, imm, BPF_SIZE(code), ctx);
 
-		ret = add_exception_handler(insn, REG_DONT_CLEAR_MARKER, ctx);
-		if (ret)
-			return ret;
+		if (BPF_MODE(insn->code) == BPF_PROBE_MEM32) {
+			ret = add_exception_handler(REG_DONT_CLEAR_MARKER, ctx);
+			if (ret)
+				return ret;
+		}
 		break;
 
 	/* STX: *(size *)(dst + off) = src */
@@ -1981,9 +1985,11 @@ int bpf_jit_emit_insn(const struct bpf_insn *insn, struct rv_jit_context *ctx,
 
 		emit_stx(rd, off, rs, BPF_SIZE(code), ctx);
 
-		ret = add_exception_handler(insn, REG_DONT_CLEAR_MARKER, ctx);
-		if (ret)
-			return ret;
+		if (BPF_MODE(insn->code) == BPF_PROBE_MEM32) {
+			ret = add_exception_handler(REG_DONT_CLEAR_MARKER, ctx);
+			if (ret)
+				return ret;
+		}
 		break;
 
 	/* Atomics */
@@ -2001,7 +2007,7 @@ int bpf_jit_emit_insn(const struct bpf_insn *insn, struct rv_jit_context *ctx,
 			ret = emit_atomic_rmw(rd, rs, insn, ctx);
 
 		/* ret can be 1 (skip-zext); extable entry still needs to be added */
-		if (ret >= 0) {
+		if (ret >= 0 && BPF_MODE(insn->code) == BPF_PROBE_ATOMIC) {
 			/*
 			 * A load-acquire reads into dst_reg, and a read-modify-write
 			 * carrying BPF_FETCH reads the old value into src_reg, or into
@@ -2010,7 +2016,7 @@ int bpf_jit_emit_insn(const struct bpf_insn *insn, struct rv_jit_context *ctx,
 			 */
 			int load_reg = bpf_atomic_load_reg(insn);
 
-			ret = add_exception_handler(insn, load_reg < 0 ?
+			ret = add_exception_handler(load_reg < 0 ?
 					REG_DONT_CLEAR_MARKER : regmap[load_reg],
 					ctx) ?: ret;
 		}
-- 
2.50.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH bpf-next] bpf, riscv: Register extable entries at probe insn call sites

[bot+bpf-ci]

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

> bpf, riscv: Register extable entries at probe insn call sites
>
> add_exception_handler() filters insns by matching BPF_MODE() against
> the probe mode values. These values occupy unused opcode slots and
> collide with legitimate encodings: BPF_PROBE_MEM32SX (0xc0) shares
> its mode value with BPF_ATOMIC. Relying on the mode alone therefore
> risks registering exception table entries for the wrong instructions,
> as would happen for plain atomics without the LDX class check.
>
> Make the decision explicit at the call sites instead: register an
> entry only when emitting a probe load, a PROBE_MEM32 store, or a
> PROBE_ATOMIC atomic, and drop the fragile mode gate from
> add_exception_handler() along with the now unused insn argument. No
> functional change intended.
>
> Signed-off-by: Chen Pei <cp0613@linux.alibaba.com>

This looks like a bug fix for the fragile mode-based filtering in
add_exception_handler(). Should this include:

  Fixes: b4757714cc63 ("bpf, riscv: Add support for signed arena loads")


---
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/33510775602

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH bpf-next] bpf, riscv: Register extable entries at probe insn call sites

[Chen Pei]

> This looks like a bug fix for the fragile mode-based filtering in
> add_exception_handler(). Should this include:
>
>   Fixes: b4757714cc63 ("bpf, riscv: Add support for signed arena loads")

No Fixes tag, because the mode/class collision has never been a live
bug in any released tree. b4757714cc63 introduced both the
BPF_PROBE_MEM32SX support and the LDX class guard in the mode gate, in
the same commit, precisely to keep plain atomics (which share the 0xc0
mode value) out of the exception table. Its commit message says so
explicitly:

  "Since BPF_PROBE_MEM32SX shares its mode value (0xc0) with
   BPF_ATOMIC, the gate accepts it only for LDX class instructions so
   that plain atomic instructions do not register exception table
   entries."

For context: I noticed this fragile shared-state gate while working on
v1 of "bpf, riscv: Add support for signed arena loads", where the class
guard was added as a preventive measure. This patch now follows up with
the actual cleanup, making the registration decision explicit at each
call site so future probe modes cannot silently collide with legitimate
encodings.

Also, regarding sashiko-bot's separate concern about BPF_PROBE_ATOMIC
cmpxchg without ZACAS: that path is unreachable, as arena BPF_CMPXCHG
is already gated on ZACAS by bpf_jit_supports_insn(), and
add_exception_handler() resets its offsets after every registration,
so stale offsets cannot be consumed. A separate fix I have prepared,
"bpf, riscv: Make arena support depend on ZACAS", further rejects
arena map creation on non-ZACAS systems altogether.

Best regards,
Pei

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv