linux-kernel - Re: [PATCH bpf-next v2 4/6] bpf: Introduce bpf_per_cpu

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAEf4BzZWr7aw0D3d0Etmm=AVrLVsgDbY5W+1Aj9o8iBYkhJWew@mail.gmail.com>
Date:   Mon, 14 Sep 2020 11:09:05 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Hao Luo <haoluo@...gle.com>
Cc:     Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
        open list <linux-kernel@...r.kernel.org>,
        "open list:KERNEL SELFTEST FRAMEWORK" 
        <linux-kselftest@...r.kernel.org>, Shuah Khan <shuah@...nel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Andrii Nakryiko <andriin@...com>,
        Daniel Borkmann <daniel@...earbox.net>,
        Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        John Fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...omium.org>,
        Quentin Monnet <quentin@...valent.com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Ingo Molnar <mingo@...hat.com>, Andrey Ignatov <rdna@...com>,
        Jakub Sitnicki <jakub@...udflare.com>
Subject: Re: [PATCH bpf-next v2 4/6] bpf: Introduce bpf_per_cpu_ptr()

On Sun, Sep 13, 2020 at 10:01 PM Hao Luo <haoluo@...gle.com> 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@...il.com> wrote:
> >
> > On Thu, Sep 3, 2020 at 3:35 PM Hao Luo <haoluo@...gle.com> 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@...com>
> > > Signed-off-by: Hao Luo <haoluo@...gle.com>
> > > ---
> > >  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;
> > > +

[...]