Re: [PATCH bpf-next v2 17/18] selftests/bpf: add basic testcases for tracing_multi

Re: [PATCH bpf-next v2 17/18] selftests/bpf: add basic testcases for tracing_multi

[Ihor Solodrai]

On 7/14/25 4:49 PM, Ihor Solodrai wrote:
> On 7/8/25 1:07 PM, Alexei Starovoitov wrote:
>> On Thu, Jul 3, 2025 at 5:18 AM Menglong Dong 
>> <menglong8.dong@gmail.com> wrote:
>>>
>>> +               return true;
>>> +
>>> +       /* Following symbols have multi definition in kallsyms, take
>>> +        * "t_next" for example:
>>> +        *
>>> +        *     ffffffff813c10d0 t t_next
>>> +        *     ffffffff813d31b0 t t_next
>>> +        *     ffffffff813e06b0 t t_next
>>> +        *     ffffffff813eb360 t t_next
>>> +        *     ffffffff81613360 t t_next
>>> +        *
>>> +        * but only one of them have corresponding mrecord:
>>> +        *     ffffffff81613364 t_next
>>> +        *
>>> +        * The kernel search the target function address by the symbol
>>> +        * name "t_next" with kallsyms_lookup_name() during attaching
>>> +        * and the function "0xffffffff813c10d0" can be matched, which
>>> +        * doesn't have a corresponding mrecord. And this will make
>>> +        * the attach failing. Skip the functions like this.
>>> +        *
>>> +        * The list maybe not whole, so we still can fail......We need a
>>> +        * way to make the whole things right. Yes, we need fix it :/
>>> +        */
>>> +       if (!strcmp(name, "kill_pid_usb_asyncio"))
>>> +               return true;
>>> +       if (!strcmp(name, "t_next"))
>>> +               return true;
>>> +       if (!strcmp(name, "t_stop"))
>>> +               return true;

This little patch will filter out from BTF any static functions with
the same name that appear more than once.

diff --git a/btf_encoder.c b/btf_encoder.c
index 0bc2334..6441269 100644
--- a/btf_encoder.c
+++ b/btf_encoder.c
@@ -96,7 +96,8 @@ struct elf_function {
         const char      *name;
         char            *alias;
         size_t          prefixlen;
-       bool            kfunc;
+       uint8_t         is_static:1;
+       uint8_t         kfunc:1;
         uint32_t        kfunc_flags;
  };

