[PATCH bpf-next v3 1/6] bpf: Derive the atomic load register in one place

[PATCH bpf-next v3 1/6] bpf: Derive the atomic load register in one place

[Daniel Borkmann]

check_atomic_rmw() open codes the mapping from a BPF_ATOMIC to the register
it reads the old value into, the BPF_STX case of insn_def_regno() open codes
the very same mapping a second time, the const folding and the liveness
transfer functions a third and a fourth time, and BPF JITs need it as well
to know which register a faulting BPF_PROBE_ATOMIC has to clear.

Add a small helper so that all of them can share it. No functional change.
The BPF_LOAD_ACQ case is there for the JITs, which do walk all instruction
classes. const_reg_xfer() loses its explicit BPF_ATOMIC mode test since the
helper checks class and mode itself; the BPF_PROBE_ATOMIC it additionally
accepts cannot be seen there as it is only set from bpf_do_misc_fixups(),
that is, after const folding has run. arg_track_xfer() keeps its mode test
since that also guards the stack clearing next to it.

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
---
  v2 -> v3:
   - Also use the helper for const_fold and liveness (bpf ci)
   - Drop the return value comment in verifier since the helper
     comment already covers it (Jakub)
  v1 -> v2:
   - also convert insn_def_regno (Eduard, sashiko)

 include/linux/filter.h  | 24 ++++++++++++++++++++++++
 kernel/bpf/const_fold.c | 11 +++--------
 kernel/bpf/fixups.c     | 11 +----------
 kernel/bpf/liveness.c   |  9 +++------
 kernel/bpf/verifier.c   | 13 ++-----------
 5 files changed, 33 insertions(+), 35 deletions(-)

diff --git a/include/linux/filter.h b/include/linux/filter.h
index 4edba8182db1..15d83684c6e9 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -414,6 +414,30 @@ static inline bool bpf_atomic_is_load_acq(const struct bpf_insn *insn)
 	       insn->imm == BPF_LOAD_ACQ;
 }
 
+/*
+ * Given an instruction @insn, return the number of the BPF register that a
+ * BPF_ATOMIC reads the value at its memory operand into, or -1 if there is
+ * no such register. That is the register a BPF_PROBE_ATOMIC has to clear when
+ * the access faults. Like bpf_atomic_is_load_acq(), @insn is not assumed to
+ * be a BPF_ATOMIC here.
+ */
+static inline int bpf_atomic_load_reg(const struct bpf_insn *insn)
+{
+	if (BPF_CLASS(insn->code) != BPF_STX ||
+	    (BPF_MODE(insn->code) != BPF_ATOMIC &&
+	     BPF_MODE(insn->code) != BPF_PROBE_ATOMIC))
+		return -1;
+
+	switch (insn->imm) {
+	case BPF_LOAD_ACQ:
+		return insn->dst_reg;
+	case BPF_CMPXCHG:
+		return BPF_REG_0;
+	default:
+		return (insn->imm & BPF_FETCH) ? insn->src_reg : -1;
+	}
+}
+
 /* Memory store, *(uint *) (dst_reg + off16) = imm32 */
 
 #define BPF_ST_MEM(SIZE, DST, OFF, IMM)				\
diff --git a/kernel/bpf/const_fold.c b/kernel/bpf/const_fold.c
index b2a19acadb91..4cf120c7b2cb 100644
--- a/kernel/bpf/const_fold.c
+++ b/kernel/bpf/const_fold.c
@@ -199,14 +199,9 @@ static void const_reg_xfer(struct bpf_verifier_env *env, struct const_arg_info *
 			ci_out[r] = unknown;
 		break;
 	case BPF_STX:
-		if (mode != BPF_ATOMIC)
-			break;
-		if (insn->imm == BPF_CMPXCHG)
-			ci_out[BPF_REG_0] = unknown;
-		else if (insn->imm == BPF_LOAD_ACQ)
-			*dst = unknown;
-		else if (insn->imm & BPF_FETCH)
-			*src = unknown;
+		r = bpf_atomic_load_reg(insn);
+		if (r >= 0)
+			ci_out[r] = unknown;
 		break;
 	}
 }
diff --git a/kernel/bpf/fixups.c b/kernel/bpf/fixups.c
index 661e2d13a604..c4bd70befbb5 100644
--- a/kernel/bpf/fixups.c
+++ b/kernel/bpf/fixups.c
@@ -49,16 +49,7 @@ static int insn_def_regno(const struct bpf_insn *insn)
 	case BPF_ST:
 		return -1;
 	case BPF_STX:
-		if (BPF_MODE(insn->code) == BPF_ATOMIC ||
-		    BPF_MODE(insn->code) == BPF_PROBE_ATOMIC) {
-			if (insn->imm == BPF_CMPXCHG)
-				return BPF_REG_0;
-			else if (insn->imm == BPF_LOAD_ACQ)
-				return insn->dst_reg;
-			else if (insn->imm & BPF_FETCH)
-				return insn->src_reg;
-		}
-		return -1;
+		return bpf_atomic_load_reg(insn);
 	default:
 		return insn->dst_reg;
 	}
diff --git a/kernel/bpf/liveness.c b/kernel/bpf/liveness.c
index ef9a5a922887..1c997aeba6fa 100644
--- a/kernel/bpf/liveness.c
+++ b/kernel/bpf/liveness.c
@@ -1209,12 +1209,9 @@ static void arg_track_xfer(struct bpf_verifier_env *env, struct bpf_insn *insn,
 				clear_stack_for_all_offs(insn, at_out, insn->dst_reg,
 							 at_stack_out, sz);
 
-			if (insn->imm == BPF_CMPXCHG)
-				at_out[BPF_REG_0] = none;
-			else if (insn->imm == BPF_LOAD_ACQ)
-				*dst = none;
-			else if (insn->imm & BPF_FETCH)
-				*src = none;
+			r = bpf_atomic_load_reg(insn);
+			if (r >= 0)
+				at_out[r] = none;
 		}
 	} else if (class == BPF_ST && BPF_MODE(insn->code) == BPF_MEM) {
 		u32 sz = bpf_size_to_bytes(BPF_SIZE(insn->code));
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index add3affc5703..61ef43325c6f 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -6485,21 +6485,12 @@ static int check_atomic_rmw(struct bpf_verifier_env *env,
 		return -EACCES;
 	}
 
-	if (insn->imm & BPF_FETCH) {
-		if (insn->imm == BPF_CMPXCHG)
-			load_reg = BPF_REG_0;
-		else
-			load_reg = insn->src_reg;
-
+	load_reg = bpf_atomic_load_reg(insn);
+	if (load_reg >= 0) {
 		/* check and record load of old value */
 		err = check_reg_arg(env, load_reg, DST_OP);
 		if (err)
 			return err;
-	} else {
-		/* This instruction accesses a memory location but doesn't
-		 * actually load it into a register.
-		 */
-		load_reg = -1;
 	}
 
 	dst_reg = cur_regs(env) + insn->dst_reg;
-- 
2.43.0

[PATCH bpf-next v3 2/6] bpf, riscv: Clear fetch destination on faulting arena atomic

[Daniel Borkmann]

A RMW atomic on an arena pointer is converted to BPF_PROBE_ATOMIC and
gets an exception table entry, but that entry records no destination
register to clear unless the instruction is a load-acquire today. That
is right for a plain BPF_{ADD,AND,OR,XOR}, which only writes memory,
but an RMW carrying BPF_FETCH also reads the old value into a register:
src_reg for BPF_{ADD,AND,OR,XOR} | BPF_FETCH and BPF_XCHG, and r0 for
BPF_CMPXCHG. emit_atomic_rmw() emits it that way, e.g.:

  [...]
  case BPF_XCHG:
          ctx->ex_insn_off = ctx->ninsns;
          emit(is64 ? rv_amoswap_d(rs, rs, rd, 1, 1) :
               rv_amoswap_w(rs, rs, rd, 1, 1), ctx);
  [...]

Thus, a fault over an unmapped arena page ex_handler_bpf() jumps over
the access but leaves rs untouched, and the program resumes with
whatever it held before the atomic instead of the 0 that every other
BPF_PROBE_* access delivers. Fill the exception table entry in from
bpf_atomic_load_reg(), which returns the BPF register an atomic reads
the memory operand into or -1 when it has none. A load-acquire ends up
with the same register it gets today, it just goes through the helper.
Unlike x86-64 and arm64, riscv64 does not report arena violations from
its exception handler, so there is no access direction to correct here,
only the missing register clear.

Fixes: fb7cefabae81 ("riscv, bpf: Add support arena atomics for RV64")
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Reviewed-by: Pu Lehui <pulehui@huawei.com>
---
 arch/riscv/net/bpf_jit_comp64.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/arch/riscv/net/bpf_jit_comp64.c b/arch/riscv/net/bpf_jit_comp64.c
index 6b9972b07c1b..2504df1fa111 100644
--- a/arch/riscv/net/bpf_jit_comp64.c
+++ b/arch/riscv/net/bpf_jit_comp64.c
@@ -1992,10 +1992,19 @@ 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)
-			ret = add_exception_handler(insn,
-				bpf_atomic_is_load_acq(insn) ? rd : REG_DONT_CLEAR_MARKER,
-				ctx) ?: ret;
+		if (ret >= 0) {
+			/*
+			 * A load-acquire reads into dst_reg, and a read-modify-write
+			 * carrying BPF_FETCH reads the old value into src_reg, or into
+			 * r0 for a BPF_CMPXCHG. Clear that register on fault, the
+			 * remaining atomics have no destination register.
+			 */
+			int load_reg = bpf_atomic_load_reg(insn);
+
+			ret = add_exception_handler(insn, load_reg < 0 ?
+					REG_DONT_CLEAR_MARKER : regmap[load_reg],
+					ctx) ?: ret;
+		}
 
 		if (ret)
 			return ret;
-- 
2.43.0

[PATCH bpf-next v3 3/6] bpf, x86: Clear fetch destination on faulting arena atomic

[Daniel Borkmann]

populate_extable() encodes "there is no destination register to clear" as
DONT_CLEAR in the DST_REG field of the exception table metadata, and later
ex_handler_bpf() then reuses that very value to derive the direction it
reports the fault with is_write = (reg == DONT_CLEAR). The two coincide
for a plain load or store, but not for a RMW carrying BPF_FETCH. Such an
atomic writes memory, so it has to be reported as a WRITE, and it also reads
the old value into a register, src_reg for BPF_ADD | BPF_FETCH and BPF_XCHG,
r0 for BPF_CMPXCHG, so that register has to be cleared on fault. A single
DONT_CLEAR cannot say both, and the store branch picks it unconditionally:

  [...]
  } else {
          arena_reg = reg2pt_regs[dst_reg];
          fixup_reg = DONT_CLEAR;
  }
  [...]

