Re: [PATCH] bpf: Mark kfuncs as __noclone

[Alan Maguire <alan.maguire@oracle.com>]

On 27/08/2025 20:13, Eduard Zingerman wrote:
> On Wed, 2025-08-27 at 10:00 -0700, Yonghong Song wrote:
>>
>> On 8/26/25 10:02 PM, Eduard Zingerman wrote:
>>> On Tue, 2025-08-26 at 13:17 -0700, Yonghong Song wrote:
>>>
>>> [...]
>>>
>>>> I tried with gcc14 and can reproduced the issue described in the above.
>>>> I build the kernel like below with gcc14
>>>>     make KCFLAGS='-O3' -j
>>>> and get the following build error
>>>>     WARN: resolve_btfids: unresolved symbol bpf_strnchr
>>>>     make[2]: *** [/home/yhs/work/bpf-next/scripts/Makefile.vmlinux:91: vmlinux] Error 255
>>>>     make[2]: *** Deleting file 'vmlinux'
>>>> Checking the symbol table:
>>>>      22276: ffffffff81b15260   249 FUNC    LOCAL  DEFAULT    1 bpf_strnchr.cons[...]
>>>>     235128: ffffffff81b1f540   296 FUNC    GLOBAL DEFAULT    1 bpf_strnchr
>>>> and the disasm code:
>>>>     bpf_strnchr:
>>>>       ...
>>>>
>>>>     bpf_strchr:
>>>>       ...
>>>>       bpf_strnchr.constprop.0
>>>>       ...
>>>>
>>>> So in symbol table, we have both bpf_strnchr.constprop.0 and bpf_strnchr.
>>>> For such case, pahole will skip func bpf_strnchr hence the above resolve_btfids
>>>> failure.
>>>>
>>>> The solution in this patch can indeed resolve this issue.
>>> It looks like instead of adding __noclone there is an option to
>>> improve pahole's filtering of ambiguous functions.
>>> Abstractly, there is nothing wrong with having a clone of a global
>>> function that has undergone additional optimizations. As long as the
>>> original symbol exists, everything should be fine.
>>
>> Right. The generated code itself is totally fine. The problem is
>> currently pahole will filter out bpf_strnchr since in the symbol table
>> having both bpf_strnchr and bpf_strnchr.constprop.0. It there is
>> no explicit dwarf-level signature in dwarf for bpf_strnchr.constprop.0.
>> (For this particular .constprop.0 case, it is possible to derive the
>>   signature. but it will be hard for other suffixes like .isra).
>> The current pahole will have strip out suffixes so the function
>> name is 'bpf_strnchr' which covers bpf_strnchr and bpf_strnchr.constprop.0.
>> Since two underlying signature is different, the 'bpf_strnchr'
>> will be filtered out.
> 
> Yes, I understand the mechanics. My question is: is it really
> necessary for pahole to go through this process?
> 
> It sees two functions: 'bpf_strnchr', 'bpf_strnchr.constprop.0',
> first global, second local, first with DWARF signature, second w/o
> DWARF signature. So, why conflating the two?
> 
> For non-lto build the function being global guarantees signature
> correctness, and below you confirm that it is the case for lto builds
> as well. So, it looks like we are just loosing 'bpf_strnchr' for no
> good reason.
>

I'm working on a small 2-patch series at the moment to improve this. The
problem is we currently have no way to associate the DWARF with the
relevant ELF function; DWARF representations of functions do not have
"." suffixes either so we are just matching by name prefix when we
collect DWARF info about a particular function.

The series I'm working on uses DWARF addresses to improve the DWARF/ELF
association, ensuring that we don't toss functions that look
inconsistent but just have .part or .cold suffixed components that have
non-matching DWARF function signatures. ".constprop" isn't covered yet
however.

>> I am actually working to improve such cases in llvm to address
>> like foo() and foo.<...>() functions and they will have their
>> own respective functions. We will discuss with gcc folks
>> about how to implement similar approaches in gcc.
>>
>>>
>>> Since kfuncs are global, this should guarantee that the compiler does not
>>> change their signature, correct? Does this also hold for LTO builds?
>>
>> Yes, the original signature will not changed. This holds for LTO build
>> and global variables/functions will not be renamed.
>>
>>> If so, when pahole sees a set of symbols like [foo, foo.1, foo.2, ...],
>>
>> The compiler needs to emit the signature in dwarf for foo.1, foo.2, etc. and this
>> is something I am working on.
>>
>>> with 'foo' being global and the rest local, then there is no real need
>>> to filter out 'foo'.
>>
>> I think the current __noclone approach is okay as the full implementation
>> for signature changes (foo, foo.1, ...) might takes a while for both llvm
>> and gcc.
>>
>>>
>>> Wdyt?
>>>
>>> [...]