Date:   Fri, 19 Nov 2021 15:51:21 -0800
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Andrii Nakryiko <andrii.nakryiko@...il.com>
Cc:     Mauricio Vásquez <mauricio@...volk.io>,
        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 09:25:03AM -0800, Andrii Nakryiko 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.
> 
> 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 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..)

I was thinking about such split as well, but for a different reason :)
Since we've discussed future kernel flag 'check what libbpf had done'
the idea is to use bpf_core_relo_res after first step and let kernel
look at insn to see whether libbpf relocated the insn the same way
as kernel is going to do.

Also I was thinking to pass struct bpf_core_spec [3] and
struct bpf_core_relo_res [2] as two arrays into bpf_core_calc_relo() to
reduce stack size, since reduction of BPF_CORE_SPEC_MAX_LEN to 32
is not enough when all kconfig debugs are on on some architectures.

I was planning to work on that as a follow up to my set.

In the light of BTFgen I was thinking whether bpf_core_relo_res should
be part of uapi returned by the kernel, but that is probably overkill.

Powered by blists - more mailing lists