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

[Andrii Nakryiko]

On Sun, Sep 13, 2020 at 10:01 PM Hao Luo <haoluo@xxxxxxxxxx> wrote:
>
> Thanks for review, Andrii.
>
> One question, should I add bpf_{per, this}_cpu_ptr() to the
> bpf_base_func_proto() in kernel/bpf/helpers.c?

Yes, probably, but given it allows poking at kernel memory, it
probably needs to be guarded by perfmon_capable() check, similar to
bpf_get_current_task_proto.

>
> On Fri, Sep 4, 2020 at 1:04 PM Andrii Nakryiko
> <andrii.nakryiko@xxxxxxxxx> wrote:
> >
> > On Thu, Sep 3, 2020 at 3:35 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            |  3 ++
> > >  include/linux/btf.h            | 11 ++++++
> > >  include/uapi/linux/bpf.h       | 17 +++++++++
> > >  kernel/bpf/btf.c               | 10 ------
> > >  kernel/bpf/verifier.c          | 66 +++++++++++++++++++++++++++++++---
> > >  kernel/trace/bpf_trace.c       | 18 ++++++++++
> > >  tools/include/uapi/linux/bpf.h | 17 +++++++++
> > >  7 files changed, 128 insertions(+), 14 deletions(-)
> > >

[...]

> > > @@ -5002,6 +5016,30 @@ static int check_helper_call(struct bpf_verifier_env *env, int func_id, int insn
> > >                 regs[BPF_REG_0].type = PTR_TO_MEM_OR_NULL;
> > >                 regs[BPF_REG_0].id = ++env->id_gen;
> > >                 regs[BPF_REG_0].mem_size = meta.mem_size;
> > > +       } else if (fn->ret_type == RET_PTR_TO_MEM_OR_BTF_ID_OR_NULL) {
> >
> > Given this is internal implementation detail, this return type is
> > fine, but I'm wondering if it would be better to just make
> > PTR_TO_BTF_ID to allow not just structs? E.g., if we have an int, just
> > allow reading those 4 bytes.
> >
> > Not sure what the implications are in terms of implementation, but
> > conceptually that shouldn't be a problem, given we do have BTF type ID
> > describing size and all.
> >
>
> Yeah. Totally agree. I looked at it initially. My take is
> PTR_TO_BTF_ID is meant for struct types. It required some code
> refactoring to break this assumption. I can add it to my TODO list and
> investigate it if this makes more sense.

PTR_TO_BTF_ID was *implemented* for struct, but at least naming-wise
nothing suggests it has to be restricted to structs. But yeah, this
should be a separate change, don't block your patches on that.

>
> > > +               const struct btf_type *t;
> > > +

[...]