Introduce load-acquire and store-release BPF instructions

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:

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

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

[1] https://lore.kernel.org/all/[email protected]/

Signed-off-by Peilin Ye <[email protected]>

---

kernel/bpf/verifier.c +15 -0

+static int check_atomic_load(struct bpf_verifier_env *env, int insn_idx,
+			     struct bpf_insn *insn)
+{
+	struct bpf_reg_state *regs = cur_regs(env);
+	int err;
+
+	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;
+
+	if (!atomic_ptr_type_ok(env, insn->src_reg, insn)) {

kernel/bpf/verifier.c +55 -0

+	}
+
+	/* Check whether we can read the memory. */
+	err = check_mem_access(env, insn_idx, insn->src_reg, insn->off,
+			       BPF_SIZE(insn->code), BPF_READ, insn->dst_reg,
+			       true, false);
+	if (err)
+		return err;
+
+	err = reg_bounds_sanity_check(env, &regs[insn->dst_reg], "atomic_load");
+	if (err)
+		return err;
+	return 0;
+}
+
+static int check_atomic_store(struct bpf_verifier_env *env, int insn_idx,
+			      struct bpf_insn *insn)
+{
+	int err;
+
+	err = check_reg_arg(env, insn->src_reg, SRC_OP);
+	if (err)
+		return err;
+
+	err = check_reg_arg(env, insn->dst_reg, SRC_OP);
+	if (err)
+		return err;
+
+	if (is_pointer_value(env, insn->src_reg)) {
+		verbose(env, "R%d leaks addr into mem\n", insn->src_reg);
+		return -EACCES;
+	}
+
+	if (!atomic_ptr_type_ok(env, insn->dst_reg, insn)) {
+		verbose(env, "BPF_ATOMIC stores into R%d %s is not allowed\n",
+			insn->dst_reg,
+			reg_type_str(env, reg_state(env, insn->dst_reg)->type));
+		return -EACCES;
+	}
+
+	if (is_arena_reg(env, insn->dst_reg)) {
+		err = save_aux_ptr_type(env, PTR_TO_ARENA, false);
+		if (err)
+			return err;
+	}
+
+	/* Check whether we can write into the memory. */
+	err = check_mem_access(env, insn_idx, insn->dst_reg, insn->off,
+			       BPF_SIZE(insn->code), BPF_WRITE, insn->src_reg,
+			       true, false);
+	if (err)
+		return err;
+	return 0;
+}
+

Quoted 2 lines · Peilin Ye

Signed-off-by Peilin Ye <[email protected]>

---

I think bpf_jit_supports_insn() in arch/{x86,s390}/net/bpf_jit_comp.c need an update, as both would accept BPF_LOAD_ACQ/BPF_STORE_REL at the moment.

Acked-by Eduard Zingerman <[email protected]>

Quoted 21 lines

kernel/bpf/verifier.c +21 -0

