Re: [RFC dwarves 5/6] btf_encoder: store ELF function representations sorted by name _and_ address

[Jiri Olsa <olsajiri@gmail.com>]

On Mon, May 22, 2023 at 02:31:01PM -0700, Andrii Nakryiko wrote:
> On Thu, May 18, 2023 at 5:26 PM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> >
> > On Thu, May 18, 2023 at 11:26 AM Yonghong Song <yhs@meta.com> wrote:
> > > > I wonder now when the address will be stored as number (not string) we
> > > > could somehow generate relocation records and have the module loader
> > > > do the relocation automatically
> > > >
> > > > not sure how that works for vmlinux when it's loaded/relocated on boot
> > >
> > > Right, actual module address will mostly not match the one in dwarf.
> > > Some during module btf load, we should modify btf address as well
> > > for later use? Yes, may need to reuse some routines used in initial
> > > module relocation.
> >
> >
> > Few thoughts:
> >
> > Initially I felt that single FUNC with multiple DECL_TAG(addr)
> > is better, since BTF for all funcs is the same and it's likely
> > one static inline function that the compiler decided not to inline
> > (like cpumask_weight), so when libbpf wants to attach prog to it
> > the kernel should automatically attach in all places.
> > But then noticed that actually different functions with
> > the same name and proto will be deduplicated into one.
> > Their bodies at different locations will be different.
> > Example: seq_show.
> > In this case it's better to let libbpf pick the exact one to attach.
> > Then realized that even the same function like cpumask_weight()
> > might have different body at different locations due to optimizations.
> > I don't think dwarf contains enough info to distinguish all the combinations.
> >
> > Considering all that it's better to keep one BTF kind_func -> one addr.
> > If it's extended the way Alan is proposing with kind_flag
> > the dedup logic will not combine them due to different addresses.
> 
> I've discussed this w/ Alexei and Yonghong offline, so will summarize
> what I said here. I don't think that we should go the route of adding
> kflag to BTF_KIND_FUNC. As Yonghong pointed out, previously only vlen
> and kind determined byte size of the type, and so adding a third
> variable (kflag), which would apply only to BTF_KIND_FUNC, seems like
> an unnecessary new complication.
> 
> I propose to go with an entirely new kind instead, we have plenty of
> them left. This new kind will be pretty kernel-specific, so could be
> targeted for kernel use cases better without adding unnecessary
> complications to Clang. BTF_KIND_FUNCs generated by Clang for .bpf.o
> files don't need addr, they are meaningless and Clang doesn't know
> anything about addresses anyways. So we can keep Clang unchanged and
> more backwards compatible.
> 
> But now that this new kind (BTF_KIND_KERNEL_FUNC? KFUNC would be
> misleading, unfortunately) is kernel-specific and generated by pahole
> only, besides addr we can add some flags field and use them to mark
> function as defined as kfunc or not, or (as a hypothetical example)
> traceable or not, or maybe we even have inline flag some day, etc.
> Something that makes sense mostly for kernel functions.
> 
> Having said all that, given we are going to break all existing
> BTF-aware tools again with a new kind, we should really couple all
> this work with making BTF self-describing as discussed in [0], so that
> future changes like this won't break older bpftool and other similar
> tools, unnecessarily.

nice, would be great to have this and eventually got rid of new pahole
enable/disable options, makes sense to do this before adding new type

jirka

> 
> Which, btw, is another reason to not use kflag to determine the size
> of btf_type. Proposed solution in [0] assumes that kind + vlen defines
> the size. We should probably have dedicated discussion for
> self-describing BTF, but I think both changes have to be done in the
> same release window.
> 
>   [0] https://lore.kernel.org/bpf/CAEf4BzYjWHRdNNw4B=eOXOs_ONrDwrgX4bn=Nuc1g8JPFC34MA@mail.gmail.com/#t
> 
> >
> > Also turned out that the kernel doesn't validate decl_tag string.
> > The following code loads without error:
> > __attribute__((btf_decl_tag("\x10\xf0")));
> >
> > I'm not sure whether we want to tighten decl_tag validation and how.
> > If we keep it as-is we can use func+decl_tag approach
> > to add 4 bytes of addr in the binary format (if 1st byte is not zero).
> > But it feels like a hack, since the kernel needs to be changed
> > anyway to adjust the addresses after module loading and kernel relocation.
> > So func with kind_flag seems like the best approach.
> >
> > Regarding relocation of address in the kernel and modules...
> > We just need to add base_addr to all addrs-es recorded in BTF.
> > Both for kernel and for module BTFs.
> > Shouldn't be too complicated.
> 
> yep, KASLR seems simple enough to handle by the kernel itself at boot time.