Re: [PATCH bpf-next] bpf: Add --skip_encoding_btf_inconsistent_proto, --btf_gen_optimized to pahole flags for v1.25

[Yonghong Song <yhs@meta.com>]

On 5/12/23 7:59 PM, Yonghong Song wrote:
> 
> 
> On 5/12/23 2:54 PM, Jiri Olsa wrote:
>> On Fri, May 12, 2023 at 11:59:34AM -0700, Alexei Starovoitov wrote:
>>> On Fri, May 12, 2023 at 9:04 AM Alan Maguire 
>>> <alan.maguire@oracle.com> wrote:
>>>>
>>>> On 12/05/2023 03:51, Yafang Shao wrote:
>>>>> On Wed, May 10, 2023 at 9:03 PM Alan Maguire 
>>>>> <alan.maguire@oracle.com> wrote:
>>>>>>
>>>>>> v1.25 of pahole supports filtering out functions with multiple 
>>>>>> inconsistent
>>>>>> function prototypes or optimized-out parameters from the BTF 
>>>>>> representation.
>>>>>> These present problems because there is no additional info in BTF 
>>>>>> saying which
>>>>>> inconsistent prototype matches which function instance to help 
>>>>>> guide attachment,
>>>>>> and functions with optimized-out parameters can lead to incorrect 
>>>>>> assumptions
>>>>>> about register contents.
>>>>>>
>>>>>> So for now, filter out such functions while adding BTF 
>>>>>> representations for
>>>>>> functions that have "."-suffixes (foo.isra.0) but not 
>>>>>> optimized-out parameters.
>>>>>> This patch assumes that below linked changes land in pahole for 
>>>>>> v1.25.
>>>>>>
>>>>>> Issues with pahole filtering being too aggressive in removing 
>>>>>> functions
>>>>>> appear to be resolved now, but CI and further testing will confirm.
>>>>>>
>>>>>> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
>>>>>> ---
>>>>>>   scripts/pahole-flags.sh | 3 +++
>>>>>>   1 file changed, 3 insertions(+)
>>>>>>
>>>>>> diff --git a/scripts/pahole-flags.sh b/scripts/pahole-flags.sh
>>>>>> index 1f1f1d397c39..728d55190d97 100755
>>>>>> --- a/scripts/pahole-flags.sh
>>>>>> +++ b/scripts/pahole-flags.sh
>>>>>> @@ -23,5 +23,8 @@ if [ "${pahole_ver}" -ge "124" ]; then
>>>>>>          # see PAHOLE_HAS_LANG_EXCLUDE
>>>>>>          extra_paholeopt="${extra_paholeopt} --lang_exclude=rust"
>>>>>>   fi
>>>>>> +if [ "${pahole_ver}" -ge "125" ]; then
>>>>>> +       extra_paholeopt="${extra_paholeopt} 
>>>>>> --skip_encoding_btf_inconsistent_proto --btf_gen_optimized"
>>>>>> +fi
>>>>>>
>>>>>>   echo ${extra_paholeopt}
>>>>>> -- 
>>>>>> 2.31.1
>>>>>>
>>>>>
>>>>> That change looks like a workaround to me.
>>>>> There may be multiple functions that have the same proto, e.g.:
>>>>>
>>>>>    $ grep -r "bpf_iter_detach_map(struct bpf_iter_aux_info \*aux)"
>>>>> kernel/bpf/ net/core/
>>>>>    kernel/bpf/map_iter.c:static void bpf_iter_detach_map(struct
>>>>> bpf_iter_aux_info *aux)
>>>>>    net/core/bpf_sk_storage.c:static void bpf_iter_detach_map(struct
>>>>> bpf_iter_aux_info *aux)
>>>>>
>>>>>    $ bpftool btf dump file /sys/kernel/btf/vmlinux   |  grep -B 2
>>>>> bpf_iter_detach_map
>>>>>    [34691] FUNC_PROTO '(anon)' ret_type_id=0 vlen=1
>>>>>    'aux' type_id=2638
>>>>>    [34692] FUNC 'bpf_iter_detach_map' type_id=34691 linkage=static
>>>>>
>>>>> We don't know which one it is in the BTF.
>>>>> However, I'm not against this change, as it can avoid some issues.
>>>>>
>>>>
>>>> In the above case, the BTF representation is consistent though.
>>>> That is, if I attach fentry progs to either of these functions
>>>> based on that BTF representation, nothing will crash.
>>>>
>>>> That's ultimately what those changes are about; ensuring
>>>> consistency in BTF representation, so when a function is in
>>>> BTF we can know the signature of the function can be safely
>>>> used by fentry for example.
>>>>
>>>> The question of being able to identify functions (as opposed
>>>> to having a consistent representation) is the next step.
>>>> Finding a way to link between kallsyms and BTF would allow us to
>>>> have multiple inconsistent functions in BTF, since we could map
>>>> from BTF -> kallsyms safely. So two functions called "foo"
>>>> with different function signatures would be okay, because
>>>> we'd know which was which in kallsyms and could attach
>>>> safely. Something like a BTF tag for the function that could
>>>> clarify that mapping - but just for cases where it would
>>>> otherwise be ambiguous - is probably the way forward
>>>> longer term.
>>>>
>>>> Jiri's talking about this topic at LSF/MM/BPF this week I believe.
>>>
>>> Jiri presented a few ideas during LSFMMBPF.
>>>
>>> I feel the best approach is to add a set of addr-s to BTF
>>> via a special decl_tag.
>>> We can also consider extending KIND_FUNC.
>>> The advantage that every BTF func will have one or more addrs
>>> associated with it and bpf prog loading logic wouldn't need to do
>>> fragile name comparison between btf and kallsyms.
>>> pahole can take addrs from dwarf and optionally double check with 
>>> kallsyms.
>>
>> Yonghong summed it up in another email discussion, pasting it in here:
>>
>>    So overall we have three options as kallsyms representation now:
>>      (a) "addr module:foo:dir_a/dir_b/core.c"
>>      (b) "addr module:foo"
>>      (c) "addr module:foo:btf_id"
>>
>>    option (a):
>>      'dir_a/dir_b/core.c' needs to be encoded in BTF.
>>      user space either check file path or func signature
>>      to find attach_btf_id and pass to the kernel.
>>      kernel can find file path in BTF and then lookup
>>      kallsyms to find addr.
>>
>>    option (b):
>>      "addr" needs to be encoded in BTF.
>>      user space checks func signature to find
>>      attach_btf_id and pass to the kernel.
>>      kernel can find addr in BTF and use it.
>>
>>    option (c):
>>      if user can decide which function to attach, e.g.,
>>      through func signature, then no BTF encoding
>>      is necessary. attach_btf_id is passed to the
>>      kernel and search kallsyms to find the matching
>>      btf_id and 'addr' will be available then.
>>
>>    For option (b) and (c), user space needs to check
>>    func signature to find which btf_id to use. If
>>    same-name static functions having the identical
>>    signatures, then user space would have a hard time
>>    to differentiate. I think it should be very
>>    rare same-name static functions in the kernel will have
>>    identical signatures. But if we want 100% correctness,
>>    we may need file path in which case option (a)
>>    is preferable.
> 
> As Alexei mentioned in previous email, for such a extreme case,
> if user is willing to go through extra step to check dwarf
> to find and match file path, then (b) and (c) should work
> perfectly as well.

