Re: [PATCH bpf-next v2 4/4] libbpf: Expose CO-RE relocation results

On Fri, Dec 3, 2021 at 1:08 PM Mauricio Vásquez Bernal
[off-list ref] wrote:

On Fri, Nov 19, 2021 at 12:25 PM Andrii Nakryiko
[off-list ref] wrote:

quoted

On Tue, Nov 16, 2021 at 8:42 AM Mauricio Vásquez [off-list ref] wrote:

quoted

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 <redacted>
Signed-off-by: Rafael David Tinoco <redacted>
Signed-off-by: Lorenzo Fontana <redacted>
Signed-off-by: Leonardo Di Donato <redacted>
---
 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.

quoted

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.

either move it to relo_core.h (if it's ok to be used in kernel, not
sure, try it) or we can put it into libbpf_internal.h

quoted

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.

yeah, two helpers exposed through libbpf_internal.h is the lesser evil

- 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.

Yeah, bpf_object__btf_ext() sounds good, it's a natural counterpart to
btf_object__btf().

It probably makes sense to also move struct btf_ext_header into btf.h
(btf_header is part of kernel UAPI, BTF.ext header is not going to
change in a non-backwards-compatible way either). There are also
btf_ext_info_sec, bpf_func_info_min and bpf_line_info_min (and Alexei
already exposed core reloc record), which probably makes sense to make
part of btf.h, but that should be tackled separately, probably.

- 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.

record_relo_core can be moved out of bpf_core_apply_relo(). It only
needs prog and relo. It can be called as an alternative to
bpf_core_apply_relo() if gen_loader is set.

- 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.

ok, this one I'm confused with. btf_custom_path in
bpf_object_open_opts doesn't work?..

- 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.

ok

quoted

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?

No, not really, see above. It might be a slightly smaller amount of
refactoring to achieve this, but will make for more complicated code,
I think. Let's try the original approach, see my suggestions above.
Seems like everything is pretty straightforward at this point.

Thanks!

quoted

[...]