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

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