Message-ID: <20220224084322.vmyvusyukanc6z45@apollo.legion>
Date:   Thu, 24 Feb 2022 14:13:22 +0530
From:   Kumar Kartikeya Dwivedi <memxor@...il.com>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc:     bpf <bpf@...r.kernel.org>, Alexei Starovoitov <ast@...nel.org>,
        Andrii Nakryiko <andrii@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Toke Høiland-Jørgensen <toke@...hat.com>,
        Jesper Dangaard Brouer <hawk@...nel.org>,
        netfilter-devel <netfilter-devel@...r.kernel.org>,
        Network Development <netdev@...r.kernel.org>
Subject: 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
> <memxor@...il.com> 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@...il.com> 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.

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

Powered by blists - more mailing lists