Re: [PATCH bpf-next v3 4/6] bpf: Introduce bpf_per_cpu_ptr()

[Hao Luo]

Hi, Andrii,

Thanks for taking a look. Sorry for the late reply. Spent some time on
rebasing and fixing a build issue in my development environment that
started happening in v5.9.

On Mon, Sep 21, 2020 at 11:09 AM Andrii Nakryiko
<andrii.nakryiko@xxxxxxxxx> wrote:
>
> On Wed, Sep 16, 2020 at 3:39 PM Hao Luo <haoluo@xxxxxxxxxx> wrote:
> >
> > Add bpf_per_cpu_ptr() to help bpf programs access percpu vars.
> > bpf_per_cpu_ptr() has the same semantic as per_cpu_ptr() in the kernel
> > except that it may return NULL. This happens when the cpu parameter is
> > out of range. So the caller must check the returned value.
> >
> > Acked-by: Andrii Nakryiko <andriin@xxxxxx>
> > Signed-off-by: Hao Luo <haoluo@xxxxxxxxxx>
> > ---
> >  include/linux/bpf.h            |  4 +++
> >  include/linux/btf.h            | 11 ++++++
> >  include/uapi/linux/bpf.h       | 18 ++++++++++
> >  kernel/bpf/btf.c               | 10 ------
> >  kernel/bpf/helpers.c           | 18 ++++++++++
> >  kernel/bpf/verifier.c          | 64 ++++++++++++++++++++++++++++++++--
> >  kernel/trace/bpf_trace.c       |  2 ++
> >  tools/include/uapi/linux/bpf.h | 18 ++++++++++
> >  8 files changed, 132 insertions(+), 13 deletions(-)
> >
>
> I already acked this, but see my concern about O(N) look up for
> .data..percpu. Feel free to follow up on this with a separate patch.
> Thanks!
>
> [...]
>
> > @@ -4003,6 +4008,15 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
> >                         if (type != expected_type)
> >                                 goto err_type;
> >                 }
> > +       } else if (arg_type == ARG_PTR_TO_PERCPU_BTF_ID) {
> > +               expected_type = PTR_TO_PERCPU_BTF_ID;
> > +               if (type != expected_type)
> > +                       goto err_type;
> > +               if (!reg->btf_id) {
> > +                       verbose(env, "Helper has invalid btf_id in R%d\n", regno);
> > +                       return -EACCES;
> > +               }
> > +               meta->ret_btf_id = reg->btf_id;
>
> FYI, this will conflict with Lorenz's refactoring, so you might need
> to rebase and solve the conflicts if his patch set lands first.
>

Indeed. Do hit this while rebasing but managed to resolve it. Please
take a look and let me know if you have comments there in v4

> > @@ -7413,6 +7451,7 @@ static int check_ld_imm(struct bpf_verifier_env *env, struct bpf_insn *insn)
> >                         dst_reg->mem_size = aux->btf_var.mem_size;
> >                         break;
> >                 case PTR_TO_BTF_ID:
> > +               case PTR_TO_PERCPU_BTF_ID:
> >                         dst_reg->btf_id = aux->btf_var.btf_id;
> >                         break;
> >                 default:
> > @@ -9313,10 +9352,14 @@ static int check_pseudo_btf_id(struct bpf_verifier_env *env,
> >                                struct bpf_insn *insn,
> >                                struct bpf_insn_aux_data *aux)
> >  {
> > -       u32 type, id = insn->imm;
> > +       u32 datasec_id, type, id = insn->imm;
> > +       const struct btf_var_secinfo *vsi;
> > +       const struct btf_type *datasec;
> >         const struct btf_type *t;
> >         const char *sym_name;
> > +       bool percpu = false;
> >         u64 addr;
> > +       int i;
> >
> >         if (!btf_vmlinux) {
> >                 verbose(env, "kernel is missing BTF, make sure CONFIG_DEBUG_INFO_BTF=y is specified in Kconfig.\n");
> > @@ -9348,12 +9391,27 @@ static int check_pseudo_btf_id(struct bpf_verifier_env *env,
> >                 return -ENOENT;
> >         }
> >
> > +       datasec_id = btf_find_by_name_kind(btf_vmlinux, ".data..percpu",
> > +                                          BTF_KIND_DATASEC);
>
> this is a relatively expensive O(N) operation, it probably makes sense
> to cache it (there are about 80'000 types now in BTF for my typical
> kernel config, so iterating that much for every single ldimm64 for
> ksym is kind of expensive.
>

ACK. This currently works. I can do it in another patch.



Hao