On 12/05/2023 03:51, Yafang Shao wrote:
> On Wed, May 10, 2023 at 9:03 PM Alan Maguire <alan.maguire@xxxxxxxxxx> 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@xxxxxxxxxx>
>> ---
>> 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.
Thanks!
Alan
