Re: [PATCH bpf-next v2 3/5] libbpf: Handle <orig_name>.llvm.<hash> symbol properly

[Yonghong Song <yonghong.song@linux.dev>]

On 3/21/24 5:17 PM, Andrii Nakryiko wrote:
> On Thu, Mar 21, 2024 at 5:02 PM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
>> On Thu, Mar 21, 2024 at 4:55 PM Yonghong Song <yonghong.song@linux.dev> wrote:
>>>>>    static int kallsyms_cb(unsigned long long sym_addr, char sym_type,
>>>>>                          const char *sym_name, void *ctx)
>>>>>    {
>>>>> +       int lto_enabled = check_lto_kernel();
>>>>> +       char orig_name[PATH_MAX], *res;
>>>>>           struct bpf_object *obj = ctx;
>>>>>           const struct btf_type *t;
>>>>>           struct extern_desc *ext;
>>>>>
>>>>> -       ext = find_extern_by_name(obj, sym_name);
>>>>> +       /* Only check static variables in data sections */
>>>>> +       if (sym_type == 'd' && obj->need_kallsyms && lto_enabled == 1) {
>>>> why bother grepping config.gz ?
>>>> I see no harm in doing below strstr unconditionally.
>>> Do you mean we skip condition
>>>          sym_type == 'd' && obj->need_kallsyms && lto_enabled == 1
>>> all together?
>>>
>>> For condition sym_type == 'd', Andrii suggested (in private discussion)
>>> to focus on data first since that is the issue we hitted. Of course
>>> we could do all symbols here too.
>>>
>>> For condition obj->need_kallsyms, I can remove this one.
>>>
>>> For lto_enabled == 1, the main goal is to avoid extra overhead for
>>> not-lto kernels.
>>>
>>> I guess that the overhead is not that bad since typically symbol name
>>> is not long. So removing all conditions seems indeed a viable solution.
>> I was suggesting to remove the last lto_enabled == 1 check and
>> check_lto_kernel() since it will simplify the patch significantly and
>> won't cause any slowdown.
> +1, grepping Kconfig seems like an overkill
>
>> The first two checks... I'm not sure.
> I'd say we shouldn't do this for functions, because if LLVM rewrites
> them, then usually that means that function signature is changed,
> which seems dangerous to just silently resolve. I'd start with data
> symbols for now.

For this particular suffix '.llvm.<hash>', function signature will
not change. But considering all funcitons annotated with __ksym are
kfunc's and I cannot see currently how kfunc's could be cross-file
inlined.

It might be possible a kernel function is attached with __ksym as
bpf program wants to know the address of that kernel function.
But this should be a really corner case.

So agree and let me keep sym_type == 'd' condition only.

>
>> The less corner cases the better. strstr is fast enough.
> yep, I doubt it would be noticeable if we do strstr()
>> Ok to remove them all.