Received-SPF: pass (google.com: domain of linux-kernel+bounces-206514-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240605032120.3179157-1-song@kernel.org> <alpine.LSU.2.21.2406071458531.29080@pobox.suse.cz>
In-Reply-To: <alpine.LSU.2.21.2406071458531.29080@pobox.suse.cz>
From: Song Liu <song@kernel.org>
Date: Fri, 7 Jun 2024 09:53:28 -0700
Message-ID: <CAPhsuW5th55V3PfskJvpG=4bwacKP8c8DpVYUyVUzt70KC7=gw@mail.gmail.com>
Subject: Re: [PATCH] kallsyms, livepatch: Fix livepatch with CONFIG_LTO_CLANG
To: Miroslav Benes <mbenes@suse.cz>
Cc: live-patching@vger.kernel.org, linux-kernel@vger.kernel.org, 
	jpoimboe@kernel.org, jikos@kernel.org, pmladek@suse.com, 
	joe.lawrence@redhat.com, nathan@kernel.org, morbo@google.com, 
	justinstitt@google.com, mcgrof@kernel.org, thunder.leizhen@huawei.com, 
	kees@kernel.org, kernel-team@meta.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi Miroslav,

Thanks for reviewing the patch!

On Fri, Jun 7, 2024 at 6:06=E2=80=AFAM Miroslav Benes <mbenes@suse.cz> wrot=
e:
>
> Hi,
>
> On Tue, 4 Jun 2024, Song Liu wrote:
>
> > With CONFIG_LTO_CLANG, the compiler may postfix symbols with .llvm.<has=
h>
> > 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 name=
s.
> > 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@kernel.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.

> Moreover, isn't there a similar problem for ftrace, kprobes, ebpf,...?

Yes, there is a similar problem with tracing use cases. But the requirement=
s
are not the same:

For livepatch, we have to point to the exact symbol we want to patch or
relocation to. We have sympos API defined to differentiate different symbol=
s
with the same name.

For tracing, some discrepancy is acceptable. AFAICT, there isn't an API
similar to sympos yet. Also, we can play some tricks with tracing. For
example, we can use "uniq symbol + offset" to point a kprobe to one of
the duplicated symbols.

Given livepatch has a well defined API, while the APIs at tracing side
may still change, we can change kallsyms to make sure livepatch side
works. Work on the tracing side can wait.

Thanks,
Song