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
