Re: [PATCH bpf-next v1 04/15] bpf: Allow storing referenced PTR_TO_BTF_ID in map

On Thu, Feb 24, 2022 at 03:22:43AM IST, Alexei Starovoitov wrote:

On Tue, Feb 22, 2022 at 7:04 PM Kumar Kartikeya Dwivedi
[off-list ref] wrote:

quoted

On Tue, Feb 22, 2022 at 09:50:00PM IST, Alexei Starovoitov wrote:

quoted

On Mon, Feb 21, 2022 at 11:10 PM Kumar Kartikeya Dwivedi
[off-list ref] wrote:

quoted

On Tue, Feb 22, 2022 at 12:23:49PM IST, Alexei Starovoitov wrote:

quoted

On Sun, Feb 20, 2022 at 07:18:02PM +0530, Kumar Kartikeya Dwivedi wrote:

quoted

 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.

quoted

 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.

quoted

quoted

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.

I didn't mean passing a callback, I meant passing a struct like you mentioned in
a previous comment to another patch (btf_field_info) where we can set state that
must be updated for the register, and then updates are done by the caller, to
separate the 'side effects' from the other checks. is_valid_access verifier
callback receive a similar bpf_insn_access_aux parameter which is then used to
update register state.

Do you mind going with a bpf_kptr_xchg() helper for now
and optimizing into direct xchg insn later?

I don't have a problem with that. I just didn't see any advantages (except the
wider architecture support that you pointed out). We still have to special case
some places in check_helper_call (since it needs to transfer R1's btf_id to R0,
and work with all PTR_TO_BTF_ID, not just 1), so the implementation is similar.

I guess for most usecases it wouldn't matter much.

It's not clear whether it's going to be faster to be noticeable.

Just for curiosity, I measured a loop of 5000 xchg ops, one with bpf_xchg, one
with bpf_kptr_xchg. This is the simple case (uncontended, raw cost of both
operations) xchg insn is at ~4 nsecs, bpf_kptr_xchg is at ~8 nsecs (single
socket 8 core Intel i5 @ 2.5GHz). I'm guessing in a complicated case spill/fill
of caller saved regs will also come into play for the helper case.

--
Kartikeya