On 25/05/2023 09:52, Jiri Olsa wrote: > 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 > agreed; I'd been thinking the same and I've been working on a proof-of- concept of this based on our previous discussions, I'll send it out as soon as I've got it roughly working. With respect to the question of having a new kind, I'm not sure I agree with the above. We've already broken the "vlen == number of objects following" for BTF_KIND_FUNC, where vlen is used to represent linkage information instead. To me, it feels more natural to have continuity across different object types (kernel versus BPF program) with BTF_KIND_FUNC: the fact that it's hard to come up with an alternate name is perhaps a reflection of this. Most characteristics (aside from "is a kfunc") seem to be shared across kernel and BPF program functions, but the best way to judge is probably to come up with as complete a list as is possible I suppose. In order to accommodate a metadata description using existing BTF_KIND_FUNC, we can have a metadata flag that can say "KFLAG set means singular object following of object_size" that is set for BTF_KIND_FUNC. We can mark it as discouraged for future use. One argument I definitely see for a new kind representing kernel functions is if it were the case that we might need N elements _and_ a singular object following the btf_type to represent it. I don't currently see any use for such a model for function representation, but if that is anticipated somehow, it might be worth having a new kind to support that sort of representation. Alan >> >> 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. >
