Date:   Fri, 3 Dec 2021 16:08:26 -0500
From:   Mauricio Vásquez Bernal <mauricio@...volk.io>
To:     Andrii Nakryiko <andrii.nakryiko@...il.com>
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 4/4] libbpf: Expose CO-RE relocation results

On Fri, Nov 19, 2021 at 12:25 PM Andrii Nakryiko
<andrii.nakryiko@...il.com> wrote:
>
> On Tue, Nov 16, 2021 at 8:42 AM Mauricio Vásquez <mauricio@...volk.io> wrote:
> >
> > The result of the CO-RE relocations can be useful for some use cases
> > like BTFGen[0]. This commit adds a new ‘record_core_relos’ option to
> > save the result of such relocations and a couple of functions to access
> > them.
> >
> > [0]: https://github.com/kinvolk/btfgen/
> >
> > 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>
> > ---
> >  tools/lib/bpf/libbpf.c    | 63 ++++++++++++++++++++++++++++++++++++++-
> >  tools/lib/bpf/libbpf.h    | 49 +++++++++++++++++++++++++++++-
> >  tools/lib/bpf/libbpf.map  |  2 ++
> >  tools/lib/bpf/relo_core.c | 28 +++++++++++++++--
> >  tools/lib/bpf/relo_core.h | 21 ++-----------
> >  5 files changed, 140 insertions(+), 23 deletions(-)
> >
>
> Ok, I've meditated on this patch set long enough. I still don't like
> that libbpf will be doing all this just for the sake of BTFGen's use
> case.
>
> In the end, I think giving bpftool access to internal APIs of libbpf
> is more appropriate, and it seems like it's pretty easy to achieve. It
> might actually clean up gen_loader parts a bit as well. So we'll need
> to coordinate all this with Alexei's current work on CO-RE for kernel
> as well.

Fine for us. I followed the CO-RE in the kernel patch and I didn't
spot any change that could complicate the BTFGen implementation.

> But here's what I think could be done to keep libbpf internals simple.
> We split bpf_core_apply_relo() into two parts: 1) calculating the
> struct bpf_core_relo_res and

For the BTFGen case we actually need "struct bpf_core_relo_res". I
suppose it's not a big deal to move its definition to a header file
that can be included by bpftool.

> 2) patching the instruction. If you look
> at bpf_core_apply_relo, it needs prog just for prog_name (which we can
> just pass everywhere for logging purposes) and to extract one specific
> instruction to be patched. This instruction is passed at the very end
> to bpf_core_patch_insn() after bpf_core_relo_res is calculated. So I
> propose to make those two explicitly separate steps done by libbpf. So
> bpf_core_apply_relo() (which we should rename to bpf_core_calc_relo()
> or something like that) won't do any modification to the program
> instructions. bpf_object__relocate_core() will do bpf_core_calc_relo()
> first, if that's successful, it will pass the result into
> bpf_core_patch_insn(). Simple and clean, unless I missed some
> complication (happens all the time, but..)

While implementing a prototype of this idea I faced the following challenges:
- bpf_core_apply_relo() requires a candidate cache. I think we can
create two helpers functions to create / destroy a candidate cache so
we don't have to worry about it's internals in bpftool.
- we need to access obj->btf_ext in bpftool. It should be fine to
create bpf_object__btf_ext() as part of the public libbpf api.
- bpf_core_apply_relo() requires the bpf program as argument. Before
Alexei's patches it was used only for logging and getting the
instruction. Now it's also used to call record_relo_core(). Getting it
from bpftool is not that easy, in order to do I had to expose
bpf_program__sec_idx() and find_prog_by_sec_insn() to bpftool. We
could find a way to avoid passing prog but I think it's important for
the logs.
- obj->btf_vmlinux_override needs to be set in order to calculate the
core relocations. It's only set in bpf_object__relocate_core() but
we're not using this function. I created and exposed a
bpf_object_set_vmlinux_override() function to bpftool.
- There are also some naming complications but we can discuss them
when I send the patch.

I'm going to polish a bit more and finish rebasing on top of "CO-RE in
the kernel" changes to then send the patch. Please let me know if you
have any big concerns regarding my points above.

> At this point, we can teach bpftool to just take btf_ext (from BPF
> object file), iterate over all CO-RE relos and call only
> bpf_core_calc_relo() (no instruction patching). Using
> bpf_core_relo_res bpftool will know types and fields that need to be
> preserved and it will be able to construct minimal btf. So interface
> for bpftool looks like this:
>
>    bpftool gen distill_btf (or whatever the name) <file.bpf.o>
> <distilled_raw.btf>
>
> BTFGen as a solution, then, can use bpftool to process each pair of
> btf + bpf object.
>
> Thoughts?

I have the feeling that it could be easier to extend
bpf_object__relocate_core() to be able to calculate the core
relocations without patching the instructions (something similar to
what we did in v1). bpftool could pass two parameters to gather this
information and the normal libbpf workflow could just pass NULL to
indicate the instructions should be actually patched. I think this
could help specially with the difficulty to get the prog argument from
bpftool (we are almost implementing the same logic present on
bpf_object__relocate_core() to get sec_idx, prog and so on).  Does it
make any sense to you?

Thanks!

> [...]

Powered by blists - more mailing lists