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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <e294e3c318e2c7a646e4b2e43516378a0689ea3b.camel@gmail.com>
Date: Mon, 06 Jan 2025 17:20:56 -0800
From: Eduard Zingerman <eddyz87@...il.com>
To: Peilin Ye <yepeilin@...gle.com>
Cc: bpf@...r.kernel.org, Alexei Starovoitov <ast@...nel.org>, Song Liu	
 <song@...nel.org>, Yonghong Song <yonghong.song@...ux.dev>, Daniel Borkmann
	 <daniel@...earbox.net>, Andrii Nakryiko <andrii@...nel.org>, Martin KaFai
 Lau	 <martin.lau@...ux.dev>, John Fastabend <john.fastabend@...il.com>, KP
 Singh	 <kpsingh@...nel.org>, Stanislav Fomichev <sdf@...ichev.me>, Hao Luo	
 <haoluo@...gle.com>, Jiri Olsa <jolsa@...nel.org>, "Paul E. McKenney"	
 <paulmck@...nel.org>, Puranjay Mohan <puranjay@...nel.org>, Xu Kuohai	
 <xukuohai@...weicloud.com>, Catalin Marinas <catalin.marinas@....com>, Will
 Deacon <will@...nel.org>, Quentin Monnet <qmo@...nel.org>, Mykola Lysenko
 <mykolal@...com>,  Shuah Khan <shuah@...nel.org>, Josh Don
 <joshdon@...gle.com>, Barret Rhoden <brho@...gle.com>, Neel Natu	
 <neelnatu@...gle.com>, Benjamin Segall <bsegall@...gle.com>, David Vernet	
 <dvernet@...a.com>, Dave Marchevsky <davemarchevsky@...a.com>, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH RFC bpf-next v1 2/4] bpf: Introduce load-acquire and
 store-release instructions

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