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

[Eduard Zingerman <eddyz87@gmail.com>]

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