On Tue, Feb 22, 2022 at 7:04 PM Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx> wrote: > > On Tue, Feb 22, 2022 at 09:50:00PM IST, Alexei Starovoitov wrote: > > On Mon, Feb 21, 2022 at 11:10 PM Kumar Kartikeya Dwivedi > > <memxor@xxxxxxxxx> wrote: > > > > > > On Tue, Feb 22, 2022 at 12:23:49PM IST, Alexei Starovoitov wrote: > > > > On Sun, Feb 20, 2022 at 07:18:02PM +0530, Kumar Kartikeya Dwivedi wrote: > > > > > static int check_mem_access(struct bpf_verifier_env *env, int insn_idx, u32 regno, > > > > > int off, int bpf_size, enum bpf_access_type t, > > > > > - int value_regno, bool strict_alignment_once) > > > > > + int value_regno, bool strict_alignment_once, > > > > > + struct bpf_reg_state *atomic_load_reg) > > > > > > > > No new side effects please. > > > > value_regno is not pretty already. > > > > At least its known ugliness that we need to clean up one day. > > > > > > > > > static int check_atomic(struct bpf_verifier_env *env, int insn_idx, struct bpf_insn *insn) > > > > > { > > > > > + struct bpf_reg_state atomic_load_reg; > > > > > int load_reg; > > > > > int err; > > > > > > > > > > + __mark_reg_unknown(env, &atomic_load_reg); > > > > > + > > > > > switch (insn->imm) { > > > > > case BPF_ADD: > > > > > case BPF_ADD | BPF_FETCH: > > > > > @@ -4813,6 +4894,7 @@ static int check_atomic(struct bpf_verifier_env *env, int insn_idx, struct bpf_i > > > > > else > > > > > load_reg = insn->src_reg; > > > > > > > > > > + atomic_load_reg = *reg_state(env, load_reg); > > > > > /* check and record load of old value */ > > > > > err = check_reg_arg(env, load_reg, DST_OP); > > > > > if (err) > > > > > @@ -4825,20 +4907,21 @@ static int check_atomic(struct bpf_verifier_env *env, int insn_idx, struct bpf_i > > > > > } > > > > > > > > > > /* Check whether we can read the memory, with second call for fetch > > > > > - * case to simulate the register fill. > > > > > + * case to simulate the register fill, which also triggers checks > > > > > + * for manipulation of BTF ID pointers embedded in BPF maps. > > > > > */ > > > > > err = check_mem_access(env, insn_idx, insn->dst_reg, insn->off, > > > > > - BPF_SIZE(insn->code), BPF_READ, -1, true); > > > > > + BPF_SIZE(insn->code), BPF_READ, -1, true, NULL); > > > > > if (!err && load_reg >= 0) > > > > > err = check_mem_access(env, insn_idx, insn->dst_reg, insn->off, > > > > > BPF_SIZE(insn->code), BPF_READ, load_reg, > > > > > - true); > > > > > + true, load_reg >= 0 ? &atomic_load_reg : NULL); > > > > > > > > Special xchg logic should be down outside of check_mem_access() > > > > instead of hidden by layers of calls. > > > > > > Right, it's ugly, but if we don't capture the reg state before that > > > check_reg_arg(env, load_reg, DST_OP), it's not possible to see the actual > > > PTR_TO_BTF_ID being moved into the map, since check_reg_arg will do a > > > mark_reg_unknown for value_regno. Any other ideas on what I can do? > > > > > > 37086bfdc737 ("bpf: Propagate stack bounds to registers in atomics w/ BPF_FETCH") > > > changed the order of check_mem_access and DST_OP check_reg_arg. > > > > That highlights my point that side effects are bad. > > That commit tries to work around that behavior and makes things > > harder to extend like you found out with xchg logic. > > Another option would be to add bpf_kptr_xchg() helper > > instead of dealing with insn. It will be tiny bit slower, > > but it will work on all architectures. While xchg bpf jit is > > on x86,s390,mips so far. > > Right, but kfunc is currently limited to x86, which is required to obtain a > refcounted PTR_TO_BTF_ID that you can move into the map, so it wouldn't make > much of a difference. Well the patches to add trampoline support to powerpc were already posted. > > We need to think more on how to refactor check_mem_acess without > > digging ourselves into an even bigger hole. > > So I'm ok with working on untangling check_mem_access as a follow up, but for > now should we go forward with how it is? Just looking at it yesterday makes me > think it's going to require a fair amount of refactoring and discussion. > > Also, do you have any ideas on how to change it? Do you want it to work like how > is_valid_access callbacks work? So passing something like a bpf_insn_access_aux > into the call, where it sets how it'd like to update the register, and then > actual updates take place in caller context? I don't like callbacks in general. They're fine for walk_the_tree, for_each_elem accessors, but passing a callback into check_mem_access is not great. Do you mind going with a bpf_kptr_xchg() helper for now and optimizing into direct xchg insn later? It's not clear whether it's going to be faster to be noticeable.