Re: [PATCH bpf-next v3 3/3] bpftool: Implement btfgen

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Fri, Dec 17, 2021 at 10:57 AM Mauricio Vásquez <mauricio@kinvolk.io> wrote:
>
> The BTFGen's goal is to produce a BTF file that contains **only** the
> information that is needed by an eBPF program. This algorithm does a
> first step collecting the types involved for each relocation present on
> the object and "marking" them as needed. Types are collected in
> different ways according to the type relocation, for field based
> relocations only the union and structures members involved are
> considered, for type based relocations the whole types are added, enum
> field relocations are not supported in this iteration yet.
>
> A second step generates a BTF file from the "marked" types. This step
> accesses the original BTF file extracting the types and their members
> that were "marked" as needed in the first step.
>
> This command is implemented under the "gen" command in bpftool and the
> syntax is the following:
>
> $ bpftool gen btf INPUT OUTPUT OBJECT(S)
>
> INPUT can be either a single BTF file or a folder containing BTF files,
> when it's a folder, a BTF file is generated for each BTF file contained
> in this folder. OUTPUT is the file (or folder) where generated files are
> stored and OBJECT(S) is the list of bpf objects we want to generate the
> BTF file(s) for (each generated BTF file contains all the types needed
> by all the objects).
>
> Signed-off-by: Mauricio Vásquez <mauricio@kinvolk.io>
> Signed-off-by: Rafael David Tinoco <rafael.tinoco@aquasec.com>
> Signed-off-by: Lorenzo Fontana <lorenzo.fontana@elastic.co>
> Signed-off-by: Leonardo Di Donato <leonardo.didonato@elastic.co>
> ---
>  tools/bpf/bpftool/gen.c | 892 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 892 insertions(+)
>

I haven't looked through details of stripping BTF itself, let's
finalize CO-RE relocation parts first. Maybe for the next revision you
could split bpftool changes in two in some reasonable way so that each
patch concentrates on different steps of the process a bit more? E.g.,
first patch might set up new command and BTF stripping parts but leave
the CO-RE relocation logic unimplemented, and the second path fills in
that part. Should make it easier to review this big patch.

Please also cc Quentin Monnet to review bpftool parts as well.


[...]

