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

[Petr Mladek <pmladek@suse.com>]

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:
> >
> >
> >
> > On 2023/6/17 1:37, Song Liu wrote:
> > >
> > >
> > >> On Jun 16, 2023, at 2:31 AM, Leizhen (ThunderTown) <thunder.leizhen@huawei.com> wrote:
> > >>
> > >>
> > >>
> > >> On 2023/6/16 1:00, Song Liu wrote:
> > >>> With CONFIG_LTO_CLANG, kallsyms.c:cleanup_symbol_name() removes symbols
> > >>> suffixes during comparison. This is problematic for livepatch, as
> > >>> kallsyms_on_each_match_symbol may find multiple matches for the same
> > >>> symbol, and fail with:
> > >>>
> > >>>  livepatch: unresolvable ambiguity for symbol 'xxx' in object 'yyy'
> > >>>
> > >>> Make kallsyms_on_each_match_symbol() to match symbols exactly. Since
> > >>> livepatch is the only user of kallsyms_on_each_match_symbol(), this
> > >>> change is safe.
> > >>>
> > >>> Signed-off-by: Song Liu <song@kernel.org>
> > >>> ---
> > >>> kernel/kallsyms.c | 17 +++++++++--------
> > >>> 1 file changed, 9 insertions(+), 8 deletions(-)
> > >>>
> > >>> diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
> > >>> index 77747391f49b..2ab459b43084 100644
> > >>> --- a/kernel/kallsyms.c
> > >>> +++ b/kernel/kallsyms.c
> > >>> @@ -187,7 +187,7 @@ static bool cleanup_symbol_name(char *s)
> > >>> return false;
> > >>> }
> > >>>
> > >>> -static int compare_symbol_name(const char *name, char *namebuf)
> > >>> +static int compare_symbol_name(const char *name, char *namebuf, bool match_exactly)
> > >>> {
> > >>> int ret;
> > >>>
> > >>> @@ -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.

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.

Best Regards,
Petr