netdev - Re: [PATCH v2 bpf-next 05/12] libbpf: auto-detect btf_id of raw

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]
Date:   Fri, 11 Oct 2019 21:39:25 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Alexei Starovoitov <ast@...com>
Cc:     Alexei Starovoitov <ast@...nel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Daniel Borkmann <daniel@...earbox.net>,
        "x86@...nel.org" <x86@...nel.org>,
        Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
        Kernel Team <Kernel-team@...com>
Subject: Re: [PATCH v2 bpf-next 05/12] libbpf: auto-detect btf_id of raw_tracepoint

On Fri, Oct 11, 2019 at 5:40 PM Alexei Starovoitov <ast@...com> wrote:
>
> On 10/11/19 11:07 AM, Andrii Nakryiko wrote:
> > On Wed, Oct 9, 2019 at 9:17 PM Alexei Starovoitov <ast@...nel.org> wrote:
> >>
> >> For raw tracepoint program types libbpf will try to find
> >> btf_id of raw tracepoint in vmlinux's BTF.
> >> It's a responsiblity of bpf program author to annotate the program
> >> with SEC("raw_tracepoint/name") where "name" is a valid raw tracepoint.
> >> If "name" is indeed a valid raw tracepoint then in-kernel BTF
> >> will have "btf_trace_##name" typedef that points to function
> >> prototype of that raw tracepoint. BTF description captures
> >> exact argument the kernel C code is passing into raw tracepoint.
> >> The kernel verifier will check the types while loading bpf program.
> >>
> >> libbpf keeps BTF type id in expected_attach_type, but since
> >> kernel ignores this attribute for tracing programs copy it
> >> into attach_btf_id attribute before loading.
> >>
> >> Signed-off-by: Alexei Starovoitov <ast@...nel.org>
> >> ---
> >>   tools/lib/bpf/bpf.c    |  3 +++
> >>   tools/lib/bpf/libbpf.c | 17 +++++++++++++++++
> >>   2 files changed, 20 insertions(+)
> >>
> >> diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
> >> index cbb933532981..79046067720f 100644
> >> --- a/tools/lib/bpf/bpf.c
> >> +++ b/tools/lib/bpf/bpf.c
> >> @@ -228,6 +228,9 @@ int bpf_load_program_xattr(const struct bpf_load_program_attr *load_attr,
> >>          memset(&attr, 0, sizeof(attr));
> >>          attr.prog_type = load_attr->prog_type;
> >>          attr.expected_attach_type = load_attr->expected_attach_type;
> >> +       if (attr.prog_type == BPF_PROG_TYPE_RAW_TRACEPOINT)
> >> +               /* expected_attach_type is ignored for tracing progs */
> >> +               attr.attach_btf_id = attr.expected_attach_type;
> >>          attr.insn_cnt = (__u32)load_attr->insns_cnt;
> >>          attr.insns = ptr_to_u64(load_attr->insns);
> >>          attr.license = ptr_to_u64(load_attr->license);
> >> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> >> index a02cdedc4e3f..8bf30a67428c 100644
> >> --- a/tools/lib/bpf/libbpf.c
> >> +++ b/tools/lib/bpf/libbpf.c
> >> @@ -4586,6 +4586,23 @@ int libbpf_prog_type_by_name(const char *name, enum bpf_prog_type *prog_type,
> >>                          continue;
> >>                  *prog_type = section_names[i].prog_type;
> >>                  *expected_attach_type = section_names[i].expected_attach_type;
> >> +               if (*prog_type == BPF_PROG_TYPE_RAW_TRACEPOINT) {
> >> +                       struct btf *btf = bpf_core_find_kernel_btf();
> >> +                       char raw_tp_btf_name[128] = "btf_trace_";
> >> +                       char *dst = raw_tp_btf_name + sizeof("btf_trace_") - 1;
> >> +                       int ret;
> >> +
> >> +                       if (IS_ERR(btf))
> >> +                               /* lack of kernel BTF is not a failure */
> >> +                               return 0;
> >> +                       /* prepend "btf_trace_" prefix per kernel convention */
> >> +                       strncat(dst, name + section_names[i].len,
> >> +                               sizeof(raw_tp_btf_name) - (dst - raw_tp_btf_name));
> >> +                       ret = btf__find_by_name(btf, raw_tp_btf_name);
> >> +                       if (ret > 0)
> >> +                               *expected_attach_type = ret;
> >
> > Well, actually, I realized after I gave Acked-by, so not yet :)
> >
> > This needs kernel feature probe of whether kernel supports specifying
> > attach_btf_id, otherwise on older kernels we'll stop successfully
> > loading valid program.
>
> The code above won't find anything on older kernels.
> The patch 1 of the series has to be there for proper btf to be
> generated by pahole.
> Before that happens expected_attach_type will stay zero
> and corresponding copy in attach_btf_id will be zero as well.
> I see no issues being compatible with older kernels.

indeed, this one is not an issue.

>
> > But even if kernel supports attach_btf_id, I think users still need to
> > opt in into specifying attach_btf_id by libbpf. Think about existing
> > raw_tp programs that are using bpf_probe_read() because they were not
> > created with this kernel feature in mind. They will suddenly stop
> > working without any of user's fault.
>
> This one is excellent catch.
> loop1.c should have caught it, since it has
> SEC("raw_tracepoint/kfree_skb")
> {
>    int nested_loops(volatile struct pt_regs* ctx)
>     .. = PT_REGS_RC(ctx);
>
> and verifier would have rejected it.
> But the way the test is written it's not using libbpf's autodetect
> of program type, so everything is passing.