Okay, it looks like this is more complex if the function signature is
the same. In such cases, current BTF dedup will merge these
functions as a single BTF func. In such cases, we could have:

    decl_tag_1   ----> dedup'ed static_func
                          ^
                          |
    decl_tag_2   ---------

For such cases, just passing btf_id of static func to kernel
won't work since the kernel won't be able to know which
decl_tag to be associated with.

(I did a simple test with vmlinux, it looks we have
  issues with decl_tag_1/decl_tag_2 -> dedup'ed static_func
  as well since only one of decl_tag survives.
  But this is a different issue.
)

So if we intend to add decl tag (addr or file_path), we
should not dedup static functions or generally any functions.

> 
>>
>>    Current option (a) kallsyms format is under review.
>>    option (c) also needs kallsyms change...
>>
>>
>> my thoughts so far is that I like the idea of storing functions address
>> in BTF (option b), because it's the easiest way
>>
>> on the other hand, user would need debug info to find address for the 
>> function
>> to trace.. but still just for small subset of functions that share 
>> same name
>>
>> also I like Lorenz's idea of storing BTF ID in kalsyms and verifier being
>> able to lookup address based on BTF ID.. seems like easier kallsyms 
>> change,
>> but it would still require storing objects paths in BTF to pick up the
>> correct one
>>
>> cc-ing other folks
>>
>> jirka