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

[Petr Mladek <pmladek@suse.com>]

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:
> 
> [...]
> 
> >>>>>> 
> >>>>>> @@ -195,7 +195,7 @@ static int compare_symbol_name(const char *name, char *namebuf)
> >>>>>> if (!ret)
> >>>>>> return ret;
> >>>>>> 
> >>>>>> - if (cleanup_symbol_name(namebuf) && !strcmp(name, namebuf))
> >>>>>> + if (!match_exactly && cleanup_symbol_name(namebuf) && !strcmp(name, namebuf))
> >>>>> 
> >>>>> This may affect the lookup of static functions.
> >>>> 
> >>>> I am not following why would this be a problem. Could you give an
> >>>> example of it?
> >>> 
> >>> Here are the comments in cleanup_symbol_name(). If the compiler adds a suffix to the
> >>> static function, but we do not remove the suffix, will the symbol match fail?
> >>> 
> >>>        /*
> >>>         * LLVM appends various suffixes for local functions and variables that
> >>>         * must be promoted to global scope as part of LTO.  This can break
> >>>         * hooking of static functions with kprobes. '.' is not a valid
> >>>         * character in an identifier in C. Suffixes observed:
> >>>         * - foo.llvm.[0-9a-f]+
> >>>         * - foo.[0-9a-f]+
> >>>         */
> >> 
> >> I think livepatch will not fail, as the tool chain should already match the
> >> suffix for the function being patched. If the tool chain failed to do so,
> >> livepatch can fail for other reasons (missing symbols, etc.)
> > 
> > cleanup_symbol_name() has been added by the commit 8b8e6b5d3b013b0bd8
> > ("kallsyms: strip ThinLTO hashes from static functions"). The
> > motivation is that user space tools pass the symbol names found
> > in sources. They do not know about the "random" suffix added
> > by the "random" compiler.
> 
> I am not quite sure how tracing tools would work without knowing about
> what the compiler did to the code. But I guess we are not addressing
> that part here.

I expect that the tracers access only symbols that are unique even
after removing the extra suffix.

Otherwise they would have the same problem even without suffix.
Each symbol create its own entry in kallsyms. There might be
more static symbols of the same name. This is why there is
"old_sympos" in struct klp_func. See also klp-convert,
https://lore.kernel.org/r/20230306140824.3858543-1-joe.lawrence@redhat.com

Fortunately, the same names are rare and "old_sympos" is used
only rarely. This is why it probably works for the tracers as well.


> > While livepatching might want to work with the full symbol names.
> > It helps to locate avoid duplication and find the right symbol.
> > 
> > At least, this should be beneficial for kpatch tool which works directly
> > with the generated symbols.
> > 
> > Well, in theory, the cleaned symbol names might be useful for
> > source-based livepatches. But there might be problem to
> > distinguish different symbols with the same name and symbols
> > duplicated because of inlining. Well, we tend to livepatch
> > the caller anyway.
> 
> 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. Let me think loudly.

kpatch produce livepatches by comparing binaries of the original
and fixed kernel. It adds a symbol into the livepatch when
the same symbol has different code in the two binaries.

So one important question is how clang generates the suffix.
Is the suffix the same in every build? Is it the same even
after the function gets modified by a fix?

If the suffix is always the same then then the full symbol name
would be better for kpatch. kpatch would get it for free.
And kpatch would not longer need to use "old_sympos".

But if the suffix is different then kpatch has a problem.
kpatch would need to match symbols with different suffixes.
It would be easy for symbols which are unique after removing
the suffix. But it would be tricky for comparing symbols
which do not have an unique name. kpatch would need to find
which suffix in the original binary matches an other suffix
in the fixed binary. In this case, it might be easier
to use the stripped symbol names.

And the suffix might be problematic also for source based
livepatches. They define struct klp_func in sources so
they would need to hardcode the suffix there. It might
be easy to keep using the stripped name and "old_sympos".

I expect that this patch actually breaks the livepatch
selftests when the kernel is compiled with clang LTO.

Best Regards,
Petr