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

[Yonghong Song <yhs@meta.com>]

On 6/22/23 6:35 AM, Petr Mladek wrote:
> Hi,
> 
> I have added people mentioned in commits which modified
> cleanup_symbol_name() in kallsyms.c.
> 
> I think that stripping ".*" suffix does not work for static
> variables defined locally from symbol does always work,
> see below.
> 
> 
> 
> On Wed 2023-06-21 15:34:27, Yonghong Song wrote:
>> On 6/21/23 12:18 PM, Song Liu wrote:
>>>> On Jun 21, 2023, at 1:52 AM, Petr Mladek <pmladek@suse.com> wrote:
>>>> On Tue 2023-06-20 22:36:14, Song Liu wrote:
>>>>>> On Jun 19, 2023, at 4:32 AM, Petr Mladek <pmladek@suse.com> wrote:
>>>>>> On Sun 2023-06-18 22:05:19, Song Liu wrote:
>>>>>>> On Sun, Jun 18, 2023 at 8:32 PM Leizhen (ThunderTown)
>>>>>>> <thunder.leizhen@huawei.com> wrote:
>>>>> I am not quite following the direction here. Do we need more
>>>>> work for this patch?
>>>>
>>>> Good question. I primary tried to add more details so that
>>>> we better understand the problem.
>>>>
>>>> Honestly, I do not know the answer. I am neither familiar with
>>>> kpatch nor with clang.
> 
>>> This is pretty complicated.
>>>
>>> 1. clang with LTO does not use the suffix to eliminated duplicated
>>> kallsyms, so old_sympos is still needed. Here is an example:
>>>
>>> # grep init_once /proc/kallsyms
>>> ffffffff8120ba80 t init_once.llvm.14172910296636650566
>>> ffffffff8120ba90 t inode_init_once
>>> ffffffff813ea5d0 t bpf_user_rnd_init_once
>>> ffffffff813fd5b0 t init_once.llvm.17912494158778303782
>>> ffffffff813ffbf0 t init_once
>>> ffffffff813ffc60 t init_once
>>> ffffffff813ffc70 t init_once
>>> ffffffff813ffcd0 t init_once
>>> ffffffff813ffce0 t init_once
>>>
>>> There are two "init_once" with suffix, but there are also ones
>>> without them.
>>
>> This is correct. At LTO mode, when a static function/variable
>> is promoted to the global. The '.llvm.<hash>' is added to the
>> static function/variable name to form a global function name.
>> The '<hash>' here is computed based on IR of the compiled file
>> before LTO. So if one file is not modified, then <hash>
>> won't change after additional code change in other files.
> 
> OK, so the ".llvm.<hash>" suffix is added when a symbol is promoted
> from static to global. Right?

Right at lest for llvm >= 15.
There are an alternative format '.llvm.<file_path>' suffix with
a more involved compilation process.
 
https://github.com/llvm/llvm-project/blob/main/llvm/test/ThinLTO/X86/promote-local-name.ll

But for kernel lto build, yes, only '.llvm.<hash>'.

> 
>>> 2. kpatch-build does "Building original source", "Building patched
>>> source", and then do binary diff of the two. From our experiments,
>>> the suffix doesn't change between the two builds. However, we need
>>> to match the build environment (path of kernel source, etc.) to
>>> make sure suffix from kpatch matches the kernel.
>>
>> The goal here is to generate the same IR (hence <hash>) if
>> file content is not changed. This way, <hash> value will
>> change only for those changed files.
> 
> Hmm, how does kpatch match the fixed functions? They are modified
> so that they should get different IR (hash). Or do I miss
> anything, please?

If the static function are promoted to global function and the file
containing static function changed, then that modified static
function will appear to be a *new* function. Live change should
be able to do it, right?

> 
>>> 3. The goal of this patch is NOT to resolve different suffix by
>>> llvm (.llvm.[0-9]+). Instead, we are trying fix issues like:
>>>
>>> #  grep bpf_verifier_vlog /proc/kallsyms
>>> ffffffff81549f60 t bpf_verifier_vlog
>>> ffffffff8268b430 d bpf_verifier_vlog._entry
>>> ffffffff8282a958 d bpf_verifier_vlog._entry_ptr
>>> ffffffff82e12a1f d bpf_verifier_vlog.__already_done
> 
> And <function>.<symbol> notation seems to be used for static symbols
> defined inside a function.

That is correct.

> 
> I guess that it is used when the symbols stay statics. It would
> probably get additional ".llvm.<hash>" when it got promoted
> from static to global. But this probably never happens.

I have not see a case like this yet.

> 
> Do I get it correctly?

yes, that is correct.

> 
> It means that we have two different types of name changes:
> 
>    1. .llvm.<hash> suffix
> 
>       If we remove this suffix then we will not longer distinguish
>       symbols which stayed static and which were promoted to global
>       ones.
> 
>       The result should be basically the same as without LTO.
>       Some symbols might have duplicated name. But most symbols
>       would have an unique one.
> 
> 
>    2. <function>.<symbol> name
> 
>       In this case, <symbol> is _not_ suffix. It is actually
>       the original symbol name.
> 
>       The extra thing is the <function>. prefix.
> 
>       These static variables seem to have special handling even
>       with gcc without LTO. gcc adds an extra id instead,
>       for example:
> 
> 	$> nm vmlinux | grep " _entry_ptr" | head
> 	ffffffff82a2e800 d _entry_ptr.100135
> 	ffffffff82a2e7f8 d _entry_ptr.100178
> 	ffffffff82a32e70 d _entry_ptr.100798
> 	ffffffff82a1e240 d _entry_ptr.10342
> 	ffffffff82a33930 d _entry_ptr.104764
> 	ffffffff82a339c8 d _entry_ptr.104830
> 	ffffffff82a33928 d _entry_ptr.104871
> 	ffffffff82a33920 d _entry_ptr.104877
> 	ffffffff82a33918 d _entry_ptr.104893
> 	ffffffff82a339c0 d _entry_ptr.104905
> 
> 	$> nm vmlinux | grep panic_console_dropped
> 	ffffffff853618e0 b panic_console_dropped.54158

IIRC, yes, these 'id' might change if source code changed.

> 
> 
> 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?

> 
> 
> 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 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.

> 
> Best Regards,
> Petr