@@ -1374,7 +1375,7 @@ static int saved_functions_combine(struct 
btf_encoder_func_state *a, struct btf_
                 return ret;
         optimized = a->optimized_parms | b->optimized_parms;
         unexpected = a->unexpected_reg | b->unexpected_reg;
-       inconsistent = a->inconsistent_proto | b->inconsistent_proto;
+       inconsistent = a->inconsistent_proto | b->inconsistent_proto | 
a->elf->is_static | b->elf->is_static;
         if (!unexpected && !inconsistent && !funcs__match(a, b))
                 inconsistent = 1;
         a->optimized_parms = b->optimized_parms = optimized;
@@ -1461,6 +1462,8 @@ static void elf_functions__collect_function(struct 
elf_functions *functions, GEl

         func = &functions->entries[functions->cnt];
         func->name = name;
+       func->is_static = elf_sym__bind(sym) == STB_LOCAL;
+
         if (strchr(name, '.')) {
                 const char *suffix = strchr(name, '.');

See the full BTF functions diff here (from vmlinux 6.15.3):
https://gist.github.com/theihor/3f8fabc32d916e592f8e84f434d9950c

This covers t_next and t_stop, but not all functions in the list. Some
of them are not static, such as kill_pid_usb_asyncio [1]. And p_next,
for example, appears only once [2].

So filtering statics in pahole might not be the only problem here.

[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git/tree/kernel/signal.c#n1521
[2] 
https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git/tree/kernel/trace/trace_events.c#n1717


>>
>> This looks like pahole bug. It shouldn't emit BTF for static
>> functions with the same name in different files.
>> I recall we discussed it in the past and I thought the fix had landed.
> 
> I checked this particular case (the t_next function), and what seems
> to be happening is that all function prototypes match, according to
> this check in pahole's BTF encoding:
> 
> * https://github.com/acmel/dwarves/blob/v1.30/btf_encoder.c#L1378
> * https://github.com/acmel/dwarves/blob/v1.30/btf_encoder.c#L1112-L1152
> 
> That is: the name, number and types of parameters all match.
> 
> So at least according to the current pahole logic the prototypes are
> *consistent*. As a result, a single BTF function t_next is emitted.
> 
> Maybe funcs__match() check should be even more strict? Say, disallow
> static functions?
> 
> I am not sure that the draft that Jiri sent [1] is right as it just
> filters out duplicates by name.
> 
> [1] https://lore.kernel.org/bpf/aHD0IdJBqd3XNybw@krava/
> 

Re: [PATCH bpf-next v2 17/18] selftests/bpf: add basic testcases for tracing_multi

[Alexei Starovoitov]

On Tue, Jul 15, 2025 at 5:27 PM Ihor Solodrai <ihor.solodrai@linux.dev> wrote:
>
> On 7/14/25 4:49 PM, Ihor Solodrai wrote:
> > On 7/8/25 1:07 PM, Alexei Starovoitov wrote:
> >> On Thu, Jul 3, 2025 at 5:18 AM Menglong Dong
> >> <menglong8.dong@gmail.com> wrote:
> >>>
> >>> +               return true;
> >>> +
> >>> +       /* Following symbols have multi definition in kallsyms, take
> >>> +        * "t_next" for example:
> >>> +        *
> >>> +        *     ffffffff813c10d0 t t_next
> >>> +        *     ffffffff813d31b0 t t_next
> >>> +        *     ffffffff813e06b0 t t_next
> >>> +        *     ffffffff813eb360 t t_next
> >>> +        *     ffffffff81613360 t t_next
> >>> +        *
> >>> +        * but only one of them have corresponding mrecord:
> >>> +        *     ffffffff81613364 t_next
> >>> +        *
> >>> +        * The kernel search the target function address by the symbol
> >>> +        * name "t_next" with kallsyms_lookup_name() during attaching
> >>> +        * and the function "0xffffffff813c10d0" can be matched, which
> >>> +        * doesn't have a corresponding mrecord. And this will make
> >>> +        * the attach failing. Skip the functions like this.
> >>> +        *
> >>> +        * The list maybe not whole, so we still can fail......We need a
> >>> +        * way to make the whole things right. Yes, we need fix it :/
> >>> +        */
> >>> +       if (!strcmp(name, "kill_pid_usb_asyncio"))
> >>> +               return true;
> >>> +       if (!strcmp(name, "t_next"))
> >>> +               return true;
> >>> +       if (!strcmp(name, "t_stop"))
> >>> +               return true;
>
> This little patch will filter out from BTF any static functions with
> the same name that appear more than once.
>
> diff --git a/btf_encoder.c b/btf_encoder.c
> index 0bc2334..6441269 100644
> --- a/btf_encoder.c
> +++ b/btf_encoder.c
> @@ -96,7 +96,8 @@ struct elf_function {
>          const char      *name;
>          char            *alias;
>          size_t          prefixlen;
> -       bool            kfunc;
> +       uint8_t         is_static:1;
> +       uint8_t         kfunc:1;
>          uint32_t        kfunc_flags;
>   };
>
> @@ -1374,7 +1375,7 @@ static int saved_functions_combine(struct
> btf_encoder_func_state *a, struct btf_
>                  return ret;
>          optimized = a->optimized_parms | b->optimized_parms;
>          unexpected = a->unexpected_reg | b->unexpected_reg;
> -       inconsistent = a->inconsistent_proto | b->inconsistent_proto;
> +       inconsistent = a->inconsistent_proto | b->inconsistent_proto |
> a->elf->is_static | b->elf->is_static;
>          if (!unexpected && !inconsistent && !funcs__match(a, b))
>                  inconsistent = 1;
>          a->optimized_parms = b->optimized_parms = optimized;
> @@ -1461,6 +1462,8 @@ static void elf_functions__collect_function(struct
> elf_functions *functions, GEl
>
>          func = &functions->entries[functions->cnt];
>          func->name = name;
> +       func->is_static = elf_sym__bind(sym) == STB_LOCAL;
> +

Hmm. We definitely don't want to filter out all static functions.
That's too drastic.

Re: [PATCH bpf-next v2 17/18] selftests/bpf: add basic testcases for tracing_multi

[Ihor Solodrai]

On 7/15/25 5:31 PM, Alexei Starovoitov wrote:
> On Tue, Jul 15, 2025 at 5:27 PM Ihor Solodrai <ihor.solodrai@linux.dev> wrote:
>>
>> On 7/14/25 4:49 PM, Ihor Solodrai wrote:
>>> On 7/8/25 1:07 PM, Alexei Starovoitov wrote:
>>>> On Thu, Jul 3, 2025 at 5:18 AM Menglong Dong
>>>> <menglong8.dong@gmail.com> wrote:
>>>>>
>>>>> +               return true;
>>>>> +
>>>>> +       /* Following symbols have multi definition in kallsyms, take
>>>>> +        * "t_next" for example:
>>>>> +        *
>>>>> +        *     ffffffff813c10d0 t t_next
>>>>> +        *     ffffffff813d31b0 t t_next
>>>>> +        *     ffffffff813e06b0 t t_next
>>>>> +        *     ffffffff813eb360 t t_next
>>>>> +        *     ffffffff81613360 t t_next
>>>>> +        *
>>>>> +        * but only one of them have corresponding mrecord:
>>>>> +        *     ffffffff81613364 t_next
>>>>> +        *
>>>>> +        * The kernel search the target function address by the symbol
>>>>> +        * name "t_next" with kallsyms_lookup_name() during attaching
>>>>> +        * and the function "0xffffffff813c10d0" can be matched, which
>>>>> +        * doesn't have a corresponding mrecord. And this will make
>>>>> +        * the attach failing. Skip the functions like this.
>>>>> +        *
>>>>> +        * The list maybe not whole, so we still can fail......We need a
>>>>> +        * way to make the whole things right. Yes, we need fix it :/
>>>>> +        */
>>>>> +       if (!strcmp(name, "kill_pid_usb_asyncio"))
>>>>> +               return true;
>>>>> +       if (!strcmp(name, "t_next"))
>>>>> +               return true;
>>>>> +       if (!strcmp(name, "t_stop"))
>>>>> +               return true;
>>
>> This little patch will filter out from BTF any static functions with
>> the same name that appear more than once.
>>
>> diff --git a/btf_encoder.c b/btf_encoder.c
>> index 0bc2334..6441269 100644
>> --- a/btf_encoder.c
>> +++ b/btf_encoder.c
>> @@ -96,7 +96,8 @@ struct elf_function {
>>           const char      *name;
>>           char            *alias;
>>           size_t          prefixlen;
>> -       bool            kfunc;
>> +       uint8_t         is_static:1;
>> +       uint8_t         kfunc:1;
>>           uint32_t        kfunc_flags;
>>    };
>>
>> @@ -1374,7 +1375,7 @@ static int saved_functions_combine(struct
>> btf_encoder_func_state *a, struct btf_
>>                   return ret;
>>           optimized = a->optimized_parms | b->optimized_parms;
>>           unexpected = a->unexpected_reg | b->unexpected_reg;
>> -       inconsistent = a->inconsistent_proto | b->inconsistent_proto;
>> +       inconsistent = a->inconsistent_proto | b->inconsistent_proto |
>> a->elf->is_static | b->elf->is_static;
>>           if (!unexpected && !inconsistent && !funcs__match(a, b))
>>                   inconsistent = 1;
>>           a->optimized_parms = b->optimized_parms = optimized;
>> @@ -1461,6 +1462,8 @@ static void elf_functions__collect_function(struct
>> elf_functions *functions, GEl
>>
>>           func = &functions->entries[functions->cnt];
>>           func->name = name;
>> +       func->is_static = elf_sym__bind(sym) == STB_LOCAL;
>> +
> 
> Hmm. We definitely don't want to filter out all static functions.
> That's too drastic.

Not all static functions. Only those that match by name.