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@xxxxxxxxxx/ > > --- 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
