Message-ID: <CAEf4BzaaUdMnfADQdT=myDJtQtHoQ_aW7T8XidrCkYZ=pGXuaQ@mail.gmail.com>
Date:   Fri, 13 Nov 2020 22:58:56 -0800
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Alan Maguire <alan.maguire@...cle.com>
Cc:     Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>, Martin Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        john fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...omium.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        Ingo Molnar <mingo@...hat.com>, Hao Luo <haoluo@...gle.com>,
        Jiri Olsa <jolsa@...nel.org>,
        Quentin Monnet <quentin@...valent.com>,
        open list <linux-kernel@...r.kernel.org>,
        Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>
Subject: Re: [RFC bpf-next 1/3] bpf: add module support to btf display helpers

On Fri, Nov 13, 2020 at 10:11 AM Alan Maguire <alan.maguire@...cle.com> wrote:
>
> bpf_snprintf_btf and bpf_seq_printf_btf use a "struct btf_ptr *"
> argument that specifies type information about the type to
> be displayed.  Augment this information to include a module
> name, allowing such display to support module types.
>
> Signed-off-by: Alan Maguire <alan.maguire@...cle.com>
> ---
>  include/linux/btf.h            |  8 ++++++++
>  include/uapi/linux/bpf.h       |  5 ++++-
>  kernel/bpf/btf.c               | 18 ++++++++++++++++++
>  kernel/trace/bpf_trace.c       | 42 ++++++++++++++++++++++++++++++++----------
>  tools/include/uapi/linux/bpf.h |  5 ++++-
>  5 files changed, 66 insertions(+), 12 deletions(-)
>
> diff --git a/include/linux/btf.h b/include/linux/btf.h
> index 2bf6418..d55ca00 100644
> --- a/include/linux/btf.h
> +++ b/include/linux/btf.h
> @@ -209,6 +209,14 @@ static inline const struct btf_var_secinfo *btf_type_var_secinfo(
>  const struct btf_type *btf_type_by_id(const struct btf *btf, u32 type_id);
>  const char *btf_name_by_offset(const struct btf *btf, u32 offset);
>  struct btf *btf_parse_vmlinux(void);
> +#ifdef CONFIG_DEBUG_INFO_BTF_MODULES
> +struct btf *bpf_get_btf_module(const char *name);
> +#else
> +static inline struct btf *bpf_get_btf_module(const char *name)
> +{
> +       return ERR_PTR(-ENOTSUPP);
> +}
> +#endif
>  struct btf *bpf_prog_get_target_btf(const struct bpf_prog *prog);
>  #else
>  static inline const struct btf_type *btf_type_by_id(const struct btf *btf,
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 162999b..26978be 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -3636,7 +3636,8 @@ struct bpf_stack_build_id {
>   *             the pointer data is carried out to avoid kernel crashes during
>   *             operation.  Smaller types can use string space on the stack;
>   *             larger programs can use map data to store the string
> - *             representation.
> + *             representation.  Module-specific data structures can be
> + *             displayed if the module name is supplied.
>   *
>   *             The string can be subsequently shared with userspace via
>   *             bpf_perf_event_output() or ring buffer interfaces.
> @@ -5076,11 +5077,13 @@ struct bpf_sk_lookup {
>   * potentially to specify additional details about the BTF pointer
>   * (rather than its mode of display) - is included for future use.
>   * Display flags - BTF_F_* - are passed to bpf_snprintf_btf separately.
> + * A module name can be specified for module-specific data.
>   */
>  struct btf_ptr {
>         void *ptr;
>         __u32 type_id;
>         __u32 flags;            /* BTF ptr flags; unused at present. */
> +       const char *module;     /* optional module name. */

I think module name is a wrong API here, similarly how type name was
wrong API for specifying the type (and thus we use type_id here).
Using the module's BTF ID seems like a more suitable interface. That's
what I'm going to use for all kinds of existing BPF APIs that expect
BTF type to attach BPF programs.

Right now, we use only type_id and implicitly know that it's in
vmlinux BTF. With module BTFs, we now need a pair of BTF object ID +
BTF type ID to uniquely identify the type. vmlinux BTF now can be
specified in two different ways: either leaving BTF object ID as zero
(for simplicity and backwards compatibility) or specifying it's actual
BTF obj ID (which pretty much always should be 1, btw). This feels
like a natural extension, WDYT?

And similar to type_id, no one should expect users to specify these
IDs by hand, Clang built-in and libbpf should work together to figure
this out for the kernel to use.

BTW, with module names there is an extra problem for end users. Some
types could be either built-in or built as a module (e.g., XFS data
structures). Why would we require BPF users to care which is the case
on any given host? It feels right now that we should just extend the
existing __builtin_btf_type_id() helper to generate ldimm64
instructions that would encode both BTF type ID and BTF object ID.
This would just naturally add transparent module BTF support without
BPF programs having to do any changes.

But we need to do a bit of thinking and experimentation with Yonghong,
haven't gotten around to this yet, you are running a bit ahead of me
with module BTFs. :)

>  };
>
>  /*

[...]

>  struct btf_ptr {
>         void *ptr;
>         __u32 type_id;
>         __u32 flags;            /* BTF ptr flags; unused at present. */

Also, if flags are not used at present, can we repurpose it to just
encode btf_obj_id and avoid (at least for now) the backwards
compatibility checks based on btf_ptr size?

> +       const char *module;     /* optional module name. */
>  };
>
>  /*
> --
> 1.8.3.1
>

Powered by blists - more mailing lists