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