The reported direction is therefore right, but on a fault over an unmapped
arena page the fetch destination keeps whatever it held before the atomic,
where every other BPF_PROBE_* access delivers 0. Give the metadata its own
ARENA_WRITE bit so that the reported direction no longer depends on whether
there is a register to clear, and fill DST_REG in from bpf_atomic_load_reg().
BPF_{AND,OR,XOR} | BPF_FETCH need no handling here, bpf_jit_supports_insn()
already rejects those in the arena.

Fixes: d503a04f8bc0 ("bpf: Add support for certain atomics in bpf_arena to x86 JIT")
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Acked-by: Eduard Zingerman <eddyz87@gmail.com>
Reviewed-by: Puranjay Mohan <puranjay@kernel.org>
---
 arch/x86/net/bpf_jit_comp.c | 35 +++++++++++++++++++++++++++--------
 1 file changed, 27 insertions(+), 8 deletions(-)

diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
index 8dddb5d7af21..d920772af7d5 100644
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c
@@ -1473,17 +1473,20 @@ static int emit_atomic_ld_st_index(u8 **pprog, u32 atomic_op, u32 size,
  *
  * Bit layout of `fixup` (32-bit):
  *
- * +-----------+--------+-----------+---------+----------+
- * | 31        | 30-24  |   23-16   |   15-8  |    7-0   |
- * |           |        |           |         |          |
- * | ARENA_ACC | Unused | ARENA_REG | DST_REG | INSN_LEN |
- * +-----------+--------+-----------+---------+----------+
+ * +-----------+-------------+--------+-----------+---------+----------+
+ * | 31        | 30          | 29-24  |   23-16   |   15-8  |    7-0   |
+ * |           |             |        |           |         |          |
+ * | ARENA_ACC | ARENA_WRITE | Unused | ARENA_REG | DST_REG | INSN_LEN |
+ * +-----------+-------------+--------+-----------+---------+----------+
  *
  * - INSN_LEN (8 bits): Length of faulting insn (max x86 insn = 15 bytes (fits in 8 bits)).
  * - DST_REG  (8 bits): Offset of dst_reg from reg2pt_regs[] (max offset = 112 (fits in 8 bits)).
- *                      This is set to DONT_CLEAR if the insn is a store.
+ *                      This is set to DONT_CLEAR if the insn does not read into a register.
  * - ARENA_REG (8 bits): Offset of the register that is used to calculate the
  *                       address for load/store when accessing the arena region.
+ * - ARENA_WRITE (1 bit): This bit is set when the faulting instruction wrote to the arena region.
+ *                        It is independent of DST_REG, since a read-modify-write both writes to
+ *                        memory and reads the old value into a register.
  * - ARENA_ACCESS (1 bit): This bit is set when the faulting instruction accessed the arena region.
  *
  * Bit layout of `data` (32-bit):
@@ -1502,6 +1505,7 @@ static int emit_atomic_ld_st_index(u8 **pprog, u32 atomic_op, u32 size,
 #define FIXUP_INSN_LEN_MASK	GENMASK(7, 0)
 #define FIXUP_REG_MASK		GENMASK(15, 8)
 #define FIXUP_ARENA_REG_MASK	GENMASK(23, 16)
+#define FIXUP_ARENA_WRITE	BIT(30)
 #define FIXUP_ARENA_ACCESS	BIT(31)
 #define DATA_ARENA_OFFSET_MASK	GENMASK(31, 16)
 
@@ -1510,7 +1514,7 @@ bool ex_handler_bpf(const struct exception_table_entry *x, struct pt_regs *regs)
 	u32 reg = FIELD_GET(FIXUP_REG_MASK, x->fixup);
 	u32 insn_len = FIELD_GET(FIXUP_INSN_LEN_MASK, x->fixup);
 	bool is_arena = !!(x->fixup & FIXUP_ARENA_ACCESS);
-	bool is_write = (reg == DONT_CLEAR);
+	bool is_write = !!(x->fixup & FIXUP_ARENA_WRITE);
 	unsigned long addr;
 	s16 off;
 	u32 arena_reg;
@@ -2348,6 +2352,7 @@ st:			insn_off = insn->off;
 				struct exception_table_entry *ex;
 				u8 *_insn = image + proglen + (start_of_ldx - temp);
 				u32 arena_reg, fixup_reg;
+				bool is_write;
 				s64 delta;
 
 				if (!bpf_prog->aux->extable)
@@ -2384,15 +2389,29 @@ st:			insn_off = insn->off;
 				    bpf_atomic_is_load_acq(insn)) {
 					arena_reg = reg2pt_regs[src_reg];
 					fixup_reg = reg2pt_regs[dst_reg];
+					is_write = false;
 				} else {
+					/*
+					 * A store has no destination register to clear,
+					 * except for a read-modify-write with BPF_FETCH,
+					 * which also reads the old value into src_reg, or
+					 * into r0 for a BPF_CMPXCHG. Either way the access
+					 * is still reported as a write.
+					 */
+					int load_reg = bpf_atomic_load_reg(insn);
+
 					arena_reg = reg2pt_regs[dst_reg];
-					fixup_reg = DONT_CLEAR;
+					fixup_reg = load_reg < 0 ? DONT_CLEAR :
+						    reg2pt_regs[load_reg];
+					is_write = true;
 				}
 
 				ex->fixup = FIELD_PREP(FIXUP_INSN_LEN_MASK, prog - start_of_ldx) |
 					    FIELD_PREP(FIXUP_ARENA_REG_MASK, arena_reg) |
 					    FIELD_PREP(FIXUP_REG_MASK, fixup_reg);
 				ex->fixup |= FIXUP_ARENA_ACCESS;
+				if (is_write)
+					ex->fixup |= FIXUP_ARENA_WRITE;
 
 				ex->data |= FIELD_PREP(DATA_ARENA_OFFSET_MASK, insn->off);
 			}
-- 
2.43.0

[PATCH bpf-next v3 4/6] bpf, arm64: Clear fetch destination on faulting arena atomic

[Daniel Borkmann]

Same problem as on x86-64: add_exception_handler() folds "there is no
destination register to clear" and "this is a store" into one DONT_CLEAR
value ...

  if (BPF_CLASS(insn->code) != BPF_LDX && !bpf_atomic_is_load_acq(insn))
          dst_reg = DONT_CLEAR;

... which ex_handler_bpf() then reads back as the access direction:

  bool is_write = (dst_reg == DONT_CLEAR);

A RMW carrying BPF_FETCH is both. emit_lse_atomic() reads the old value
into src_reg for BPF_{ADD,AND,OR,XOR} | BPF_FETCH and BPF_XCHG, and into
r0 for BPF_CMPXCHG, so a fault over an unmapped arena page is correctly
reported as a WRITE but leaves that register holding a stale value instead
of the 0 that every other BPF_PROBE_* access delivers. Same as on x86-64,
add a separate ARENA_WRITE bit for the direction.