+static int check_atomic_load(struct bpf_verifier_env *env, int insn_idx,
+			     struct bpf_insn *insn)
+{
+	struct bpf_reg_state *regs = cur_regs(env);
+	int err;
+
+	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;
+
+	if (!atomic_ptr_type_ok(env, insn->src_reg, insn)) {
+		verbose(env, "BPF_ATOMIC loads from R%d %s is not allowed\n",
+			insn->src_reg,
+			reg_type_str(env, reg_state(env, insn->src_reg)->type));
+		return -EACCES;
+	}
+

kernel/bpf/verifier.c static int check_atomic_rmw(struct bpf_verifier_env *env, int insn_idx, · line 7659

Quoted 4 lines

kernel/bpf/verifier.c +4 -0

+	if (is_arena_reg(env, insn->src_reg)) {
+		err = save_aux_ptr_type(env, PTR_TO_ARENA, false);
+		if (err)
+			return err;

Nit: this and the next function look very similar to processing of
generic load and store in do_check(). Maybe extract that code as an auxiliary function and call it in both places?

The only major difference is is_arena_reg() check guarding save_aux_ptr_type(), but I think it is ok to do save_aux_ptr_type unconditionally. Fwiw, the code would be a bit simpler,
just spent half an hour convincing myself that such conditional handling is not an error. Wdyt?

Quoted 9 lines

kernel/bpf/verifier.c +9 -0

+	}
+
+	/* Check whether we can read the memory. */
+	err = check_mem_access(env, insn_idx, insn->src_reg, insn->off,
+			       BPF_SIZE(insn->code), BPF_READ, insn->dst_reg,
+			       true, false);
+	if (err)
+		return err;
+

kernel/bpf/verifier.c static int check_atomic_rmw(struct bpf_verifier_env *env, int insn_idx, · line 7676

Quoted 8 lines

kernel/bpf/verifier.c +8 -0

+	err = reg_bounds_sanity_check(env, &regs[insn->dst_reg], "atomic_load");
+	if (err)
+		return err;
+	return 0;
+}
+
+static int check_atomic_store(struct bpf_verifier_env *env, int insn_idx,
+			      struct bpf_insn *insn)

Quoted 21 lines

Quoted 21 lines

kernel/bpf/verifier.c +21 -0

+static int check_atomic_load(struct bpf_verifier_env *env, int insn_idx,
+			     struct bpf_insn *insn)
+{
+	struct bpf_reg_state *regs = cur_regs(env);
+	int err;
+
+	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;
+
+	if (!atomic_ptr_type_ok(env, insn->src_reg, insn)) {
+		verbose(env, "BPF_ATOMIC loads from R%d %s is not allowed\n",
+			insn->src_reg,
+			reg_type_str(env, reg_state(env, insn->src_reg)->type));
+		return -EACCES;
+	}
+

Quoted 8 lines

kernel/bpf/verifier.c static int check_atomic_rmw(struct bpf_verifier_env *env, int insn_idx, · line 7659

Quoted 4 lines

kernel/bpf/verifier.c +4 -0

+	if (is_arena_reg(env, insn->src_reg)) {
+		err = save_aux_ptr_type(env, PTR_TO_ARENA, false);
+		if (err)
+			return err;

Nit: this and the next function look very similar to processing of
generic load and store in do_check(). Maybe extract that code as an auxiliary function and call it in both places?

Sure, I agree that they look a bit repetitive.

Quoted 5 lines

The only major difference is is_arena_reg() check guarding save_aux_ptr_type(), but I think it is ok to do save_aux_ptr_type unconditionally. Fwiw, the code would be a bit simpler,
just spent half an hour convincing myself that such conditional handling is not an error. Wdyt?

:-O

Thanks a lot for that; would you mind sharing a bit more on how you reasoned about it (i.e., why is it OK to save_aux_ptr_type() unconditionally) ?

Quoted 3 lines · Peilin Ye

Thanks a lot for that; would you mind sharing a bit more on how you reasoned about it (i.e., why is it OK to save_aux_ptr_type() unconditionally) ?

Well, save_aux_ptr_type() does two things:
- if there is no env->insn_aux_data[env->insn_idx].ptr_type associated with the instruction it saves one;
- if there is .ptr_type, it checks if a new one is compatible and errors out if it's not.

The .ptr_type is used in convert_ctx_accesses() to rewrite access instruction (STX/LDX, atomic or not) in a way specific to pointer type.

So, doing save_aux_ptr_type() conditionally is already sketchy, as there is a risk to miss if some instruction is used in a context where pointer type requires different rewrites.

convert_ctx_accesses() rewrites instruction for pointer following types:
- PTR_TO_CTX
- PTR_TO_SOCKET
- PTR_TO_SOCK_COMMON
- PTR_TO_TCP_SOCK
- PTR_TO_XDP_SOCK
- PTR_TO_BTF_ID
- PTR_TO_ARENA

atomic_ptr_type_ok() allows the following pointer types:
- CONST_PTR_TO_MAP
- PTR_TO_MAP_VALUE
- PTR_TO_MAP_KEY
- PTR_TO_STACK
- PTR_TO_BTF_ID
- PTR_TO_MEM
- PTR_TO_ARENA
- PTR_TO_BUF
- PTR_TO_FUNC
- CONST_PTR_TO_DYNPTR

One has to check rewrites applied by convert_ctx_accesses() to atomic instructions to reason about correctness of the conditional save_aux_ptr_type() call.

If is_arena_reg() guard is removed from save_aux_ptr_type() we risk to reject programs that do atomic load/store where same instruction is used to modify a pointer that can be either of the above types. I speculate that this is not the problem, as do_check() processing for BPF_STX/BPF_LDX already calls save_aux_ptr_type() unconditionally.

kernel/bpf/verifier.c +7 -0

+static int check_atomic_store(struct bpf_verifier_env *env, int insn_idx,
+			      struct bpf_insn *insn)
+{
+	int err;
+
+	err = check_reg_arg(env, insn->src_reg, SRC_OP);
+	if (err)

kernel/bpf/verifier.c +22 -0

+
+	if (!atomic_ptr_type_ok(env, insn->dst_reg, insn)) {
+		verbose(env, "BPF_ATOMIC stores into R%d %s is not allowed\n",
+			insn->dst_reg,
+			reg_type_str(env, reg_state(env, insn->dst_reg)->type));
+		return -EACCES;
+	}
+
+	if (is_arena_reg(env, insn->dst_reg)) {
+		err = save_aux_ptr_type(env, PTR_TO_ARENA, false);
+		if (err)
+			return err;
+	}
+
+	/* Check whether we can write into the memory. */
+	err = check_mem_access(env, insn_idx, insn->dst_reg, insn->off,
+			       BPF_SIZE(insn->code), BPF_WRITE, insn->src_reg,
+			       true, false);
+	if (err)
+		return err;
+	return 0;
+}

Quoted 13 lines · Peilin Ye

kernel/bpf/verifier.c +13 -0

+static int check_atomic_store(struct bpf_verifier_env *env, int insn_idx,
+			      struct bpf_insn *insn)
+{
+	int err;
+
+	err = check_reg_arg(env, insn->src_reg, SRC_OP);
+	if (err)
+		return err;
+
+	err = check_reg_arg(env, insn->dst_reg, SRC_OP);
+	if (err)
+		return err;
+

kernel/bpf/verifier.c static int check_atomic_rmw(struct bpf_verifier_env *env, int insn_idx, · line 7691

Quoted 4 lines

kernel/bpf/verifier.c +4 -0

+	if (is_pointer_value(env, insn->src_reg)) {
+		verbose(env, "R%d leaks addr into mem\n", insn->src_reg);
+		return -EACCES;
+	}

Nit: this check is done by check_mem_access(), albeit only for
PTR_TO_MEM, I think it's better to be consistent with what happens for regular stores and avoid this check here.

kernel/bpf/core.c +5 -2

index da729cbbaeb9..ab082ab9d535 100644
@@ -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),                     \

Quoted 26 lines

kernel/bpf/core.c +5 -2

index da729cbbaeb9..ab082ab9d535 100644
@@ -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)
 

kernel/bpf/core.c static u64 ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn) · line 2179

Quoted 7 lines

kernel/bpf/core.c +2 -0

        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)

STX_ATOMI_[BH] looks wrong.
It will do atomic64_*() ops in wrong size. BPF_INSN_MAP() applies to bpf_opcode_in_insntable() and the verifier will allow such new insns.

Add several ./test_progs tests:

- atomics/load_acquire
- atomics/store_release
- arena_atomics/load_acquire
- arena_atomics/store_release
- verifier_load_acquire/*
- verifier_store_release/*
- verifier_precision/bpf_load_acquire
- verifier_precision/bpf_store_release

The last two tests are added to check if backtrack_insn() handles the new instructions correctly.

Additionally, the last test also makes sure that the verifier "remembers" the value (in src_reg) we store-release into e.g. a stack slot. For example, if we take a look at the test program:

    #0:  "r1 = 8;"
    #1:  "store_release((u64 *)(r10 - 8), r1);"
    #2:  "r1 = *(u64 *)(r10 - 8);"
    #3:  "r2 = r10;"
    #4:  "r2 += r1;"	/* mark_precise */
    #5:  "r0 = 0;"
    #6:  "exit;"

At #1, if the verifier doesn't remember that we wrote 8 to the stack, then later at #4 we would be adding an unbounded scalar value to the stack pointer, which would cause the program to be rejected:

VERIFIER LOG:
=============
...
math between fp pointer and register with unbounded min value is not allowed

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

tools/testing/selftests/bpf/prog_tests/atomics.c +24 -0

+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");
+}

tools/testing/selftests/bpf/progs/arena_atomics.c +0 -0

tools/testing/selftests/bpf/progs/arena_atomics.c +10 -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;
+}

Quoted 71 lines · Peilin Ye

Add several ./test_progs tests:

- atomics/load_acquire

- atomics/store_release

- arena_atomics/load_acquire

- arena_atomics/store_release

- verifier_load_acquire/*

- verifier_store_release/*

- verifier_precision/bpf_load_acquire

- verifier_precision/bpf_store_release

The last two tests are added to check if backtrack_insn() handles the

new instructions correctly.

Additionally, the last test also makes sure that the verifier

"remembers" the value (in src_reg) we store-release into e.g. a stack

slot. For example, if we take a look at the test program:

#0: "r1 = 8;"

#1: "store_release((u64 *)(r10 - 8), r1);"

#2: "r1 = *(u64 *)(r10 - 8);"

#3: "r2 = r10;"

#4: "r2 += r1;" /* mark_precise */

#5: "r0 = 0;"

#6: "exit;"

At #1, if the verifier doesn't remember that we wrote 8 to the stack,

then later at #4 we would be adding an unbounded scalar value to the

stack pointer, which would cause the program to be rejected:

VERIFIER LOG:

=============

...

math between fp pointer and register with unbounded min value is not allowed

Quoted 4 lines

All new tests depend on the pre-defined __BPF_FEATURE_LOAD_ACQ_STORE_REL

feature macro, which implies -mcpu>=v4.

This restriction would mean that tests are skipped on BPF CI, as it

currently runs using llvm 17 and 18. Instead, I suggest using some

macro hiding an inline assembly as below:

asm volatile (".8byte %[insn];"

:

: [insn]"i"(*(long *)&(BPF_RAW_INSN(...)))

: /* correct clobbers here */);

See the usage of the __imm_insn() macro in the test suite.

Also, "BPF_ATOMIC loads from R%d %s is not allowed\n" and

"BPF_ATOMIC stores into R%d %s is not allowed\n"

situations are not tested.

[...]

Quoted 1 line

[...]

Quoted 3 lines

This restriction would mean that tests are skipped on BPF CI, as it

currently runs using llvm 17 and 18. Instead, I suggest using some

Eduard, if this feature requires a particular version of LLVM, it's not difficult to add a configuration for it to BPF CI.

Whether we want to do it is an open question though. Issues may come up with other tests when newer LLVM is used.

Quoted 2 lines · Peilin Ye

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

This restriction would mean that tests are skipped on BPF CI, as it currently runs using llvm 17 and 18. Instead, I suggest using some macro hiding an inline assembly as below:

	asm volatile (".8byte %[insn];"
	              :
	              : [insn]"i"(*(long *)&(BPF_RAW_INSN(...)))
	              : /* correct clobbers here */);

See the usage of the __imm_insn() macro in the test suite.

Also, "BPF_ATOMIC loads from R%d %s is not allowed\n" and
"BPF_ATOMIC stores into R%d %s is not allowed\n"
situations are not tested.

tools/testing/selftests/bpf/prog_tests/atomics.c static void test_xchg(struct atomics_lskel *skel) · line 163

Quoted 5 lines

tools/testing/selftests/bpf/prog_tests/atomics.c +0 -0

@@ -162,6 +162,56 @@ static void test_xchg(struct atomics_lskel *skel)
 	ASSERT_EQ(skel->bss->xchg32_result, 1, "xchg32_result");
 }

