Message-ID: <CAEf4BzbrwiAJobuU01rd3XEw_b-vbUiL-uqM4_5_FZuAT7rSxA@mail.gmail.com>
Date:   Tue, 8 Jun 2021 22:18:21 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Jiri Olsa <jolsa@...nel.org>
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 13/19] bpf: Add support to link multi func tracing program

On Sat, Jun 5, 2021 at 4:12 AM Jiri Olsa <jolsa@...nel.org> wrote:
>
> Adding support to attach multiple functions to tracing program
> by using the link_create/link_update interface.
>
> Adding multi_btf_ids/multi_btf_ids_cnt pair to link_create struct
> API, that define array of functions btf ids that will be attached
> to prog_fd.
>
> The prog_fd needs to be multi prog tracing program (BPF_F_MULTI_FUNC).

So I'm not sure why we added a new load flag instead of just using a
new BPF program type or expected attach type?  We have different
trampolines and different kinds of links for them, so why not be
consistent and use the new type of BPF program?.. It does change BPF
verifier's treatment of input arguments, so it's not just a slight
variation, it's quite different type of program.

>
> The new link_create interface creates new BPF_LINK_TYPE_TRACING_MULTI
> link type, which creates separate bpf_trampoline and registers it
> as direct function for all specified btf ids.
>
> The new bpf_trampoline is out of scope (bpf_trampoline_lookup) of
> standard trampolines, so all registered functions need to be free
> of direct functions, otherwise the link fails.
>
> The new bpf_trampoline will store and pass to bpf program the highest
> number of arguments from all given functions.
>
> New programs (fentry or fexit) can be added to the existing trampoline
> through the link_update interface via new_prog_fd descriptor.
>
> Signed-off-by: Jiri Olsa <jolsa@...nel.org>
> ---
>  include/linux/bpf.h            |   3 +
>  include/uapi/linux/bpf.h       |   5 +
>  kernel/bpf/syscall.c           | 185 ++++++++++++++++++++++++++++++++-
>  kernel/bpf/trampoline.c        |  53 +++++++---
>  tools/include/uapi/linux/bpf.h |   5 +
>  5 files changed, 237 insertions(+), 14 deletions(-)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 23221e0e8d3c..99a81c6c22e6 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -661,6 +661,7 @@ struct bpf_trampoline {
>         struct bpf_tramp_image *cur_image;
>         u64 selector;
>         struct module *mod;
> +       bool multi;
>  };
>
>  struct bpf_attach_target_info {
> @@ -746,6 +747,8 @@ void bpf_ksym_add(struct bpf_ksym *ksym);
>  void bpf_ksym_del(struct bpf_ksym *ksym);
>  int bpf_jit_charge_modmem(u32 pages);
>  void bpf_jit_uncharge_modmem(u32 pages);
> +struct bpf_trampoline *bpf_trampoline_multi_alloc(void);
> +void bpf_trampoline_multi_free(struct bpf_trampoline *tr);
>  #else
>  static inline int bpf_trampoline_link_prog(struct bpf_prog *prog,
>                                            struct bpf_trampoline *tr)
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index ad9340fb14d4..5fd6ff64e8dc 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -1007,6 +1007,7 @@ enum bpf_link_type {
>         BPF_LINK_TYPE_ITER = 4,
>         BPF_LINK_TYPE_NETNS = 5,
>         BPF_LINK_TYPE_XDP = 6,
> +       BPF_LINK_TYPE_TRACING_MULTI = 7,
>
>         MAX_BPF_LINK_TYPE,
>  };
> @@ -1454,6 +1455,10 @@ union bpf_attr {
>                                 __aligned_u64   iter_info;      /* extra bpf_iter_link_info */
>                                 __u32           iter_info_len;  /* iter_info length */
>                         };
> +                       struct {
> +                               __aligned_u64   multi_btf_ids;          /* addresses to attach */
> +                               __u32           multi_btf_ids_cnt;      /* addresses count */
> +                       };

let's do what bpf_link-based TC-BPF API is doing, put it into a named
field (I'd do the same for iter_info/iter_info_len above as well, I'm
not sure why we did this flat naming scheme, we now it's inconvenient
when extending stuff).

struct {
    __aligned_u64 btf_ids;
    __u32 btf_ids_cnt;
} multi;

>                 };
>         } link_create;
>

[...]

> +static int bpf_tracing_multi_link_update(struct bpf_link *link,
> +                                        struct bpf_prog *new_prog,
> +                                        struct bpf_prog *old_prog __maybe_unused)
> +{

BPF_LINK_UPDATE command supports passing old_fd and extra flags. We
can use that to implement both updating existing BPF program in-place
(by passing BPF_F_REPLACE and old_fd) or adding the program to the
list of programs, if old_fd == 0. WDYT?

> +       struct bpf_tracing_multi_link *tr_link =
> +               container_of(link, struct bpf_tracing_multi_link, link);
> +       int err;
> +
> +       if (check_multi_prog_type(new_prog))
> +               return -EINVAL;
> +
> +       err = bpf_trampoline_link_prog(new_prog, tr_link->tr);
> +       if (err)
> +               return err;
> +
> +       err = modify_ftrace_direct_multi(&tr_link->ops,
> +                                        (unsigned long) tr_link->tr->cur_image->image);
> +       return WARN_ON(err);
> +}
> +

[...]

Powered by blists - more mailing lists