netdev - Re: [PATCH bpf-next v4 17/27] bpf: Add multi trampoline attach support

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: <CAEf4Bza5wz49r0QfuJ8d_3bxw9Cy3D_vGtFPkQ1OUJDQn6XKbQ@mail.gmail.com>
Date:   Tue, 31 Aug 2021 17:02:00 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Jiri Olsa <jolsa@...hat.com>
Cc:     Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andriin@...com>,
        "Steven Rostedt (VMware)" <rostedt@...dmis.org>,
        Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
        Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        John Fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...omium.org>, Daniel Xu <dxu@...uu.xyz>,
        Viktor Malik <vmalik@...hat.com>
Subject: Re: [PATCH bpf-next v4 17/27] bpf: Add multi trampoline attach support

On Tue, Aug 31, 2021 at 4:36 PM Andrii Nakryiko
<andrii.nakryiko@...il.com> wrote:
>
> On Thu, Aug 26, 2021 at 12:41 PM Jiri Olsa <jolsa@...hat.com> wrote:
> >
> > Adding new multi trampoline link (BPF_LINK_TYPE_TRACING_MULTI)
> > as an interface to attach program to multiple functions.
> >
> > The link_create bpf_attr interface already has 'bpf_prog' file
> > descriptor, that defines the program to be attached. It must be
> > loaded with BPF_F_MULTI_FUNC flag.
> >
> > Adding new multi_btf_ids/multi_btf_ids_cnt link_create bpf_attr
> > fields that provides BTF ids.
> >
> > The new link gets multi trampoline (via bpf_trampoline_multi_get)
> > and links the provided program with embedded trampolines and the
> > 'main' trampoline with new multi link/unlink functions:
> >
> >   int bpf_trampoline_multi_link_prog(struct bpf_prog *prog,
> >                                      struct bpf_trampoline_multi *tr);
> >   int bpf_trampoline_multi_unlink_prog(struct bpf_prog *prog,
> >                                        struct bpf_trampoline_multi *tr);
> >
> > If embedded trampoline contains fexit programs, we need to switch
> > its model to the multi trampoline model (because of the final 'ret'
> > argument). We keep the count of attached multi func programs for each
> > trampoline, so we can tell when to switch the model.

Related to my comments on the next patch, if we switch the order of
return value and always reserve 6 slots for input args, regardless of
the actual number of function input args, that should make this
upgrade logic unnecessary, right?

> >
> > Signed-off-by: Jiri Olsa <jolsa@...nel.org>
> > ---
> >  include/linux/bpf.h            |   5 ++
> >  include/uapi/linux/bpf.h       |   5 ++
> >  kernel/bpf/core.c              |   1 +
> >  kernel/bpf/syscall.c           | 120 +++++++++++++++++++++++++++++++++
> >  kernel/bpf/trampoline.c        |  87 ++++++++++++++++++++++--
> >  tools/include/uapi/linux/bpf.h |   5 ++
> >  6 files changed, 219 insertions(+), 4 deletions(-)
> >
>
> [...]
>
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index 1f9d336861f0..9533200ffadf 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -1008,6 +1008,7 @@ enum bpf_link_type {
> >         BPF_LINK_TYPE_NETNS = 5,
> >         BPF_LINK_TYPE_XDP = 6,
> >         BPF_LINK_TYPE_PERF_EVENT = 7,
> > +       BPF_LINK_TYPE_TRACING_MULTI = 8,
> >
> >         MAX_BPF_LINK_TYPE,
> >  };
> > @@ -1462,6 +1463,10 @@ union bpf_attr {
> >                                  */
> >                                 __u64           bpf_cookie;
> >                         } perf_event;
> > +                       struct {
> > +                               __aligned_u64   multi_btf_ids;          /* addresses to attach */
> > +                               __u32           multi_btf_ids_cnt;      /* addresses count */
> > +                       };
>
> Please follow the pattern of perf_event, name this struct "multi".
>
> >                 };
> >         } link_create;
> >
> > diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> > index bad03dde97a2..6c16ac43dd91 100644
> > --- a/kernel/bpf/core.c
> > +++ b/kernel/bpf/core.c
>
> [...]
>
> > +
> > +       bpf_link_init(&link->link, BPF_LINK_TYPE_TRACING_MULTI,
> > +                     &bpf_tracing_multi_link_lops, prog);
> > +       link->attach_type = prog->expected_attach_type;
> > +       link->multi = multi;
> > +
> > +       err = bpf_link_prime(&link->link, &link_primer);
> > +       if (err)
> > +               goto out_free;
> > +       err = bpf_trampoline_multi_link_prog(prog, multi);
> > +       if (err)
> > +               goto out_free;
>
> bpf_link_cleanup(), can't free link after priming. Look at other
> places using bpf_link.
>
>
> > +       return bpf_link_settle(&link_primer);
> > +
> > +out_free:
> > +       bpf_trampoline_multi_put(multi);
> > +       kfree(link);
> > +out_free_ids:
> > +       kfree(btf_ids);
> > +       return err;
> > +}
> > +
>
> [...]