[PATCH RFC bpf-next v1 0/4] Introduce load-acquire and store-release BPF instructions

[PATCH RFC bpf-next v1 0/4] Introduce load-acquire and store-release BPF instructions

[Peilin Ye]

Hi all!

This RFC patchset adds kernel support for BPF load-acquire and store-release
instructions (for background, please see [1]).  Currently only arm64 is
supported for RFC.  The corresponding LLVM changes can be found at:
  https://github.com/llvm/llvm-project/pull/108636

As discussed on GitHub [2], define both load-acquire and store-release as
BPF_STX | BPF_ATOMIC instructions.  The following new flags are introduced:

  BPF_ATOMIC_LOAD    0x10
  BPF_ATOMIC_STORE   0x20

  BPF_RELAXED        0x0
  BPF_ACQUIRE        0x1
  BPF_RELEASE        0x2
  BPF_ACQ_REL        0x3
  BPF_SEQ_CST        0x4

  BPF_LOAD_ACQ       (BPF_ATOMIC_LOAD | BPF_ACQUIRE)
  BPF_STORE_REL      (BPF_ATOMIC_STORE | BPF_RELEASE)

Bit 4-7 of 'imm' encodes the new atomic operations (load and store), and bit
0-3 specifies the memory order.  A load-acquire is a BPF_STX | BPF_ATOMIC
instruction with 'imm' set to BPF_LOAD_ACQ (0x11).  Similarly, a store-release
is a BPF_STX | BPF_ATOMIC instruction with 'imm' set to BPF_STORE_REL (0x22).

For bit 4-7 of 'imm' we need to avoid conflicts with existing
BPF_STX | BPF_ATOMIC instructions.  Currently the following values (a subset
of BPFArithOp<>) are in use:

  def BPF_ADD  : BPFArithOp<0x0>;
  def BPF_OR   : BPFArithOp<0x4>;
  def BPF_AND  : BPFArithOp<0x5>;
  def BPF_XOR  : BPFArithOp<0xa>;
  def BPF_XCHG    : BPFArithOp<0xe>;
  def BPF_CMPXCHG : BPFArithOp<0xf>;

0x1 and 0x2 were chosen for the new instructions because:

  * BPFArithOp<0x1> is BPF_SUB.  Compilers already handle atomic subtraction
    by generating a BPF NEG followed by a BPF ADD instruction.

  * BPFArithOp<0x2> is BPF_MUL, and we do not have a plan for adding BPF
    atomic multiplication instructions.

So we think by choosing 0x1 and 0x2, we can avoid having conflicts with
BPFArithOp<> in the future.  Previously 0xb was chosen because we will never
need BPF_MOV (BPFArithOp<0xb>) for BPF_ATOMIC.  Please suggest if you think
different values should be used.

Based on [3], the BPF load-acquire, the arm64 JIT compiler generates LDAR
(RCsc) instead of LDAPR (RCpc).  Will Deacon also suggested LDAR over LDAPR in
an offlist conversation for the following reasons:

  a. Not all CPUs support LDAPR, as also pointed out in Paul E. McKenney's
     email (search for "older ARM64 hardware" in [3]).

  b. The extra ordering provided by RCsc is important in some use cases e.g.
     locks.

  c. The arm64 ISA does not provide e.g. other atomic memory operations in
     RCpc.  In other words, it is not worth losing the extra ordering that
     LDAR provides, if we would still be using RCsc for all other cases.

Unlike existing atomic operations that only support BPF_W (32-bit) and
BPF_DW (64-bit) size modifiers, load-acquires and store-releases also
support BPF_B (8-bit) and BPF_H (16-bit).  An 8- or 16-bit load-acquire
zero-extends the value before writing it to a 32-bit register, just like
LDARH and friends.

Examples of using the new instructions (assuming little-endian):

  long foo(long *ptr) {
      return __atomic_load_n(ptr, __ATOMIC_ACQUIRE);
  }

Using clang -mcpu=v4, foo() can be compiled to:

  db 10 00 00 11 00 00 00  r0 = load_acquire((u64 *)(r1 + 0x0))
  95 00 00 00 00 00 00 00  exit

  opcode (0xdb): BPF_ATOMIC | BPF_DW | BPF_STX
  imm (0x00000011): BPF_LOAD_ACQ

For arm64, an LDAR instruction would be generated by the JIT compiler for
the above, e.g.:

  ldar  x7, [x0]

Similarly, consider this 16-bit store-release:

  void bar(short *ptr, short val) {
      __atomic_store_n(ptr, val, __ATOMIC_RELEASE);
  }

bar() can be compiled to (again, using clang -mcpu=v4):

  cb 21 00 00 22 00 00 00  store_release((u16 *)(r1 + 0x0), w2)
  95 00 00 00 00 00 00 00  exit

  opcode (0xcb): BPF_ATOMIC | BPF_H | BPF_STX
  imm (0x00000022): BPF_ATOMIC_STORE | BPF_RELEASE

An STLRH will be generated for it, e.g.:

  stlrh  w1, [x0]

For a complete mapping for ARM64:

  load-acquire     8-bit  LDARB
 (BPF_LOAD_ACQ)   16-bit  LDARH
                  32-bit  LDAR (32-bit)
                  64-bit  LDAR (64-bit)
  store-release    8-bit  STLRB
 (BPF_STORE_REL)  16-bit  STLRH
                  32-bit  STLR (32-bit)
                  64-bit  STLR (64-bit)

Using in arena is supported.  Inline assembly is also supported.  For example:

  asm volatile("%0 = load_acquire((u64 *)(%1 + 0x0))" :
               "=r"(ret) : "r"(ptr) : "memory");

A new pre-defined macro, __BPF_FEATURE_LOAD_ACQ_STORE_REL, can be used to
detect if clang supports BPF load-acquire and store-release.

Please refer to individual kernel patches (and LLVM commits) for details.
Any suggestions or corrections would be much appreciated!

[1] https://lore.kernel.org/all/20240729183246.4110549-1-yepeilin@google.com/
[2] https://github.com/llvm/llvm-project/pull/108636#issuecomment-2389403477
[3] https://lore.kernel.org/bpf/75d1352e-c05e-4fdf-96bf-b1c3daaf41f0@paulmck-laptop/

Thanks,
Peilin Ye (4):
  bpf/verifier: Factor out check_load()
  bpf: Introduce load-acquire and store-release instructions
  selftests/bpf: Delete duplicate verifier/atomic_invalid tests
  selftests/bpf: Add selftests for load-acquire and store-release
    instructions

 arch/arm64/include/asm/insn.h                 |  8 ++
 arch/arm64/lib/insn.c                         | 34 +++++++
 arch/arm64/net/bpf_jit.h                      | 20 +++++
 arch/arm64/net/bpf_jit_comp.c                 | 85 +++++++++++++++++-
 include/linux/filter.h                        |  2 +
 include/uapi/linux/bpf.h                      | 13 +++
 kernel/bpf/core.c                             | 41 ++++++++-
 kernel/bpf/disasm.c                           | 14 +++
 kernel/bpf/verifier.c                         | 88 ++++++++++++-------
 tools/include/uapi/linux/bpf.h                | 13 +++
 .../selftests/bpf/prog_tests/arena_atomics.c  | 61 ++++++++++++-
 .../selftests/bpf/prog_tests/atomics.c        | 57 +++++++++++-
 .../selftests/bpf/progs/arena_atomics.c       | 62 ++++++++++++-
 tools/testing/selftests/bpf/progs/atomics.c   | 62 ++++++++++++-
 .../selftests/bpf/verifier/atomic_invalid.c   | 28 +++---
 .../selftests/bpf/verifier/atomic_load.c      | 71 +++++++++++++++
 .../selftests/bpf/verifier/atomic_store.c     | 70 +++++++++++++++
 17 files changed, 672 insertions(+), 57 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/verifier/atomic_load.c
 create mode 100644 tools/testing/selftests/bpf/verifier/atomic_store.c

-- 
2.47.1.613.gc27f4b7a9f-goog

[PATCH RFC bpf-next v1 1/4] bpf/verifier: Factor out check_load()

[Peilin Ye]

No functional changes intended.  While we are here, make that comment
about "reserved fields" more specific.

