Date: Mon, 1 Jul 2024 15:13:23 +0200
From: Petr Mladek <pmladek@...e.com>
To: Luis Chamberlain <mcgrof@...nel.org>
Cc: Miroslav Benes <mbenes@...e.cz>,
	Sami Tolvanen <samitolvanen@...gle.com>, Song Liu <song@...nel.org>,
	live-patching@...r.kernel.org, linux-kernel@...r.kernel.org,
	jpoimboe@...nel.org, jikos@...nel.org, joe.lawrence@...hat.com,
	nathan@...nel.org, morbo@...gle.com, justinstitt@...gle.com,
	thunder.leizhen@...wei.com, kees@...nel.org, kernel-team@...a.com
Subject: Re: [PATCH] kallsyms, livepatch: Fix livepatch with CONFIG_LTO_CLANG

On Fri 2024-06-28 10:36:45, Luis Chamberlain wrote:
> On Fri, Jun 28, 2024 at 02:23:49PM +0200, Miroslav Benes wrote:
> > On Fri, 7 Jun 2024, Song Liu wrote:
> > 
> > > Hi Miroslav,
> > > 
> > > Thanks for reviewing the patch!
> > > 
> > > On Fri, Jun 7, 2024 at 6:06 AM Miroslav Benes <mbenes@...e.cz> wrote:
> > > >
> > > > Hi,
> > > >
> > > > On Tue, 4 Jun 2024, Song Liu wrote:
> > > >
> > > > > With CONFIG_LTO_CLANG, the compiler may postfix symbols with .llvm.<hash>
> > > > > to avoid symbol duplication. scripts/kallsyms.c sorted the symbols
> > > > > without these postfixes. The default symbol lookup also removes these
> > > > > postfixes before comparing symbols.
> > > > >
> > > > > On the other hand, livepatch need to look up symbols with the full names.
> > > > > However, calling kallsyms_on_each_match_symbol with full name (with the
> > > > > postfix) cannot find the symbol(s). As a result, we cannot livepatch
> > > > > kernel functions with .llvm.<hash> postfix or kernel functions that use
> > > > > relocation information to symbols with .llvm.<hash> postfixes.
> > > > >
> > > > > Fix this by calling kallsyms_on_each_match_symbol without the postfix;
> > > > > and then match the full name (with postfix) in klp_match_callback.
> > > > >
> > > > > Signed-off-by: Song Liu <song@...nel.org>
> > > > > ---
> > > > >  include/linux/kallsyms.h | 13 +++++++++++++
> > > > >  kernel/kallsyms.c        | 21 ++++++++++++++++-----
> > > > >  kernel/livepatch/core.c  | 32 +++++++++++++++++++++++++++++++-
> > > > >  3 files changed, 60 insertions(+), 6 deletions(-)
> > > >
> > > > I do not like much that something which seems to be kallsyms-internal is
> > > > leaked out. You need to export cleanup_symbol_name() and there is now a
> > > > lot of code outside. I would feel much more comfortable if it is all
> > > > hidden from kallsyms users and kept there. Would it be possible?
> > > 
> > > I think it is possible. Currently, kallsyms_on_each_match_symbol matches
> > > symbols without the postfix. We can add a variation or a parameter, so
> > > that it matches the full name with post fix.
> > 
> > I think it might be better.

Also, I agree that we need another variant of kallsyms_on_each_match_symbol()
which would match the full name.

> > Luis, what is your take on this?

[ Luis probably removed too much context here. IMHO, the following
  sentence was talking about another problem in the original mail..

> > If I am not mistaken, there was a patch set to address this. Luis might 
> > remember more.

I believe that Miroslav was talking about
https://lore.kernel.org/all/20231204214635.2916691-1-alessandro.carminati@gmail.com/

It is a patch solving the opposite problem. It allows to match
an exact symbol even when there were more symbols of the same name.
It would allow to get rid of the sympos.


> Yeah this is a real issue outside of CONFIG_LTO_CLANG, Rust modules is
> another example where instead of symbol names they want to use full
> hashes. So, as I hinted to you Sami, can we knock two birds with one stone
> here and move CONFIG_LTO_CLANG to use the same strategy as Rust so we
> have two users instead of just one? Then we resolve this. In fact
> what I suggested was even to allow even non-Rust, and in this case
> even with gcc to enable this world. This gives much more wider scope
> of testing / review / impact of these sorts of changes and world view
> and it would resolve the Rust case, the live patch CONFIG_LTO_CLANG
> world too.

So, you suggest to search the symbols by a hash. Do I get it correctly?

Well, it might bring back the original problem. I mean
the commit 8b8e6b5d3b013b0 ("kallsyms: strip ThinLTO hashes from
static functions") added cleanup_symbol_name() so that user-space
tool do not need to take care of the "unstable" suffix.

So, it seems that we have two use cases:

   1. Some user-space tools want to ignore the extra suffix. I guess
      that it is in the case when the suffix is added only because
      the function was optimized.

      It can't work if there are two different functions of the same
      name. Otherwise, the user-space tool would not know which one
      they are tracing.


   2. There are other use-cases, including livepatching, where we
      want to be 100% sure that we match the right symbol.

      They want to match the full names. They even need to distinguish
      symbols with the same name.


IMHO, we need a separate API for each use-case.

Best Regards,
Petr

Powered by blists - more mailing lists