FIXUP_REG is now filled in by the callers of add_exception_handler(), the
BPF_PROBE_ATOMIC one deriving it from bpf_atomic_load_reg(), so that the
helper only has to determine the direction. This is how the riscv64 JIT
already does it, and it stops the two store callers from handing in a
dst_reg that was only going to be overwritten with DONT_CLEAR anyway.

Fixes: e612b5c1d3ee ("bpf, arm64: Add support for lse atomics in bpf_arena")
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Cc: Puranjay Mohan <puranjay@kernel.org>
---
 v1 -> v2:
   - move bpf_atomic_load_reg into build_insn (Eduard)

 arch/arm64/net/bpf_jit_comp.c | 44 ++++++++++++++++++++++++-----------
 1 file changed, 30 insertions(+), 14 deletions(-)

diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
index d14d297ebb96..74b4083791da 100644
--- a/arch/arm64/net/bpf_jit_comp.c
+++ b/arch/arm64/net/bpf_jit_comp.c
@@ -1082,23 +1082,27 @@ static void build_epilogue(struct jit_ctx *ctx, bool was_classic)
  *
  * Bit layout of `fixup` (32-bit):
  *
- * +-----------+--------+-----------+-----------+----------+
- * |   31-27   | 26-22  |     21    |   20-16   |   15-0   |
- * |           |        |           |           |          |
- * | FIXUP_REG | Unused | ARENA_ACC | ARENA_REG |  OFFSET  |
- * +-----------+--------+-----------+-----------+----------+
+ * +-----------+--------+-------------+-----------+-----------+----------+
+ * |   31-27   | 26-23  |      22     |     21    |   20-16   |   15-0   |
+ * |           |        |             |           |           |          |
+ * | FIXUP_REG | Unused | ARENA_WRITE | ARENA_ACC | ARENA_REG |  OFFSET  |
+ * +-----------+--------+-------------+-----------+-----------+----------+
  *
  * - OFFSET (16 bits): Offset used to compute address for Load/Store instruction.
  * - ARENA_REG (5 bits): Register that is used to calculate the address for load/store when
  *                       accessing the arena region.
  * - ARENA_ACCESS (1 bit): This bit is set when the faulting instruction accessed the arena region.
+ * - ARENA_WRITE (1 bit): This bit is set when the faulting instruction wrote to the arena region.
+ *                        It is independent of FIXUP_REG, since a read-modify-write both writes to
+ *                        memory and reads the old value into a register.
  * - FIXUP_REG (5 bits): Destination register for the load instruction (cleared on fault) or set to
- *                       DONT_CLEAR if it is a store instruction.
+ *                       DONT_CLEAR if the instruction does not read into a register.
  */
 
 #define BPF_FIXUP_OFFSET_MASK      GENMASK(15, 0)
 #define BPF_FIXUP_ARENA_REG_MASK   GENMASK(20, 16)
 #define BPF_ARENA_ACCESS           BIT(21)
+#define BPF_ARENA_WRITE            BIT(22)
 #define BPF_FIXUP_REG_MASK	GENMASK(31, 27)
 #define DONT_CLEAR 5 /* Unused ARM64 register from BPF's POV */
 
