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;
> > > +
[...]