> +static int btf_reloc_info_gen_type(struct btf_reloc_info *info, struct bpf_core_spec *targ_spec)
> +{
> +       struct btf *btf = (struct btf *) info->src_btf;
> +       struct btf_type *btf_type;
> +       int err = 0;
> +
> +       btf_type = (struct btf_type *) btf__type_by_id(btf, targ_spec->root_type_id);
> +
> +       return btf_reloc_put_type_all(btf, info, btf_type, targ_spec->root_type_id);
> +}
> +
> +static int btf_reloc_info_gen_enumval(struct btf_reloc_info *info, struct bpf_core_spec *targ_spec)
> +{
> +       p_err("untreated enumval based relocation");

why untreated? what's the problem supporting it?

> +       return -EOPNOTSUPP;
> +}
> +
> +static int btf_reloc_info_gen(struct btf_reloc_info *info, struct bpf_core_spec *res)
> +{
> +       if (core_relo_is_type_based(res->relo_kind))
> +               return btf_reloc_info_gen_type(info, res);
> +
> +       if (core_relo_is_enumval_based(res->relo_kind))
> +               return btf_reloc_info_gen_enumval(info, res);
> +
> +       if (core_relo_is_field_based(res->relo_kind))
> +               return btf_reloc_info_gen_field(info, res);

you can have a simple switch here instead of exposing libbpf internal helpers

> +
> +       return -EINVAL;
> +}
> +
> +#define BPF_INSN_SZ (sizeof(struct bpf_insn))
> +
> +static int btfgen_obj_reloc_info_gen(struct btf_reloc_info *reloc_info, struct bpf_object *obj)
> +{
> +       const struct btf_ext_info_sec *sec;
> +       const struct bpf_core_relo *rec;
> +       const struct btf_ext_info *seg;
> +       struct hashmap *cand_cache;
> +       int err, insn_idx, sec_idx;
> +       struct bpf_program *prog;
> +       struct btf_ext *btf_ext;
> +       const char *sec_name;
> +       size_t nr_programs;
> +       struct btf *btf;
> +       unsigned int i;
> +
> +       btf = bpf_object__btf(obj);
> +       btf_ext = bpf_object__btf_ext(obj);
> +
> +       if (btf_ext->core_relo_info.len == 0)
> +               return 0;
> +
> +       cand_cache = bpf_core_create_cand_cache();
> +       if (IS_ERR(cand_cache))
> +               return PTR_ERR(cand_cache);
> +
> +       bpf_object_set_vmlinux_override(obj, reloc_info->src_btf);
> +
> +       seg = &btf_ext->core_relo_info;
> +       for_each_btf_ext_sec(seg, sec) {
> +               bool prog_found;
> +
> +               sec_name = btf__name_by_offset(btf, sec->sec_name_off);
> +               if (str_is_empty(sec_name)) {
> +                       err = -EINVAL;
> +                       goto out;
> +               }
> +
> +               prog_found = false;
> +               nr_programs = bpf_object__get_nr_programs(obj);
> +               for (i = 0; i < nr_programs; i++)       {
> +                       prog = bpf_object__get_program(obj, i);
> +                       if (strcmp(bpf_program__section_name(prog), sec_name) == 0) {
> +                               prog_found = true;
> +                               break;
> +                       }
> +               }
> +
> +               if (!prog_found) {
> +                       pr_warn("sec '%s': failed to find a BPF program\n", sec_name);
> +                       err = -EINVAL;
> +                       goto out;
> +               }
> +
> +               sec_idx = bpf_program__sec_idx(prog);
> +
> +               for_each_btf_ext_rec(seg, sec, i, rec) {
> +                       struct bpf_core_relo_res targ_res;
> +                       struct bpf_core_spec targ_spec;
> +
> +                       insn_idx = rec->insn_off / BPF_INSN_SZ;
> +
> +                       prog = find_prog_by_sec_insn(obj, sec_idx, insn_idx);
> +                       if (!prog) {
> +                               pr_warn("sec '%s': failed to find program at insn #%d for CO-RE offset relocation #%d\n",
> +                                       sec_name, insn_idx, i);
> +                               err = -EINVAL;
> +                               goto out;
> +                       }
> +
> +                       err = bpf_core_calc_relo_res(prog, rec, i, btf, cand_cache, &targ_res,
> +                                                    &targ_spec);


I don't think you need to do *exactly* what libbpf is doing.
bpf_core_calc_relo_res() doesn't add much on top of
bpf_core_calc_relo_insn(), if you use bpf_core_calc_relo_insn()
directly and expose bpf_core_add_cands/bpf_core_free_cands and use
them directly as well, bypassing bpf_object completely, you won't need
btf_vmlinux override and make everything less coupled, I think. In the
future, if you'd like to support BTF module BTFs, you'll have all the
necessary flexibility to do that, while if you try to reuse every
single line of bpf_object's code we'll be adding more hacks like your
bpf_object_set_vmlinux_override().

Fundamentally, you don't care about bpf_object and bpf_programs. All
you need to do is parse .BTF.ext (you can do that just btf__parse, no
need to even construct bpf_object!). Construct candidate cache, then
iterate each CO-RE record, add find/add candidates, calculate
relocation result, use it for your algorithm.

Yes, there will be a bit of duplication (candidate search), but that's
better than trying to turn bpf_object inside out with all the custom
getters/setters that you are exposing (even if it's libbpf internal
only, still makes it really hard to reasons what's going on and what
are consequences of manual control over a lot of bpf_object internal
implementation details).


> +                       if (err)
> +                               goto out;
> +
> +                       err = btf_reloc_info_gen(reloc_info, &targ_spec);
> +                       if (err)
> +                               goto out;
> +               }
> +       }
> +
> +out:
> +       bpf_core_free_cand_cache(cand_cache);
> +
> +       return err;
> +}
> +

[...]