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@xxxxxxxx> wrote:
> > > On Tue 2023-06-20 22:36:14, Song Liu wrote:
> > > > > On Jun 19, 2023, at 4:32 AM, Petr Mladek <pmladek@xxxxxxxx> 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@xxxxxxxxxx> 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?
> > 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?
> > 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.
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.
Do I get it correctly?
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
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.
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?
Best Regards,
Petr
