Re: [PATCH RFC bpf-next v1 2/4] bpf: Introduce load-acquire and store-release instructions

[Peilin Ye <yepeilin@google.com>]

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