Re: [PATCH] kallsyms: let kallsyms_on_each_match_symbol match symbols exactly

[Yonghong Song <yhs@meta.com>]

On 6/22/23 1:33 PM, Song Liu wrote:
> 
> 
>> On Jun 22, 2023, at 9:10 AM, Yonghong Song <yhs@meta.com> wrote:
> 
> [...]
> 
>>
>>> Effect from the tracers POV?
>>>    1. .llvm.<hash> suffix
>>>       The names without the .llvm.<hash> suffix are the same as without
>>>       LTO. This is probably why commit 8b8e6b5d3b013b0b ("kallsyms: strip
>>>       ThinLTO hashes from static functions") worked. The tracers probably
>>>       wanted to access only the symbols with uniqueue names
>>>    2. <function>.<symbol> name
>>>       The name without the .<symbol> suffix is the same as the function
>>>       name. The result are duplicated function names.
>>>       I do not understand why this was not a problem for tracers.
>>>       Note that this is pretty common. _entry and _entry_ptr are
>>>       added into any function calling printk().
>>>       It seems to be working only by chance. Maybe, the tracers always
>>>       take the first matched symbol. And the function name, without
>>>       any suffix, is always the first one in the sorted list.
>>
>> Note this only happens in LTO mode. Maybe lto kernel is not used
>> wide enough to discover this issue?
> 
> I think this is because all these <function>.<symbol> are data, while
> tracers are looking for functions.
> 
>>
>>> Effect from livepatching POV:
>>>    1. .llvm.<hash> suffix
>>>        Comparing the full symbol name looks fragile to me because
>>>        the <hash> might change.
>>>        IMHO, it would be better to compare the names without
>>>        the .llvm.<hash> suffix even for livepatches.
>>>     2. <function>.<symbol> name
>>>        The removal of <.symbol> suffix is a bad idea. The livepatch
>>>        code is not able to distinguish the symbol of the <function>
>>>        and static variables defined in this function.
>>>        IMHO, it would be better to compare the full
>>>         <function>.<symbol> name.
>>> Result:
>>> IMHO, cleanup_symbol_name() should remove only .llwn.* suffix.
>>> And it should be used for both tracers and livepatching.
>>> Does this makes any sense?
>>
>> Song, does this fix the problem?
> 
> I think this should work. We also see .str.<num.>llvm.<hash>.
> But those should not matter.

For some string constants, llvm may create a local symbol with
name like '.str'. If there are more than one such local
symbols, they become '.str.<num>'. Then when they got promoted
to global they become '.str.<num>.llvm.<hash>'.

> 
> Thanks,
> Song
> 
>>
>> I only checked llvm15 and llvm17, not sure what kind of
>> suffix'es used for early llvm (>= llvm11).
>> Nick, could you help answer this question? What kind
>> of suffix are used for lto when promoting a local symbol
>> to a global one, considering all versions of llvm >= 11
>> since llvm 11 is the minimum supported version for kernel build.
>