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)
[...]
