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@xxxxxxxxx> wrote: > > > > On Thu, May 18, 2023 at 11:26 AM Yonghong Song <yhs@xxxxxxxx> 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@xxxxxxxxxxxxxx/#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.
