Re: [PATCH] kallsyms, livepatch: Fix livepatch with CONFIG_LTO_CLANG
On Tue, Jul 02, 2024 at 10:56:41PM -0700, Josh Poimboeuf wrote:
> On Mon, Jul 01, 2024 at 03:13:23PM +0200, Petr Mladek wrote:
> > So, you suggest to search the symbols by a hash. Do I get it correctly?
I meant, that in the Rust world the symbols go over the allowed limit,
and so an alternative for them is to just use a hash. What I'm
suggesting is for a new kconfig option where that world is the
new one, so that they have to also do the proper userspace tooling
for it. Without that, I don't see it as properly tested or scalable.
And if we're gonna have that option for Rust for modules, then it begs
the question if this can be used by other users.
> > 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.
>
> Are symbol names really considered user ABI?? That's already broken by
> design. Even without LTO, the toolchain can mangle them for a variety
> of reasons.
>
> If a user space tool doesn't want the suffixes, surely it can figure out
> a way to deal with that on their own?
>
> > 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.
>
> We should just always link with -zunique-symbols so the duplicate
> symbols no longer exist. That would solve a lot of problems.
While it might solve this other issue, it doesn't solve the rust module
long symbol name issue.
Luis
Re: [PATCH] kallsyms, livepatch: Fix livepatch with CONFIG_LTO_CLANG
On Mon, Jul 01, 2024 at 03:13:23PM +0200, Petr Mladek wrote:
> 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.
Are symbol names really considered user ABI?? That's already broken by
design. Even without LTO, the toolchain can mangle them for a variety
of reasons.
If a user space tool doesn't want the suffixes, surely it can figure out
a way to deal with that on their own?
> 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.
We should just always link with -zunique-symbols so the duplicate
symbols no longer exist. That would solve a lot of problems.
--
Josh
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 wrote:
> > > >
> > > > Hi,
> > > >
> > > > On Tue, 4 Jun 2024, Song Liu wrote:
> > > >
> > > > > With CONFIG_LTO_CLANG, the compiler may postfix symbols with
> > > > > .llvm.
> > > > > 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. postfix or kernel functions that
> > > > > use
> > > > > relocation information to symbols with .llvm. 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
> > > > > ---
> > > > > 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.carmin...@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
Re: [PATCH] kallsyms, livepatch: Fix livepatch with CONFIG_LTO_CLANG
Hi Luis, On Fri, Jun 28, 2024 at 10:36 AM 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! > > > > > > 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. > > > > Luis, what is your take on this? > > > > If I am not mistaken, there was a patch set to address this. Luis might > > remember more. > > 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? I'm all for finding generic solutions, but perhaps I've missed the patch set Miroslav mentioned because I'm not quite sure how these problems are related. LTO makes duplicate symbol names globally unique by appending a postfix to them, which complicates looking up symbols by name. Rust, on the other hand, has a problem with CONFIG_MODVERSIONS because the long symbol names it generates cannot fit in the small buffer in struct modversion_info. The only reason we proposed storing a cryptographic hash in modversion_info was to avoid breaking userspace tools that parse this data structure, but AFAIK nobody wants to use hashed symbol names anywhere else. In fact, if there's a better solution for addressing modversion_info limitations, I would be happy not to hash anything. Sami
Re: [PATCH] kallsyms, livepatch: Fix livepatch with CONFIG_LTO_CLANG
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 wrote: > > > > > > Hi, > > > > > > On Tue, 4 Jun 2024, Song Liu wrote: > > > > > > > With CONFIG_LTO_CLANG, the compiler may postfix symbols with > > > > .llvm. > > > > 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. postfix or kernel functions that use > > > > relocation information to symbols with .llvm. 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 > > > > --- > > > > 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. > > Luis, what is your take on this? > > If I am not mistaken, there was a patch set to address this. Luis might > remember more. 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. Thoughts? Luis
Re: [PATCH] kallsyms, livepatch: Fix livepatch with CONFIG_LTO_CLANG
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 wrote: > > > > Hi, > > > > On Tue, 4 Jun 2024, Song Liu wrote: > > > > > With CONFIG_LTO_CLANG, the compiler may postfix symbols with .llvm. > > > 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. postfix or kernel functions that use > > > relocation information to symbols with .llvm. 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 > > > --- > > > 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. Luis, what is your take on this? > > Moreover, isn't there a similar problem for ftrace, kprobes, ebpf,...? > > Yes, there is a similar problem with tracing use cases. But the requirements > 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 symbols > with the same name. Yes. In fact, sympos may be used to solve even this problem. The user would disregard .llvm. suffix and they are suddenly in the same situation which sympos aims to solve. I will not argue with you if say it is cumbersome. > 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. If I am not mistaken, there was a patch set to address this. Luis might remember more. Regards, Miroslav
Re: [PATCH] kallsyms, livepatch: Fix livepatch with CONFIG_LTO_CLANG
Hi Miroslav, Thanks for reviewing the patch! On Fri, Jun 7, 2024 at 6:06 AM Miroslav Benes wrote: > > Hi, > > On Tue, 4 Jun 2024, Song Liu wrote: > > > With CONFIG_LTO_CLANG, the compiler may postfix symbols with .llvm. > > 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. postfix or kernel functions that use > > relocation information to symbols with .llvm. 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 > > --- > > 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 requirements 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 symbols 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
Re: [PATCH] kallsyms, livepatch: Fix livepatch with CONFIG_LTO_CLANG
Hi, On Tue, 4 Jun 2024, Song Liu wrote: > With CONFIG_LTO_CLANG, the compiler may postfix symbols with .llvm. > 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. postfix or kernel functions that use > relocation information to symbols with .llvm. 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 > --- > 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? Moreover, isn't there a similar problem for ftrace, kprobes, ebpf,...? Thank you, Miroslav
[PATCH] kallsyms, livepatch: Fix livepatch with CONFIG_LTO_CLANG
With CONFIG_LTO_CLANG, the compiler may postfix symbols with .llvm.
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. postfix or kernel functions that use
relocation information to symbols with .llvm. 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
---
include/linux/kallsyms.h | 13 +
kernel/kallsyms.c| 21 -
kernel/livepatch/core.c | 32 +++-
3 files changed, 60 insertions(+), 6 deletions(-)
diff --git a/include/linux/kallsyms.h b/include/linux/kallsyms.h
index c3f075e8f60c..d7d07a134ae4 100644
--- a/include/linux/kallsyms.h
+++ b/include/linux/kallsyms.h
@@ -97,6 +97,10 @@ extern int sprint_backtrace_build_id(char *buffer, unsigned
long address);
int lookup_symbol_name(unsigned long addr, char *symname);
+int kallsyms_lookup_symbol_full_name(unsigned long addr, char *symname);
+
+void kallsyms_cleanup_symbol_name(char *s);
+
#else /* !CONFIG_KALLSYMS */
static inline unsigned long kallsyms_lookup_name(const char *name)
@@ -165,6 +169,15 @@ static inline int kallsyms_on_each_match_symbol(int
(*fn)(void *, unsigned long)
{
return -EOPNOTSUPP;
}
+
+static inline int kallsyms_lookup_symbol_full_name(unsigned long addr, char
*symname)
+{
+ return -ERANGE;
+}
+
+static inline void kallsyms_cleanup_symbol_name(char *s)
+{
+}
#endif /*CONFIG_KALLSYMS*/
static inline void print_ip_sym(const char *loglvl, unsigned long ip)
diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
index 22ea19a36e6e..f0d04fa1bbb4 100644
--- a/kernel/kallsyms.c
+++ b/kernel/kallsyms.c
@@ -163,7 +163,7 @@ unsigned long kallsyms_sym_address(int idx)
return kallsyms_relative_base - 1 - kallsyms_offsets[idx];
}
-static void cleanup_symbol_name(char *s)
+void kallsyms_cleanup_symbol_name(char *s)
{
char *res;
@@ -191,7 +191,7 @@ static int compare_symbol_name(const char *name, char
*namebuf)
* To ensure correct bisection in kallsyms_lookup_names(), do
* cleanup_symbol_name(namebuf) before comparing name and namebuf.
*/
- cleanup_symbol_name(namebuf);
+ kallsyms_cleanup_symbol_name(namebuf);
return strcmp(name, namebuf);
}
@@ -426,7 +426,7 @@ static const char *kallsyms_lookup_buildid(unsigned long
addr,
offset, modname, namebuf);
found:
- cleanup_symbol_name(namebuf);
+ kallsyms_cleanup_symbol_name(namebuf);
return ret;
}
@@ -446,7 +446,7 @@ const char *kallsyms_lookup(unsigned long addr,
NULL, namebuf);
}
-int lookup_symbol_name(unsigned long addr, char *symname)
+static int __lookup_symbol_name(unsigned long addr, char *symname, bool
cleanup)
{
int res;
@@ -468,10 +468,21 @@ int lookup_symbol_name(unsigned long addr, char *symname)
return res;
found:
- cleanup_symbol_name(symname);
+ if (cleanup)
+ kallsyms_cleanup_symbol_name(symname);
return 0;
}
+int lookup_symbol_name(unsigned long addr, char *symname)
+{
+ return __lookup_symbol_name(addr, symname, true);
+}
+
+int kallsyms_lookup_symbol_full_name(unsigned long addr, char *symname)
+{
+ return __lookup_symbol_name(addr, symname, false);
+}
+
/* Look up a kernel symbol and return it in a text buffer. */
static int __sprint_symbol(char *buffer, unsigned long address,
int symbol_offset, int add_offset, int add_buildid)
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 52426665eecc..284220e04801 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -128,6 +128,19 @@ struct klp_find_arg {
static int klp_match_callback(void *data, unsigned long addr)
{
struct klp_find_arg *args = data;
+#ifdef CONFIG_LTO_CLANG
+ char full_name[KSYM_NAME_LEN];
+
+ /*
+* With CONFIG_LTO_CLANG, we need to compare the full name of the
+* symbol (with .llvm. postfix).
+*/
+ if (kallsyms_lookup_symbol_full_name(addr, full_name))
+ return 0;
+
+ if (strcmp(args->name, full_name))
+ return 0;
+#endif
args->addr = addr;
args->count++;
@@ -145,10 +158,12 @@ static int klp_match_callback(void *data, unsigned long
addr)
static int klp_find_callback(void *data, const char *name, unsigned long addr)
{
+#ifndef CONFIG_LTO_CLANG
struct klp_find_arg *args = data;
if (strcmp(args->name,