Nit: Given the tests in verifier_load_acquire.c and verifier_store_release.c
that use __retval annotation, are these tests really necessary? (assuming that verifier_store_release.c tests are modified to read
stored location into r0 before exit).

Quoted 1 line

tools/testing/selftests/bpf/prog_tests/atomics.c +1 -0

+static void test_load_acquire(struct atomics_lskel *skel)

Quoted 2 lines

tools/testing/selftests/bpf/progs/arena_atomics.c +0 -0

[...]

tools/testing/selftests/bpf/progs/arena_atomics.c int uaf(const void *ctx) · line 297

Quoted 5 lines

tools/testing/selftests/bpf/progs/arena_atomics.c +5 -0

+SEC("raw_tp/sys_enter")
+int load_acquire(const void *ctx)
+{
+	if (pid != (bpf_get_current_pid_tgid() >> 32))
+		return 0;

Nit: This check is not needed, since bpf_prog_test_run_opts() is used
to run the tests.

Quoted 2 lines · Peilin Ye

tools/testing/selftests/bpf/progs/arena_atomics.c +0 -0

[...]

tools/testing/selftests/bpf/progs/arena_atomics.c int uaf(const void *ctx) · line 297

Quoted 5 lines

tools/testing/selftests/bpf/progs/arena_atomics.c +5 -0

+SEC("raw_tp/sys_enter")
+int load_acquire(const void *ctx)
+{
+	if (pid != (bpf_get_current_pid_tgid() >> 32))
+		return 0;

Nit: This check is not needed, since bpf_prog_test_run_opts() is used
to run the tests.

Hi Eduard,

One more question (for my understanding):

Quoted 5 lines · Eduard Zingerman

Quoted 2 lines · Peilin Ye

tools/testing/selftests/bpf/progs/arena_atomics.c +0 -0

[...]

Quoted 8 lines

tools/testing/selftests/bpf/progs/arena_atomics.c int uaf(const void *ctx) · line 297

Quoted 5 lines

tools/testing/selftests/bpf/progs/arena_atomics.c +5 -0

+SEC("raw_tp/sys_enter")
+int load_acquire(const void *ctx)
+{
+	if (pid != (bpf_get_current_pid_tgid() >> 32))
+		return 0;

Nit: This check is not needed, since bpf_prog_test_run_opts() is used
to run the tests.

Could you explain a bit more why it's not needed?

I read commit 0f4feacc9155 ("selftests/bpf: Adding pid filtering for atomics test") which added those 'pid' checks to atomics/ tests. The commit message [1] says the purpose was to "make atomics test able to run in parallel with other tests", which I couldn't understand.

How using bpf_prog_test_run_opts() makes those 'pid' checks unnecessary?

[1] https://lore.kernel.org/bpf/[email protected]/#r

Quoted 15 lines

Quoted 4 lines

Quoted 2 lines

tools/testing/selftests/bpf/progs/arena_atomics.c +0 -0

[...]

Quoted 8 lines

tools/testing/selftests/bpf/progs/arena_atomics.c int uaf(const void *ctx) · line 297

Quoted 5 lines

tools/testing/selftests/bpf/progs/arena_atomics.c +5 -0

+SEC("raw_tp/sys_enter")
+int load_acquire(const void *ctx)
+{
+	if (pid != (bpf_get_current_pid_tgid() >> 32))
+		return 0;

Nit: This check is not needed, since bpf_prog_test_run_opts() is used
to run the tests.

Could you explain a bit more why it's not needed?

Quoted 8 lines

I read commit 0f4feacc9155 ("selftests/bpf: Adding pid filtering for atomics test") which added those 'pid' checks to atomics/ tests. The commit message [1] says the purpose was to "make atomics test able to run in parallel with other tests", which I couldn't understand.

How using bpf_prog_test_run_opts() makes those 'pid' checks unnecessary?

[1] https://lore.kernel.org/bpf/[email protected]/#r

Hi Peilin,

The entry point for the test looks as follows:

    void test_arena_atomics(void)
    {
    	...
    	skel = arena_atomics__open();
    	if (!ASSERT_OK_PTR(skel, "arena atomics skeleton open"))
    		return;
    
    	if (skel->data->skip_tests) { ... }
    	err = arena_atomics__load(skel);
    	if (!ASSERT_OK(err, "arena atomics skeleton load"))
    		return;
    	skel->bss->pid = getpid();
    
    	if (test__start_subtest("add"))
    		test_add(skel);
            ...
    
    cleanup:
    	arena_atomics__destroy(skel);
    }

Note arena_atomics__{open,load} calls but absence of the arena_atomics__attach call. W/o arena_atomics__attach call the programs would not be hooked to the designated extension points, e.g. "raw_tp/sys_enter".

The bpf_prog_test_run_opts() invokes BPF_PROG_TEST_RUN command of the bpf system call, which does not attach the program either, but executes jitted code directly with fake context.
(See bpf_prog_ops->test_run callback (method?) and
bpf_prog_test_run_raw_tp()).

Same happens in prog{,_tests}/arena.c: no attachment happens after commit [2]. Commit [1] is unnecessary after [2].

[2] commit 04fcb5f9a104 ("selftests/bpf: Migrate from bpf_prog_test_run")

Update documentation for the new load-acquire and store-release instructions. Rename existing atomic operations as "atomic read-modify-write (RMW) operations".

Following RFC 9669, section 7.3. "Adding Instructions", create new conformance groups "atomic32v2" and "atomic64v2", where:

* atomic32v2: includes all instructions in "atomic32", plus the new 8-bit, 16-bit and 32-bit atomic load-acquire and store-release instructions

* atomic64v2: includes all instructions in "atomic64" and "atomic32v2", plus the new 64-bit atomic load-acquire and store-release instructions

Cc [email protected]

Signed-off-by Peilin Ye <[email protected]>

---
 .../bpf/standardization/instruction-set.rst   | 114 +++++++++++++++---
 1 file changed, 98 insertions(+), 16 deletions(-)

Documentation/bpf/standardization/instruction-set.rst +59 -16

index ab820d565052..86917932e9ef 100644
@@ -139,8 +139,14 @@ This document defines the following conformance groups:
   specification unless otherwise noted.
 * base64: includes base32, plus instructions explicitly noted
   as being in the base64 conformance group.
-* atomic32: includes 32-bit atomic operation instructions (see `Atomic operations`_).
-* atomic64: includes atomic32, plus 64-bit atomic operation instructions.
+* atomic32: includes 32-bit atomic read-modify-write instructions (see
+  `Atomic operations`_).
+* atomic32v2: includes atomic32, plus 8-bit, 16-bit and 32-bit atomic
+  load-acquire and store-release instructions.
+* atomic64: includes atomic32, plus 64-bit atomic read-modify-write
+  instructions.
+* atomic64v2: unifies atomic32v2 and atomic64, plus 64-bit atomic load-acquire
+  and store-release instructions.
 * divmul32: includes 32-bit division, multiplication, and modulo instructions.
 * divmul64: includes divmul32, plus 64-bit division, multiplication,
   and modulo instructions.

@@ -653,20 +659,31 @@ Atomic operations are operations that operate on memory and can not be
 interrupted or corrupted by other access to the same memory region
 by other BPF programs or means outside of this specification.
 
-All atomic operations supported by BPF are encoded as store operations
-that use the ``ATOMIC`` mode modifier as follows:
+All atomic operations supported by BPF are encoded as ``STX`` instructions
+that use the ``ATOMIC`` mode modifier, with the 'imm' field encoding the
+actual atomic operation.  These operations are categorized based on the second
+lowest nibble (bits 4-7) of the 'imm' field:
 
-* ``{ATOMIC, W, STX}`` for 32-bit operations, which are
+* ``ATOMIC_LOAD`` and ``ATOMIC_STORE`` indicate atomic load and store
+  operations, respectively (see `Atomic load and store operations`_).
+* All other defined values indicate an atomic read-modify-write operation, as
+  described in the following section.
+
+Atomic read-modify-write operations
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+The atomic read-modify-write (RMW) operations are encoded as follows:
+
+* ``{ATOMIC, W, STX}`` for 32-bit RMW operations, which are
   part of the "atomic32" conformance group.
-* ``{ATOMIC, DW, STX}`` for 64-bit operations, which are
+* ``{ATOMIC, DW, STX}`` for 64-bit RMW operations, which are
   part of the "atomic64" conformance group.
-* 8-bit and 16-bit wide atomic operations are not supported.
+* 8-bit and 16-bit wide atomic RMW operations are not supported.
 
-The 'imm' field is used to encode the actual atomic operation.
-Simple atomic operation use a subset of the values defined to encode
-arithmetic operations in the 'imm' field to encode the atomic operation:
+Simple atomic RMW operation use a subset of the values defined to encode
+arithmetic operations in the 'imm' field to encode the atomic RMW operation:
 
-.. table:: Simple atomic operations
+.. table:: Simple atomic read-modify-write operations
 
   ========  =====  ===========
   imm       value  description

@@ -686,10 +703,10 @@ arithmetic operations in the 'imm' field to encode the atomic operation:
 
   *(u64 *)(dst + offset) += src
 
-In addition to the simple atomic operations, there also is a modifier and
-two complex atomic operations:
+In addition to the simple atomic RMW operations, there also is a modifier and
+two complex atomic RMW operations:
 
-.. table:: Complex atomic operations
+.. table:: Complex atomic read-modify-write operations
 
   ===========  ================  ===========================
   imm          value             description

@@ -699,8 +716,8 @@ two complex atomic operations:
   CMPXCHG      0xf0 | FETCH      atomic compare and exchange
   ===========  ================  ===========================
 
-The ``FETCH`` modifier is optional for simple atomic operations, and
-always set for the complex atomic operations.  If the ``FETCH`` flag
+The ``FETCH`` modifier is optional for simple atomic RMW operations, and
+always set for the complex atomic RMW operations.  If the ``FETCH`` flag
 is set, then the operation also overwrites ``src`` with the value that
 was in memory before it was modified.
 

@@ -713,6 +730,71 @@ The ``CMPXCHG`` operation atomically compares the value addressed by
 value that was at ``dst + offset`` before the operation is zero-extended
 and loaded back to ``R0``.
 
+Atomic load and store operations
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+To encode an atomic load or store operation, the lowest 8 bits of the 'imm'
+field are divided as follows::
+
+  +-+-+-+-+-+-+-+-+
+  | type  | order |
+  +-+-+-+-+-+-+-+-+
+
+**type**
+  The operation type is one of:
+
+.. table:: Atomic load and store operation types
+
+  ============  =====  ============
+  type          value  description
+  ============  =====  ============
+  ATOMIC_LOAD   0x1    atomic load
+  ATOMIC_STORE  0x2    atomic store
+  ============  =====  ============
+
+**order**
+  The memory order is one of:
+
+.. table:: Memory orders

Quoted 32 lines · Peilin Ye

Documentation/bpf/standardization/instruction-set.rst +32 -0

+Atomic load and store operations
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+To encode an atomic load or store operation, the lowest 8 bits of the 'imm'
+field are divided as follows::
+
+  +-+-+-+-+-+-+-+-+
+  | type  | order |
+  +-+-+-+-+-+-+-+-+
+
+**type**
+  The operation type is one of:
+
+.. table:: Atomic load and store operation types
+
+  ============  =====  ============
+  type          value  description
+  ============  =====  ============
+  ATOMIC_LOAD   0x1    atomic load
+  ATOMIC_STORE  0x2    atomic store
+  ============  =====  ============
+
+**order**
+  The memory order is one of:
+
+.. table:: Memory orders
+
+  =======  =====  =======================
+  order    value  description
+  =======  =====  =======================
+  RELAXED  0x0    relaxed
+  ACQUIRE  0x1    acquire

Documentation/bpf/standardization/instruction-set.rst The ``CMPXCHG`` operation atomically compares the value addressed by · line 768

Quoted 4 lines

Documentation/bpf/standardization/instruction-set.rst +4 -0

+  RELEASE  0x2    release
+  ACQ_REL  0x3    acquire and release
+  SEQ_CST  0x4    sequentially consistent
+  =======  =====  =======================

I understand that this is inspired by C,
but what are the chances this will map meaningfully to hw? What JITs suppose to do with all other combinations ?