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

[Alan Maguire <alan.maguire@oracle.com>]

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

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