@@ -1109,7 +1113,7 @@ bool ex_handler_bpf(const struct exception_table_entry *ex,
 	s16 off = FIELD_GET(BPF_FIXUP_OFFSET_MASK, ex->fixup);
 	int arena_reg = FIELD_GET(BPF_FIXUP_ARENA_REG_MASK, ex->fixup);
 	bool is_arena = !!(ex->fixup & BPF_ARENA_ACCESS);
-	bool is_write = (dst_reg == DONT_CLEAR);
+	bool is_write = !!(ex->fixup & BPF_ARENA_WRITE);
 	unsigned long addr;
 
 	if (is_arena) {
@@ -1132,7 +1136,7 @@ static int add_exception_handler(const struct bpf_insn *insn,
 {
 	off_t ins_offset;
 	s16 off = insn->off;
-	bool is_arena;
+	bool is_arena, is_write;
 	int arena_reg;
 	unsigned long pc;
 	struct exception_table_entry *ex;
@@ -1181,15 +1185,18 @@ static int add_exception_handler(const struct bpf_insn *insn,
 	/*
 	 * A load-acquire is of BPF_STX class, but reads from src_reg into
 	 * dst_reg like a BPF_LDX does, hence it must not be treated as a store
-	 * here.
+	 * here. A read-modify-write carrying BPF_FETCH is reported as a write
+	 * even though it does have a register to clear, see the callers.
 	 */
-	if (BPF_CLASS(insn->code) != BPF_LDX && !bpf_atomic_is_load_acq(insn))
-		dst_reg = DONT_CLEAR;
+	is_write = BPF_CLASS(insn->code) != BPF_LDX &&
+		   !bpf_atomic_is_load_acq(insn);
 
 	ex->fixup = FIELD_PREP(BPF_FIXUP_REG_MASK, dst_reg);
 
 	if (is_arena) {
 		ex->fixup |= BPF_ARENA_ACCESS;
+		if (is_write)
+			ex->fixup |= BPF_ARENA_WRITE;
 		/*
 		 * insn->src_reg/dst_reg holds the address in the arena region with upper 32-bits
 		 * being zero because of a preceding addr_space_cast(r<n>, 0x0, 0x1) instruction.
@@ -1889,7 +1896,7 @@ static int build_insn(const struct bpf_verifier_env *env, const struct bpf_insn
 			break;
 		}
 
-		ret = add_exception_handler(insn, ctx, dst);
+		ret = add_exception_handler(insn, ctx, DONT_CLEAR);
 		if (ret)
 			return ret;
 		break;
@@ -1956,7 +1963,7 @@ static int build_insn(const struct bpf_verifier_env *env, const struct bpf_insn
 			break;
 		}
 
-		ret = add_exception_handler(insn, ctx, dst);
+		ret = add_exception_handler(insn, ctx, DONT_CLEAR);
 		if (ret)
 			return ret;
 		break;
@@ -1979,7 +1986,16 @@ static int build_insn(const struct bpf_verifier_env *env, const struct bpf_insn
 			return ret;
 
 		if (BPF_MODE(insn->code) == BPF_PROBE_ATOMIC) {
-			ret = add_exception_handler(insn, ctx, dst);
+			/*
+			 * A load-acquire reads into dst_reg, and a read-modify-write
+			 * carrying BPF_FETCH reads the old value into src_reg, or into
+			 * r0 for a BPF_CMPXCHG. Clear that register on fault, the
+			 * remaining atomics have no destination register.
+			 */
+			int load_reg = bpf_atomic_load_reg(insn);
+
+			ret = add_exception_handler(insn, ctx, load_reg < 0 ?
+						    DONT_CLEAR : bpf2a64[load_reg]);
 			if (ret)
 				return ret;
 		}
-- 
2.43.0

[PATCH bpf-next v3 5/6] bpf, s390: Clear fetch destination on faulting arena atomic

[Daniel Borkmann]

Same missing register clear as on riscv64. A RMW atomic on an arena pointer
is converted to BPF_PROBE_ATOMIC and gets an exception table entry, but
bpf_jit_probe_atomic_pre() only fills in the arena base and the probe
offset, leaving probe->reg at the -1 that bpf_jit_probe_init() set, which
bpf_jit_probe_post() writes into the entry and ex_handler_bpf() then reads
back as "there is nothing to clear".

That is right for a plain BPF_{ADD,AND,OR,XOR}, which only writes memory,
but an RMW carrying BPF_FETCH also reads the old value into a register:
src_reg for BPF_{ADD,AND,OR,XOR} | BPF_FETCH and BPF_XCHG, and r0 for
BPF_CMPXCHG. So on a fault over an unmapped arena page the program resumes
at the landing pad with whatever that register held before the atomic
instead of the 0 that every other BPF_PROBE_* access delivers.

Fill probe->reg in from bpf_atomic_load_reg(). Unlike x86-64 and arm64,
s390x does not report arena violations from its exception handler, so there
is no access direction to correct here, only the missing register clear.

Fixes: 2f9469484a3b ("s390/bpf: Support arena atomics")
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Cc: Ilya Leoshkevich <iii@linux.ibm.com>
---
 arch/s390/net/bpf_jit_comp.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/arch/s390/net/bpf_jit_comp.c b/arch/s390/net/bpf_jit_comp.c
index b60877478b45..c46872b071ce 100644
--- a/arch/s390/net/bpf_jit_comp.c
+++ b/arch/s390/net/bpf_jit_comp.c
@@ -774,6 +774,8 @@ static void bpf_jit_probe_atomic_pre(struct bpf_jit *jit,
 				     struct bpf_insn *insn,
 				     struct bpf_jit_probe *probe)
 {
+	int load_reg;
+
 	if (BPF_MODE(insn->code) != BPF_PROBE_ATOMIC)
 		return;
 
@@ -783,6 +785,14 @@ static void bpf_jit_probe_atomic_pre(struct bpf_jit *jit,
 	EMIT4(0xb9080000, REG_W1, insn->dst_reg);
 	probe->arena_reg = REG_W1;
 	probe->prg = jit->prg;
+	/*
+	 * A read-modify-write carrying BPF_FETCH reads the old value into
+	 * src_reg, or into r0 for a BPF_CMPXCHG. Clear that register on
+	 * fault, the remaining atomics only write memory.
+	 */
+	load_reg = bpf_atomic_load_reg(insn);
+	if (load_reg >= 0)
+		probe->reg = reg2hex[load_reg];
 }
 
 static int bpf_jit_probe_post(struct bpf_jit *jit, struct bpf_prog *fp,
@@ -1684,6 +1694,7 @@ static noinline int bpf_jit_insn(struct bpf_jit *jit, struct bpf_prog *fp,
 			if (load_probe.prg != -1) {
 				probe.prg = jit->prg;
 				probe.arena_reg = load_probe.arena_reg;
+				probe.reg = load_probe.reg;
 			}
 			loop_start = jit->prg;
 			/* 0: {csy|csg} %w0,%src,off(%arena) */
-- 
2.43.0

[PATCH bpf-next v3 6/6] selftests/bpf: Add arena fault tests for atomics with fetch

[Daniel Borkmann]

Add stream_arena_xchg_fault and stream_arena_cmpxchg_fault next to the
existing read, write and load-acquire fault tests, covering the two
places a read-modify-write can deposit the old value: src_reg for a
BPF_XCHG and r0 for a BPF_CMPXCHG. Both cover both halves of the JIT
bug that left the fetch destination alone when a RMW on an arena pointer
faulted:

  - the fault has to be reported as a WRITE, and at the address held by
    the destination register, which __stderr() and test_address() check
  - the register receiving the fetched value has to be cleared by the
    fault handler, which the programs check by poisoning it before the
    atomic and returning it, so __retval(0) fails if it is left untouched

The __stderr() annotation can only wildcard the faulting address since
the arena base is not known until runtime, hence the two test_address()
subtests on top, which pin it to the address held by dst_reg rather than
src_reg.

Note, the atomics are open coded since linux/filter.h cannot be included
alongside vmlinux.h.

  # LDLIBS=-static PKG_CONFIG='pkg-config --static' ./vmtest.sh -- ./test_progs -t stream
  [...]
  #464/1   stream_arena_fault_address/read_fault:OK
  #464/2   stream_arena_fault_address/write_fault:OK
  #464/3   stream_arena_fault_address/load_acquire_fault:OK
  #464/4   stream_arena_fault_address/xchg_fault:OK
  #464/5   stream_arena_fault_address/cmpxchg_fault:OK
  #464     stream_arena_fault_address:OK
  [...]
  #466/5   stream_success/stream_arena_write_fault:OK
  #466/6   stream_success/stream_arena_read_fault:OK
  #466/7   stream_success/stream_arena_load_acquire_fault:OK
  #466/8   stream_success/stream_arena_xchg_fault:OK
  #466/9   stream_success/stream_arena_cmpxchg_fault:OK
  [...]
  Summary: 4/22 PASSED, 0 SKIPPED, 0 FAILED

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Acked-by: Eduard Zingerman <eddyz87@gmail.com>
---
 v1 -> v2:
   - Updated commit msg wrt test_address structuring

 .../testing/selftests/bpf/prog_tests/stream.c |   4 +
 tools/testing/selftests/bpf/progs/stream.c    | 101 ++++++++++++++++++
 2 files changed, 105 insertions(+)

diff --git a/tools/testing/selftests/bpf/prog_tests/stream.c b/tools/testing/selftests/bpf/prog_tests/stream.c
index 15dd3ae2a84b..e4e9374309e2 100644
--- a/tools/testing/selftests/bpf/prog_tests/stream.c
+++ b/tools/testing/selftests/bpf/prog_tests/stream.c
@@ -105,6 +105,10 @@ void test_stream_arena_fault_address(void)
 		test_address(skel->progs.stream_arena_write_fault, &skel->bss->fault_addr);
 	if (test__start_subtest("load_acquire_fault"))
 		test_address(skel->progs.stream_arena_load_acquire_fault, &skel->bss->fault_addr);
+	if (test__start_subtest("xchg_fault"))
+		test_address(skel->progs.stream_arena_xchg_fault, &skel->bss->fault_addr);
+	if (test__start_subtest("cmpxchg_fault"))
+		test_address(skel->progs.stream_arena_cmpxchg_fault, &skel->bss->fault_addr);
 
 	stream__destroy(skel);
 }
diff --git a/tools/testing/selftests/bpf/progs/stream.c b/tools/testing/selftests/bpf/progs/stream.c
index cf5533e11f39..00a37933e411 100644
--- a/tools/testing/selftests/bpf/progs/stream.c
+++ b/tools/testing/selftests/bpf/progs/stream.c
@@ -229,6 +229,107 @@ int stream_arena_load_acquire_fault(void *ctx)
 	return val;
 }
 
+SEC("syscall")
+__arch_x86_64
+__arch_arm64
+__success __retval(0)
+__stderr("ERROR: Arena WRITE access at unmapped address 0x{{.*}}")
+__stderr("CPU: {{[0-9]+}} UID: 0 PID: {{[0-9]+}} Comm: {{.*}}")
+__stderr("Call trace:\n"
+"{{([a-zA-Z_][a-zA-Z0-9_]*\\+0x[0-9a-fA-F]+/0x[0-9a-fA-F]+\n"
+"|[ \t]+[^\n]+\n)*}}")
+int stream_arena_xchg_fault(void *ctx)
+{
+	static const struct bpf_insn xchg_insn = {
+		.code	 = 0xc3,	/* BPF_STX | BPF_ATOMIC | BPF_W */
+		.dst_reg = 1,		/* BPF_REG_1 */
+		.src_reg = 2,		/* BPF_REG_2 */
+		.off	 = 0x7fff,
+		.imm	 = 0xe1,	/* BPF_XCHG */
+	};
+	struct bpf_arena *ptr = (void *)&arena;
+	u64 user_vm_start, val;
+
+	/*
+	 * Prevent GCC bounds warning: casting &arena to struct bpf_arena *
+	 * triggers bounds checking since the map definition is smaller than
+	 * struct bpf_arena. barrier_var() makes the pointer opaque to GCC,
+	 * preventing the bounds analysis.
+	 */
+	barrier_var(ptr);
+	user_vm_start = ptr->user_vm_start;
+	fault_addr = user_vm_start + 0x7fff;
+	bpf_addr_space_cast(user_vm_start, 0, 1);
+	/*
+	 * A read-modify-write carrying BPF_FETCH writes to memory, so the fault
+	 * has to be reported as a WRITE from the dst_reg address, but it also
+	 * reads the old value into src_reg, so the exception handler has to
+	 * clear src_reg. Poison it up front, the returned value must be 0.
+	 */
+	asm volatile (
+		"r1 = %[user_vm_start];"
+		"r2 = 1;"
+		".8byte %[xchg_insn];" /* r2 = xchg((u32 *)(r1 + 0x7fff), r2) */
+		"%[val] = r2;"
+		: [val] "=r" (val)
+		: [user_vm_start] "r" (user_vm_start),
+		  __imm_insn(xchg_insn, xchg_insn)
+		: "r1", "r2"
+	);
+	return val;
+}
+
+SEC("syscall")
+__arch_x86_64
+__arch_arm64
+__success __retval(0)
+__stderr("ERROR: Arena WRITE access at unmapped address 0x{{.*}}")
+__stderr("CPU: {{[0-9]+}} UID: 0 PID: {{[0-9]+}} Comm: {{.*}}")
+__stderr("Call trace:\n"
+"{{([a-zA-Z_][a-zA-Z0-9_]*\\+0x[0-9a-fA-F]+/0x[0-9a-fA-F]+\n"
+"|[ \t]+[^\n]+\n)*}}")
+int stream_arena_cmpxchg_fault(void *ctx)
+{
+	static const struct bpf_insn cmpxchg_insn = {
+		.code	 = 0xc3,	/* BPF_STX | BPF_ATOMIC | BPF_W */
+		.dst_reg = 1,		/* BPF_REG_1 */
+		.src_reg = 2,		/* BPF_REG_2 */
+		.off	 = 0x7fff,
+		.imm	 = 0xf1,	/* BPF_CMPXCHG */
+	};
+	struct bpf_arena *ptr = (void *)&arena;
+	u64 user_vm_start, val;
+
+	/*
+	 * Prevent GCC bounds warning: casting &arena to struct bpf_arena *
+	 * triggers bounds checking since the map definition is smaller than
+	 * struct bpf_arena. barrier_var() makes the pointer opaque to GCC,
+	 * preventing the bounds analysis.
+	 */
+	barrier_var(ptr);
+	user_vm_start = ptr->user_vm_start;
+	fault_addr = user_vm_start + 0x7fff;
+	bpf_addr_space_cast(user_vm_start, 0, 1);
+	/*
+	 * Same as the exchange above, except that a BPF_CMPXCHG reads the old
+	 * value into r0 rather than into src_reg, so r0 is the register the
+	 * exception handler has to clear. It doubles as the compare value, but
+	 * the comparison never happens since the access faults first.
+	 */
+	asm volatile (
+		"r1 = %[user_vm_start];"
+		"r0 = 1;"
+		"r2 = 2;"
+		".8byte %[cmpxchg_insn];" /* r0 = cmpxchg((u32 *)(r1 + 0x7fff), r0, r2) */
+		"%[val] = r0;"
+		: [val] "=r" (val)
+		: [user_vm_start] "r" (user_vm_start),
+		  __imm_insn(cmpxchg_insn, cmpxchg_insn)
+		: "r0", "r1", "r2"
+	);
+	return val;
+}
+
 static __noinline void subprog(void)
 {
 	int __arena *addr = (int __arena *)0xdeadbeef;
-- 
2.43.0

Re: [PATCH bpf-next v3 6/6] selftests/bpf: Add arena fault tests for atomics with fetch

[bot+bpf-ci]

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

> diff --git a/tools/testing/selftests/bpf/prog_tests/stream.c b/tools/testing/selftests/bpf/prog_tests/stream.c
> --- a/tools/testing/selftests/bpf/prog_tests/stream.c
> +++ b/tools/testing/selftests/bpf/prog_tests/stream.c
> @@ -105,6 +105,10 @@ void test_stream_arena_fault_address(void)
>  		test_address(skel->progs.stream_arena_write_fault, &skel->bss->fault_addr);
>  	if (test__start_subtest("load_acquire_fault"))
>  		test_address(skel->progs.stream_arena_load_acquire_fault, &skel->bss->fault_addr);
> +	if (test__start_subtest("xchg_fault"))
> +		test_address(skel->progs.stream_arena_xchg_fault, &skel->bss->fault_addr);
> +	if (test__start_subtest("cmpxchg_fault"))
> +		test_address(skel->progs.stream_arena_cmpxchg_fault, &skel->bss->fault_addr);
>
>  	stream__destroy(skel);
>  }
> diff --git a/tools/testing/selftests/bpf/progs/stream.c b/tools/testing/selftests/bpf/progs/stream.c
> --- a/tools/testing/selftests/bpf/progs/stream.c
> +++ b/tools/testing/selftests/bpf/progs/stream.c
> @@ -229,6 +229,107 @@ int stream_arena_load_acquire_fault(void *ctx)
>  	return val;
>  }
>
> +SEC("syscall")
> +__arch_x86_64
> +__arch_arm64
> +__success __retval(0)
> +__stderr("ERROR: Arena WRITE access at unmapped address 0x{{.*}}")

The series includes fixes for four JITs (x86, arm64, riscv, s390), but
these new programs only cover two of them. Could __arch_riscv64 and
__arch_s390x variants be added for the other half?

The riscv64 and s390 JIT fixes exist in the same series:

  a49d067b71354 bpf, riscv: Clear fetch destination on faulting arena atomic
  e0de31f547978 bpf, s390:  Clear fetch destination on faulting arena atomic

but the __arch gate provably excludes them from these tests.

The __stderr() half unavoidably gates on x86_64 and arm64, since only
those two JITs call bpf_prog_report_arena_violation(). Both the riscv
and s390 changelogs state this explicitly.

But the __retval(0) half does not depend on arena violation reporting.
A variant tagged __arch_riscv64 __arch_s390x with __success __retval(0)
and no __stderr() would exercise exactly the register-clear fix those two
commits make, using the same poison-and-return mechanism. As written, two
of the four fixed JITs ship with no test coverage.

> +__stderr("CPU: {{[0-9]+}} UID: 0 PID: {{[0-9]+}} Comm: {{.*}}")
> +__stderr("Call trace:\n"
> +"{{([a-zA-Z_][a-zA-Z0-9_]*\\+0x[0-9a-fA-F]+/0x[0-9a-fA-F]+\n"
> +"|[ \t]+[^\n]+\n)*}}")
> +int stream_arena_xchg_fault(void *ctx)
> +{
> +	static const struct bpf_insn xchg_insn = {
> +		.code	 = 0xc3,	/* BPF_STX | BPF_ATOMIC | BPF_W */
> +		.dst_reg = 1,		/* BPF_REG_1 */
> +		.src_reg = 2,		/* BPF_REG_2 */
> +		.off	 = 0x7fff,
> +		.imm	 = 0xe1,	/* BPF_XCHG */
> +	};

Does the misaligned .off = 0x7fff cause the test to validate a different
fault path on arm64 than the commit message describes?

user_vm_start is page aligned, so the effective address is
user_vm_start + 0x7fff, which is 3 mod 4. The 4-byte BPF_W access also
straddles the page boundary at 0x8000.

For plain LDR/STR (stream_arena_read_fault, stream_arena_write_fault),
this offset works as intended: arm64 permits unaligned access with
SCTLR_EL1.A=0, so those really do take a translation fault on the unmapped
page.

But atomics require natural alignment. arm64 LSE CAS/SWP raise an EL1
Alignment fault (ESR FSC 0b100001) when the access is misaligned or crosses
a 16-byte granule, and that takes priority over the translation fault.

The test still passes - fixup_exception() in arch/arm64/mm/fault.c runs
before any die_kernel_fault path, so ex_handler_bpf() still clears the
fetch register and still prints "Arena WRITE access at unmapped address".
The fix under test is fault-type independent.

But the arm64 arm of both new subtests validates the alignment-fault path
rather than the unmapped-arena-page path, while x86-64 (where page fault
is delivered ahead of split-lock alignment check) validates the intended
one, so the two architectures end up testing different code paths.

A 4-byte-aligned offset inside the unmapped page (e.g. 0x7ffc) would keep
the fault a translation fault on both architectures. The pre-existing
stream_arena_load_acquire_fault has the same property with its 0x7fff.

[ ... ]

> +	return val;
> +}
> +
> +SEC("syscall")
> +__arch_x86_64
> +__arch_arm64

A subsystem pattern flags this as potentially concerning: On arm64 these
two programs only load on CPUs that implement LSE atomics, and nothing in
the test gates on that.

arch/arm64/net/bpf_jit_comp.c:bpf_jit_supports_insn() rejects arena RMW
atomics without LSE:

  bool bpf_jit_supports_insn(struct bpf_insn *insn, bool in_arena)
  {
  	if (!in_arena)
  		return true;
  	switch (insn->code) {
  	case BPF_STX | BPF_ATOMIC | BPF_W:
  	case BPF_STX | BPF_ATOMIC | BPF_DW:
  		if (!bpf_atomic_is_load_store(insn) &&
  		    !cpus_have_cap(ARM64_HAS_LSE_ATOMICS))
  			return false;
  	}
  	return true;
  }

The verifier consults it for arena destinations in atomic_ptr_type_ok()
and on failure rejects the program with "BPF_ATOMIC stores into R1 arena
is not allowed". BPF_XCHG (0xe1) and BPF_CMPXCHG (0xf1) are not load-store
forms, so bpf_atomic_is_load_store() returns false for both and the LSE
check applies.

This is new for this object. Before the patch the arena programs in
progs/stream.c used plain BPF_LDX/BPF_STX and one BPF_LOAD_ACQ, and
BPF_LOAD_ACQ is exempted by bpf_atomic_is_load_store(). progs/stream.c
therefore had no CPU-feature dependency until now.

On an ARMv8.0 core (Cortex-A53/A57/A72) the consequences reach beyond the
two new subtests, because stream__open_and_load() is an all-or-nothing
whole-object load:

  - test_stream_arena_fault_address(): stream__open_and_load() returns
    NULL, ASSERT_OK_PTR fails and the function returns, so read_fault,
    write_fault and load_acquire_fault are lost too - three subtests
    that pass today.
  - test_stream_syscall(): same whole-object load, and this test is
    otherwise entirely architecture-neutral.
  - test_stream_success() (RUN_TESTS) loads programs individually
    (bpf_program__set_autoload), so there the damage is limited to the
    two new programs reporting load failure.

Would a runtime LSE probe, or moving the two RMW programs into a separate
object so a load failure cannot take down the unrelated subtests, contain
this?

progs/arena_atomics.c already does arena xchg/cmpxchg guarded only by
compile-time checks, prog_tests/arena_atomics.c loads that whole object
with arena_atomics__load(), and DENYLIST.aarch64 is empty - so the
arena_atomics test presumably already fails on non-LSE arm64. This class
of gap is pre-existing in the tree.

> +__success __retval(0)
> +__stderr("ERROR: Arena WRITE access at unmapped address 0x{{.*}}")

[ ... ]


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

Re: [PATCH bpf-next v3 6/6] selftests/bpf: Add arena fault tests for atomics with fetch

[Daniel Borkmann]

On 8/11/26 4:27 PM, bot+bpf-ci@kernel.org wrote:
[...]
>> diff --git a/tools/testing/selftests/bpf/progs/stream.c b/tools/testing/selftests/bpf/progs/stream.c
>> --- a/tools/testing/selftests/bpf/progs/stream.c
>> +++ b/tools/testing/selftests/bpf/progs/stream.c
>> @@ -229,6 +229,107 @@ int stream_arena_load_acquire_fault(void *ctx)
>>   	return val;
>>   }
>>
>> +SEC("syscall")
>> +__arch_x86_64
>> +__arch_arm64
>> +__success __retval(0)
>> +__stderr("ERROR: Arena WRITE access at unmapped address 0x{{.*}}")
> 
> The series includes fixes for four JITs (x86, arm64, riscv, s390), but
> these new programs only cover two of them. Could __arch_riscv64 and
> __arch_s390x variants be added for the other half?

Hm, so the __arch gate is forced by __stderr() and bpf_prog_report_arena_violation()
currently only has callers in the x86-64 and arm64 ex handlers. I'd leave
as-is for now.

[...]
> But the __retval(0) half does not depend on arena violation reporting.
> A variant tagged __arch_riscv64 __arch_s390x with __success __retval(0)
> and no __stderr() would exercise exactly the register-clear fix those two
> commits make, using the same poison-and-return mechanism. As written, two
> of the four fixed JITs ship with no test coverage.
> 
>> +__stderr("CPU: {{[0-9]+}} UID: 0 PID: {{[0-9]+}} Comm: {{.*}}")
>> +__stderr("Call trace:\n"
>> +"{{([a-zA-Z_][a-zA-Z0-9_]*\\+0x[0-9a-fA-F]+/0x[0-9a-fA-F]+\n"
>> +"|[ \t]+[^\n]+\n)*}}")
>> +int stream_arena_xchg_fault(void *ctx)
>> +{
>> +	static const struct bpf_insn xchg_insn = {
>> +		.code	 = 0xc3,	/* BPF_STX | BPF_ATOMIC | BPF_W */
>> +		.dst_reg = 1,		/* BPF_REG_1 */
>> +		.src_reg = 2,		/* BPF_REG_2 */
>> +		.off	 = 0x7fff,
>> +		.imm	 = 0xe1,	/* BPF_XCHG */
>> +	};
> 
> Does the misaligned .off = 0x7fff cause the test to validate a different
> fault path on arm64 than the commit message describes?
> 
> user_vm_start is page aligned, so the effective address is
> user_vm_start + 0x7fff, which is 3 mod 4. The 4-byte BPF_W access also
> straddles the page boundary at 0x8000.
> 
> For plain LDR/STR (stream_arena_read_fault, stream_arena_write_fault),
> this offset works as intended: arm64 permits unaligned access with
> SCTLR_EL1.A=0, so those really do take a translation fault on the unmapped
> page.
> 
> But atomics require natural alignment. arm64 LSE CAS/SWP raise an EL1
> Alignment fault (ESR FSC 0b100001) when the access is misaligned or crosses
> a 16-byte granule, and that takes priority over the translation fault.
> 
> The test still passes - fixup_exception() in arch/arm64/mm/fault.c runs
> before any die_kernel_fault path, so ex_handler_bpf() still clears the
> fetch register and still prints "Arena WRITE access at unmapped address".
> The fix under test is fault-type independent.
> 
> But the arm64 arm of both new subtests validates the alignment-fault path
> rather than the unmapped-arena-page path, while x86-64 (where page fault
> is delivered ahead of split-lock alignment check) validates the intended
> one, so the two architectures end up testing different code paths.
> 
> A 4-byte-aligned offset inside the unmapped page (e.g. 0x7ffc) would keep
> the fault a translation fault on both architectures. The pre-existing
> stream_arena_load_acquire_fault has the same property with its 0x7fff.

For the sake of what we test here, I'd leave as-is since the fault type
doesn't really matter here but rather the fact that we match on arena WRITE
plus the register clear.

> [ ... ]
> 
>> +	return val;
>> +}
>> +
>> +SEC("syscall")
>> +__arch_x86_64
>> +__arch_arm64
> 
> A subsystem pattern flags this as potentially concerning: On arm64 these
> two programs only load on CPUs that implement LSE atomics, and nothing in
> the test gates on that.
Hm, not relevant for upstream BPF CI given green as can be seen, but I can
look into that if desired.

Re: [PATCH bpf-next v3 6/6] selftests/bpf: Add arena fault tests for atomics with fetch

[Puranjay Mohan]

On Tue, Aug 11, 2026 at 2:16 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> Add stream_arena_xchg_fault and stream_arena_cmpxchg_fault next to the
> existing read, write and load-acquire fault tests, covering the two
> places a read-modify-write can deposit the old value: src_reg for a
> BPF_XCHG and r0 for a BPF_CMPXCHG. Both cover both halves of the JIT
> bug that left the fetch destination alone when a RMW on an arena pointer
> faulted:
>
>   - the fault has to be reported as a WRITE, and at the address held by
>     the destination register, which __stderr() and test_address() check
>   - the register receiving the fetched value has to be cleared by the
>     fault handler, which the programs check by poisoning it before the
>     atomic and returning it, so __retval(0) fails if it is left untouched
>
> The __stderr() annotation can only wildcard the faulting address since
> the arena base is not known until runtime, hence the two test_address()
> subtests on top, which pin it to the address held by dst_reg rather than
> src_reg.
>
> Note, the atomics are open coded since linux/filter.h cannot be included
> alongside vmlinux.h.
>
>   # LDLIBS=-static PKG_CONFIG='pkg-config --static' ./vmtest.sh -- ./test_progs -t stream
>   [...]
>   #464/1   stream_arena_fault_address/read_fault:OK
>   #464/2   stream_arena_fault_address/write_fault:OK
>   #464/3   stream_arena_fault_address/load_acquire_fault:OK
>   #464/4   stream_arena_fault_address/xchg_fault:OK
>   #464/5   stream_arena_fault_address/cmpxchg_fault:OK
>   #464     stream_arena_fault_address:OK
>   [...]
>   #466/5   stream_success/stream_arena_write_fault:OK
>   #466/6   stream_success/stream_arena_read_fault:OK
>   #466/7   stream_success/stream_arena_load_acquire_fault:OK
>   #466/8   stream_success/stream_arena_xchg_fault:OK
>   #466/9   stream_success/stream_arena_cmpxchg_fault:OK
>   [...]
>   Summary: 4/22 PASSED, 0 SKIPPED, 0 FAILED
>
> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
> Acked-by: Eduard Zingerman <eddyz87@gmail.com>
> ---
>  v1 -> v2:
>    - Updated commit msg wrt test_address structuring
>
>  .../testing/selftests/bpf/prog_tests/stream.c |   4 +
>  tools/testing/selftests/bpf/progs/stream.c    | 101 ++++++++++++++++++
>  2 files changed, 105 insertions(+)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/stream.c b/tools/testing/selftests/bpf/prog_tests/stream.c
> index 15dd3ae2a84b..e4e9374309e2 100644
> --- a/tools/testing/selftests/bpf/prog_tests/stream.c
> +++ b/tools/testing/selftests/bpf/prog_tests/stream.c
> @@ -105,6 +105,10 @@ void test_stream_arena_fault_address(void)
>                 test_address(skel->progs.stream_arena_write_fault, &skel->bss->fault_addr);
>         if (test__start_subtest("load_acquire_fault"))
>                 test_address(skel->progs.stream_arena_load_acquire_fault, &skel->bss->fault_addr);
> +       if (test__start_subtest("xchg_fault"))
> +               test_address(skel->progs.stream_arena_xchg_fault, &skel->bss->fault_addr);
> +       if (test__start_subtest("cmpxchg_fault"))
> +               test_address(skel->progs.stream_arena_cmpxchg_fault, &skel->bss->fault_addr);
>
>         stream__destroy(skel);
>  }
> diff --git a/tools/testing/selftests/bpf/progs/stream.c b/tools/testing/selftests/bpf/progs/stream.c
> index cf5533e11f39..00a37933e411 100644
> --- a/tools/testing/selftests/bpf/progs/stream.c
> +++ b/tools/testing/selftests/bpf/progs/stream.c
> @@ -229,6 +229,107 @@ int stream_arena_load_acquire_fault(void *ctx)
>         return val;
>  }
>
> +SEC("syscall")
> +__arch_x86_64
> +__arch_arm64
> +__success __retval(0)
> +__stderr("ERROR: Arena WRITE access at unmapped address 0x{{.*}}")
> +__stderr("CPU: {{[0-9]+}} UID: 0 PID: {{[0-9]+}} Comm: {{.*}}")
> +__stderr("Call trace:\n"
> +"{{([a-zA-Z_][a-zA-Z0-9_]*\\+0x[0-9a-fA-F]+/0x[0-9a-fA-F]+\n"
> +"|[ \t]+[^\n]+\n)*}}")
> +int stream_arena_xchg_fault(void *ctx)
> +{
> +       static const struct bpf_insn xchg_insn = {
> +               .code    = 0xc3,        /* BPF_STX | BPF_ATOMIC | BPF_W */
> +               .dst_reg = 1,           /* BPF_REG_1 */
> +               .src_reg = 2,           /* BPF_REG_2 */
> +               .off     = 0x7fff,
> +               .imm     = 0xe1,        /* BPF_XCHG */
> +       };
> +       struct bpf_arena *ptr = (void *)&arena;
> +       u64 user_vm_start, val;
> +
> +       /*
> +        * Prevent GCC bounds warning: casting &arena to struct bpf_arena *
> +        * triggers bounds checking since the map definition is smaller than
> +        * struct bpf_arena. barrier_var() makes the pointer opaque to GCC,
> +        * preventing the bounds analysis.
> +        */
> +       barrier_var(ptr);
> +       user_vm_start = ptr->user_vm_start;
> +       fault_addr = user_vm_start + 0x7fff;
> +       bpf_addr_space_cast(user_vm_start, 0, 1);
> +       /*
> +        * A read-modify-write carrying BPF_FETCH writes to memory, so the fault
> +        * has to be reported as a WRITE from the dst_reg address, but it also
> +        * reads the old value into src_reg, so the exception handler has to
> +        * clear src_reg. Poison it up front, the returned value must be 0.
> +        */
> +       asm volatile (
> +               "r1 = %[user_vm_start];"
> +               "r2 = 1;"
> +               ".8byte %[xchg_insn];" /* r2 = xchg((u32 *)(r1 + 0x7fff), r2) */
> +               "%[val] = r2;"
> +               : [val] "=r" (val)
> +               : [user_vm_start] "r" (user_vm_start),
> +                 __imm_insn(xchg_insn, xchg_insn)
> +               : "r1", "r2"
> +       );
> +       return val;
> +}
> +
> +SEC("syscall")
> +__arch_x86_64
> +__arch_arm64
> +__success __retval(0)
> +__stderr("ERROR: Arena WRITE access at unmapped address 0x{{.*}}")
> +__stderr("CPU: {{[0-9]+}} UID: 0 PID: {{[0-9]+}} Comm: {{.*}}")
> +__stderr("Call trace:\n"
> +"{{([a-zA-Z_][a-zA-Z0-9_]*\\+0x[0-9a-fA-F]+/0x[0-9a-fA-F]+\n"
> +"|[ \t]+[^\n]+\n)*}}")
> +int stream_arena_cmpxchg_fault(void *ctx)
> +{
> +       static const struct bpf_insn cmpxchg_insn = {
> +               .code    = 0xc3,        /* BPF_STX | BPF_ATOMIC | BPF_W */
> +               .dst_reg = 1,           /* BPF_REG_1 */
> +               .src_reg = 2,           /* BPF_REG_2 */
> +               .off     = 0x7fff,
> +               .imm     = 0xf1,        /* BPF_CMPXCHG */
> +       };
> +       struct bpf_arena *ptr = (void *)&arena;
> +       u64 user_vm_start, val;
> +
> +       /*
> +        * Prevent GCC bounds warning: casting &arena to struct bpf_arena *
> +        * triggers bounds checking since the map definition is smaller than
> +        * struct bpf_arena. barrier_var() makes the pointer opaque to GCC,
> +        * preventing the bounds analysis.
> +        */
> +       barrier_var(ptr);
> +       user_vm_start = ptr->user_vm_start;
> +       fault_addr = user_vm_start + 0x7fff;
> +       bpf_addr_space_cast(user_vm_start, 0, 1);
> +       /*
> +        * Same as the exchange above, except that a BPF_CMPXCHG reads the old
> +        * value into r0 rather than into src_reg, so r0 is the register the
> +        * exception handler has to clear. It doubles as the compare value, but
> +        * the comparison never happens since the access faults first.
> +        */
> +       asm volatile (
> +               "r1 = %[user_vm_start];"
> +               "r0 = 1;"
> +               "r2 = 2;"
> +               ".8byte %[cmpxchg_insn];" /* r0 = cmpxchg((u32 *)(r1 + 0x7fff), r0, r2) */
> +               "%[val] = r0;"
> +               : [val] "=r" (val)
> +               : [user_vm_start] "r" (user_vm_start),
> +                 __imm_insn(cmpxchg_insn, cmpxchg_insn)
> +               : "r0", "r1", "r2"
> +       );
> +       return val;
> +}
> +
>  static __noinline void subprog(void)
>  {
>         int __arena *addr = (int __arena *)0xdeadbeef;
> --
> 2.43.0
>



Acked-by: Puranjay Mohan <puranjay@kernel.org>

Thanks,
Puranjay

Re: [PATCH bpf-next v3 1/6] bpf: Derive the atomic load register in one place

[Eduard Zingerman]

On Tue, 2026-08-11 at 15:15 +0200, Daniel Borkmann wrote:
> check_atomic_rmw() open codes the mapping from a BPF_ATOMIC to the register
> it reads the old value into, the BPF_STX case of insn_def_regno() open codes
> the very same mapping a second time, the const folding and the liveness
> transfer functions a third and a fourth time, and BPF JITs need it as well
> to know which register a faulting BPF_PROBE_ATOMIC has to clear.
> 
> Add a small helper so that all of them can share it. No functional change.
> The BPF_LOAD_ACQ case is there for the JITs, which do walk all instruction
> classes. const_reg_xfer() loses its explicit BPF_ATOMIC mode test since the
> helper checks class and mode itself; the BPF_PROBE_ATOMIC it additionally
> accepts cannot be seen there as it is only set from bpf_do_misc_fixups(),
> that is, after const folding has run. arg_track_xfer() keeps its mode test
> since that also guards the stack clearing next to it.
> 
> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
> ---
>   v2 -> v3:
>    - Also use the helper for const_fold and liveness (bpf ci)
>    - Drop the return value comment in verifier since the helper
>      comment already covers it (Jakub)
>   v1 -> v2:
>    - also convert insn_def_regno (Eduard, sashiko)
> 
>  include/linux/filter.h  | 24 ++++++++++++++++++++++++
>  kernel/bpf/const_fold.c | 11 +++--------
>  kernel/bpf/fixups.c     | 11 +----------
>  kernel/bpf/liveness.c   |  9 +++------
>  kernel/bpf/verifier.c   | 13 ++-----------
>  5 files changed, 33 insertions(+), 35 deletions(-)

Acked-by: Eduard Zingerman <eddyz87@gmail.com>

...

Re: [PATCH bpf-next v3 5/6] bpf, s390: Clear fetch destination on faulting arena atomic

[Ilya Leoshkevich]

On 8/11/26 15:15, Daniel Borkmann wrote:
> Same missing register clear as on riscv64. A RMW atomic on an arena pointer
> is converted to BPF_PROBE_ATOMIC and gets an exception table entry, but
> bpf_jit_probe_atomic_pre() only fills in the arena base and the probe
> offset, leaving probe->reg at the -1 that bpf_jit_probe_init() set, which
> bpf_jit_probe_post() writes into the entry and ex_handler_bpf() then reads
> back as "there is nothing to clear".
> 
> That is right for a plain BPF_{ADD,AND,OR,XOR}, which only writes memory,
> but an RMW carrying BPF_FETCH also reads the old value into a register:
> src_reg for BPF_{ADD,AND,OR,XOR} | BPF_FETCH and BPF_XCHG, and r0 for
> BPF_CMPXCHG. So on a fault over an unmapped arena page the program resumes
> at the landing pad with whatever that register held before the atomic
> instead of the 0 that every other BPF_PROBE_* access delivers.
> 
> Fill probe->reg in from bpf_atomic_load_reg(). Unlike x86-64 and arm64,
> s390x does not report arena violations from its exception handler, so there
> is no access direction to correct here, only the missing register clear.
> 
> Fixes: 2f9469484a3b ("s390/bpf: Support arena atomics")
> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
> Cc: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>   arch/s390/net/bpf_jit_comp.c | 11 +++++++++++
>   1 file changed, 11 insertions(+)

Looks reasonable, thanks!

Reviewed-by: Ilya Leoshkevich <iii@linux.ibm.com>

Re: [PATCH bpf-next v3 4/6] bpf, arm64: Clear fetch destination on faulting arena atomic

[Puranjay Mohan]

On Tue, Aug 11, 2026 at 2:16 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> Same problem as on x86-64: add_exception_handler() folds "there is no
> destination register to clear" and "this is a store" into one DONT_CLEAR
> value ...
>
>   if (BPF_CLASS(insn->code) != BPF_LDX && !bpf_atomic_is_load_acq(insn))
>           dst_reg = DONT_CLEAR;
>
> ... which ex_handler_bpf() then reads back as the access direction:
>
>   bool is_write = (dst_reg == DONT_CLEAR);
>
> A RMW carrying BPF_FETCH is both. emit_lse_atomic() reads the old value
> into src_reg for BPF_{ADD,AND,OR,XOR} | BPF_FETCH and BPF_XCHG, and into
> r0 for BPF_CMPXCHG, so a fault over an unmapped arena page is correctly
> reported as a WRITE but leaves that register holding a stale value instead
> of the 0 that every other BPF_PROBE_* access delivers. Same as on x86-64,
> add a separate ARENA_WRITE bit for the direction.
>
> FIXUP_REG is now filled in by the callers of add_exception_handler(), the
> BPF_PROBE_ATOMIC one deriving it from bpf_atomic_load_reg(), so that the
> helper only has to determine the direction. This is how the riscv64 JIT
> already does it, and it stops the two store callers from handing in a
> dst_reg that was only going to be overwritten with DONT_CLEAR anyway.
>
> Fixes: e612b5c1d3ee ("bpf, arm64: Add support for lse atomics in bpf_arena")
> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
> Cc: Puranjay Mohan <puranjay@kernel.org>
> ---
>  v1 -> v2:
>    - move bpf_atomic_load_reg into build_insn (Eduard)
>
>  arch/arm64/net/bpf_jit_comp.c | 44 ++++++++++++++++++++++++-----------
>  1 file changed, 30 insertions(+), 14 deletions(-)
>
> diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
> index d14d297ebb96..74b4083791da 100644
> --- a/arch/arm64/net/bpf_jit_comp.c
> +++ b/arch/arm64/net/bpf_jit_comp.c
> @@ -1082,23 +1082,27 @@ static void build_epilogue(struct jit_ctx *ctx, bool was_classic)
>   *
>   * Bit layout of `fixup` (32-bit):
>   *
> - * +-----------+--------+-----------+-----------+----------+
> - * |   31-27   | 26-22  |     21    |   20-16   |   15-0   |
> - * |           |        |           |           |          |
> - * | FIXUP_REG | Unused | ARENA_ACC | ARENA_REG |  OFFSET  |
> - * +-----------+--------+-----------+-----------+----------+
> + * +-----------+--------+-------------+-----------+-----------+----------+
> + * |   31-27   | 26-23  |      22     |     21    |   20-16   |   15-0   |
> + * |           |        |             |           |           |          |
> + * | FIXUP_REG | Unused | ARENA_WRITE | ARENA_ACC | ARENA_REG |  OFFSET  |
> + * +-----------+--------+-------------+-----------+-----------+----------+
>   *
>   * - OFFSET (16 bits): Offset used to compute address for Load/Store instruction.
>   * - ARENA_REG (5 bits): Register that is used to calculate the address for load/store when
>   *                       accessing the arena region.
>   * - ARENA_ACCESS (1 bit): This bit is set when the faulting instruction accessed the arena region.
> + * - ARENA_WRITE (1 bit): This bit is set when the faulting instruction wrote to the arena region.
> + *                        It is independent of FIXUP_REG, since a read-modify-write both writes to
> + *                        memory and reads the old value into a register.
>   * - FIXUP_REG (5 bits): Destination register for the load instruction (cleared on fault) or set to
> - *                       DONT_CLEAR if it is a store instruction.
> + *                       DONT_CLEAR if the instruction does not read into a register.
>   */
>
>  #define BPF_FIXUP_OFFSET_MASK      GENMASK(15, 0)
>  #define BPF_FIXUP_ARENA_REG_MASK   GENMASK(20, 16)
>  #define BPF_ARENA_ACCESS           BIT(21)
> +#define BPF_ARENA_WRITE            BIT(22)
>  #define BPF_FIXUP_REG_MASK     GENMASK(31, 27)
>  #define DONT_CLEAR 5 /* Unused ARM64 register from BPF's POV */
>
> @@ -1109,7 +1113,7 @@ bool ex_handler_bpf(const struct exception_table_entry *ex,
>         s16 off = FIELD_GET(BPF_FIXUP_OFFSET_MASK, ex->fixup);
>         int arena_reg = FIELD_GET(BPF_FIXUP_ARENA_REG_MASK, ex->fixup);
>         bool is_arena = !!(ex->fixup & BPF_ARENA_ACCESS);
> -       bool is_write = (dst_reg == DONT_CLEAR);
> +       bool is_write = !!(ex->fixup & BPF_ARENA_WRITE);
>         unsigned long addr;
>
>         if (is_arena) {
> @@ -1132,7 +1136,7 @@ static int add_exception_handler(const struct bpf_insn *insn,
>  {
>         off_t ins_offset;
>         s16 off = insn->off;
> -       bool is_arena;
> +       bool is_arena, is_write;
>         int arena_reg;
>         unsigned long pc;
>         struct exception_table_entry *ex;
> @@ -1181,15 +1185,18 @@ static int add_exception_handler(const struct bpf_insn *insn,
>         /*
>          * A load-acquire is of BPF_STX class, but reads from src_reg into
>          * dst_reg like a BPF_LDX does, hence it must not be treated as a store
> -        * here.
> +        * here. A read-modify-write carrying BPF_FETCH is reported as a write
> +        * even though it does have a register to clear, see the callers.
>          */
> -       if (BPF_CLASS(insn->code) != BPF_LDX && !bpf_atomic_is_load_acq(insn))
> -               dst_reg = DONT_CLEAR;
> +       is_write = BPF_CLASS(insn->code) != BPF_LDX &&
> +                  !bpf_atomic_is_load_acq(insn);
>
>         ex->fixup = FIELD_PREP(BPF_FIXUP_REG_MASK, dst_reg);
>
>         if (is_arena) {
>                 ex->fixup |= BPF_ARENA_ACCESS;
> +               if (is_write)
> +                       ex->fixup |= BPF_ARENA_WRITE;
>                 /*
>                  * insn->src_reg/dst_reg holds the address in the arena region with upper 32-bits
>                  * being zero because of a preceding addr_space_cast(r<n>, 0x0, 0x1) instruction.
> @@ -1889,7 +1896,7 @@ static int build_insn(const struct bpf_verifier_env *env, const struct bpf_insn
>                         break;
>                 }
>
> -               ret = add_exception_handler(insn, ctx, dst);
> +               ret = add_exception_handler(insn, ctx, DONT_CLEAR);
>                 if (ret)
>                         return ret;
>                 break;
> @@ -1956,7 +1963,7 @@ static int build_insn(const struct bpf_verifier_env *env, const struct bpf_insn
>                         break;
>                 }
>
> -               ret = add_exception_handler(insn, ctx, dst);
> +               ret = add_exception_handler(insn, ctx, DONT_CLEAR);
>                 if (ret)
>                         return ret;
>                 break;
> @@ -1979,7 +1986,16 @@ static int build_insn(const struct bpf_verifier_env *env, const struct bpf_insn
>                         return ret;
>
>                 if (BPF_MODE(insn->code) == BPF_PROBE_ATOMIC) {
> -                       ret = add_exception_handler(insn, ctx, dst);
> +                       /*
> +                        * A load-acquire reads into dst_reg, and a read-modify-write
> +                        * carrying BPF_FETCH reads the old value into src_reg, or into
> +                        * r0 for a BPF_CMPXCHG. Clear that register on fault, the
> +                        * remaining atomics have no destination register.
> +                        */
> +                       int load_reg = bpf_atomic_load_reg(insn);
> +
> +                       ret = add_exception_handler(insn, ctx, load_reg < 0 ?
> +                                                   DONT_CLEAR : bpf2a64[load_reg]);
>                         if (ret)
>                                 return ret;
>                 }
> --
> 2.43.0
>

Reviewed-by: Puranjay Mohan <puranjay@kernel.org>