Reviewed-by: Josh Don <joshdon@google.com>
Signed-off-by: Peilin Ye <yepeilin@google.com>
---
 kernel/bpf/verifier.c | 56 +++++++++++++++++++++++++------------------
 1 file changed, 33 insertions(+), 23 deletions(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index f27274e933e5..fa40a0440590 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -7518,6 +7518,36 @@ static int check_mem_access(struct bpf_verifier_env *env, int insn_idx, u32 regn
 static int save_aux_ptr_type(struct bpf_verifier_env *env, enum bpf_reg_type type,
 			     bool allow_trust_mismatch);
 
+static int check_load(struct bpf_verifier_env *env, struct bpf_insn *insn, const char *ctx)
+{
+	struct bpf_reg_state *regs = cur_regs(env);
+	enum bpf_reg_type src_reg_type;
+	int err;
+
+	/* check src operand */
+	err = check_reg_arg(env, insn->src_reg, SRC_OP);
+	if (err)
+		return err;
+
+	err = check_reg_arg(env, insn->dst_reg, DST_OP_NO_MARK);
+	if (err)
+		return err;
+
+	src_reg_type = regs[insn->src_reg].type;
+
+	/* check that memory (src_reg + off) is readable,
+	 * the state of dst_reg will be updated by this func
+	 */
+	err = check_mem_access(env, env->insn_idx, insn->src_reg,
+			       insn->off, BPF_SIZE(insn->code),
+			       BPF_READ, insn->dst_reg, false,
+			       BPF_MODE(insn->code) == BPF_MEMSX);
+	err = err ?: save_aux_ptr_type(env, src_reg_type, true);
+	err = err ?: reg_bounds_sanity_check(env, &regs[insn->dst_reg], ctx);
+
+	return err;
+}
+
 static int check_atomic(struct bpf_verifier_env *env, int insn_idx, struct bpf_insn *insn)
 {
 	int load_reg;
@@ -18945,30 +18975,10 @@ static int do_check(struct bpf_verifier_env *env)
 				return err;
 
 		} else if (class == BPF_LDX) {
-			enum bpf_reg_type src_reg_type;
-
-			/* check for reserved fields is already done */
-
-			/* check src operand */
-			err = check_reg_arg(env, insn->src_reg, SRC_OP);
-			if (err)
-				return err;
-
-			err = check_reg_arg(env, insn->dst_reg, DST_OP_NO_MARK);
-			if (err)
-				return err;
-
-			src_reg_type = regs[insn->src_reg].type;
-
-			/* check that memory (src_reg + off) is readable,
-			 * the state of dst_reg will be updated by this func
+			/* Check for reserved fields is already done in
+			 * resolve_pseudo_ldimm64().
 			 */
-			err = check_mem_access(env, env->insn_idx, insn->src_reg,
-					       insn->off, BPF_SIZE(insn->code),
-					       BPF_READ, insn->dst_reg, false,
-					       BPF_MODE(insn->code) == BPF_MEMSX);
-			err = err ?: save_aux_ptr_type(env, src_reg_type, true);
-			err = err ?: reg_bounds_sanity_check(env, &regs[insn->dst_reg], "ldx");
+			err = check_load(env, insn, "ldx");
 			if (err)
 				return err;
 		} else if (class == BPF_STX) {
-- 
2.47.1.613.gc27f4b7a9f-goog

[PATCH RFC bpf-next v1 2/4] bpf: Introduce load-acquire and store-release instructions

[Peilin Ye]

Introduce BPF instructions with load-acquire and store-release
semantics, as discussed in [1].  The following new flags are defined:

  BPF_ATOMIC_LOAD         0x10
  BPF_ATOMIC_STORE        0x20
  BPF_ATOMIC_TYPE(imm)    ((imm) & 0xf0)

  BPF_RELAXED        0x0
  BPF_ACQUIRE        0x1
  BPF_RELEASE        0x2
  BPF_ACQ_REL        0x3
  BPF_SEQ_CST        0x4

  BPF_LOAD_ACQ       (BPF_ATOMIC_LOAD | BPF_ACQUIRE)
  BPF_STORE_REL      (BPF_ATOMIC_STORE | BPF_RELEASE)

A "load-acquire" is a BPF_STX | BPF_ATOMIC instruction with the 'imm'
field set to BPF_LOAD_ACQ (0x11).

Similarly, a "store-release" is a BPF_STX | BPF_ATOMIC instruction with
the 'imm' field set to BPF_STORE_REL (0x22).

Unlike existing atomic operations that only support BPF_W (32-bit) and
BPF_DW (64-bit) size modifiers, load-acquires and store-releases also
support BPF_B (8-bit) and BPF_H (16-bit).  An 8- or 16-bit load-acquire
zero-extends the value before writing it to a 32-bit register, just like
ARM64 instruction LDARH and friends.

As an example, consider the following 64-bit load-acquire BPF
instruction (assuming little-endian from now on):

  db 10 00 00 11 00 00 00  r0 = load_acquire((u64 *)(r1 + 0x0))

  opcode (0xdb): BPF_ATOMIC | BPF_DW | BPF_STX
  imm (0x00000011): BPF_LOAD_ACQ

For ARM64, an LDAR instruction will be generated by the JIT compiler for
the above:

  ldar  x7, [x0]

Similarly, a 16-bit BPF store-release:

  cb 21 00 00 22 00 00 00  store_release((u16 *)(r1 + 0x0), w2)

  opcode (0xcb): BPF_ATOMIC | BPF_H | BPF_STX
  imm (0x00000022): BPF_STORE_REL

An STLRH will be generated for it:

  stlrh  w1, [x0]

For a complete mapping for ARM64:

  load-acquire     8-bit  LDARB
 (BPF_LOAD_ACQ)   16-bit  LDARH
                  32-bit  LDAR (32-bit)
                  64-bit  LDAR (64-bit)
  store-release    8-bit  STLRB
 (BPF_STORE_REL)  16-bit  STLRH
                  32-bit  STLR (32-bit)
                  64-bit  STLR (64-bit)

Reviewed-by: Josh Don <joshdon@google.com>
Reviewed-by: Barret Rhoden <brho@google.com>
Signed-off-by: Peilin Ye <yepeilin@google.com>
---
 arch/arm64/include/asm/insn.h  |  8 ++++
 arch/arm64/lib/insn.c          | 34 ++++++++++++++
 arch/arm64/net/bpf_jit.h       | 20 ++++++++
 arch/arm64/net/bpf_jit_comp.c  | 85 +++++++++++++++++++++++++++++++++-
 include/uapi/linux/bpf.h       | 13 ++++++
 kernel/bpf/core.c              | 41 +++++++++++++++-
 kernel/bpf/disasm.c            | 14 ++++++
 kernel/bpf/verifier.c          | 32 +++++++++----
 tools/include/uapi/linux/bpf.h | 13 ++++++
 9 files changed, 246 insertions(+), 14 deletions(-)

diff --git a/arch/arm64/include/asm/insn.h b/arch/arm64/include/asm/insn.h
index e390c432f546..bbfdbe570ff6 100644
--- a/arch/arm64/include/asm/insn.h
+++ b/arch/arm64/include/asm/insn.h
@@ -188,8 +188,10 @@ enum aarch64_insn_ldst_type {
 	AARCH64_INSN_LDST_STORE_PAIR_PRE_INDEX,
 	AARCH64_INSN_LDST_LOAD_PAIR_POST_INDEX,
 	AARCH64_INSN_LDST_STORE_PAIR_POST_INDEX,
+	AARCH64_INSN_LDST_LOAD_ACQ,
 	AARCH64_INSN_LDST_LOAD_EX,
 	AARCH64_INSN_LDST_LOAD_ACQ_EX,
+	AARCH64_INSN_LDST_STORE_REL,
 	AARCH64_INSN_LDST_STORE_EX,
 	AARCH64_INSN_LDST_STORE_REL_EX,
 	AARCH64_INSN_LDST_SIGNED_LOAD_IMM_OFFSET,
@@ -351,6 +353,8 @@ __AARCH64_INSN_FUNCS(ldr_imm,	0x3FC00000, 0x39400000)
 __AARCH64_INSN_FUNCS(ldr_lit,	0xBF000000, 0x18000000)
 __AARCH64_INSN_FUNCS(ldrsw_lit,	0xFF000000, 0x98000000)
 __AARCH64_INSN_FUNCS(exclusive,	0x3F800000, 0x08000000)
+__AARCH64_INSN_FUNCS(load_acq,  0x3FC08000, 0x08C08000)
+__AARCH64_INSN_FUNCS(store_rel, 0x3FC08000, 0x08808000)
 __AARCH64_INSN_FUNCS(load_ex,	0x3F400000, 0x08400000)
 __AARCH64_INSN_FUNCS(store_ex,	0x3F400000, 0x08000000)
 __AARCH64_INSN_FUNCS(mops,	0x3B200C00, 0x19000400)
@@ -602,6 +606,10 @@ u32 aarch64_insn_gen_load_store_pair(enum aarch64_insn_register reg1,
 				     int offset,
 				     enum aarch64_insn_variant variant,
 				     enum aarch64_insn_ldst_type type);
+u32 aarch64_insn_gen_load_acq_store_rel(enum aarch64_insn_register reg,
+					enum aarch64_insn_register base,
+					enum aarch64_insn_size_type size,
+					enum aarch64_insn_ldst_type type);
 u32 aarch64_insn_gen_load_store_ex(enum aarch64_insn_register reg,
 				   enum aarch64_insn_register base,
 				   enum aarch64_insn_register state,
diff --git a/arch/arm64/lib/insn.c b/arch/arm64/lib/insn.c
index b008a9b46a7f..80e5b191d96a 100644
--- a/arch/arm64/lib/insn.c
+++ b/arch/arm64/lib/insn.c
@@ -540,6 +540,40 @@ u32 aarch64_insn_gen_load_store_pair(enum aarch64_insn_register reg1,
 					     offset >> shift);
 }
 
+u32 aarch64_insn_gen_load_acq_store_rel(enum aarch64_insn_register reg,
+					enum aarch64_insn_register base,
+					enum aarch64_insn_size_type size,
+					enum aarch64_insn_ldst_type type)
+{
+	u32 insn;
+
+	switch (type) {
+	case AARCH64_INSN_LDST_LOAD_ACQ:
+		insn = aarch64_insn_get_load_acq_value();
+		break;
+	case AARCH64_INSN_LDST_STORE_REL:
+		insn = aarch64_insn_get_store_rel_value();
+		break;
+	default:
+		pr_err("%s: unknown load-acquire/store-release encoding %d\n", __func__, type);
+		return AARCH64_BREAK_FAULT;
+	}
+
+	insn = aarch64_insn_encode_ldst_size(size, insn);
+
+	insn = aarch64_insn_encode_register(AARCH64_INSN_REGTYPE_RT, insn,
+					    reg);
+
+	insn = aarch64_insn_encode_register(AARCH64_INSN_REGTYPE_RN, insn,
+					    base);
+
+	insn = aarch64_insn_encode_register(AARCH64_INSN_REGTYPE_RT2, insn,
+					    AARCH64_INSN_REG_ZR);
+
+	return aarch64_insn_encode_register(AARCH64_INSN_REGTYPE_RS, insn,
+					    AARCH64_INSN_REG_ZR);
+}
+
 u32 aarch64_insn_gen_load_store_ex(enum aarch64_insn_register reg,
 				   enum aarch64_insn_register base,
 				   enum aarch64_insn_register state,
diff --git a/arch/arm64/net/bpf_jit.h b/arch/arm64/net/bpf_jit.h
index b22ab2f97a30..a3b0e693a125 100644
--- a/arch/arm64/net/bpf_jit.h
+++ b/arch/arm64/net/bpf_jit.h
@@ -119,6 +119,26 @@
 	aarch64_insn_gen_load_store_ex(Rt, Rn, Rs, A64_SIZE(sf), \
 				       AARCH64_INSN_LDST_STORE_REL_EX)
 
+/* Load-acquire & store-release */
+#define A64_LDAR(Rt, Rn, size)  \
+	aarch64_insn_gen_load_acq_store_rel(Rt, Rn, AARCH64_INSN_SIZE_##size, \
+					    AARCH64_INSN_LDST_LOAD_ACQ)
+#define A64_STLR(Rt, Rn, size)  \
+	aarch64_insn_gen_load_acq_store_rel(Rt, Rn, AARCH64_INSN_SIZE_##size, \
+					    AARCH64_INSN_LDST_STORE_REL)
+
+/* Rt = [Rn] (load acquire) */
+#define A64_LDARB(Wt, Xn)	A64_LDAR(Wt, Xn, 8)
+#define A64_LDARH(Wt, Xn)	A64_LDAR(Wt, Xn, 16)
+#define A64_LDAR32(Wt, Xn)	A64_LDAR(Wt, Xn, 32)
+#define A64_LDAR64(Xt, Xn)	A64_LDAR(Xt, Xn, 64)
+
+/* [Rn] = Rt (store release) */
+#define A64_STLRB(Wt, Xn)	A64_STLR(Wt, Xn, 8)
+#define A64_STLRH(Wt, Xn)	A64_STLR(Wt, Xn, 16)
+#define A64_STLR32(Wt, Xn)	A64_STLR(Wt, Xn, 32)
+#define A64_STLR64(Xt, Xn)	A64_STLR(Xt, Xn, 64)
+
 /*
  * LSE atomics
  *
diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
index 66708b95493a..15fc0f391f14 100644
--- a/arch/arm64/net/bpf_jit_comp.c
+++ b/arch/arm64/net/bpf_jit_comp.c
@@ -634,6 +634,80 @@ static int emit_bpf_tail_call(struct jit_ctx *ctx)
 	return 0;
 }
 
+static inline bool is_atomic_load_store(const s32 imm)
+{
+	const s32 type = BPF_ATOMIC_TYPE(imm);
+
+	return type == BPF_ATOMIC_LOAD || type == BPF_ATOMIC_STORE;
+}
+
+static int emit_atomic_load_store(const struct bpf_insn *insn, struct jit_ctx *ctx)
+{
+	const s16 off = insn->off;
+	const u8 code = insn->code;
+	const bool arena = BPF_MODE(code) == BPF_PROBE_ATOMIC;
+	const u8 arena_vm_base = bpf2a64[ARENA_VM_START];
+	const u8 dst = bpf2a64[insn->dst_reg];
+	const u8 src = bpf2a64[insn->src_reg];
+	const u8 tmp = bpf2a64[TMP_REG_1];
+	u8 ptr;
+
+	if (BPF_ATOMIC_TYPE(insn->imm) == BPF_ATOMIC_LOAD)
+		ptr = src;
+	else
+		ptr = dst;
+
+	if (off) {
+		emit_a64_mov_i(true, tmp, off, ctx);
+		emit(A64_ADD(true, tmp, tmp, ptr), ctx);
+		ptr = tmp;
+	}
+	if (arena) {
+		emit(A64_ADD(true, tmp, ptr, arena_vm_base), ctx);
+		ptr = tmp;
+	}
+
+	switch (insn->imm) {
+	case BPF_LOAD_ACQ:
+		switch (BPF_SIZE(code)) {
+		case BPF_B:
+			emit(A64_LDARB(dst, ptr), ctx);
+			break;
+		case BPF_H:
+			emit(A64_LDARH(dst, ptr), ctx);
+			break;
+		case BPF_W:
+			emit(A64_LDAR32(dst, ptr), ctx);
+			break;
+		case BPF_DW:
+			emit(A64_LDAR64(dst, ptr), ctx);
+			break;
+		}
+		break;
+	case BPF_STORE_REL:
+		switch (BPF_SIZE(code)) {
+		case BPF_B:
+			emit(A64_STLRB(src, ptr), ctx);
+			break;
+		case BPF_H:
+			emit(A64_STLRH(src, ptr), ctx);
+			break;
+		case BPF_W:
+			emit(A64_STLR32(src, ptr), ctx);
+			break;
+		case BPF_DW:
+			emit(A64_STLR64(src, ptr), ctx);
+			break;
+		}
+		break;
+	default:
+		pr_err_once("unknown atomic load/store op code %02x\n", insn->imm);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 #ifdef CONFIG_ARM64_LSE_ATOMICS
 static int emit_lse_atomic(const struct bpf_insn *insn, struct jit_ctx *ctx)
 {
@@ -1641,11 +1715,17 @@ static int build_insn(const struct bpf_insn *insn, struct jit_ctx *ctx,
 			return ret;
 		break;
 
+	case BPF_STX | BPF_ATOMIC | BPF_B:
+	case BPF_STX | BPF_ATOMIC | BPF_H:
 	case BPF_STX | BPF_ATOMIC | BPF_W:
 	case BPF_STX | BPF_ATOMIC | BPF_DW:
+	case BPF_STX | BPF_PROBE_ATOMIC | BPF_B:
+	case BPF_STX | BPF_PROBE_ATOMIC | BPF_H:
 	case BPF_STX | BPF_PROBE_ATOMIC | BPF_W:
 	case BPF_STX | BPF_PROBE_ATOMIC | BPF_DW:
-		if (cpus_have_cap(ARM64_HAS_LSE_ATOMICS))
+		if (is_atomic_load_store(insn->imm))
+			ret = emit_atomic_load_store(insn, ctx);
+		else if (cpus_have_cap(ARM64_HAS_LSE_ATOMICS))
 			ret = emit_lse_atomic(insn, ctx);
 		else
 			ret = emit_ll_sc_atomic(insn, ctx);
@@ -2669,7 +2749,8 @@ bool bpf_jit_supports_insn(struct bpf_insn *insn, bool in_arena)
 	switch (insn->code) {
 	case BPF_STX | BPF_ATOMIC | BPF_W:
 	case BPF_STX | BPF_ATOMIC | BPF_DW:
-		if (!cpus_have_cap(ARM64_HAS_LSE_ATOMICS))
+		if (!is_atomic_load_store(insn->imm) &&
+		    !cpus_have_cap(ARM64_HAS_LSE_ATOMICS))
 			return false;
 	}
 	return true;
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 2acf9b336371..4a20a125eb46 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -51,6 +51,19 @@
 #define BPF_XCHG	(0xe0 | BPF_FETCH)	/* atomic exchange */
 #define BPF_CMPXCHG	(0xf0 | BPF_FETCH)	/* atomic compare-and-write */
 
+#define BPF_ATOMIC_LOAD		0x10
+#define BPF_ATOMIC_STORE	0x20
+#define BPF_ATOMIC_TYPE(imm)	((imm) & 0xf0)
+
+#define BPF_RELAXED	0x00
+#define BPF_ACQUIRE	0x01
+#define BPF_RELEASE	0x02
+#define BPF_ACQ_REL	0x03
+#define BPF_SEQ_CST	0x04
+
+#define BPF_LOAD_ACQ	(BPF_ATOMIC_LOAD | BPF_ACQUIRE)		/* load-acquire */
+#define BPF_STORE_REL	(BPF_ATOMIC_STORE | BPF_RELEASE)	/* store-release */
+
 enum bpf_cond_pseudo_jmp {
 	BPF_MAY_GOTO = 0,
 };
diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index da729cbbaeb9..ab082ab9d535 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -1663,14 +1663,17 @@ EXPORT_SYMBOL_GPL(__bpf_call_base);
 	INSN_3(JMP, JSET, K),			\
 	INSN_2(JMP, JA),			\
 	INSN_2(JMP32, JA),			\
+	/* Atomic operations. */		\
+	INSN_3(STX, ATOMIC, B),			\
+	INSN_3(STX, ATOMIC, H),			\
+	INSN_3(STX, ATOMIC, W),			\
+	INSN_3(STX, ATOMIC, DW),		\
 	/* Store instructions. */		\
 	/*   Register based. */			\
 	INSN_3(STX, MEM,  B),			\
 	INSN_3(STX, MEM,  H),			\
 	INSN_3(STX, MEM,  W),			\
 	INSN_3(STX, MEM,  DW),			\
-	INSN_3(STX, ATOMIC, W),			\
-	INSN_3(STX, ATOMIC, DW),		\
 	/*   Immediate based. */		\
 	INSN_3(ST, MEM, B),			\
 	INSN_3(ST, MEM, H),			\
@@ -2169,6 +2172,8 @@ static u64 ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn)
 
 	STX_ATOMIC_DW:
 	STX_ATOMIC_W:
+	STX_ATOMIC_H:
+	STX_ATOMIC_B:
 		switch (IMM) {
 		ATOMIC_ALU_OP(BPF_ADD, add)
 		ATOMIC_ALU_OP(BPF_AND, and)
@@ -2196,6 +2201,38 @@ static u64 ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn)
 					(atomic64_t *)(unsigned long) (DST + insn->off),
 					(u64) BPF_R0, (u64) SRC);
 			break;
+		case BPF_LOAD_ACQ:
+			switch (BPF_SIZE(insn->code)) {
+#define LOAD_ACQUIRE(SIZEOP, SIZE)				\
+			case BPF_##SIZEOP:			\
+				DST = (SIZE)smp_load_acquire(	\
+					(SIZE *)(unsigned long)(SRC + insn->off));	\
+				break;
+			LOAD_ACQUIRE(B,   u8)
+			LOAD_ACQUIRE(H,  u16)
+			LOAD_ACQUIRE(W,  u32)
+			LOAD_ACQUIRE(DW, u64)
+#undef LOAD_ACQUIRE
+			default:
+				goto default_label;
+			}
+			break;
+		case BPF_STORE_REL:
+			switch (BPF_SIZE(insn->code)) {
+#define STORE_RELEASE(SIZEOP, SIZE)			\
+			case BPF_##SIZEOP:		\
+				smp_store_release(	\
+					(SIZE *)(unsigned long)(DST + insn->off), (SIZE)SRC);	\
+				break;
+			STORE_RELEASE(B,   u8)
+			STORE_RELEASE(H,  u16)
+			STORE_RELEASE(W,  u32)
+			STORE_RELEASE(DW, u64)
+#undef STORE_RELEASE
+			default:
+				goto default_label;
+			}
+			break;
 
 		default:
 			goto default_label;
diff --git a/kernel/bpf/disasm.c b/kernel/bpf/disasm.c
index 309c4aa1b026..2a354a44f209 100644
--- a/kernel/bpf/disasm.c
+++ b/kernel/bpf/disasm.c
@@ -267,6 +267,20 @@ void print_bpf_insn(const struct bpf_insn_cbs *cbs,
 				BPF_SIZE(insn->code) == BPF_DW ? "64" : "",
 				bpf_ldst_string[BPF_SIZE(insn->code) >> 3],
 				insn->dst_reg, insn->off, insn->src_reg);
+		} else if (BPF_MODE(insn->code) == BPF_ATOMIC &&
+			   insn->imm == BPF_LOAD_ACQ) {
+			verbose(cbs->private_data, "(%02x) %s%d = load_acquire((%s *)(r%d %+d))\n",
+				insn->code,
+				BPF_SIZE(insn->code) == BPF_DW ? "r" : "w", insn->dst_reg,
+				bpf_ldst_string[BPF_SIZE(insn->code) >> 3],
+				insn->src_reg, insn->off);
+		} else if (BPF_MODE(insn->code) == BPF_ATOMIC &&
+			   insn->imm == BPF_STORE_REL) {
+			verbose(cbs->private_data, "(%02x) store_release((%s *)(r%d %+d), %s%d)\n",
+				insn->code,
+				bpf_ldst_string[BPF_SIZE(insn->code) >> 3],
+				insn->dst_reg, insn->off,
+				BPF_SIZE(insn->code) == BPF_DW ? "r" : "w", insn->src_reg);
 		} else {
 			verbose(cbs->private_data, "BUG_%02x\n", insn->code);
 		}
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index fa40a0440590..dc3ecc925b97 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -3480,7 +3480,7 @@ static bool is_reg64(struct bpf_verifier_env *env, struct bpf_insn *insn,
 	}
 
 	if (class == BPF_STX) {
-		/* BPF_STX (including atomic variants) has multiple source
+		/* BPF_STX (including atomic variants) has one or more source
 		 * operands, one of which is a ptr. Check whether the caller is
 		 * asking about it.
 		 */
@@ -7550,6 +7550,8 @@ static int check_load(struct bpf_verifier_env *env, struct bpf_insn *insn, const
 
 static int check_atomic(struct bpf_verifier_env *env, int insn_idx, struct bpf_insn *insn)
 {
+	const int bpf_size = BPF_SIZE(insn->code);
+	bool write_only = false;
 	int load_reg;
 	int err;
 
@@ -7564,17 +7566,21 @@ static int check_atomic(struct bpf_verifier_env *env, int insn_idx, struct bpf_i
 	case BPF_XOR | BPF_FETCH:
 	case BPF_XCHG:
 	case BPF_CMPXCHG:
+		if (bpf_size != BPF_W && bpf_size != BPF_DW) {
+			verbose(env, "invalid atomic operand size\n");
+			return -EINVAL;
+		}
+		break;
+	case BPF_LOAD_ACQ:
+		return check_load(env, insn, "atomic");
+	case BPF_STORE_REL:
+		write_only = true;
 		break;
 	default:
 		verbose(env, "BPF_ATOMIC uses invalid atomic opcode %02x\n", insn->imm);
 		return -EINVAL;
 	}
 
-	if (BPF_SIZE(insn->code) != BPF_W && BPF_SIZE(insn->code) != BPF_DW) {
-		verbose(env, "invalid atomic operand size\n");
-		return -EINVAL;
-	}
-
 	/* check src1 operand */
 	err = check_reg_arg(env, insn->src_reg, SRC_OP);
 	if (err)
@@ -7615,6 +7621,9 @@ static int check_atomic(struct bpf_verifier_env *env, int insn_idx, struct bpf_i
 		return -EACCES;
 	}
 
+	if (write_only)
+		goto skip_read_check;
+
 	if (insn->imm & BPF_FETCH) {
 		if (insn->imm == BPF_CMPXCHG)
 			load_reg = BPF_REG_0;
@@ -7636,14 +7645,15 @@ static int check_atomic(struct bpf_verifier_env *env, int insn_idx, struct bpf_i
 	 * case to simulate the register fill.
 	 */
 	err = check_mem_access(env, insn_idx, insn->dst_reg, insn->off,
-			       BPF_SIZE(insn->code), BPF_READ, -1, true, false);
+			       bpf_size, BPF_READ, -1, true, false);
 	if (!err && load_reg >= 0)
 		err = check_mem_access(env, insn_idx, insn->dst_reg, insn->off,
-				       BPF_SIZE(insn->code), BPF_READ, load_reg,
-				       true, false);
+				       bpf_size, BPF_READ, load_reg, true,
+				       false);
 	if (err)
 		return err;
 
+skip_read_check:
 	if (is_arena_reg(env, insn->dst_reg)) {
 		err = save_aux_ptr_type(env, PTR_TO_ARENA, false);
 		if (err)
@@ -20320,7 +20330,9 @@ static int convert_ctx_accesses(struct bpf_verifier_env *env)
 			   insn->code == (BPF_ST | BPF_MEM | BPF_W) ||
 			   insn->code == (BPF_ST | BPF_MEM | BPF_DW)) {
 			type = BPF_WRITE;
-		} else if ((insn->code == (BPF_STX | BPF_ATOMIC | BPF_W) ||
+		} else if ((insn->code == (BPF_STX | BPF_ATOMIC | BPF_B) ||
+			    insn->code == (BPF_STX | BPF_ATOMIC | BPF_H) ||
+			    insn->code == (BPF_STX | BPF_ATOMIC | BPF_W) ||
 			    insn->code == (BPF_STX | BPF_ATOMIC | BPF_DW)) &&
 			   env->insn_aux_data[i + delta].ptr_type == PTR_TO_ARENA) {
 			insn->code = BPF_STX | BPF_PROBE_ATOMIC | BPF_SIZE(insn->code);
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 2acf9b336371..4a20a125eb46 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -51,6 +51,19 @@
 #define BPF_XCHG	(0xe0 | BPF_FETCH)	/* atomic exchange */
 #define BPF_CMPXCHG	(0xf0 | BPF_FETCH)	/* atomic compare-and-write */
 
+#define BPF_ATOMIC_LOAD		0x10
+#define BPF_ATOMIC_STORE	0x20
+#define BPF_ATOMIC_TYPE(imm)	((imm) & 0xf0)
+
+#define BPF_RELAXED	0x00
+#define BPF_ACQUIRE	0x01
+#define BPF_RELEASE	0x02
+#define BPF_ACQ_REL	0x03
+#define BPF_SEQ_CST	0x04
+
+#define BPF_LOAD_ACQ	(BPF_ATOMIC_LOAD | BPF_ACQUIRE)		/* load-acquire */
+#define BPF_STORE_REL	(BPF_ATOMIC_STORE | BPF_RELEASE)	/* store-release */
+
 enum bpf_cond_pseudo_jmp {
 	BPF_MAY_GOTO = 0,
 };
-- 
2.47.1.613.gc27f4b7a9f-goog

[PATCH RFC bpf-next v1 3/4] selftests/bpf: Delete duplicate verifier/atomic_invalid tests

[Peilin Ye]

Right now, the BPF_ADD and BPF_ADD | BPF_FETCH cases are tested twice:

  #55/u atomic BPF_ADD access through non-pointer  OK
  #55/p atomic BPF_ADD access through non-pointer  OK
  #56/u atomic BPF_ADD | BPF_FETCH access through non-pointer  OK
  #56/p atomic BPF_ADD | BPF_FETCH access through non-pointer  OK
  #57/u atomic BPF_ADD access through non-pointer  OK
  #57/p atomic BPF_ADD access through non-pointer  OK
  #58/u atomic BPF_ADD | BPF_FETCH access through non-pointer  OK
  #58/p atomic BPF_ADD | BPF_FETCH access through non-pointer  OK

Reviewed-by: Josh Don <joshdon@google.com>
Signed-off-by: Peilin Ye <yepeilin@google.com>
---
 tools/testing/selftests/bpf/verifier/atomic_invalid.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/tools/testing/selftests/bpf/verifier/atomic_invalid.c b/tools/testing/selftests/bpf/verifier/atomic_invalid.c
index 25f4ac1c69ab..8c52ad682067 100644
--- a/tools/testing/selftests/bpf/verifier/atomic_invalid.c
+++ b/tools/testing/selftests/bpf/verifier/atomic_invalid.c
@@ -13,8 +13,6 @@
 	}
 __INVALID_ATOMIC_ACCESS_TEST(BPF_ADD),
 __INVALID_ATOMIC_ACCESS_TEST(BPF_ADD | BPF_FETCH),
-__INVALID_ATOMIC_ACCESS_TEST(BPF_ADD),
-__INVALID_ATOMIC_ACCESS_TEST(BPF_ADD | BPF_FETCH),
 __INVALID_ATOMIC_ACCESS_TEST(BPF_AND),
 __INVALID_ATOMIC_ACCESS_TEST(BPF_AND | BPF_FETCH),
 __INVALID_ATOMIC_ACCESS_TEST(BPF_OR),
-- 
2.47.1.613.gc27f4b7a9f-goog

[PATCH RFC bpf-next v1 4/4] selftests/bpf: Add selftests for load-acquire and store-release instructions

[Peilin Ye]

Add the following ./test_progs tests:

  * atomics/load_acquire
  * atomics/store_release
  * arena_atomics/load_acquire
  * arena_atomics/store_release

They depend on the pre-defined __BPF_FEATURE_LOAD_ACQ_STORE_REL feature
macro, which implies -mcpu>=v4.

  $ ALLOWLIST=atomics/load_acquire,atomics/store_release,
  $ ALLOWLIST+=arena_atomics/load_acquire,arena_atomics/store_release

  $ ./test_progs-cpuv4 -a $ALLOWLIST

  #3/9     arena_atomics/load_acquire:OK
  #3/10    arena_atomics/store_release:OK
...
  #10/8    atomics/load_acquire:OK
  #10/9    atomics/store_release:OK

  $ ./test_progs -v -a $ALLOWLIST

  test_load_acquire:SKIP:Clang does not support BPF load-acquire or addr_space_cast
  #3/9     arena_atomics/load_acquire:SKIP
  test_store_release:SKIP:Clang does not support BPF store-release or addr_space_cast
  #3/10    arena_atomics/store_release:SKIP
...
  test_load_acquire:SKIP:Clang does not support BPF load-acquire
  #10/8    atomics/load_acquire:SKIP
  test_store_release:SKIP:Clang does not support BPF store-release
  #10/9    atomics/store_release:SKIP

Additionally, add several ./test_verifier tests:

  #65/u atomic BPF_LOAD_ACQ access through non-pointer  OK
  #65/p atomic BPF_LOAD_ACQ access through non-pointer  OK
  #66/u atomic BPF_STORE_REL access through non-pointer  OK
  #66/p atomic BPF_STORE_REL access through non-pointer  OK

  #67/u BPF_ATOMIC load-acquire, 8-bit OK
  #67/p BPF_ATOMIC load-acquire, 8-bit OK
  #68/u BPF_ATOMIC load-acquire, 16-bit OK
  #68/p BPF_ATOMIC load-acquire, 16-bit OK
  #69/u BPF_ATOMIC load-acquire, 32-bit OK
  #69/p BPF_ATOMIC load-acquire, 32-bit OK
  #70/u BPF_ATOMIC load-acquire, 64-bit OK
  #70/p BPF_ATOMIC load-acquire, 64-bit OK
  #71/u Cannot load-acquire from uninitialized src_reg OK
  #71/p Cannot load-acquire from uninitialized src_reg OK

  #76/u BPF_ATOMIC store-release, 8-bit OK
  #76/p BPF_ATOMIC store-release, 8-bit OK
  #77/u BPF_ATOMIC store-release, 16-bit OK
  #77/p BPF_ATOMIC store-release, 16-bit OK
  #78/u BPF_ATOMIC store-release, 32-bit OK
  #78/p BPF_ATOMIC store-release, 32-bit OK
  #79/u BPF_ATOMIC store-release, 64-bit OK
  #79/p BPF_ATOMIC store-release, 64-bit OK
  #80/u Cannot store-release from uninitialized src_reg OK
  #80/p Cannot store-release from uninitialized src_reg OK

Reviewed-by: Josh Don <joshdon@google.com>
Signed-off-by: Peilin Ye <yepeilin@google.com>
---
 include/linux/filter.h                        |  2 +
 .../selftests/bpf/prog_tests/arena_atomics.c  | 61 +++++++++++++++-
 .../selftests/bpf/prog_tests/atomics.c        | 57 ++++++++++++++-
 .../selftests/bpf/progs/arena_atomics.c       | 62 +++++++++++++++-
 tools/testing/selftests/bpf/progs/atomics.c   | 62 +++++++++++++++-
 .../selftests/bpf/verifier/atomic_invalid.c   | 26 +++----
 .../selftests/bpf/verifier/atomic_load.c      | 71 +++++++++++++++++++
 .../selftests/bpf/verifier/atomic_store.c     | 70 ++++++++++++++++++
 8 files changed, 393 insertions(+), 18 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/verifier/atomic_load.c
 create mode 100644 tools/testing/selftests/bpf/verifier/atomic_store.c

diff --git a/include/linux/filter.h b/include/linux/filter.h
index 0477254bc2d3..c264d723dc9e 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -364,6 +364,8 @@ static inline bool insn_is_cast_user(const struct bpf_insn *insn)
  *   BPF_XOR | BPF_FETCH      src_reg = atomic_fetch_xor(dst_reg + off16, src_reg);
  *   BPF_XCHG                 src_reg = atomic_xchg(dst_reg + off16, src_reg)
  *   BPF_CMPXCHG              r0 = atomic_cmpxchg(dst_reg + off16, r0, src_reg)
+ *   BPF_LOAD_ACQ             dst_reg = smp_load_acquire(src_reg + off16)
+ *   BPF_STORE_REL            smp_store_release(dst_reg + off16, src_reg)
  */
 
 #define BPF_ATOMIC_OP(SIZE, OP, DST, SRC, OFF)			\
diff --git a/tools/testing/selftests/bpf/prog_tests/arena_atomics.c b/tools/testing/selftests/bpf/prog_tests/arena_atomics.c
index 26e7c06c6cb4..81d3575d7652 100644
--- a/tools/testing/selftests/bpf/prog_tests/arena_atomics.c
+++ b/tools/testing/selftests/bpf/prog_tests/arena_atomics.c
@@ -162,6 +162,60 @@ static void test_uaf(struct arena_atomics *skel)
 	ASSERT_EQ(skel->arena->uaf_recovery_fails, 0, "uaf_recovery_fails");
 }
 
+static void test_load_acquire(struct arena_atomics *skel)
+{
+	LIBBPF_OPTS(bpf_test_run_opts, topts);
+	int err, prog_fd;
+
+	if (skel->data->skip_lacq_srel_tests) {
+		printf("%s:SKIP:Clang does not support BPF load-acquire or addr_space_cast\n",
+		       __func__);
+		test__skip();
+		return;
+	}
+
+	/* No need to attach it, just run it directly */
+	prog_fd = bpf_program__fd(skel->progs.load_acquire);
+	err = bpf_prog_test_run_opts(prog_fd, &topts);
+	if (!ASSERT_OK(err, "test_run_opts err"))
+		return;
+	if (!ASSERT_OK(topts.retval, "test_run_opts retval"))
+		return;
+
+	ASSERT_EQ(skel->arena->load_acquire8_result, 0x12, "load_acquire8_result");
+	ASSERT_EQ(skel->arena->load_acquire16_result, 0x1234, "load_acquire16_result");
+	ASSERT_EQ(skel->arena->load_acquire32_result, 0x12345678, "load_acquire32_result");
+	ASSERT_EQ(skel->arena->load_acquire64_result, 0x1234567890abcdef,
+		  "load_acquire64_result");
+}
+
+static void test_store_release(struct arena_atomics *skel)
+{
+	LIBBPF_OPTS(bpf_test_run_opts, topts);
+	int err, prog_fd;
+
+	if (skel->data->skip_lacq_srel_tests) {
+		printf("%s:SKIP:Clang does not support BPF store-release or addr_space_cast\n",
+		       __func__);
+		test__skip();
+		return;
+	}
+
+	/* No need to attach it, just run it directly */
+	prog_fd = bpf_program__fd(skel->progs.store_release);
+	err = bpf_prog_test_run_opts(prog_fd, &topts);
+	if (!ASSERT_OK(err, "test_run_opts err"))
+		return;
+	if (!ASSERT_OK(topts.retval, "test_run_opts retval"))
+		return;
+
+	ASSERT_EQ(skel->arena->store_release8_result, 0x12, "store_release8_result");
+	ASSERT_EQ(skel->arena->store_release16_result, 0x1234, "store_release16_result");
+	ASSERT_EQ(skel->arena->store_release32_result, 0x12345678, "store_release32_result");
+	ASSERT_EQ(skel->arena->store_release64_result, 0x1234567890abcdef,
+		  "store_release64_result");
+}
+
 void test_arena_atomics(void)
 {
 	struct arena_atomics *skel;
@@ -171,7 +225,7 @@ void test_arena_atomics(void)
 	if (!ASSERT_OK_PTR(skel, "arena atomics skeleton open"))
 		return;
 
-	if (skel->data->skip_tests) {
+	if (skel->data->skip_all_tests) {
 		printf("%s:SKIP:no ENABLE_ATOMICS_TESTS or no addr_space_cast support in clang",
 		       __func__);
 		test__skip();
@@ -199,6 +253,11 @@ void test_arena_atomics(void)
 	if (test__start_subtest("uaf"))
 		test_uaf(skel);
 
+	if (test__start_subtest("load_acquire"))
+		test_load_acquire(skel);
+	if (test__start_subtest("store_release"))
+		test_store_release(skel);
+
 cleanup:
 	arena_atomics__destroy(skel);
 }
diff --git a/tools/testing/selftests/bpf/prog_tests/atomics.c b/tools/testing/selftests/bpf/prog_tests/atomics.c
index 13e101f370a1..5d7cff3eed2b 100644
--- a/tools/testing/selftests/bpf/prog_tests/atomics.c
+++ b/tools/testing/selftests/bpf/prog_tests/atomics.c
@@ -162,6 +162,56 @@ static void test_xchg(struct atomics_lskel *skel)
 	ASSERT_EQ(skel->bss->xchg32_result, 1, "xchg32_result");
 }
 
+static void test_load_acquire(struct atomics_lskel *skel)
+{
+	LIBBPF_OPTS(bpf_test_run_opts, topts);
+	int err, prog_fd;
+
+	if (skel->data->skip_lacq_srel_tests) {
+		printf("%s:SKIP:Clang does not support BPF load-acquire\n", __func__);
+		test__skip();
+		return;
+	}
+
+	/* No need to attach it, just run it directly */
+	prog_fd = skel->progs.load_acquire.prog_fd;
+	err = bpf_prog_test_run_opts(prog_fd, &topts);
+	if (!ASSERT_OK(err, "test_run_opts err"))
+		return;
+	if (!ASSERT_OK(topts.retval, "test_run_opts retval"))
+		return;
+
+	ASSERT_EQ(skel->bss->load_acquire8_result, 0x12, "load_acquire8_result");
+	ASSERT_EQ(skel->bss->load_acquire16_result, 0x1234, "load_acquire16_result");
+	ASSERT_EQ(skel->bss->load_acquire32_result, 0x12345678, "load_acquire32_result");
+	ASSERT_EQ(skel->bss->load_acquire64_result, 0x1234567890abcdef, "load_acquire64_result");
+}
+
+static void test_store_release(struct atomics_lskel *skel)
+{
+	LIBBPF_OPTS(bpf_test_run_opts, topts);
+	int err, prog_fd;
+
+	if (skel->data->skip_lacq_srel_tests) {
+		printf("%s:SKIP:Clang does not support BPF store-release\n", __func__);
+		test__skip();
+		return;
+	}
+
+	/* No need to attach it, just run it directly */
+	prog_fd = skel->progs.store_release.prog_fd;
+	err = bpf_prog_test_run_opts(prog_fd, &topts);
+	if (!ASSERT_OK(err, "test_run_opts err"))
+		return;
+	if (!ASSERT_OK(topts.retval, "test_run_opts retval"))
+		return;
+
+	ASSERT_EQ(skel->bss->store_release8_result, 0x12, "store_release8_result");
+	ASSERT_EQ(skel->bss->store_release16_result, 0x1234, "store_release16_result");
+	ASSERT_EQ(skel->bss->store_release32_result, 0x12345678, "store_release32_result");
+	ASSERT_EQ(skel->bss->store_release64_result, 0x1234567890abcdef, "store_release64_result");
+}
+
 void test_atomics(void)
 {
 	struct atomics_lskel *skel;
@@ -170,7 +220,7 @@ void test_atomics(void)
 	if (!ASSERT_OK_PTR(skel, "atomics skeleton load"))
 		return;
 
-	if (skel->data->skip_tests) {
+	if (skel->data->skip_all_tests) {
 		printf("%s:SKIP:no ENABLE_ATOMICS_TESTS (missing Clang BPF atomics support)",
 		       __func__);
 		test__skip();
@@ -193,6 +243,11 @@ void test_atomics(void)
 	if (test__start_subtest("xchg"))
 		test_xchg(skel);
 
+	if (test__start_subtest("load_acquire"))
+		test_load_acquire(skel);
+	if (test__start_subtest("store_release"))
+		test_store_release(skel);
+
 cleanup:
 	atomics_lskel__destroy(skel);
 }
diff --git a/tools/testing/selftests/bpf/progs/arena_atomics.c b/tools/testing/selftests/bpf/progs/arena_atomics.c
index 40dd57fca5cc..fe8b67d9c87b 100644
--- a/tools/testing/selftests/bpf/progs/arena_atomics.c
+++ b/tools/testing/selftests/bpf/progs/arena_atomics.c
@@ -19,9 +19,15 @@ struct {
 } arena SEC(".maps");
 
 #if defined(ENABLE_ATOMICS_TESTS) && defined(__BPF_FEATURE_ADDR_SPACE_CAST)
-bool skip_tests __attribute((__section__(".data"))) = false;
+bool skip_all_tests __attribute((__section__(".data"))) = false;
 #else
-bool skip_tests = true;
+bool skip_all_tests = true;
+#endif
+
+#if defined(__BPF_FEATURE_LOAD_ACQ_STORE_REL) && defined(__BPF_FEATURE_ADDR_SPACE_CAST)
+bool skip_lacq_srel_tests __attribute((__section__(".data"))) = false;
+#else
+bool skip_lacq_srel_tests = true;
 #endif
 
 __u32 pid = 0;
@@ -274,4 +280,56 @@ int uaf(const void *ctx)
 	return 0;
 }
 
+__u8 __arena_global load_acquire8_value = 0x12;
+__u16 __arena_global load_acquire16_value = 0x1234;
+__u32 __arena_global load_acquire32_value = 0x12345678;
+__u64 __arena_global load_acquire64_value = 0x1234567890abcdef;
+
+__u8 __arena_global load_acquire8_result = 0;
+__u16 __arena_global load_acquire16_result = 0;
+__u32 __arena_global load_acquire32_result = 0;
+__u64 __arena_global load_acquire64_result = 0;
+
+SEC("raw_tp/sys_enter")
+int load_acquire(const void *ctx)
+{
+	if (pid != (bpf_get_current_pid_tgid() >> 32))
+		return 0;
+
+#ifdef __BPF_FEATURE_LOAD_ACQ_STORE_REL
+	load_acquire8_result = __atomic_load_n(&load_acquire8_value, __ATOMIC_ACQUIRE);
+	load_acquire16_result = __atomic_load_n(&load_acquire16_value, __ATOMIC_ACQUIRE);
+	load_acquire32_result = __atomic_load_n(&load_acquire32_value, __ATOMIC_ACQUIRE);
+	load_acquire64_result = __atomic_load_n(&load_acquire64_value, __ATOMIC_ACQUIRE);
+#endif
+
+	return 0;
+}
+
+__u8 __arena_global store_release8_result = 0;
+__u16 __arena_global store_release16_result = 0;
+__u32 __arena_global store_release32_result = 0;
+__u64 __arena_global store_release64_result = 0;
+
+SEC("raw_tp/sys_enter")
+int store_release(const void *ctx)
+{
+	if (pid != (bpf_get_current_pid_tgid() >> 32))
+		return 0;
+
+#ifdef __BPF_FEATURE_LOAD_ACQ_STORE_REL
+	__u8 val8 = 0x12;
+	__u16 val16 = 0x1234;
+	__u32 val32 = 0x12345678;
+	__u64 val64 = 0x1234567890abcdef;
+
+	__atomic_store_n(&store_release8_result, val8, __ATOMIC_RELEASE);
+	__atomic_store_n(&store_release16_result, val16, __ATOMIC_RELEASE);
+	__atomic_store_n(&store_release32_result, val32, __ATOMIC_RELEASE);
+	__atomic_store_n(&store_release64_result, val64, __ATOMIC_RELEASE);
+#endif
+
+	return 0;
+}
+
 char _license[] SEC("license") = "GPL";
diff --git a/tools/testing/selftests/bpf/progs/atomics.c b/tools/testing/selftests/bpf/progs/atomics.c
index f89c7f0cc53b..4c23d7d0d37d 100644
--- a/tools/testing/selftests/bpf/progs/atomics.c
+++ b/tools/testing/selftests/bpf/progs/atomics.c
@@ -5,9 +5,15 @@
 #include <stdbool.h>
 
 #ifdef ENABLE_ATOMICS_TESTS
-bool skip_tests __attribute((__section__(".data"))) = false;
+bool skip_all_tests __attribute((__section__(".data"))) = false;
 #else
-bool skip_tests = true;
+bool skip_all_tests = true;
+#endif
+
+#ifdef __BPF_FEATURE_LOAD_ACQ_STORE_REL
+bool skip_lacq_srel_tests __attribute((__section__(".data"))) = false;
+#else
+bool skip_lacq_srel_tests = true;
 #endif
 
 __u32 pid = 0;
@@ -168,3 +174,55 @@ int xchg(const void *ctx)
 
 	return 0;
 }
+
+__u8 load_acquire8_value = 0x12;
+__u16 load_acquire16_value = 0x1234;
+__u32 load_acquire32_value = 0x12345678;
+__u64 load_acquire64_value = 0x1234567890abcdef;
+
+__u8 load_acquire8_result = 0;
+__u16 load_acquire16_result = 0;
+__u32 load_acquire32_result = 0;
+__u64 load_acquire64_result = 0;
+
+SEC("raw_tp/sys_enter")
+int load_acquire(const void *ctx)
+{
+	if (pid != (bpf_get_current_pid_tgid() >> 32))
+		return 0;
+
+#ifdef __BPF_FEATURE_LOAD_ACQ_STORE_REL
+	load_acquire8_result = __atomic_load_n(&load_acquire8_value, __ATOMIC_ACQUIRE);
+	load_acquire16_result = __atomic_load_n(&load_acquire16_value, __ATOMIC_ACQUIRE);
+	load_acquire32_result = __atomic_load_n(&load_acquire32_value, __ATOMIC_ACQUIRE);
+	load_acquire64_result = __atomic_load_n(&load_acquire64_value, __ATOMIC_ACQUIRE);
+#endif
+
+	return 0;
+}
+
+__u8 store_release8_result = 0;
+__u16 store_release16_result = 0;
+__u32 store_release32_result = 0;
+__u64 store_release64_result = 0;
+
+SEC("raw_tp/sys_enter")
+int store_release(const void *ctx)
+{
+	if (pid != (bpf_get_current_pid_tgid() >> 32))
+		return 0;
+
+#ifdef __BPF_FEATURE_LOAD_ACQ_STORE_REL
+	__u8 val8 = 0x12;
+	__u16 val16 = 0x1234;
+	__u32 val32 = 0x12345678;
+	__u64 val64 = 0x1234567890abcdef;
+
+	__atomic_store_n(&store_release8_result, val8, __ATOMIC_RELEASE);
+	__atomic_store_n(&store_release16_result, val16, __ATOMIC_RELEASE);
+	__atomic_store_n(&store_release32_result, val32, __ATOMIC_RELEASE);
+	__atomic_store_n(&store_release64_result, val64, __ATOMIC_RELEASE);
+#endif
+
+	return 0;
+}
diff --git a/tools/testing/selftests/bpf/verifier/atomic_invalid.c b/tools/testing/selftests/bpf/verifier/atomic_invalid.c
index 8c52ad682067..3f90d8f8a9c0 100644
--- a/tools/testing/selftests/bpf/verifier/atomic_invalid.c
+++ b/tools/testing/selftests/bpf/verifier/atomic_invalid.c
@@ -1,4 +1,4 @@
-#define __INVALID_ATOMIC_ACCESS_TEST(op)				\
+#define __INVALID_ATOMIC_ACCESS_TEST(op, reg)				\
 	{								\
 		"atomic " #op " access through non-pointer ",		\
 		.insns = {						\
@@ -9,15 +9,17 @@
 			BPF_EXIT_INSN(),				\
 		},							\
 		.result = REJECT,					\
-		.errstr = "R1 invalid mem access 'scalar'"		\
+		.errstr = #reg " invalid mem access 'scalar'"		\
 	}
-__INVALID_ATOMIC_ACCESS_TEST(BPF_ADD),
-__INVALID_ATOMIC_ACCESS_TEST(BPF_ADD | BPF_FETCH),
-__INVALID_ATOMIC_ACCESS_TEST(BPF_AND),
-__INVALID_ATOMIC_ACCESS_TEST(BPF_AND | BPF_FETCH),
-__INVALID_ATOMIC_ACCESS_TEST(BPF_OR),
-__INVALID_ATOMIC_ACCESS_TEST(BPF_OR | BPF_FETCH),
-__INVALID_ATOMIC_ACCESS_TEST(BPF_XOR),
-__INVALID_ATOMIC_ACCESS_TEST(BPF_XOR | BPF_FETCH),
-__INVALID_ATOMIC_ACCESS_TEST(BPF_XCHG),
-__INVALID_ATOMIC_ACCESS_TEST(BPF_CMPXCHG),
+__INVALID_ATOMIC_ACCESS_TEST(BPF_ADD, R1),
+__INVALID_ATOMIC_ACCESS_TEST(BPF_ADD | BPF_FETCH, R1),
+__INVALID_ATOMIC_ACCESS_TEST(BPF_AND, R1),
+__INVALID_ATOMIC_ACCESS_TEST(BPF_AND | BPF_FETCH, R1),
+__INVALID_ATOMIC_ACCESS_TEST(BPF_OR, R1),
+__INVALID_ATOMIC_ACCESS_TEST(BPF_OR | BPF_FETCH, R1),
+__INVALID_ATOMIC_ACCESS_TEST(BPF_XOR, R1),
+__INVALID_ATOMIC_ACCESS_TEST(BPF_XOR | BPF_FETCH, R1),
+__INVALID_ATOMIC_ACCESS_TEST(BPF_XCHG, R1),
+__INVALID_ATOMIC_ACCESS_TEST(BPF_CMPXCHG, R1),
+__INVALID_ATOMIC_ACCESS_TEST(BPF_LOAD_ACQ, R0),
+__INVALID_ATOMIC_ACCESS_TEST(BPF_STORE_REL, R1),
\ No newline at end of file
diff --git a/tools/testing/selftests/bpf/verifier/atomic_load.c b/tools/testing/selftests/bpf/verifier/atomic_load.c
new file mode 100644
index 000000000000..5186f71b6009
--- /dev/null
+++ b/tools/testing/selftests/bpf/verifier/atomic_load.c
@@ -0,0 +1,71 @@
+{
+	"BPF_ATOMIC load-acquire, 8-bit",
+	.insns = {
+		BPF_MOV64_IMM(BPF_REG_0, 0),
+		/* Write 1 to stack. */
+		BPF_ST_MEM(BPF_B, BPF_REG_10, -1, 0x12),
+		/* Load-acquire it from stack to R1. */
+		BPF_ATOMIC_OP(BPF_B, BPF_LOAD_ACQ, BPF_REG_1, BPF_REG_10, -1),
+		/* Check loaded value is 0x12. */
+		BPF_JMP32_IMM(BPF_JEQ, BPF_REG_1, 0x12, 1),
+		BPF_MOV64_IMM(BPF_REG_0, 1),
+		BPF_EXIT_INSN(),
+	},
+	.result = ACCEPT,
+},
+{
+	"BPF_ATOMIC load-acquire, 16-bit",
+	.insns = {
+		BPF_MOV64_IMM(BPF_REG_0, 0),
+		/* Write 0x1234 to stack. */
+		BPF_ST_MEM(BPF_H, BPF_REG_10, -2, 0x1234),
+		/* Load-acquire it from stack to R1. */
+		BPF_ATOMIC_OP(BPF_H, BPF_LOAD_ACQ, BPF_REG_1, BPF_REG_10, -2),
+		/* Check loaded value is 0x1234. */
+		BPF_JMP32_IMM(BPF_JEQ, BPF_REG_1, 0x1234, 1),
+		BPF_MOV64_IMM(BPF_REG_0, 1),
+		BPF_EXIT_INSN(),
+	},
+	.result = ACCEPT,
+},
+{
+	"BPF_ATOMIC load-acquire, 32-bit",
+	.insns = {
+		BPF_MOV64_IMM(BPF_REG_0, 0),
+		/* Write 0x12345678 to stack. */
+		BPF_ST_MEM(BPF_W, BPF_REG_10, -4, 0x12345678),
+		/* Load-acquire it from stack to R1. */
+		BPF_ATOMIC_OP(BPF_W, BPF_LOAD_ACQ, BPF_REG_1, BPF_REG_10, -4),
+		/* Check loaded value is 0x12345678. */
+		BPF_JMP32_IMM(BPF_JEQ, BPF_REG_1, 0x12345678, 1),
+		BPF_MOV64_IMM(BPF_REG_0, 1),
+		BPF_EXIT_INSN(),
+	},
+	.result = ACCEPT,
+},
+{
+	"BPF_ATOMIC load-acquire, 64-bit",
+	.insns = {
+		BPF_MOV64_IMM(BPF_REG_0, 0),
+		/* Save 0x1234567890abcdef to R1, then write it to stack. */
+		BPF_LD_IMM64(BPF_REG_1, 0x1234567890abcdef),
+		BPF_STX_MEM(BPF_DW, BPF_REG_10, BPF_REG_1, -8),
+		/* Load-acquire it from stack to R2. */
+		BPF_ATOMIC_OP(BPF_DW, BPF_LOAD_ACQ, BPF_REG_2, BPF_REG_10, -8),
+		/* Check loaded value is 0x1234567890abcdef. */
+		BPF_JMP_REG(BPF_JEQ, BPF_REG_2, BPF_REG_1, 1),
+		BPF_MOV64_IMM(BPF_REG_0, 1),
+		BPF_EXIT_INSN(),
+	},
+	.result = ACCEPT,
+},
+{
+	"Cannot load-acquire from uninitialized src_reg",
+	.insns = {
+		BPF_MOV64_IMM(BPF_REG_0, 0),
+		BPF_ATOMIC_OP(BPF_DW, BPF_LOAD_ACQ, BPF_REG_1, BPF_REG_2, -8),
+		BPF_EXIT_INSN(),
+	},
+	.result = REJECT,
+	.errstr = "R2 !read_ok",
+},
diff --git a/tools/testing/selftests/bpf/verifier/atomic_store.c b/tools/testing/selftests/bpf/verifier/atomic_store.c
new file mode 100644
index 000000000000..23f2d5c46ea5
--- /dev/null
+++ b/tools/testing/selftests/bpf/verifier/atomic_store.c
@@ -0,0 +1,70 @@
+{
+	"BPF_ATOMIC store-release, 8-bit",
+	.insns = {
+		BPF_MOV64_IMM(BPF_REG_0, 0),
+		/* Store-release 0x12 to stack. */
+		BPF_MOV64_IMM(BPF_REG_1, 0x12),
+		BPF_ATOMIC_OP(BPF_B, BPF_STORE_REL, BPF_REG_10, BPF_REG_1, -1),
+		/* Check loaded value is 0x12. */
+		BPF_LDX_MEM(BPF_B, BPF_REG_2, BPF_REG_10, -1),
+		BPF_JMP32_REG(BPF_JEQ, BPF_REG_2, BPF_REG_1, 1),
+		BPF_MOV64_IMM(BPF_REG_0, 1),
+		BPF_EXIT_INSN(),
+	},
+	.result = ACCEPT,
+},
+{
+	"BPF_ATOMIC store-release, 16-bit",
+	.insns = {
+		BPF_MOV64_IMM(BPF_REG_0, 0),
+		/* Store-release 0x1234 to stack. */
+		BPF_MOV64_IMM(BPF_REG_1, 0x1234),
+		BPF_ATOMIC_OP(BPF_H, BPF_STORE_REL, BPF_REG_10, BPF_REG_1, -2),
+		/* Check loaded value is 0x1234. */
+		BPF_LDX_MEM(BPF_H, BPF_REG_2, BPF_REG_10, -2),
+		BPF_JMP32_REG(BPF_JEQ, BPF_REG_2, BPF_REG_1, 1),
+		BPF_MOV64_IMM(BPF_REG_0, 1),
+		BPF_EXIT_INSN(),
+	},
+	.result = ACCEPT,
+},
+{
+	"BPF_ATOMIC store-release, 32-bit",
+	.insns = {
+		BPF_MOV64_IMM(BPF_REG_0, 0),
+		/* Store-release 0x12345678 to stack. */
+		BPF_MOV64_IMM(BPF_REG_1, 0x12345678),
+		BPF_ATOMIC_OP(BPF_W, BPF_STORE_REL, BPF_REG_10, BPF_REG_1, -4),
+		/* Check loaded value is 0x12345678. */
+		BPF_LDX_MEM(BPF_W, BPF_REG_2, BPF_REG_10, -4),
+		BPF_JMP32_REG(BPF_JEQ, BPF_REG_2, BPF_REG_1, 1),
+		BPF_MOV64_IMM(BPF_REG_0, 1),
+		BPF_EXIT_INSN(),
+	},
+	.result = ACCEPT,
+},
+{
+	"BPF_ATOMIC store-release, 64-bit",
+	.insns = {
+		BPF_MOV64_IMM(BPF_REG_0, 0),
+		/* Store-release 0x1234567890abcdef to stack. */
+		BPF_LD_IMM64(BPF_REG_1, 0x1234567890abcdef),
+		BPF_ATOMIC_OP(BPF_DW, BPF_STORE_REL, BPF_REG_10, BPF_REG_1, -8),
+		/* Check loaded value is 0x1234567890abcdef. */
+		BPF_LDX_MEM(BPF_DW, BPF_REG_2, BPF_REG_10, -8),
+		BPF_JMP_REG(BPF_JEQ, BPF_REG_2, BPF_REG_1, 1),
+		BPF_MOV64_IMM(BPF_REG_0, 1),
+		BPF_EXIT_INSN(),
+	},
+	.result = ACCEPT,
+},
+{
+	"Cannot store-release with uninitialized src_reg",
+	.insns = {
+		BPF_MOV64_IMM(BPF_REG_0, 0),
+		BPF_ATOMIC_OP(BPF_DW, BPF_STORE_REL, BPF_REG_10, BPF_REG_2, -8),
+		BPF_EXIT_INSN(),
+	},
+	.result = REJECT,
+	.errstr = "R2 !read_ok",
+},
-- 
2.47.1.613.gc27f4b7a9f-goog

Re: [PATCH RFC bpf-next v1 0/4] Introduce load-acquire and store-release BPF instructions

[Peilin Ye]

By this:

On Sat, Dec 21, 2024 at 01:22:04AM +0000, Peilin Ye wrote:
> Based on [3], the BPF load-acquire, the arm64 JIT compiler generates LDAR
> (RCsc) instead of LDAPR (RCpc).

I actually meant:

  Based on [3], for BPF load-acquire, make the arm64 JIT compiler
  generate LDAR (RCsc) instead of LDAPR (RCpc).

Sorry for any confusion.

Re: [PATCH RFC bpf-next v1 4/4] selftests/bpf: Add selftests for load-acquire and store-release instructions

[Peilin Ye]

> --- /dev/null
> +++ b/tools/testing/selftests/bpf/verifier/atomic_load.c
> @@ -0,0 +1,71 @@
> +{
> +	"BPF_ATOMIC load-acquire, 8-bit",
> +	.insns = {
> +		BPF_MOV64_IMM(BPF_REG_0, 0),
> +		/* Write 1 to stack. */
                         ~
                /* Write 0x12 to stack. */
                         ~~~~
> +		BPF_ST_MEM(BPF_B, BPF_REG_10, -1, 0x12),
> +		/* Load-acquire it from stack to R1. */
> +		BPF_ATOMIC_OP(BPF_B, BPF_LOAD_ACQ, BPF_REG_1, BPF_REG_10, -1),
> +		/* Check loaded value is 0x12. */

Will be fixed in the next version.

Thanks,
Peilin Ye

Re: [PATCH RFC bpf-next v1 2/4] bpf: Introduce load-acquire and store-release instructions

[Xu Kuohai]

On 12/21/2024 9:25 AM, Peilin Ye wrote:
> Introduce BPF instructions with load-acquire and store-release
> semantics, as discussed in [1].  The following new flags are defined:
> 
>    BPF_ATOMIC_LOAD         0x10
>    BPF_ATOMIC_STORE        0x20
>    BPF_ATOMIC_TYPE(imm)    ((imm) & 0xf0)
> 
>    BPF_RELAXED        0x0
>    BPF_ACQUIRE        0x1
>    BPF_RELEASE        0x2
>    BPF_ACQ_REL        0x3
>    BPF_SEQ_CST        0x4
> 
>    BPF_LOAD_ACQ       (BPF_ATOMIC_LOAD | BPF_ACQUIRE)
>    BPF_STORE_REL      (BPF_ATOMIC_STORE | BPF_RELEASE)
> 
> A "load-acquire" is a BPF_STX | BPF_ATOMIC instruction with the 'imm'
> field set to BPF_LOAD_ACQ (0x11).
> 
> Similarly, a "store-release" is a BPF_STX | BPF_ATOMIC instruction with
> the 'imm' field set to BPF_STORE_REL (0x22).
> 
> Unlike existing atomic operations that only support BPF_W (32-bit) and
> BPF_DW (64-bit) size modifiers, load-acquires and store-releases also
> support BPF_B (8-bit) and BPF_H (16-bit).  An 8- or 16-bit load-acquire
> zero-extends the value before writing it to a 32-bit register, just like
> ARM64 instruction LDARH and friends.
> 
> As an example, consider the following 64-bit load-acquire BPF
> instruction (assuming little-endian from now on):
> 
>    db 10 00 00 11 00 00 00  r0 = load_acquire((u64 *)(r1 + 0x0))
> 
>    opcode (0xdb): BPF_ATOMIC | BPF_DW | BPF_STX
>    imm (0x00000011): BPF_LOAD_ACQ
> 
> For ARM64, an LDAR instruction will be generated by the JIT compiler for
> the above:
> 
>    ldar  x7, [x0]
> 
> Similarly, a 16-bit BPF store-release:
> 
>    cb 21 00 00 22 00 00 00  store_release((u16 *)(r1 + 0x0), w2)
> 
>    opcode (0xcb): BPF_ATOMIC | BPF_H | BPF_STX
>    imm (0x00000022): BPF_STORE_REL
> 
> An STLRH will be generated for it:
> 
>    stlrh  w1, [x0]
> 
> For a complete mapping for ARM64:
> 
>    load-acquire     8-bit  LDARB
>   (BPF_LOAD_ACQ)   16-bit  LDARH
>                    32-bit  LDAR (32-bit)
>                    64-bit  LDAR (64-bit)
>    store-release    8-bit  STLRB
>   (BPF_STORE_REL)  16-bit  STLRH
>                    32-bit  STLR (32-bit)
>                    64-bit  STLR (64-bit)
> 
> Reviewed-by: Josh Don <joshdon@google.com>
> Reviewed-by: Barret Rhoden <brho@google.com>
> Signed-off-by: Peilin Ye <yepeilin@google.com>
> ---
>   arch/arm64/include/asm/insn.h  |  8 ++++
>   arch/arm64/lib/insn.c          | 34 ++++++++++++++
>   arch/arm64/net/bpf_jit.h       | 20 ++++++++
>   arch/arm64/net/bpf_jit_comp.c  | 85 +++++++++++++++++++++++++++++++++-
>   include/uapi/linux/bpf.h       | 13 ++++++
>   kernel/bpf/core.c              | 41 +++++++++++++++-
>   kernel/bpf/disasm.c            | 14 ++++++
>   kernel/bpf/verifier.c          | 32 +++++++++----
>   tools/include/uapi/linux/bpf.h | 13 ++++++
>   9 files changed, 246 insertions(+), 14 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/insn.h b/arch/arm64/include/asm/insn.h
> index e390c432f546..bbfdbe570ff6 100644
> --- a/arch/arm64/include/asm/insn.h
> +++ b/arch/arm64/include/asm/insn.h
> @@ -188,8 +188,10 @@ enum aarch64_insn_ldst_type {
>   	AARCH64_INSN_LDST_STORE_PAIR_PRE_INDEX,
>   	AARCH64_INSN_LDST_LOAD_PAIR_POST_INDEX,
>   	AARCH64_INSN_LDST_STORE_PAIR_POST_INDEX,
> +	AARCH64_INSN_LDST_LOAD_ACQ,
>   	AARCH64_INSN_LDST_LOAD_EX,
>   	AARCH64_INSN_LDST_LOAD_ACQ_EX,
> +	AARCH64_INSN_LDST_STORE_REL,
>   	AARCH64_INSN_LDST_STORE_EX,
>   	AARCH64_INSN_LDST_STORE_REL_EX,
>   	AARCH64_INSN_LDST_SIGNED_LOAD_IMM_OFFSET,
> @@ -351,6 +353,8 @@ __AARCH64_INSN_FUNCS(ldr_imm,	0x3FC00000, 0x39400000)
>   __AARCH64_INSN_FUNCS(ldr_lit,	0xBF000000, 0x18000000)
>   __AARCH64_INSN_FUNCS(ldrsw_lit,	0xFF000000, 0x98000000)
>   __AARCH64_INSN_FUNCS(exclusive,	0x3F800000, 0x08000000)
> +__AARCH64_INSN_FUNCS(load_acq,  0x3FC08000, 0x08C08000)
> +__AARCH64_INSN_FUNCS(store_rel, 0x3FC08000, 0x08808000)


I checked Arm Architecture Reference Manual [1].

Section C6.2.{168,169,170,371,372,373} state that field Rt2 (bits 10-14) and
Rs (bits 16-20) for LDARB/LDARH/LDAR/STLRB/STLRH and no offset type STLR
instructions are fixed to (1).

Section C2.2.2 explains that (1) means a Should-Be-One (SBO) bit.

And the Glossary section says "Arm strongly recommends that software writes
the field as all 1s. If software writes a value that is not all 1s, it must
expect an UNPREDICTABLE or CONSTRAINED UNPREDICTABLE result."

Although the pre-index type of STLR is an excetpion, it is not used in this
series. Therefore, both bits 10-14 and 16-20 in mask and value should be set
to 1s.

[1] https://developer.arm.com/documentation/ddi0487/latest/


>   __AARCH64_INSN_FUNCS(load_ex,	0x3F400000, 0x08400000)
>   __AARCH64_INSN_FUNCS(store_ex,	0x3F400000, 0x08000000)
>   __AARCH64_INSN_FUNCS(mops,	0x3B200C00, 0x19000400)
> @@ -602,6 +606,10 @@ u32 aarch64_insn_gen_load_store_pair(enum aarch64_insn_register reg1,
>   				     int offset,
>   				     enum aarch64_insn_variant variant,
>   				     enum aarch64_insn_ldst_type type);
> +u32 aarch64_insn_gen_load_acq_store_rel(enum aarch64_insn_register reg,
> +					enum aarch64_insn_register base,
> +					enum aarch64_insn_size_type size,
> +					enum aarch64_insn_ldst_type type);
>   u32 aarch64_insn_gen_load_store_ex(enum aarch64_insn_register reg,
>   				   enum aarch64_insn_register base,
>   				   enum aarch64_insn_register state,
> diff --git a/arch/arm64/lib/insn.c b/arch/arm64/lib/insn.c
> index b008a9b46a7f..80e5b191d96a 100644
> --- a/arch/arm64/lib/insn.c
> +++ b/arch/arm64/lib/insn.c
> @@ -540,6 +540,40 @@ u32 aarch64_insn_gen_load_store_pair(enum aarch64_insn_register reg1,
>   					     offset >> shift);
>   }
>   
> +u32 aarch64_insn_gen_load_acq_store_rel(enum aarch64_insn_register reg,
> +					enum aarch64_insn_register base,
> +					enum aarch64_insn_size_type size,
> +					enum aarch64_insn_ldst_type type)
> +{
> +	u32 insn;
> +
> +	switch (type) {
> +	case AARCH64_INSN_LDST_LOAD_ACQ:
> +		insn = aarch64_insn_get_load_acq_value();
> +		break;
> +	case AARCH64_INSN_LDST_STORE_REL:
> +		insn = aarch64_insn_get_store_rel_value();
> +		break;
> +	default:
> +		pr_err("%s: unknown load-acquire/store-release encoding %d\n", __func__, type);
> +		return AARCH64_BREAK_FAULT;
> +	}
> +
> +	insn = aarch64_insn_encode_ldst_size(size, insn);
> +
> +	insn = aarch64_insn_encode_register(AARCH64_INSN_REGTYPE_RT, insn,
> +					    reg);
> +
> +	insn = aarch64_insn_encode_register(AARCH64_INSN_REGTYPE_RN, insn,
> +					    base);
> +
> +	insn = aarch64_insn_encode_register(AARCH64_INSN_REGTYPE_RT2, insn,
> +					    AARCH64_INSN_REG_ZR);
> +
> +	return aarch64_insn_encode_register(AARCH64_INSN_REGTYPE_RS, insn,
> +					    AARCH64_INSN_REG_ZR);

As explained above, RS and RT2 fields should be fixed to 1s.

> +}
> +
>   u32 aarch64_insn_gen_load_store_ex(enum aarch64_insn_register reg,
>   				   enum aarch64_insn_register base,
>   				   enum aarch64_insn_register state,
> diff --git a/arch/arm64/net/bpf_jit.h b/arch/arm64/net/bpf_jit.h
> index b22ab2f97a30..a3b0e693a125 100644
> --- a/arch/arm64/net/bpf_jit.h
> +++ b/arch/arm64/net/bpf_jit.h
> @@ -119,6 +119,26 @@
>   	aarch64_insn_gen_load_store_ex(Rt, Rn, Rs, A64_SIZE(sf), \
>   				       AARCH64_INSN_LDST_STORE_REL_EX)
>   
> +/* Load-acquire & store-release */
> +#define A64_LDAR(Rt, Rn, size)  \
> +	aarch64_insn_gen_load_acq_store_rel(Rt, Rn, AARCH64_INSN_SIZE_##size, \
> +					    AARCH64_INSN_LDST_LOAD_ACQ)
> +#define A64_STLR(Rt, Rn, size)  \
> +	aarch64_insn_gen_load_acq_store_rel(Rt, Rn, AARCH64_INSN_SIZE_##size, \
> +					    AARCH64_INSN_LDST_STORE_REL)
> +
> +/* Rt = [Rn] (load acquire) */
> +#define A64_LDARB(Wt, Xn)	A64_LDAR(Wt, Xn, 8)
> +#define A64_LDARH(Wt, Xn)	A64_LDAR(Wt, Xn, 16)
> +#define A64_LDAR32(Wt, Xn)	A64_LDAR(Wt, Xn, 32)
> +#define A64_LDAR64(Xt, Xn)	A64_LDAR(Xt, Xn, 64)
> +
> +/* [Rn] = Rt (store release) */
> +#define A64_STLRB(Wt, Xn)	A64_STLR(Wt, Xn, 8)
> +#define A64_STLRH(Wt, Xn)	A64_STLR(Wt, Xn, 16)
> +#define A64_STLR32(Wt, Xn)	A64_STLR(Wt, Xn, 32)
> +#define A64_STLR64(Xt, Xn)	A64_STLR(Xt, Xn, 64)
> +
>   /*
>    * LSE atomics
>    *
> diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
> index 66708b95493a..15fc0f391f14 100644
> --- a/arch/arm64/net/bpf_jit_comp.c
> +++ b/arch/arm64/net/bpf_jit_comp.c
> @@ -634,6 +634,80 @@ static int emit_bpf_tail_call(struct jit_ctx *ctx)
>   	return 0;
>   }
>   
> +static inline bool is_atomic_load_store(const s32 imm)
> +{
> +	const s32 type = BPF_ATOMIC_TYPE(imm);
> +
> +	return type == BPF_ATOMIC_LOAD || type == BPF_ATOMIC_STORE;
> +}
> +
> +static int emit_atomic_load_store(const struct bpf_insn *insn, struct jit_ctx *ctx)
> +{
> +	const s16 off = insn->off;
> +	const u8 code = insn->code;
> +	const bool arena = BPF_MODE(code) == BPF_PROBE_ATOMIC;
> +	const u8 arena_vm_base = bpf2a64[ARENA_VM_START];
> +	const u8 dst = bpf2a64[insn->dst_reg];
> +	const u8 src = bpf2a64[insn->src_reg];
> +	const u8 tmp = bpf2a64[TMP_REG_1];
> +	u8 ptr;
> +
> +	if (BPF_ATOMIC_TYPE(insn->imm) == BPF_ATOMIC_LOAD)
> +		ptr = src;
> +	else
> +		ptr = dst;
> +
> +	if (off) {
> +		emit_a64_mov_i(true, tmp, off, ctx);
> +		emit(A64_ADD(true, tmp, tmp, ptr), ctx);

The mov and add instructions can be optimized to a single A64_ADD_I
if is_addsub_imm(off) is true.

> +		ptr = tmp;
> +	}
> +	if (arena) {
> +		emit(A64_ADD(true, tmp, ptr, arena_vm_base), ctx);
> +		ptr = tmp;
> +	}
> +
> +	switch (insn->imm) {
> +	case BPF_LOAD_ACQ:
> +		switch (BPF_SIZE(code)) {
> +		case BPF_B:
> +			emit(A64_LDARB(dst, ptr), ctx);
> +			break;
> +		case BPF_H:
> +			emit(A64_LDARH(dst, ptr), ctx);
> +			break;
> +		case BPF_W:
> +			emit(A64_LDAR32(dst, ptr), ctx);
> +			break;
> +		case BPF_DW:
> +			emit(A64_LDAR64(dst, ptr), ctx);
> +			break;
> +		}
> +		break;
> +	case BPF_STORE_REL:
> +		switch (BPF_SIZE(code)) {
> +		case BPF_B:
> +			emit(A64_STLRB(src, ptr), ctx);
> +			break;
> +		case BPF_H:
> +			emit(A64_STLRH(src, ptr), ctx);
> +			break;
> +		case BPF_W:
> +			emit(A64_STLR32(src, ptr), ctx);
> +			break;
> +		case BPF_DW:
> +			emit(A64_STLR64(src, ptr), ctx);
> +			break;
> +		}
> +		break;
> +	default:
> +		pr_err_once("unknown atomic load/store op code %02x\n", insn->imm);
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
>   #ifdef CONFIG_ARM64_LSE_ATOMICS
>   static int emit_lse_atomic(const struct bpf_insn *insn, struct jit_ctx *ctx)
>   {
> @@ -1641,11 +1715,17 @@ static int build_insn(const struct bpf_insn *insn, struct jit_ctx *ctx,
>   			return ret;
>   		break;
>   
> +	case BPF_STX | BPF_ATOMIC | BPF_B:
> +	case BPF_STX | BPF_ATOMIC | BPF_H:
>   	case BPF_STX | BPF_ATOMIC | BPF_W:
>   	case BPF_STX | BPF_ATOMIC | BPF_DW:
> +	case BPF_STX | BPF_PROBE_ATOMIC | BPF_B:
> +	case BPF_STX | BPF_PROBE_ATOMIC | BPF_H:
>   	case BPF_STX | BPF_PROBE_ATOMIC | BPF_W:
>   	case BPF_STX | BPF_PROBE_ATOMIC | BPF_DW:
> -		if (cpus_have_cap(ARM64_HAS_LSE_ATOMICS))
> +		if (is_atomic_load_store(insn->imm))
> +			ret = emit_atomic_load_store(insn, ctx);
> +		else if (cpus_have_cap(ARM64_HAS_LSE_ATOMICS))
>   			ret = emit_lse_atomic(insn, ctx);
>   		else
>   			ret = emit_ll_sc_atomic(insn, ctx);
> @@ -2669,7 +2749,8 @@ bool bpf_jit_supports_insn(struct bpf_insn *insn, bool in_arena)
>   	switch (insn->code) {
>   	case BPF_STX | BPF_ATOMIC | BPF_W:
>   	case BPF_STX | BPF_ATOMIC | BPF_DW:
> -		if (!cpus_have_cap(ARM64_HAS_LSE_ATOMICS))
> +		if (!is_atomic_load_store(insn->imm) &&
> +		    !cpus_have_cap(ARM64_HAS_LSE_ATOMICS))
>   			return false;
>   	}
>   	return true;
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 2acf9b336371..4a20a125eb46 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -51,6 +51,19 @@
>   #define BPF_XCHG	(0xe0 | BPF_FETCH)	/* atomic exchange */
>   #define BPF_CMPXCHG	(0xf0 | BPF_FETCH)	/* atomic compare-and-write */
>   
> +#define BPF_ATOMIC_LOAD		0x10
> +#define BPF_ATOMIC_STORE	0x20
> +#define BPF_ATOMIC_TYPE(imm)	((imm) & 0xf0)
> +
> +#define BPF_RELAXED	0x00
> +#define BPF_ACQUIRE	0x01
> +#define BPF_RELEASE	0x02
> +#define BPF_ACQ_REL	0x03
> +#define BPF_SEQ_CST	0x04
> +
> +#define BPF_LOAD_ACQ	(BPF_ATOMIC_LOAD | BPF_ACQUIRE)		/* load-acquire */
> +#define BPF_STORE_REL	(BPF_ATOMIC_STORE | BPF_RELEASE)	/* store-release */
> +
>   enum bpf_cond_pseudo_jmp {
>   	BPF_MAY_GOTO = 0,
>   };
> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> index da729cbbaeb9..ab082ab9d535 100644
> --- a/kernel/bpf/core.c
> +++ b/kernel/bpf/core.c
> @@ -1663,14 +1663,17 @@ EXPORT_SYMBOL_GPL(__bpf_call_base);
>   	INSN_3(JMP, JSET, K),			\
>   	INSN_2(JMP, JA),			\
>   	INSN_2(JMP32, JA),			\
> +	/* Atomic operations. */		\
> +	INSN_3(STX, ATOMIC, B),			\
> +	INSN_3(STX, ATOMIC, H),			\
> +	INSN_3(STX, ATOMIC, W),			\
> +	INSN_3(STX, ATOMIC, DW),		\
>   	/* Store instructions. */		\
>   	/*   Register based. */			\
>   	INSN_3(STX, MEM,  B),			\
>   	INSN_3(STX, MEM,  H),			\
>   	INSN_3(STX, MEM,  W),			\
>   	INSN_3(STX, MEM,  DW),			\
> -	INSN_3(STX, ATOMIC, W),			\
> -	INSN_3(STX, ATOMIC, DW),		\
>   	/*   Immediate based. */		\
>   	INSN_3(ST, MEM, B),			\
>   	INSN_3(ST, MEM, H),			\
> @@ -2169,6 +2172,8 @@ static u64 ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn)
>   
>   	STX_ATOMIC_DW:
>   	STX_ATOMIC_W:
> +	STX_ATOMIC_H:
> +	STX_ATOMIC_B:
>   		switch (IMM) {
>   		ATOMIC_ALU_OP(BPF_ADD, add)
>   		ATOMIC_ALU_OP(BPF_AND, and)
> @@ -2196,6 +2201,38 @@ static u64 ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn)
>   					(atomic64_t *)(unsigned long) (DST + insn->off),
>   					(u64) BPF_R0, (u64) SRC);
>   			break;
> +		case BPF_LOAD_ACQ:
> +			switch (BPF_SIZE(insn->code)) {
> +#define LOAD_ACQUIRE(SIZEOP, SIZE)				\
> +			case BPF_##SIZEOP:			\
> +				DST = (SIZE)smp_load_acquire(	\
> +					(SIZE *)(unsigned long)(SRC + insn->off));	\
> +				break;
> +			LOAD_ACQUIRE(B,   u8)
> +			LOAD_ACQUIRE(H,  u16)
> +			LOAD_ACQUIRE(W,  u32)
> +			LOAD_ACQUIRE(DW, u64)
> +#undef LOAD_ACQUIRE
> +			default:
> +				goto default_label;
> +			}
> +			break;
> +		case BPF_STORE_REL:
> +			switch (BPF_SIZE(insn->code)) {
> +#define STORE_RELEASE(SIZEOP, SIZE)			\
> +			case BPF_##SIZEOP:		\
> +				smp_store_release(	\
> +					(SIZE *)(unsigned long)(DST + insn->off), (SIZE)SRC);	\
> +				break;
> +			STORE_RELEASE(B,   u8)
> +			STORE_RELEASE(H,  u16)
> +			STORE_RELEASE(W,  u32)
> +			STORE_RELEASE(DW, u64)
> +#undef STORE_RELEASE
> +			default:
> +				goto default_label;
> +			}
> +			break;
>   
>   		default:
>   			goto default_label;
> diff --git a/kernel/bpf/disasm.c b/kernel/bpf/disasm.c
> index 309c4aa1b026..2a354a44f209 100644
> --- a/kernel/bpf/disasm.c
> +++ b/kernel/bpf/disasm.c
> @@ -267,6 +267,20 @@ void print_bpf_insn(const struct bpf_insn_cbs *cbs,
>   				BPF_SIZE(insn->code) == BPF_DW ? "64" : "",
>   				bpf_ldst_string[BPF_SIZE(insn->code) >> 3],
>   				insn->dst_reg, insn->off, insn->src_reg);
> +		} else if (BPF_MODE(insn->code) == BPF_ATOMIC &&
> +			   insn->imm == BPF_LOAD_ACQ) {
> +			verbose(cbs->private_data, "(%02x) %s%d = load_acquire((%s *)(r%d %+d))\n",
> +				insn->code,
> +				BPF_SIZE(insn->code) == BPF_DW ? "r" : "w", insn->dst_reg,
> +				bpf_ldst_string[BPF_SIZE(insn->code) >> 3],
> +				insn->src_reg, insn->off);
> +		} else if (BPF_MODE(insn->code) == BPF_ATOMIC &&
> +			   insn->imm == BPF_STORE_REL) {
> +			verbose(cbs->private_data, "(%02x) store_release((%s *)(r%d %+d), %s%d)\n",
> +				insn->code,
> +				bpf_ldst_string[BPF_SIZE(insn->code) >> 3],
> +				insn->dst_reg, insn->off,
> +				BPF_SIZE(insn->code) == BPF_DW ? "r" : "w", insn->src_reg);
>   		} else {
>   			verbose(cbs->private_data, "BUG_%02x\n", insn->code);
>   		}
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index fa40a0440590..dc3ecc925b97 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -3480,7 +3480,7 @@ static bool is_reg64(struct bpf_verifier_env *env, struct bpf_insn *insn,
>   	}
>   
>   	if (class == BPF_STX) {
> -		/* BPF_STX (including atomic variants) has multiple source
> +		/* BPF_STX (including atomic variants) has one or more source
>   		 * operands, one of which is a ptr. Check whether the caller is
>   		 * asking about it.
>   		 */
> @@ -7550,6 +7550,8 @@ static int check_load(struct bpf_verifier_env *env, struct bpf_insn *insn, const
>   
>   static int check_atomic(struct bpf_verifier_env *env, int insn_idx, struct bpf_insn *insn)
>   {
> +	const int bpf_size = BPF_SIZE(insn->code);
> +	bool write_only = false;
>   	int load_reg;
>   	int err;
>   
> @@ -7564,17 +7566,21 @@ static int check_atomic(struct bpf_verifier_env *env, int insn_idx, struct bpf_i
>   	case BPF_XOR | BPF_FETCH:
>   	case BPF_XCHG:
>   	case BPF_CMPXCHG:
> +		if (bpf_size != BPF_W && bpf_size != BPF_DW) {
> +			verbose(env, "invalid atomic operand size\n");
> +			return -EINVAL;
> +		}
> +		break;
> +	case BPF_LOAD_ACQ:
> +		return check_load(env, insn, "atomic");
> +	case BPF_STORE_REL:
> +		write_only = true;
>   		break;
>   	default:
>   		verbose(env, "BPF_ATOMIC uses invalid atomic opcode %02x\n", insn->imm);
>   		return -EINVAL;
>   	}
>   
> -	if (BPF_SIZE(insn->code) != BPF_W && BPF_SIZE(insn->code) != BPF_DW) {
> -		verbose(env, "invalid atomic operand size\n");
> -		return -EINVAL;
> -	}
> -
>   	/* check src1 operand */
>   	err = check_reg_arg(env, insn->src_reg, SRC_OP);
>   	if (err)
> @@ -7615,6 +7621,9 @@ static int check_atomic(struct bpf_verifier_env *env, int insn_idx, struct bpf_i
>   		return -EACCES;
>   	}
>   
> +	if (write_only)
> +		goto skip_read_check;
> +
>   	if (insn->imm & BPF_FETCH) {
>   		if (insn->imm == BPF_CMPXCHG)
>   			load_reg = BPF_REG_0;
> @@ -7636,14 +7645,15 @@ static int check_atomic(struct bpf_verifier_env *env, int insn_idx, struct bpf_i
>   	 * case to simulate the register fill.
>   	 */
>   	err = check_mem_access(env, insn_idx, insn->dst_reg, insn->off,
> -			       BPF_SIZE(insn->code), BPF_READ, -1, true, false);
> +			       bpf_size, BPF_READ, -1, true, false);
>   	if (!err && load_reg >= 0)
>   		err = check_mem_access(env, insn_idx, insn->dst_reg, insn->off,
> -				       BPF_SIZE(insn->code), BPF_READ, load_reg,
> -				       true, false);
> +				       bpf_size, BPF_READ, load_reg, true,
> +				       false);
>   	if (err)
>   		return err;
>   
> +skip_read_check:
>   	if (is_arena_reg(env, insn->dst_reg)) {
>   		err = save_aux_ptr_type(env, PTR_TO_ARENA, false);
>   		if (err)
> @@ -20320,7 +20330,9 @@ static int convert_ctx_accesses(struct bpf_verifier_env *env)
>   			   insn->code == (BPF_ST | BPF_MEM | BPF_W) ||
>   			   insn->code == (BPF_ST | BPF_MEM | BPF_DW)) {
>   			type = BPF_WRITE;
> -		} else if ((insn->code == (BPF_STX | BPF_ATOMIC | BPF_W) ||
> +		} else if ((insn->code == (BPF_STX | BPF_ATOMIC | BPF_B) ||
> +			    insn->code == (BPF_STX | BPF_ATOMIC | BPF_H) ||
> +			    insn->code == (BPF_STX | BPF_ATOMIC | BPF_W) ||
>   			    insn->code == (BPF_STX | BPF_ATOMIC | BPF_DW)) &&
>   			   env->insn_aux_data[i + delta].ptr_type == PTR_TO_ARENA) {
>   			insn->code = BPF_STX | BPF_PROBE_ATOMIC | BPF_SIZE(insn->code);
> diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
> index 2acf9b336371..4a20a125eb46 100644
> --- a/tools/include/uapi/linux/bpf.h
> +++ b/tools/include/uapi/linux/bpf.h
> @@ -51,6 +51,19 @@
>   #define BPF_XCHG	(0xe0 | BPF_FETCH)	/* atomic exchange */
>   #define BPF_CMPXCHG	(0xf0 | BPF_FETCH)	/* atomic compare-and-write */
>   
> +#define BPF_ATOMIC_LOAD		0x10
> +#define BPF_ATOMIC_STORE	0x20
> +#define BPF_ATOMIC_TYPE(imm)	((imm) & 0xf0)
> +
> +#define BPF_RELAXED	0x00
> +#define BPF_ACQUIRE	0x01
> +#define BPF_RELEASE	0x02
> +#define BPF_ACQ_REL	0x03
> +#define BPF_SEQ_CST	0x04
> +
> +#define BPF_LOAD_ACQ	(BPF_ATOMIC_LOAD | BPF_ACQUIRE)		/* load-acquire */
> +#define BPF_STORE_REL	(BPF_ATOMIC_STORE | BPF_RELEASE)	/* store-release */
> +
>   enum bpf_cond_pseudo_jmp {
>   	BPF_MAY_GOTO = 0,
>   };


I think it's better to split the arm64 related changes into two separate
patches: one for adding the arm64 LDAR/STLR instruction encodings, and
the other for adding jit support.

Re: [PATCH RFC bpf-next v1 2/4] bpf: Introduce load-acquire and store-release instructions

[Peilin Ye]

Hi Xu,

Thanks for reviewing this!

On Tue, Dec 24, 2024 at 06:07:14PM +0800, Xu Kuohai wrote:
> On 12/21/2024 9:25 AM, Peilin Ye wrote:
> > +__AARCH64_INSN_FUNCS(load_acq,  0x3FC08000, 0x08C08000)
> > +__AARCH64_INSN_FUNCS(store_rel, 0x3FC08000, 0x08808000)
> 
> I checked Arm Architecture Reference Manual [1].
> 
> Section C6.2.{168,169,170,371,372,373} state that field Rt2 (bits 10-14) and
> Rs (bits 16-20) for LDARB/LDARH/LDAR/STLRB/STLRH and no offset type STLR
> instructions are fixed to (1).
> 
> Section C2.2.2 explains that (1) means a Should-Be-One (SBO) bit.
> 
> And the Glossary section says "Arm strongly recommends that software writes
> the field as all 1s. If software writes a value that is not all 1s, it must
> expect an UNPREDICTABLE or CONSTRAINED UNPREDICTABLE result."
> 
> Although the pre-index type of STLR is an excetpion, it is not used in this
> series. Therefore, both bits 10-14 and 16-20 in mask and value should be set
> to 1s.
> 
> [1] https://developer.arm.com/documentation/ddi0487/latest/

<...>

> > +	insn = aarch64_insn_encode_register(AARCH64_INSN_REGTYPE_RT2, insn,
> > +					    AARCH64_INSN_REG_ZR);
> > +
> > +	return aarch64_insn_encode_register(AARCH64_INSN_REGTYPE_RS, insn,
> > +					    AARCH64_INSN_REG_ZR);
> 
> As explained above, RS and RT2 fields should be fixed to 1s.

I'm already setting Rs and Rt2 to all 1's here, as AARCH64_INSN_REG_ZR
is defined as 31 (0b11111):

	AARCH64_INSN_REG_ZR = 31,

Similar to how load- and store-exclusive instructions are handled
currently:

> >   __AARCH64_INSN_FUNCS(load_ex,	0x3F400000, 0x08400000)
> >   __AARCH64_INSN_FUNCS(store_ex,	0x3F400000, 0x08000000)

For example, in the manual, Rs is all (1)'s for LDXR{,B,H}, and Rt2 is
all (1)'s for both LDXR{,B,H} and STXR{,B,H}.  However, neither Rs nor
Rt2 bits are in the mask, and (1) bits are set manually, see
aarch64_insn_gen_load_store_ex():

  insn = aarch64_insn_encode_register(AARCH64_INSN_REGTYPE_RT2, insn,
                                      AARCH64_INSN_REG_ZR);

  return aarch64_insn_encode_register(AARCH64_INSN_REGTYPE_RS, insn,
                                      state);

(For LDXR{,B,H}, 'state' is A64_ZR, which is just an alias to
AARCH64_INSN_REG_ZR (0b11111).)

- - -

On a related note, I simply grabbed {load,store}_ex's MASK and VALUE,
then set their 15th and 23rd bits to make them load-acquire and
store-release:

  +__AARCH64_INSN_FUNCS(load_acq,  0x3FC08000, 0x08C08000)
  +__AARCH64_INSN_FUNCS(store_rel, 0x3FC08000, 0x08808000)
   __AARCH64_INSN_FUNCS(load_ex,   0x3F400000, 0x08400000)
   __AARCH64_INSN_FUNCS(store_ex,  0x3F400000, 0x08000000)

My question is, should we extend {load,store}_ex's MASK to make them
contain BIT(15) and BIT(23) as well?  As-is, aarch64_insn_is_load_ex()
would return true for a load-acquire.

The only user of aarch64_insn_is_load_ex() seems to be this
arm64-specific kprobe code in arch/arm64/kernel/probes/decode-insn.c:

  #ifdef CONFIG_KPROBES
  static bool __kprobes
  is_probed_address_atomic(kprobe_opcode_t *scan_start, kprobe_opcode_t *scan_end)
  {
          while (scan_start >= scan_end) {
                  /*
                   * atomic region starts from exclusive load and ends with
                   * exclusive store.
                   */
                  if (aarch64_insn_is_store_ex(le32_to_cpu(*scan_start)))
                          return false;
                  else if (aarch64_insn_is_load_ex(le32_to_cpu(*scan_start)))
                          return true;

But I'm not sure yet if changing {load,store}_ex's MASK would affect the
above code.  Do you happen to know the context?

> > +	if (BPF_ATOMIC_TYPE(insn->imm) == BPF_ATOMIC_LOAD)
> > +		ptr = src;
> > +	else
> > +		ptr = dst;
> > +
> > +	if (off) {
> > +		emit_a64_mov_i(true, tmp, off, ctx);
> > +		emit(A64_ADD(true, tmp, tmp, ptr), ctx);
> 
> The mov and add instructions can be optimized to a single A64_ADD_I
> if is_addsub_imm(off) is true.

Thanks!  I'll try this.

> I think it's better to split the arm64 related changes into two separate
> patches: one for adding the arm64 LDAR/STLR instruction encodings, and
> the other for adding jit support.

Got it, in the next version I'll split this patch into (a) core/verifier
changes, (b) arm64 insn.{h,c} changes, and (c) arm64 JIT compiler
support.

Thanks,
Peilin Ye

Re: [PATCH RFC bpf-next v1 2/4] bpf: Introduce load-acquire and store-release instructions

[Peilin Ye]

On Thu, Dec 26, 2024 at 11:07:10PM +0000, Peilin Ye wrote:
> > > +	if (BPF_ATOMIC_TYPE(insn->imm) == BPF_ATOMIC_LOAD)
> > > +		ptr = src;
> > > +	else
> > > +		ptr = dst;
> > > +
> > > +	if (off) {
> > > +		emit_a64_mov_i(true, tmp, off, ctx);
> > > +		emit(A64_ADD(true, tmp, tmp, ptr), ctx);
> > 
> > The mov and add instructions can be optimized to a single A64_ADD_I
> > if is_addsub_imm(off) is true.
> 
> Thanks!  I'll try this.

The following diff seems to work:

--- a/arch/arm64/net/bpf_jit_comp.c
+++ b/arch/arm64/net/bpf_jit_comp.c
@@ -658,9 +658,15 @@ static int emit_atomic_load_store(const struct bpf_insn *insn, struct jit_ctx *c
                ptr = dst;

        if (off) {
-               emit_a64_mov_i(true, tmp, off, ctx);
-               emit(A64_ADD(true, tmp, tmp, ptr), ctx);
-               ptr = tmp;
+               if (is_addsub_imm(off)) {
+                       emit(A64_ADD_I(true, ptr, ptr, off), ctx);
+               } else if (is_addsub_imm(-off)) {
+                       emit(A64_SUB_I(true, ptr, ptr, -off), ctx);
+               } else {
+                       emit_a64_mov_i(true, tmp, off, ctx);
+                       emit(A64_ADD(true, tmp, tmp, ptr), ctx);
+                       ptr = tmp;
+               }
        }
        if (arena) {
                emit(A64_ADD(true, tmp, ptr, arena_vm_base), ctx);

I'll include it in the next version.  I think the same thing can be done
for emit_lse_atomic() and emit_ll_sc_atomic(); let me do that in a
separate patch.

Thanks,
Peilin Ye

Re: [PATCH RFC bpf-next v1 2/4] bpf: Introduce load-acquire and store-release instructions

[Peilin Ye]

On Fri, Dec 27, 2024 at 12:23:22AM +0000, Peilin Ye wrote:
>         if (off) {
> -               emit_a64_mov_i(true, tmp, off, ctx);
> -               emit(A64_ADD(true, tmp, tmp, ptr), ctx);
> -               ptr = tmp;
> +               if (is_addsub_imm(off)) {
> +                       emit(A64_ADD_I(true, ptr, ptr, off), ctx);
                                               ~~~

> +               } else if (is_addsub_imm(-off)) {
> +                       emit(A64_SUB_I(true, ptr, ptr, -off), ctx);
                                               ~~~

No, I must not write to the 'ptr' register here.

> +               } else {
> +                       emit_a64_mov_i(true, tmp, off, ctx);
> +                       emit(A64_ADD(true, tmp, tmp, ptr), ctx);
> +                       ptr = tmp;
> +               }
>         }

I will do this instead:

--- a/arch/arm64/net/bpf_jit_comp.c
+++ b/arch/arm64/net/bpf_jit_comp.c
@@ -658,8 +658,14 @@ static int emit_atomic_load_store(const struct bpf_insn *insn, struct jit_ctx *c
                ptr = dst;

        if (off) {
-               emit_a64_mov_i(true, tmp, off, ctx);
-               emit(A64_ADD(true, tmp, tmp, ptr), ctx);
+               if (is_addsub_imm(off)) {
+                       emit(A64_ADD_I(true, tmp, ptr, off), ctx);
+               } else if (is_addsub_imm(-off)) {
+                       emit(A64_SUB_I(true, tmp, ptr, -off), ctx);
+               } else {
+                       emit_a64_mov_i(true, tmp, off, ctx);
+                       emit(A64_ADD(true, tmp, tmp, ptr), ctx);
+               }
                ptr = tmp;
        }
        if (arena) {

Thanks,
Peilin Ye

Re: [PATCH RFC bpf-next v1 2/4] bpf: Introduce load-acquire and store-release instructions

[Xu Kuohai]

On 12/27/2024 7:07 AM, Peilin Ye wrote:
> Hi Xu,
> 
> Thanks for reviewing this!
> 
> On Tue, Dec 24, 2024 at 06:07:14PM +0800, Xu Kuohai wrote:
>> On 12/21/2024 9:25 AM, Peilin Ye wrote:
>>> +__AARCH64_INSN_FUNCS(load_acq,  0x3FC08000, 0x08C08000)
>>> +__AARCH64_INSN_FUNCS(store_rel, 0x3FC08000, 0x08808000)
>>
>> I checked Arm Architecture Reference Manual [1].
>>
>> Section C6.2.{168,169,170,371,372,373} state that field Rt2 (bits 10-14) and
>> Rs (bits 16-20) for LDARB/LDARH/LDAR/STLRB/STLRH and no offset type STLR
>> instructions are fixed to (1).
>>
>> Section C2.2.2 explains that (1) means a Should-Be-One (SBO) bit.
>>
>> And the Glossary section says "Arm strongly recommends that software writes
>> the field as all 1s. If software writes a value that is not all 1s, it must
>> expect an UNPREDICTABLE or CONSTRAINED UNPREDICTABLE result."
>>
>> Although the pre-index type of STLR is an excetpion, it is not used in this
>> series. Therefore, both bits 10-14 and 16-20 in mask and value should be set
>> to 1s.
>>
>> [1] https://developer.arm.com/documentation/ddi0487/latest/
> 
> <...>
> 
>>> +	insn = aarch64_insn_encode_register(AARCH64_INSN_REGTYPE_RT2, insn,
>>> +					    AARCH64_INSN_REG_ZR);
>>> +
>>> +	return aarch64_insn_encode_register(AARCH64_INSN_REGTYPE_RS, insn,
>>> +					    AARCH64_INSN_REG_ZR);
>>
>> As explained above, RS and RT2 fields should be fixed to 1s.
> 
> I'm already setting Rs and Rt2 to all 1's here, as AARCH64_INSN_REG_ZR
> is defined as 31 (0b11111):
> 
> 	AARCH64_INSN_REG_ZR = 31,
> 

I see, but the setting of fixed bits is smomewhat of a waste of jit time.

> Similar to how load- and store-exclusive instructions are handled
> currently:
> 
>>>    __AARCH64_INSN_FUNCS(load_ex,	0x3F400000, 0x08400000)
>>>    __AARCH64_INSN_FUNCS(store_ex,	0x3F400000, 0x08000000)
> 
> For example, in the manual, Rs is all (1)'s for LDXR{,B,H}, and Rt2 is
> all (1)'s for both LDXR{,B,H} and STXR{,B,H}.  However, neither Rs nor
> Rt2 bits are in the mask, and (1) bits are set manually, see
> aarch64_insn_gen_load_store_ex():
> 
>    insn = aarch64_insn_encode_register(AARCH64_INSN_REGTYPE_RT2, insn,
>                                        AARCH64_INSN_REG_ZR);
> 
>    return aarch64_insn_encode_register(AARCH64_INSN_REGTYPE_RS, insn,
>                                        state);
> 
> (For LDXR{,B,H}, 'state' is A64_ZR, which is just an alias to
> AARCH64_INSN_REG_ZR (0b11111).)
>
> - - -
> 
> On a related note, I simply grabbed {load,store}_ex's MASK and VALUE,
> then set their 15th and 23rd bits to make them load-acquire and
> store-release:
> 
>    +__AARCH64_INSN_FUNCS(load_acq,  0x3FC08000, 0x08C08000)
>    +__AARCH64_INSN_FUNCS(store_rel, 0x3FC08000, 0x08808000)
>     __AARCH64_INSN_FUNCS(load_ex,   0x3F400000, 0x08400000)
>     __AARCH64_INSN_FUNCS(store_ex,  0x3F400000, 0x08000000)
> 
> My question is, should we extend {load,store}_ex's MASK to make them
> contain BIT(15) and BIT(23) as well?  As-is, aarch64_insn_is_load_ex()
> would return true for a load-acquire.
> 
> The only user of aarch64_insn_is_load_ex() seems to be this
> arm64-specific kprobe code in arch/arm64/kernel/probes/decode-insn.c:
> 
>    #ifdef CONFIG_KPROBES
>    static bool __kprobes
>    is_probed_address_atomic(kprobe_opcode_t *scan_start, kprobe_opcode_t *scan_end)
>    {
>            while (scan_start >= scan_end) {
>                    /*
>                     * atomic region starts from exclusive load and ends with
>                     * exclusive store.
>                     */
>                    if (aarch64_insn_is_store_ex(le32_to_cpu(*scan_start)))
>                            return false;
>                    else if (aarch64_insn_is_load_ex(le32_to_cpu(*scan_start)))
>                            return true;
> 
> But I'm not sure yet if changing {load,store}_ex's MASK would affect the
> above code.  Do you happen to know the context?
> 

IIUC, this code prevents kprobe from interrupting the LL-SC loop constructed
by LDXR/STXR pair, as the kprobe trap causes unexpected memory access that
prevents the exclusive memory access loop from exiting.

Since load-acquire/store-release instructions are not used to construct LL-SC
loop, I think it is safe to exclude them from {load,store}_ex.

>>> +	if (BPF_ATOMIC_TYPE(insn->imm) == BPF_ATOMIC_LOAD)
>>> +		ptr = src;
>>> +	else
>>> +		ptr = dst;
>>> +
>>> +	if (off) {
>>> +		emit_a64_mov_i(true, tmp, off, ctx);
>>> +		emit(A64_ADD(true, tmp, tmp, ptr), ctx);
>>
>> The mov and add instructions can be optimized to a single A64_ADD_I
>> if is_addsub_imm(off) is true.
> 
> Thanks!  I'll try this.
> 
>> I think it's better to split the arm64 related changes into two separate
>> patches: one for adding the arm64 LDAR/STLR instruction encodings, and
>> the other for adding jit support.
> 
> Got it, in the next version I'll split this patch into (a) core/verifier
> changes, (b) arm64 insn.{h,c} changes, and (c) arm64 JIT compiler
> support.
>
> Thanks,
> Peilin Ye

Re: [PATCH RFC bpf-next v1 2/4] bpf: Introduce load-acquire and store-release instructions

[Peilin Ye]

On Mon, Dec 30, 2024 at 04:27:21PM +0800, Xu Kuohai wrote:
> > > As explained above, RS and RT2 fields should be fixed to 1s.
> > 
> > I'm already setting Rs and Rt2 to all 1's here, as AARCH64_INSN_REG_ZR
> > is defined as 31 (0b11111):
> > 
> > 	AARCH64_INSN_REG_ZR = 31,
> 
> I see, but the setting of fixed bits is smomewhat of a waste of jit time.

Fair point, I'll instead make load_acq/store_rel's MASK/VALUE include
those (1) bits.

> > On a related note, I simply grabbed {load,store}_ex's MASK and VALUE,
> > then set their 15th and 23rd bits to make them load-acquire and
> > store-release:
> > 
> >    +__AARCH64_INSN_FUNCS(load_acq,  0x3FC08000, 0x08C08000)
> >    +__AARCH64_INSN_FUNCS(store_rel, 0x3FC08000, 0x08808000)
> >     __AARCH64_INSN_FUNCS(load_ex,   0x3F400000, 0x08400000)
> >     __AARCH64_INSN_FUNCS(store_ex,  0x3F400000, 0x08000000)
> > 
> > My question is, should we extend {load,store}_ex's MASK to make them
> > contain BIT(15) and BIT(23) as well?  As-is, aarch64_insn_is_load_ex()
> > would return true for a load-acquire.
> > 
> > The only user of aarch64_insn_is_load_ex() seems to be this
> > arm64-specific kprobe code in arch/arm64/kernel/probes/decode-insn.c:
> > 
> >    #ifdef CONFIG_KPROBES
> >    static bool __kprobes
> >    is_probed_address_atomic(kprobe_opcode_t *scan_start, kprobe_opcode_t *scan_end)
> >    {
> >            while (scan_start >= scan_end) {
> >                    /*
> >                     * atomic region starts from exclusive load and ends with
> >                     * exclusive store.
> >                     */
> >                    if (aarch64_insn_is_store_ex(le32_to_cpu(*scan_start)))
> >                            return false;
> >                    else if (aarch64_insn_is_load_ex(le32_to_cpu(*scan_start)))
> >                            return true;
> > 
> > But I'm not sure yet if changing {load,store}_ex's MASK would affect the
> > above code.  Do you happen to know the context?
> 
> IIUC, this code prevents kprobe from interrupting the LL-SC loop constructed
> by LDXR/STXR pair, as the kprobe trap causes unexpected memory access that
> prevents the exclusive memory access loop from exiting.
>
> Since load-acquire/store-release instructions are not used to construct LL-SC
> loop, I think it is safe to exclude them from {load,store}_ex.

Ah, I see, thanks!  I'll extend {load,store}_ex's MASK to prevent
aarch64_insn_is_{load,store}_ex() from returning false-positives for
load-acquire/store-release.

Thanks,
Peilin Ye

Re: [PATCH RFC bpf-next v1 2/4] bpf: Introduce load-acquire and store-release instructions

[Eduard Zingerman]

On Sat, 2024-12-21 at 01:25 +0000, Peilin Ye wrote:
> Introduce BPF instructions with load-acquire and store-release
> semantics, as discussed in [1].  The following new flags are defined:

The '[1]' link is missing.

>   BPF_ATOMIC_LOAD         0x10
>   BPF_ATOMIC_STORE        0x20
>   BPF_ATOMIC_TYPE(imm)    ((imm) & 0xf0)
> 
>   BPF_RELAXED        0x0
>   BPF_ACQUIRE        0x1
>   BPF_RELEASE        0x2
>   BPF_ACQ_REL        0x3
>   BPF_SEQ_CST        0x4
> 
>   BPF_LOAD_ACQ       (BPF_ATOMIC_LOAD | BPF_ACQUIRE)
>   BPF_STORE_REL      (BPF_ATOMIC_STORE | BPF_RELEASE)
> 
> A "load-acquire" is a BPF_STX | BPF_ATOMIC instruction with the 'imm'
> field set to BPF_LOAD_ACQ (0x11).
> 
> Similarly, a "store-release" is a BPF_STX | BPF_ATOMIC instruction with
> the 'imm' field set to BPF_STORE_REL (0x22).

[...]

> diff --git a/kernel/bpf/disasm.c b/kernel/bpf/disasm.c
> index 309c4aa1b026..2a354a44f209 100644
> --- a/kernel/bpf/disasm.c
> +++ b/kernel/bpf/disasm.c
> @@ -267,6 +267,20 @@ void print_bpf_insn(const struct bpf_insn_cbs *cbs,
>  				BPF_SIZE(insn->code) == BPF_DW ? "64" : "",
>  				bpf_ldst_string[BPF_SIZE(insn->code) >> 3],
>  				insn->dst_reg, insn->off, insn->src_reg);
> +		} else if (BPF_MODE(insn->code) == BPF_ATOMIC &&
> +			   insn->imm == BPF_LOAD_ACQ) {
> +			verbose(cbs->private_data, "(%02x) %s%d = load_acquire((%s *)(r%d %+d))\n",
> +				insn->code,
> +				BPF_SIZE(insn->code) == BPF_DW ? "r" : "w", insn->dst_reg,

Nit: I think that 'BPF_DW ? "r" : "w"' part is not really necessary.

> +				bpf_ldst_string[BPF_SIZE(insn->code) >> 3],
> +				insn->src_reg, insn->off);
> +		} else if (BPF_MODE(insn->code) == BPF_ATOMIC &&
> +			   insn->imm == BPF_STORE_REL) {
> +			verbose(cbs->private_data, "(%02x) store_release((%s *)(r%d %+d), %s%d)\n",
> +				insn->code,
> +				bpf_ldst_string[BPF_SIZE(insn->code) >> 3],
> +				insn->dst_reg, insn->off,
> +				BPF_SIZE(insn->code) == BPF_DW ? "r" : "w", insn->src_reg);
>  		} else {
>  			verbose(cbs->private_data, "BUG_%02x\n", insn->code);
>  		}
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index fa40a0440590..dc3ecc925b97 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -3480,7 +3480,7 @@ static bool is_reg64(struct bpf_verifier_env *env, struct bpf_insn *insn,
>  	}
>  
>  	if (class == BPF_STX) {
> -		/* BPF_STX (including atomic variants) has multiple source
> +		/* BPF_STX (including atomic variants) has one or more source
>  		 * operands, one of which is a ptr. Check whether the caller is
>  		 * asking about it.
>  		 */
> @@ -7550,6 +7550,8 @@ static int check_load(struct bpf_verifier_env *env, struct bpf_insn *insn, const
>  
>  static int check_atomic(struct bpf_verifier_env *env, int insn_idx, struct bpf_insn *insn)
>  {
> +	const int bpf_size = BPF_SIZE(insn->code);
> +	bool write_only = false;
>  	int load_reg;
>  	int err;
>  
> @@ -7564,17 +7566,21 @@ static int check_atomic(struct bpf_verifier_env *env, int insn_idx, struct bpf_i
>  	case BPF_XOR | BPF_FETCH:
>  	case BPF_XCHG:
>  	case BPF_CMPXCHG:
> +		if (bpf_size != BPF_W && bpf_size != BPF_DW) {
> +			verbose(env, "invalid atomic operand size\n");
> +			return -EINVAL;
> +		}
> +		break;
> +	case BPF_LOAD_ACQ:

Several notes here:
- This skips the 'bpf_jit_supports_insn()' call at the end of the function.
- Also 'check_load()' allows source register to be PTR_TO_CTX,
  but convert_ctx_access() is not adjusted to handle these atomic instructions.
  (Just in case: context access is special, context structures are not "real",
   e.g. during runtime real sk_buff is passed to the program, not __sk_buff,
   in convert_ctx_access() verifier adjusts offsets of load and store instructions
   to point to real fields, this is done per program type, e.g. see
   filter.c:bpf_convert_ctx_access);
- backtrack_insn() needs special rules to handle BPF_LOAD_ACQ same way
  it handles loads.

> +		return check_load(env, insn, "atomic");
> +	case BPF_STORE_REL:
> +		write_only = true;
>  		break;
>  	default:
>  		verbose(env, "BPF_ATOMIC uses invalid atomic opcode %02x\n", insn->imm);
>  		return -EINVAL;
>  	}
>  
> -	if (BPF_SIZE(insn->code) != BPF_W && BPF_SIZE(insn->code) != BPF_DW) {
> -		verbose(env, "invalid atomic operand size\n");
> -		return -EINVAL;
> -	}
> -
>  	/* check src1 operand */
>  	err = check_reg_arg(env, insn->src_reg, SRC_OP);
>  	if (err)

Note: this code fragment looks as follows:

	/* check src1 operand */
	err = check_reg_arg(env, insn->src_reg, SRC_OP);
	if (err)
		return err;

	/* check src2 operand */
	err = check_reg_arg(env, insn->dst_reg, SRC_OP);
	if (err)
		return err;

And there is no need for 'check_reg_arg(env, insn->dst_reg, SRC_OP)'
for BPF_STORE_REL.

> @@ -7615,6 +7621,9 @@ static int check_atomic(struct bpf_verifier_env *env, int insn_idx, struct bpf_i
>  		return -EACCES;
>  	}
>  
> +	if (write_only)
> +		goto skip_read_check;
> +
>  	if (insn->imm & BPF_FETCH) {
>  		if (insn->imm == BPF_CMPXCHG)
>  			load_reg = BPF_REG_0;
> @@ -7636,14 +7645,15 @@ static int check_atomic(struct bpf_verifier_env *env, int insn_idx, struct bpf_i
>  	 * case to simulate the register fill.
>  	 */
>  	err = check_mem_access(env, insn_idx, insn->dst_reg, insn->off,
> -			       BPF_SIZE(insn->code), BPF_READ, -1, true, false);
> +			       bpf_size, BPF_READ, -1, true, false);
>  	if (!err && load_reg >= 0)
>  		err = check_mem_access(env, insn_idx, insn->dst_reg, insn->off,
> -				       BPF_SIZE(insn->code), BPF_READ, load_reg,
> -				       true, false);
> +				       bpf_size, BPF_READ, load_reg, true,
> +				       false);
>  	if (err)
>  		return err;
>  
> +skip_read_check:
>  	if (is_arena_reg(env, insn->dst_reg)) {
>  		err = save_aux_ptr_type(env, PTR_TO_ARENA, false);
>  		if (err)

[...]

Re: [PATCH RFC bpf-next v1 4/4] selftests/bpf: Add selftests for load-acquire and store-release instructions

[Eduard Zingerman]

On Sat, 2024-12-21 at 01:26 +0000, Peilin Ye wrote:
> Add the following ./test_progs tests:
> 
>   * atomics/load_acquire
>   * atomics/store_release
>   * arena_atomics/load_acquire
>   * arena_atomics/store_release

[...]

> ---
>  include/linux/filter.h                        |  2 +
>  .../selftests/bpf/prog_tests/arena_atomics.c  | 61 +++++++++++++++-
>  .../selftests/bpf/prog_tests/atomics.c        | 57 ++++++++++++++-
>  .../selftests/bpf/progs/arena_atomics.c       | 62 +++++++++++++++-
>  tools/testing/selftests/bpf/progs/atomics.c   | 62 +++++++++++++++-
>  .../selftests/bpf/verifier/atomic_invalid.c   | 26 +++----
>  .../selftests/bpf/verifier/atomic_load.c      | 71 +++++++++++++++++++
>  .../selftests/bpf/verifier/atomic_store.c     | 70 ++++++++++++++++++
>  8 files changed, 393 insertions(+), 18 deletions(-)
>  create mode 100644 tools/testing/selftests/bpf/verifier/atomic_load.c
>  create mode 100644 tools/testing/selftests/bpf/verifier/atomic_store.c

test_verifier tests runner is currently in a maintenance mode,
new tests should be added as parts of a test_progs runner.
Please take a look at selftests/bpf/progs/verifier_ldsx.c,
selftests/bpf/prog_tests/verifier.c and selftests/bpf/bpf_misc.h.

[...]

Re: [PATCH RFC bpf-next v1 2/4] bpf: Introduce load-acquire and store-release instructions

[Peilin Ye]

Hi Eduard,

Thanks for the review!

On Fri, Jan 03, 2025 at 04:12:08PM -0800, Eduard Zingerman wrote:
> On Sat, 2024-12-21 at 01:25 +0000, Peilin Ye wrote:
> > Introduce BPF instructions with load-acquire and store-release
> > semantics, as discussed in [1].  The following new flags are defined:
> 
> The '[1]' link is missing.

Oops, thanks.

[1] https://lore.kernel.org/all/20240729183246.4110549-1-yepeilin@google.com/

> > --- a/kernel/bpf/disasm.c
> > +++ b/kernel/bpf/disasm.c
> > @@ -267,6 +267,20 @@ void print_bpf_insn(const struct bpf_insn_cbs *cbs,
> >  				BPF_SIZE(insn->code) == BPF_DW ? "64" : "",
> >  				bpf_ldst_string[BPF_SIZE(insn->code) >> 3],
> >  				insn->dst_reg, insn->off, insn->src_reg);
> > +		} else if (BPF_MODE(insn->code) == BPF_ATOMIC &&
> > +			   insn->imm == BPF_LOAD_ACQ) {
> > +			verbose(cbs->private_data, "(%02x) %s%d = load_acquire((%s *)(r%d %+d))\n",
> > +				insn->code,
> > +				BPF_SIZE(insn->code) == BPF_DW ? "r" : "w", insn->dst_reg,
> 
> Nit: I think that 'BPF_DW ? "r" : "w"' part is not really necessary.

We already do that in other places in the file, so I wanted to keep it
consistent:

  $ git grep "? 'w' : 'r'" kernel/bpf/disasm.c | wc -l
  8

(Though I just realized that I could've used '%c' instead of '%s'.)

> >  static int check_atomic(struct bpf_verifier_env *env, int insn_idx, struct bpf_insn *insn)
> >  {
> > +	const int bpf_size = BPF_SIZE(insn->code);
> > +	bool write_only = false;
> >  	int load_reg;
> >  	int err;
> >  
> > @@ -7564,17 +7566,21 @@ static int check_atomic(struct bpf_verifier_env *env, int insn_idx, struct bpf_i
> >  	case BPF_XOR | BPF_FETCH:
> >  	case BPF_XCHG:
> >  	case BPF_CMPXCHG:
> > +		if (bpf_size != BPF_W && bpf_size != BPF_DW) {
> > +			verbose(env, "invalid atomic operand size\n");
> > +			return -EINVAL;
> > +		}
> > +		break;
> > +	case BPF_LOAD_ACQ:
> 
> Several notes here:
> - This skips the 'bpf_jit_supports_insn()' call at the end of the function.
> - Also 'check_load()' allows source register to be PTR_TO_CTX,
>   but convert_ctx_access() is not adjusted to handle these atomic instructions.
>   (Just in case: context access is special, context structures are not "real",
>    e.g. during runtime real sk_buff is passed to the program, not __sk_buff,
>    in convert_ctx_access() verifier adjusts offsets of load and store instructions
>    to point to real fields, this is done per program type, e.g. see
>    filter.c:bpf_convert_ctx_access);

I see, thanks for pointing these out!  I'll add logic to handle
BPF_LOAD_ACQ in check_atomic() directly, instead of introducing
check_load().  I'll disallow using BPF_LOAD_ACQ with src_reg being
PTR_TO_CTX (just like all existing BPF_ATOMIC instructions), as we don't
think there'll be a use case for it.

> - backtrack_insn() needs special rules to handle BPF_LOAD_ACQ same way
>   it handles loads.

Got it, I'll read backtrack_insn().

> > +		return check_load(env, insn, "atomic");
> > +	case BPF_STORE_REL:
> > +		write_only = true;
> >  		break;
> >  	default:
> >  		verbose(env, "BPF_ATOMIC uses invalid atomic opcode %02x\n", insn->imm);
> >  		return -EINVAL;
> >  	}
> >  
> > -	if (BPF_SIZE(insn->code) != BPF_W && BPF_SIZE(insn->code) != BPF_DW) {
> > -		verbose(env, "invalid atomic operand size\n");
> > -		return -EINVAL;
> > -	}
> > -
> >  	/* check src1 operand */
> >  	err = check_reg_arg(env, insn->src_reg, SRC_OP);
> >  	if (err)
> 
> Note: this code fragment looks as follows:
> 
> 	/* check src1 operand */
> 	err = check_reg_arg(env, insn->src_reg, SRC_OP);
> 	if (err)
> 		return err;
> 
> 	/* check src2 operand */
> 	err = check_reg_arg(env, insn->dst_reg, SRC_OP);
> 	if (err)
> 		return err;
> 
> And there is no need for 'check_reg_arg(env, insn->dst_reg, SRC_OP)'
> for BPF_STORE_REL.

Why is that?  IIUC, 'check_reg_arg(..., SRC_OP)' checks if we can read
the register, instead of the memory?  For example, doing
'check_reg_arg(env, insn->dst_reg, SRC_OP)' prevents BPF_STORE_REL from
using an uninitialized dst_reg.

We also do this check for BPF_ST in do_check():

  } else if (class == BPF_ST) {
          enum bpf_reg_type dst_reg_type;
<...>
          /* check src operand */
          err = check_reg_arg(env, insn->dst_reg, SRC_OP);
          if (err)
                  return err;

Thanks,
Peilin Ye

Re: [PATCH RFC bpf-next v1 4/4] selftests/bpf: Add selftests for load-acquire and store-release instructions

[Peilin Ye]

On Fri, Jan 03, 2025 at 05:11:11PM -0800, Eduard Zingerman wrote:
> test_verifier tests runner is currently in a maintenance mode,
> new tests should be added as parts of a test_progs runner.
> Please take a look at selftests/bpf/progs/verifier_ldsx.c,
> selftests/bpf/prog_tests/verifier.c and selftests/bpf/bpf_misc.h.

I see, thanks!

Peilin Ye

Re: [PATCH RFC bpf-next v1 2/4] bpf: Introduce load-acquire and store-release instructions

[Eduard Zingerman]

On Tue, 2025-01-07 at 01:08 +0000, Peilin Ye wrote:

Hi Peilin,

[...]

> > > --- a/kernel/bpf/disasm.c
> > > +++ b/kernel/bpf/disasm.c
> > > @@ -267,6 +267,20 @@ void print_bpf_insn(const struct bpf_insn_cbs *cbs,
> > >  				BPF_SIZE(insn->code) == BPF_DW ? "64" : "",
> > >  				bpf_ldst_string[BPF_SIZE(insn->code) >> 3],
> > >  				insn->dst_reg, insn->off, insn->src_reg);
> > > +		} else if (BPF_MODE(insn->code) == BPF_ATOMIC &&
> > > +			   insn->imm == BPF_LOAD_ACQ) {
> > > +			verbose(cbs->private_data, "(%02x) %s%d = load_acquire((%s *)(r%d %+d))\n",
> > > +				insn->code,
> > > +				BPF_SIZE(insn->code) == BPF_DW ? "r" : "w", insn->dst_reg,
> > 
> > Nit: I think that 'BPF_DW ? "r" : "w"' part is not really necessary.
> 
> We already do that in other places in the file, so I wanted to keep it
> consistent:
> 
>   $ git grep "? 'w' : 'r'" kernel/bpf/disasm.c | wc -l
>   8
> 
> (Though I just realized that I could've used '%c' instead of '%s'.)

These are used for operations that can have either BPF_ALU or
BPF_ALU32 class. I don't think there is such distinction for
BPF_LOAD_ACQ / BPF_STORE_REL.

> > >  static int check_atomic(struct bpf_verifier_env *env, int insn_idx, struct bpf_insn *insn)
> > >  {
> > > +	const int bpf_size = BPF_SIZE(insn->code);
> > > +	bool write_only = false;
> > >  	int load_reg;
> > >  	int err;
> > >  
> > > @@ -7564,17 +7566,21 @@ static int check_atomic(struct bpf_verifier_env *env, int insn_idx, struct bpf_i
> > >  	case BPF_XOR | BPF_FETCH:
> > >  	case BPF_XCHG:
> > >  	case BPF_CMPXCHG:
> > > +		if (bpf_size != BPF_W && bpf_size != BPF_DW) {
> > > +			verbose(env, "invalid atomic operand size\n");
> > > +			return -EINVAL;
> > > +		}
> > > +		break;
> > > +	case BPF_LOAD_ACQ:
> > 
> > Several notes here:
> > - This skips the 'bpf_jit_supports_insn()' call at the end of the function.
> > - Also 'check_load()' allows source register to be PTR_TO_CTX,
> >   but convert_ctx_access() is not adjusted to handle these atomic instructions.
> >   (Just in case: context access is special, context structures are not "real",
> >    e.g. during runtime real sk_buff is passed to the program, not __sk_buff,
> >    in convert_ctx_access() verifier adjusts offsets of load and store instructions
> >    to point to real fields, this is done per program type, e.g. see
> >    filter.c:bpf_convert_ctx_access);
> 
> I see, thanks for pointing these out!  I'll add logic to handle
> BPF_LOAD_ACQ in check_atomic() directly, instead of introducing
> check_load().  I'll disallow using BPF_LOAD_ACQ with src_reg being
> PTR_TO_CTX (just like all existing BPF_ATOMIC instructions), as we don't
> think there'll be a use case for it.
 
(Just in case: the full list of types currently disallowed for atomics is:
 is_ctx_reg, is_pkt_reg, is_flow_key_reg, is_sk_reg, is_arena_reg,
 see slightly below in the same function).

[...]

> > And there is no need for 'check_reg_arg(env, insn->dst_reg, SRC_OP)'
> > for BPF_STORE_REL.
> 
> Why is that?  IIUC, 'check_reg_arg(..., SRC_OP)' checks if we can read
> the register, instead of the memory?  For example, doing
> 'check_reg_arg(env, insn->dst_reg, SRC_OP)' prevents BPF_STORE_REL from
> using an uninitialized dst_reg.
> 
> We also do this check for BPF_ST in do_check():
> 
>   } else if (class == BPF_ST) {
>           enum bpf_reg_type dst_reg_type;
> <...>
>           /* check src operand */
>           err = check_reg_arg(env, insn->dst_reg, SRC_OP);
>           if (err)
>                   return err;

Sorry, my bad, the 'check_reg_arg(env, insn->dst_reg, SRC_OP)'
is necessary and is done for BPF_STX as well.

Thanks,
Eduard

Re: [PATCH RFC bpf-next v1 2/4] bpf: Introduce load-acquire and store-release instructions

[Peilin Ye]

On Mon, Jan 06, 2025 at 05:20:56PM -0800, Eduard Zingerman wrote:
> > > > --- a/kernel/bpf/disasm.c
> > > > +++ b/kernel/bpf/disasm.c
> > > > @@ -267,6 +267,20 @@ void print_bpf_insn(const struct bpf_insn_cbs *cbs,
> > > >  				BPF_SIZE(insn->code) == BPF_DW ? "64" : "",
> > > >  				bpf_ldst_string[BPF_SIZE(insn->code) >> 3],
> > > >  				insn->dst_reg, insn->off, insn->src_reg);
> > > > +		} else if (BPF_MODE(insn->code) == BPF_ATOMIC &&
> > > > +			   insn->imm == BPF_LOAD_ACQ) {
> > > > +			verbose(cbs->private_data, "(%02x) %s%d = load_acquire((%s *)(r%d %+d))\n",
> > > > +				insn->code,
> > > > +				BPF_SIZE(insn->code) == BPF_DW ? "r" : "w", insn->dst_reg,
> > > 
> > > Nit: I think that 'BPF_DW ? "r" : "w"' part is not really necessary.
> > 
> > We already do that in other places in the file, so I wanted to keep it
> > consistent:
> > 
> >   $ git grep "? 'w' : 'r'" kernel/bpf/disasm.c | wc -l
> >   8
> > 
> > (Though I just realized that I could've used '%c' instead of '%s'.)
> 
> These are used for operations that can have either BPF_ALU or
> BPF_ALU32 class. I don't think there is such distinction for
> BPF_LOAD_ACQ / BPF_STORE_REL.

I see; I just realized that the same instruction can be disassembled
differently by llvm-objdump, depending on --mcpu= version.  For example:

  63 21 00 00 00 00 00 00
  opcode (0x63): BPF_MEM | BPF_W | BPF_STX

  --mcpu=v3:           *(u32 *)(r1 + 0x0) = w2
  --mcpu=v2 (NoALU32): *(u32 *)(r1 + 0x0) = r2
                                            ^^

So from kernel's perspective, it doesn't really matter if it's 'r' or
'w', if the encoding is the same.  I'll remove the 'BPF_DW ? "r" : "w"'
part and make it always use 'r'.

> > > >  static int check_atomic(struct bpf_verifier_env *env, int insn_idx, struct bpf_insn *insn)
> > > >  {
> > > > +	const int bpf_size = BPF_SIZE(insn->code);
> > > > +	bool write_only = false;
> > > >  	int load_reg;
> > > >  	int err;
> > > >  
> > > > @@ -7564,17 +7566,21 @@ static int check_atomic(struct bpf_verifier_env *env, int insn_idx, struct bpf_i
> > > >  	case BPF_XOR | BPF_FETCH:
> > > >  	case BPF_XCHG:
> > > >  	case BPF_CMPXCHG:
> > > > +		if (bpf_size != BPF_W && bpf_size != BPF_DW) {
> > > > +			verbose(env, "invalid atomic operand size\n");
> > > > +			return -EINVAL;
> > > > +		}
> > > > +		break;
> > > > +	case BPF_LOAD_ACQ:
> > > 
> > > Several notes here:
> > > - This skips the 'bpf_jit_supports_insn()' call at the end of the function.
> > > - Also 'check_load()' allows source register to be PTR_TO_CTX,
> > >   but convert_ctx_access() is not adjusted to handle these atomic instructions.
> > >   (Just in case: context access is special, context structures are not "real",
> > >    e.g. during runtime real sk_buff is passed to the program, not __sk_buff,
> > >    in convert_ctx_access() verifier adjusts offsets of load and store instructions
> > >    to point to real fields, this is done per program type, e.g. see
> > >    filter.c:bpf_convert_ctx_access);
> > 
> > I see, thanks for pointing these out!  I'll add logic to handle
> > BPF_LOAD_ACQ in check_atomic() directly, instead of introducing
> > check_load().  I'll disallow using BPF_LOAD_ACQ with src_reg being
> > PTR_TO_CTX (just like all existing BPF_ATOMIC instructions), as we don't
> > think there'll be a use case for it.
>  
> (Just in case: the full list of types currently disallowed for atomics is:
>  is_ctx_reg, is_pkt_reg, is_flow_key_reg, is_sk_reg,

>  is_arena_reg,

...if !bpf_jit_supports_insn(), right.

>  see slightly below in the same function).

Thanks,
Peilin Ye