On Tue, Jan 28, 2025 at 04:19:25PM -0800, Eduard Zingerman wrote:
> On Sat, 2025-01-25 at 02:18 +0000, Peilin Ye wrote:
> > Signed-off-by: Peilin Ye <yepeilin@xxxxxxxxxx>
> > ---
>
> I think bpf_jit_supports_insn() in arch/{x86,s390}/net/bpf_jit_comp.c
> need an update, as both would accept BPF_LOAD_ACQ/BPF_STORE_REL at the
> moment.
Got it - I will move is_atomic_load_store() into <linux/bpf.h> for that.
> Acked-by: Eduard Zingerman <eddyz87@xxxxxxxxx>
Thanks!
> > +static int check_atomic_load(struct bpf_verifier_env *env, int insn_idx,
> > + struct bpf_insn *insn)
> > +{
> > + struct bpf_reg_state *regs = cur_regs(env);
> > + int err;
> > +
> > + err = check_reg_arg(env, insn->src_reg, SRC_OP);
> > + if (err)
> > + return err;
> > +
> > + err = check_reg_arg(env, insn->dst_reg, DST_OP_NO_MARK);
> > + if (err)
> > + return err;
> > +
> > + if (!atomic_ptr_type_ok(env, insn->src_reg, insn)) {
> > + verbose(env, "BPF_ATOMIC loads from R%d %s is not allowed\n",
> > + insn->src_reg,
> > + reg_type_str(env, reg_state(env, insn->src_reg)->type));
> > + return -EACCES;
> > + }
> > +
> > + if (is_arena_reg(env, insn->src_reg)) {
> > + err = save_aux_ptr_type(env, PTR_TO_ARENA, false);
> > + if (err)
> > + return err;
>
> Nit: this and the next function look very similar to processing of
> generic load and store in do_check(). Maybe extract that code
> as an auxiliary function and call it in both places?
Sure, I agree that they look a bit repetitive.
> The only major difference is is_arena_reg() check guarding
> save_aux_ptr_type(), but I think it is ok to do save_aux_ptr_type
> unconditionally. Fwiw, the code would be a bit simpler,
> just spent half an hour convincing myself that such conditional handling
> is not an error. Wdyt?
:-O
Thanks a lot for that; would you mind sharing a bit more on how you
reasoned about it (i.e., why is it OK to save_aux_ptr_type()
unconditionally) ?
> > + }
> > +
> > + /* Check whether we can read the memory. */
> > + err = check_mem_access(env, insn_idx, insn->src_reg, insn->off,
> > + BPF_SIZE(insn->code), BPF_READ, insn->dst_reg,
> > + true, false);
> > + if (err)
> > + return err;
> > +
> > + err = reg_bounds_sanity_check(env, ®s[insn->dst_reg], "atomic_load");
> > + if (err)
> > + return err;
> > + return 0;
> > +}
> > +
> > +static int check_atomic_store(struct bpf_verifier_env *env, int insn_idx,
> > + struct bpf_insn *insn)
Thanks,
Peilin Ye
