Re: [PATCH] kallsyms, livepatch: Fix livepatch with CONFIG_LTO_CLANG
On Mon, Jul 8, 2024 at 5:07 PM Sami Tolvanen wrote: > > On Mon, Jul 8, 2024 at 2:33 PM Luis Chamberlain wrote: > > > > Looking at this again its not to me why Masahiro Yamada's suggestion on > > that old patch series to just increase the length and put long symbols > > names into its own section [0] could not be embraced with a new kconfig > > option, so new kernels and new userspace could support it: > > > > https://lore.kernel.org/lkml/CAK7LNATsuszFR7JB5ZkqVS1W=hWr9=e7btf+mvgj+nxt3az...@mail.gmail.com/ > > Matt, was there a reason we didn't move forward with Masahiro's > proposal? It sounds reasonable to me, but I might be missing some > background here. I liked Masahiro's proposal, and implemented it [1], but in the first version of the patch, folks complained [2] that I was touching code that was hard to reason about, and that I should clean it up. In the second version of the patch, I cleaned it up, and was told the cleanup patch was too big and needed to be broken down [3]. If we want to revive that, it appears there's one or two PPC userspace tool issues I need to address first, but I could do so, and attempt to break the cleanup patch into smaller pieces to be more reviewable. I had abandoned it because even if completed, without the new work Sami did on extracting a hash from debug symbols, it would not have been accepted as a way to enable MODVERSIONS. When we generated this new approach, I suggested we do hashes to avoid modifying the module loading code given the previous reaction. [1]: https://lore.kernel.org/all/20231118025748.2778044-1-mmau...@google.com/ [2]: https://lore.kernel.org/all/zvznh%2fpa5hivr...@bombadil.infradead.org/ [3]: https://lore.kernel.org/all/2023111818-agent-verdict-99a5@gregkh/
Re: [PATCH] kallsyms, livepatch: Fix livepatch with CONFIG_LTO_CLANG
> On Jul 9, 2024, at 8:07 AM, Sami Tolvanen wrote:
[...]
>
>>> I am a bit scared because using hashed symbol names in backtraces, gdb,
>>> ... would be a nightmare. Hashes are not human readable and
>>> they would complicate the life a lot. And using different names
>>> in different interfaces would complicate the life either.
>>
>> All great points.
>>
>> The scope of the Rust issue is self contained to modversion_info,
>> whereas for CONFIG_LTO_CLANG issue commit 8b8e6b5d3b013b0
>> ("kallsyms: strip ThinLTO hashes from static functions") describes
>> the issue with userspace tools (it doesn't explain which ones)
>> which don't expect the function name to change. This seems to happen
>> to static routines so I can only suspect this isn't an issue with
>> modversioning as the only symbols that would be used there wouldn't be
>> static.
>>
>> Sami, what was the exact userspace issue with CONFIG_LTO_CLANG and these
>> long symbols?
>
> The issue with LTO wasn't symbol length. IIRC the compiler renaming
> symbols with ThinLTO caused issues for folks using dynamic kprobes,
> and I seem to recall it also breaking systrace in Android, at which
> point we decided to strip the postfix in kallsyms to avoid breaking
> anything else.
Trying to understand all the requirements and constraints. IIUC, we
can mostly agree:
(1) A way to match a symbol exactly is crucial for users like live
patching.
(2) Original symbol name is useful for backtrace, etc. (IOW hash
alone is not enough)
With these two requirements/constraints, we need
original symbol name + something
for duplicate symbols. "Something" here could be a path name
(xxx_driver_xxx_yyy_c), or a hash, or sympos.
At the moment, (1) is not met with CONFIG_LTO_CLANG. The original
patch tries to fix this, but the solution seems not optimal. I will
send another version that allows kallsyms match exactly or without
suffix.
This work shouldn't cause any problem for Rust, as Rust also need
original symbol name + "something". If we finally decide "something"
should be some format of hash, we can change all users (live patch,
etc.) to use hash, which might be better than sympos. Note: I am
not trying to say "something" should be hash.
OTOH, there is also an open question: Shall we allow tracing with
only original symbol name (without specifying _something_). I think
this a separate question and we don't have to answer it here.
Does this make sense?
Thanks,
Song
Re: [PATCH] kallsyms, livepatch: Fix livepatch with CONFIG_LTO_CLANG
On Mon, Jul 8, 2024 at 2:33 PM Luis Chamberlain wrote:
>
> Looking at this again its not to me why Masahiro Yamada's suggestion on
> that old patch series to just increase the length and put long symbols
> names into its own section [0] could not be embraced with a new kconfig
> option, so new kernels and new userspace could support it:
>
> https://lore.kernel.org/lkml/CAK7LNATsuszFR7JB5ZkqVS1W=hWr9=e7btf+mvgj+nxt3az...@mail.gmail.com/
Matt, was there a reason we didn't move forward with Masahiro's
proposal? It sounds reasonable to me, but I might be missing some
background here.
> > I am a bit scared because using hashed symbol names in backtraces, gdb,
> > ... would be a nightmare. Hashes are not human readable and
> > they would complicate the life a lot. And using different names
> > in different interfaces would complicate the life either.
>
> All great points.
>
> The scope of the Rust issue is self contained to modversion_info,
> whereas for CONFIG_LTO_CLANG issue commit 8b8e6b5d3b013b0
> ("kallsyms: strip ThinLTO hashes from static functions") describes
> the issue with userspace tools (it doesn't explain which ones)
> which don't expect the function name to change. This seems to happen
> to static routines so I can only suspect this isn't an issue with
> modversioning as the only symbols that would be used there wouldn't be
> static.
>
> Sami, what was the exact userspace issue with CONFIG_LTO_CLANG and these
> long symbols?
The issue with LTO wasn't symbol length. IIRC the compiler renaming
symbols with ThinLTO caused issues for folks using dynamic kprobes,
and I seem to recall it also breaking systrace in Android, at which
point we decided to strip the postfix in kallsyms to avoid breaking
anything else.
Sami
Re: [PATCH] kallsyms, livepatch: Fix livepatch with CONFIG_LTO_CLANG
On Thu, Jul 04, 2024 at 11:02:18AM +0200, Petr Mladek wrote:
> On Wed 2024-07-03 08:30:33, Luis Chamberlain wrote:
> > 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.
>
> I am still not sure at which level the symbol names would get hashed ;-)
Let's clarify that. The Rust world mangles symbols to be very long. In
order to allow external modules built to be usable in other systems we support
modversioning. To help with this each module has an array of crcs of each
symbol it depends on. This is added to the module.mod.c so for my built
xfs.mod.c I have for example:
static const struct modversion_info versions[]
__used __section("__versions") = {
{ 0x5b8239ca, "__x86_return_thunk" },
{ 0x93a6e0b2, "io_schedule" },
{ 0x6383b27c, "__x86_indirect_thunk_rdx" },
{ 0xc5b6f236, "queue_work_on" },
{ 0xd966a5ea, "list_lru_walk_one" },
...
{ 0xe3565824, "vfs_readlink" },
{ 0xaed0dc33, "bdev_freeze" },
{ 0x7ce18c9f, "from_kqid" },
{ 0xd36dc10c, "get_random_u32" },
{ 0xba4edeaf, "__percpu_down_read" },
{ 0xb9f07b78, "file_modified" },
{ 0x48755bd1, "module_layout"
},
};
If a module depends on a symbol from a Rust module or Rust built-in
code, it cannot fit becuase we currenlty define:
struct modversion_info {
unsigned long crc;
char name[MODULE_NAME_LEN];
};
Although perhaps this started off as thing for module versions only,
clearly we're using it for all symbols a module uses so MODULE_NAME_LEN
really was in the end silly and does not suffice today.
A version of the effort from Rust folks last year tried to modify the
length but its claimed that the blocker for that was that userspace
would need to change, so their new attempt tried to work around that
using hashes...
Looking at this again its not to me why Masahiro Yamada's suggestion on
that old patch series to just increase the length and put long symbols
names into its own section [0] could not be embraced with a new kconfig
option, so new kernels and new userspace could support it:
https://lore.kernel.org/lkml/CAK7LNATsuszFR7JB5ZkqVS1W=hWr9=e7btf+mvgj+nxt3az...@mail.gmail.com/
> The symbols names are used in many situations, e.g. backtraces,
> crashdump, objdump, nm, gdb, tracing, livepatching, kprobes, ...
>
> Would kallsyms provide some translation table between the usual
> "long" symbol name and a hash?
>
> Would it allows to search the symbols both ways?
Let's review the scope of both issues first, certainly Rust's issue is
limitted in scope, but since a new userspace change might be required
its worth reviewing if it may help the CONFIG_LTO_CLANG case as well.
> I am a bit scared because using hashed symbol names in backtraces, gdb,
> ... would be a nightmare. Hashes are not human readable and
> they would complicate the life a lot. And using different names
> in different interfaces would complicate the life either.
All great points.
The scope of the Rust issue is self contained to modversion_info,
whereas for CONFIG_LTO_CLANG issue commit 8b8e6b5d3b013b0
("kallsyms: strip ThinLTO hashes from static functions") describes
the issue with userspace tools (it doesn't explain which ones)
which don't expect the function name to change. This seems to happen
to static routines so I can only suspect this isn't an issue with
modversioning as the only symbols that would be used there wouldn't be
static.
Sami, what was the exact userspace issue with CONFIG_LTO_CLANG and these
long symbols?
Luis
Re: [PATCH] kallsyms, livepatch: Fix livepatch with CONFIG_LTO_CLANG
On Wed 2024-07-03 08:30:33, Luis Chamberlain wrote: > 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. I am still not sure at which level the symbol names would get hashed ;-) The symbols names are used in many situations, e.g. backtraces, crashdump, objdump, nm, gdb, tracing, livepatching, kprobes, ... Would kallsyms provide some translation table between the usual "long" symbol name and a hash? Would it allows to search the symbols both ways? I am a bit scared because using hashed symbol names in backtraces, gdb, ... would be a nightmare. Hashes are not human readable and they would complicate the life a lot. And using different names in different interfaces would complicate the life either. Best Regards, Petr
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
