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

[Peilin Ye <yepeilin@google.com>]

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@google.com/

> > --- 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