Date:   Fri, 19 Nov 2021 09:26:05 -0800
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Mauricio Vásquez <mauricio@...volk.io>
Cc:     Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>,
        Rafael David Tinoco <rafaeldtinoco@...il.com>,
        Lorenzo Fontana <lorenzo.fontana@...stic.co>,
        Leonardo Di Donato <leonardo.didonato@...stic.co>
Subject: Re: [PATCH bpf-next v2 3/4] libbpf: Introduce 'bpf_object__prepare()'

On Tue, Nov 16, 2021 at 8:42 AM Mauricio Vásquez <mauricio@...volk.io> wrote:
>
> BTFGen[0] requires access to the result of the CO-RE relocations without
> actually loading the bpf programs. The current libbpf API doesn't allow
> it because all the object preparation (subprogs, relocations: co-re,
> elf, maps) happens inside bpf_object__load().
>
> This commit introduces a new bpf_object__prepare() function to perform
> all the preparation steps than an ebpf object requires, allowing users
> to access the result of those preparation steps without having to load
> the program. Almost all the steps that were done in bpf_object__load()
> are now done in bpf_object__prepare(), except map creation and program
> loading.
>
> Map relocations require a bit more attention as maps are only created in
> bpf_object__load(). For this reason bpf_object__prepare() relocates maps
> using BPF_PSEUDO_MAP_IDX, if someone dumps the instructions before
> loading the program they get something meaningful. Map relocations are
> completed in bpf_object__load() once the maps are created and we have
> their fd to use with BPF_PSEUDO_MAP_FD.
>
> Users won’t see any visible changes if they’re using bpf_object__open()
> + bpf_object__load() because this commit keeps backwards compatibility
> by calling bpf_object__prepare() in bpf_object_load() if it wasn’t
> called by the user.
>
> bpf_object__prepare_xattr() is not implemented as their counterpart
> bpf_object__load_xattr() will be deprecated[1]. New options will be
> added only to bpf_object_open_opts.
>
> [0]: https://github.com/kinvolk/btfgen/
> [1]: https://github.com/libbpf/libbpf/wiki/Libbpf:-the-road-to-v1.0#libbpfh-high-level-apis
>
> Signed-off-by: Mauricio Vásquez <mauricio@...volk.io>
> Signed-off-by: Rafael David Tinoco <rafael.tinoco@...asec.com>
> Signed-off-by: Lorenzo Fontana <lorenzo.fontana@...stic.co>
> Signed-off-by: Leonardo Di Donato <leonardo.didonato@...stic.co>
> ---

Most of the comments are irrelevant after my comments on patch #4, but
still sending them out for the sake of completeness.

>  tools/lib/bpf/libbpf.c   | 130 ++++++++++++++++++++++++++++-----------
>  tools/lib/bpf/libbpf.h   |   2 +
>  tools/lib/bpf/libbpf.map |   1 +
>  3 files changed, 98 insertions(+), 35 deletions(-)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 6ca76365c6da..f50f9428bb03 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -514,6 +514,7 @@ struct bpf_object {
>         int nr_extern;
>         int kconfig_map_idx;
>
> +       bool prepared;
>         bool loaded;

let's turn `bool loaded` into an enum to represent the stage of a
bpf_object, as there is a strict sequence of state transitions.

>         bool has_subcalls;
>         bool has_rodata;

[...]

> +static int __bpf_object__prepare(struct bpf_object *obj, int log_level,
> +                                const char *target_btf_path)
> +{
> +       int err;
> +
> +       if (obj->prepared) {
> +               pr_warn("object '%s': prepare can't be attempted twice\n", obj->name);
> +               return libbpf_err(-EINVAL);
> +       }
> +
> +       if (obj->gen_loader)
> +               bpf_gen__init(obj->gen_loader, log_level);
> +
> +       err = bpf_object__probe_loading(obj);
> +       err = err ? : bpf_object__load_vmlinux_btf(obj, false);
> +       err = err ? : bpf_object__resolve_externs(obj, obj->kconfig);
> +       err = err ? : bpf_object__sanitize_and_load_btf(obj);
> +       err = err ? : bpf_object__sanitize_maps(obj);
> +       err = err ? : bpf_object__init_kern_struct_ops_maps(obj);
> +       err = err ? : bpf_object__relocate(obj, obj->btf_custom_path ? : target_btf_path);
> +

I think only load_vmlinux_btf and relocations should happen in prepare
phase. resolve_externs might fail if you don't run it on target system
(non-weak extern that's missing will error out). There is no need to
load and sanitize BTFs or sanitize maps either. struct_ops can't work
on the kernel that doesn't have BTF enabled, so BTFgen won't help
there, so it's fine to move it to load phase as well.

We need to move relocations way earlier, right after load_vmlinux_btf
(and move probe_loading to load phase, of course) and perform them in
preparation phase. We can also split relocation into individual steps
and do map relocations in the load phase, so you won't have to do the
dance with map_idx (and given it's internal process, we can always
change our mind later and rework it, if necessary; but for now let's
keep things simple).

> +       obj->prepared = true;
> +
> +       return err;
> +}
> +
> +LIBBPF_API int bpf_object__prepare(struct bpf_object *obj)

LIBBPF_API is used only in header files

> +{
> +       if (!obj)
> +               return libbpf_err(-EINVAL);

if someone passes NULL pointer instead of obj, then it's a completely
inappropriate use of libbpf APIs. No need to check for that (we don't
in a lot of APIs for sure).

> +       return __bpf_object__prepare(obj, 0, NULL);
> +}
> +
>  int bpf_object__load_xattr(struct bpf_object_load_attr *attr)
>  {
>         struct bpf_object *obj;

[...]

Powered by blists - more mailing lists