| Message ID | 20240802210836.2210140-4-song@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fix kallsyms with CONFIG_LTO_CLANG | expand |
On Fri, 2 Aug 2024 14:08:35 -0700 Song Liu <song@kernel.org> wrote: > Use the new kallsyms APIs that matches symbols name with .XXX > suffix. This allows userspace tools to get kprobes on the expected > function name, while the actual symbol has a .llvm.<hash> suffix. > > This only effects kernel compile with CONFIG_LTO_CLANG. > > Signed-off-by: Song Liu <song@kernel.org> > --- > kernel/kprobes.c | 6 +++++- > kernel/trace/trace_kprobe.c | 11 ++++++++++- > 2 files changed, 15 insertions(+), 2 deletions(-) > > diff --git a/kernel/kprobes.c b/kernel/kprobes.c > index e85de37d9e1e..99102283b076 100644 > --- a/kernel/kprobes.c > +++ b/kernel/kprobes.c > @@ -70,7 +70,11 @@ static DEFINE_PER_CPU(struct kprobe *, kprobe_instance); > kprobe_opcode_t * __weak kprobe_lookup_name(const char *name, > unsigned int __unused) > { > - return ((kprobe_opcode_t *)(kallsyms_lookup_name(name))); > + unsigned long addr = kallsyms_lookup_name(name); > + > + if (IS_ENABLED(CONFIG_LTO_CLANG) && !addr) > + addr = kallsyms_lookup_name_without_suffix(name); > + return ((kprobe_opcode_t *)(addr)); > } > > /* > diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c > index 61a6da808203..d2ad0c561c83 100644 > --- a/kernel/trace/trace_kprobe.c > +++ b/kernel/trace/trace_kprobe.c > @@ -203,6 +203,10 @@ unsigned long trace_kprobe_address(struct trace_kprobe *tk) > if (tk->symbol) { > addr = (unsigned long) > kallsyms_lookup_name(trace_kprobe_symbol(tk)); > + > + if (IS_ENABLED(CONFIG_LTO_CLANG) && !addr) > + addr = kallsyms_lookup_name_without_suffix(trace_kprobe_symbol(tk)); > + So you do the lookup twice if this is enabled? Why not just use "kallsyms_lookup_name_without_suffix()" the entire time, and it should work just the same as "kallsyms_lookup_name()" if it's not needed? > if (addr) > addr += tk->rp.kp.offset; > } else { > @@ -766,8 +770,13 @@ static unsigned int number_of_same_symbols(const char *mod, const char *func_nam > { > struct sym_count_ctx ctx = { .count = 0, .name = func_name }; > > - if (!mod) > + if (!mod) { > kallsyms_on_each_match_symbol(count_symbols, func_name, &ctx.count); > + if (IS_ENABLED(CONFIG_LTO_CLANG) && !ctx.count) { > + kallsyms_on_each_match_symbol_without_suffix( > + count_symbols, func_name, &ctx.count); Same here. -- Steve > + } > + } > > module_kallsyms_on_each_symbol(mod, count_mod_symbols, &ctx); >
Hi Steven, > On Aug 6, 2024, at 11:44 AM, Steven Rostedt <rostedt@goodmis.org> wrote: > > On Fri, 2 Aug 2024 14:08:35 -0700 > Song Liu <song@kernel.org> wrote: > >> Use the new kallsyms APIs that matches symbols name with .XXX >> suffix. This allows userspace tools to get kprobes on the expected >> function name, while the actual symbol has a .llvm.<hash> suffix. >> >> This only effects kernel compile with CONFIG_LTO_CLANG. >> >> Signed-off-by: Song Liu <song@kernel.org> >> --- >> kernel/kprobes.c | 6 +++++- >> kernel/trace/trace_kprobe.c | 11 ++++++++++- >> 2 files changed, 15 insertions(+), 2 deletions(-) >> >> diff --git a/kernel/kprobes.c b/kernel/kprobes.c >> index e85de37d9e1e..99102283b076 100644 >> --- a/kernel/kprobes.c >> +++ b/kernel/kprobes.c >> @@ -70,7 +70,11 @@ static DEFINE_PER_CPU(struct kprobe *, kprobe_instance); >> kprobe_opcode_t * __weak kprobe_lookup_name(const char *name, >> unsigned int __unused) >> { >> - return ((kprobe_opcode_t *)(kallsyms_lookup_name(name))); >> + unsigned long addr = kallsyms_lookup_name(name); >> + >> + if (IS_ENABLED(CONFIG_LTO_CLANG) && !addr) >> + addr = kallsyms_lookup_name_without_suffix(name); >> + return ((kprobe_opcode_t *)(addr)); >> } >> >> /* >> diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c >> index 61a6da808203..d2ad0c561c83 100644 >> --- a/kernel/trace/trace_kprobe.c >> +++ b/kernel/trace/trace_kprobe.c >> @@ -203,6 +203,10 @@ unsigned long trace_kprobe_address(struct trace_kprobe *tk) >> if (tk->symbol) { >> addr = (unsigned long) >> kallsyms_lookup_name(trace_kprobe_symbol(tk)); >> + >> + if (IS_ENABLED(CONFIG_LTO_CLANG) && !addr) >> + addr = kallsyms_lookup_name_without_suffix(trace_kprobe_symbol(tk)); >> + > > So you do the lookup twice if this is enabled? > > Why not just use "kallsyms_lookup_name_without_suffix()" the entire time, > and it should work just the same as "kallsyms_lookup_name()" if it's not > needed? We still want to give priority to full match. For example, we have: [root@~]# grep c_next /proc/kallsyms ffffffff81419dc0 t c_next.llvm.7567888411731313343 ffffffff81680600 t c_next ffffffff81854380 t c_next.llvm.14337844803752139461 If the goal is to explicitly trace c_next.llvm.7567888411731313343, the user can provide the full name. If we always match _without_suffix, all of the 3 will match to the first one. Does this make sense? Thanks, Song
On Tue, 6 Aug 2024 19:35:07 +0000 Song Liu <songliubraving@meta.com> wrote: > >> + if (IS_ENABLED(CONFIG_LTO_CLANG) && !addr) > >> + addr = kallsyms_lookup_name_without_suffix(trace_kprobe_symbol(tk)); > >> + > > > > So you do the lookup twice if this is enabled? > > > > Why not just use "kallsyms_lookup_name_without_suffix()" the entire time, > > and it should work just the same as "kallsyms_lookup_name()" if it's not > > needed? > > We still want to give priority to full match. For example, we have: > > [root@~]# grep c_next /proc/kallsyms > ffffffff81419dc0 t c_next.llvm.7567888411731313343 > ffffffff81680600 t c_next > ffffffff81854380 t c_next.llvm.14337844803752139461 > > If the goal is to explicitly trace c_next.llvm.7567888411731313343, the > user can provide the full name. If we always match _without_suffix, all > of the 3 will match to the first one. > > Does this make sense? Yes. Sorry, I missed the "&& !addr)" after the "IS_ENABLED()", which looked like you did the command twice. I blame jetlag ;-) -- Steve
On Tue, 6 Aug 2024 16:00:49 -0400 Steven Rostedt <rostedt@goodmis.org> wrote: > > >> + if (IS_ENABLED(CONFIG_LTO_CLANG) && !addr) > > >> + addr = kallsyms_lookup_name_without_suffix(trace_kprobe_symbol(tk)); > > >> + > > > > > > So you do the lookup twice if this is enabled? > > > > > > Why not just use "kallsyms_lookup_name_without_suffix()" the entire time, > > > and it should work just the same as "kallsyms_lookup_name()" if it's not > > > needed? > > > > We still want to give priority to full match. For example, we have: > > > > [root@~]# grep c_next /proc/kallsyms > > ffffffff81419dc0 t c_next.llvm.7567888411731313343 > > ffffffff81680600 t c_next > > ffffffff81854380 t c_next.llvm.14337844803752139461 > > > > If the goal is to explicitly trace c_next.llvm.7567888411731313343, the > > user can provide the full name. If we always match _without_suffix, all > > of the 3 will match to the first one. > > > > Does this make sense? > > Yes. Sorry, I missed the "&& !addr)" after the "IS_ENABLED()", which looked > like you did the command twice. But that said, does this only have to be for llvm? Or should we do this for even gcc? As I believe gcc can give strange symbols too. -- Steve
> On Aug 6, 2024, at 1:01 PM, Steven Rostedt <rostedt@goodmis.org> wrote: > > On Tue, 6 Aug 2024 16:00:49 -0400 > Steven Rostedt <rostedt@goodmis.org> wrote: > >>>>> + if (IS_ENABLED(CONFIG_LTO_CLANG) && !addr) >>>>> + addr = kallsyms_lookup_name_without_suffix(trace_kprobe_symbol(tk)); >>>>> + >>>> >>>> So you do the lookup twice if this is enabled? >>>> >>>> Why not just use "kallsyms_lookup_name_without_suffix()" the entire time, >>>> and it should work just the same as "kallsyms_lookup_name()" if it's not >>>> needed? >>> >>> We still want to give priority to full match. For example, we have: >>> >>> [root@~]# grep c_next /proc/kallsyms >>> ffffffff81419dc0 t c_next.llvm.7567888411731313343 >>> ffffffff81680600 t c_next >>> ffffffff81854380 t c_next.llvm.14337844803752139461 >>> >>> If the goal is to explicitly trace c_next.llvm.7567888411731313343, the >>> user can provide the full name. If we always match _without_suffix, all >>> of the 3 will match to the first one. >>> >>> Does this make sense? >> >> Yes. Sorry, I missed the "&& !addr)" after the "IS_ENABLED()", which looked >> like you did the command twice. > > But that said, does this only have to be for llvm? Or should we do this for > even gcc? As I believe gcc can give strange symbols too. I think most of the issue comes with LTO, as LTO promotes local static functions to global functions. IIUC, we don't have GCC built, LTO enabled kernel yet. In my GCC built, we have suffixes like ".constprop.0", ".part.0", ".isra.0", and ".isra.0.cold". We didn't do anything about these before this set. So I think we are OK not handling them now. We sure can enable it for GCC built kernel in the future. Thanks, Song
On Tue, 6 Aug 2024 20:12:55 +0000 Song Liu <songliubraving@meta.com> wrote: > > > > On Aug 6, 2024, at 1:01 PM, Steven Rostedt <rostedt@goodmis.org> wrote: > > > > On Tue, 6 Aug 2024 16:00:49 -0400 > > Steven Rostedt <rostedt@goodmis.org> wrote: > > > >>>>> + if (IS_ENABLED(CONFIG_LTO_CLANG) && !addr) > >>>>> + addr = kallsyms_lookup_name_without_suffix(trace_kprobe_symbol(tk)); > >>>>> + > >>>> > >>>> So you do the lookup twice if this is enabled? > >>>> > >>>> Why not just use "kallsyms_lookup_name_without_suffix()" the entire time, > >>>> and it should work just the same as "kallsyms_lookup_name()" if it's not > >>>> needed? > >>> > >>> We still want to give priority to full match. For example, we have: > >>> > >>> [root@~]# grep c_next /proc/kallsyms > >>> ffffffff81419dc0 t c_next.llvm.7567888411731313343 > >>> ffffffff81680600 t c_next > >>> ffffffff81854380 t c_next.llvm.14337844803752139461 > >>> > >>> If the goal is to explicitly trace c_next.llvm.7567888411731313343, the > >>> user can provide the full name. If we always match _without_suffix, all > >>> of the 3 will match to the first one. > >>> > >>> Does this make sense? > >> > >> Yes. Sorry, I missed the "&& !addr)" after the "IS_ENABLED()", which looked > >> like you did the command twice. > > > > But that said, does this only have to be for llvm? Or should we do this for > > even gcc? As I believe gcc can give strange symbols too. > > I think most of the issue comes with LTO, as LTO promotes local static > functions to global functions. IIUC, we don't have GCC built, LTO enabled > kernel yet. > > In my GCC built, we have suffixes like ".constprop.0", ".part.0", ".isra.0", > and ".isra.0.cold". We didn't do anything about these before this set. So I > think we are OK not handling them now. We sure can enable it for GCC built > kernel in the future. Hmm, I think it should be handled as it is. This means it should do as livepatch does. Since I expected user will check kallsyms if gets error, we should keep this as it is. (if a symbol has suffix, it should accept symbol with suffix, or user will get confused because they can not find which symbol is kprobed.) Sorry about the conclusion (so I NAK this), but this is a good discussion. Thanks, > > Thanks, > Song > > >
> On Aug 6, 2024, at 5:01 PM, Masami Hiramatsu <mhiramat@kernel.org> wrote: > > On Tue, 6 Aug 2024 20:12:55 +0000 > Song Liu <songliubraving@meta.com> wrote: > >> >> >>> On Aug 6, 2024, at 1:01 PM, Steven Rostedt <rostedt@goodmis.org> wrote: >>> >>> On Tue, 6 Aug 2024 16:00:49 -0400 >>> Steven Rostedt <rostedt@goodmis.org> wrote: >>> >>>>>>> + if (IS_ENABLED(CONFIG_LTO_CLANG) && !addr) >>>>>>> + addr = kallsyms_lookup_name_without_suffix(trace_kprobe_symbol(tk)); >>>>>>> + >>>>>> >>>>>> So you do the lookup twice if this is enabled? >>>>>> >>>>>> Why not just use "kallsyms_lookup_name_without_suffix()" the entire time, >>>>>> and it should work just the same as "kallsyms_lookup_name()" if it's not >>>>>> needed? >>>>> >>>>> We still want to give priority to full match. For example, we have: >>>>> >>>>> [root@~]# grep c_next /proc/kallsyms >>>>> ffffffff81419dc0 t c_next.llvm.7567888411731313343 >>>>> ffffffff81680600 t c_next >>>>> ffffffff81854380 t c_next.llvm.14337844803752139461 >>>>> >>>>> If the goal is to explicitly trace c_next.llvm.7567888411731313343, the >>>>> user can provide the full name. If we always match _without_suffix, all >>>>> of the 3 will match to the first one. >>>>> >>>>> Does this make sense? >>>> >>>> Yes. Sorry, I missed the "&& !addr)" after the "IS_ENABLED()", which looked >>>> like you did the command twice. >>> >>> But that said, does this only have to be for llvm? Or should we do this for >>> even gcc? As I believe gcc can give strange symbols too. >> >> I think most of the issue comes with LTO, as LTO promotes local static >> functions to global functions. IIUC, we don't have GCC built, LTO enabled >> kernel yet. >> >> In my GCC built, we have suffixes like ".constprop.0", ".part.0", ".isra.0", >> and ".isra.0.cold". We didn't do anything about these before this set. So I >> think we are OK not handling them now. We sure can enable it for GCC built >> kernel in the future. > > Hmm, I think it should be handled as it is. This means it should do as > livepatch does. Since I expected user will check kallsyms if gets error, > we should keep this as it is. (if a symbol has suffix, it should accept > symbol with suffix, or user will get confused because they can not find > which symbol is kprobed.) > > Sorry about the conclusion (so I NAK this), but this is a good discussion. Do you mean we do not want patch 3/3, but would like to keep 1/3 and part of 2/3 (remove the _without_suffix APIs)? If this is the case, we are undoing the change by Sami in [1], and thus may break some tracing tools. Sami, could you please share your thoughts on this? If this works, I will send next version with 1/3 and part of 2/3. Thanks, Song [1] https://lore.kernel.org/all/20210408182843.1754385-8-samitolvanen@google.com/
On Wed, 7 Aug 2024 00:19:20 +0000 Song Liu <songliubraving@meta.com> wrote: > > > > On Aug 6, 2024, at 5:01 PM, Masami Hiramatsu <mhiramat@kernel.org> wrote: > > > > On Tue, 6 Aug 2024 20:12:55 +0000 > > Song Liu <songliubraving@meta.com> wrote: > > > >> > >> > >>> On Aug 6, 2024, at 1:01 PM, Steven Rostedt <rostedt@goodmis.org> wrote: > >>> > >>> On Tue, 6 Aug 2024 16:00:49 -0400 > >>> Steven Rostedt <rostedt@goodmis.org> wrote: > >>> > >>>>>>> + if (IS_ENABLED(CONFIG_LTO_CLANG) && !addr) > >>>>>>> + addr = kallsyms_lookup_name_without_suffix(trace_kprobe_symbol(tk)); > >>>>>>> + > >>>>>> > >>>>>> So you do the lookup twice if this is enabled? > >>>>>> > >>>>>> Why not just use "kallsyms_lookup_name_without_suffix()" the entire time, > >>>>>> and it should work just the same as "kallsyms_lookup_name()" if it's not > >>>>>> needed? > >>>>> > >>>>> We still want to give priority to full match. For example, we have: > >>>>> > >>>>> [root@~]# grep c_next /proc/kallsyms > >>>>> ffffffff81419dc0 t c_next.llvm.7567888411731313343 > >>>>> ffffffff81680600 t c_next > >>>>> ffffffff81854380 t c_next.llvm.14337844803752139461 > >>>>> > >>>>> If the goal is to explicitly trace c_next.llvm.7567888411731313343, the > >>>>> user can provide the full name. If we always match _without_suffix, all > >>>>> of the 3 will match to the first one. > >>>>> > >>>>> Does this make sense? > >>>> > >>>> Yes. Sorry, I missed the "&& !addr)" after the "IS_ENABLED()", which looked > >>>> like you did the command twice. > >>> > >>> But that said, does this only have to be for llvm? Or should we do this for > >>> even gcc? As I believe gcc can give strange symbols too. > >> > >> I think most of the issue comes with LTO, as LTO promotes local static > >> functions to global functions. IIUC, we don't have GCC built, LTO enabled > >> kernel yet. > >> > >> In my GCC built, we have suffixes like ".constprop.0", ".part.0", ".isra.0", > >> and ".isra.0.cold". We didn't do anything about these before this set. So I > >> think we are OK not handling them now. We sure can enable it for GCC built > >> kernel in the future. > > > > Hmm, I think it should be handled as it is. This means it should do as > > livepatch does. Since I expected user will check kallsyms if gets error, > > we should keep this as it is. (if a symbol has suffix, it should accept > > symbol with suffix, or user will get confused because they can not find > > which symbol is kprobed.) > > > > Sorry about the conclusion (so I NAK this), but this is a good discussion. > > Do you mean we do not want patch 3/3, but would like to keep 1/3 and part > of 2/3 (remove the _without_suffix APIs)? If this is the case, we are > undoing the change by Sami in [1], and thus may break some tracing tools. What tracing tools may be broke and why? For this suffix problem, I would like to add another patch to allow probing on suffixed symbols. (It seems suffixed symbols are not available at this point) The problem is that the suffixed symbols maybe a "part" of the original function, thus user has to carefully use it. > > Sami, could you please share your thoughts on this? Sami, I would like to know what problem you have on kprobes. Thank you, > > If this works, I will send next version with 1/3 and part of 2/3. > > Thanks, > Song > > [1] https://lore.kernel.org/all/20210408182843.1754385-8-samitolvanen@google.com/ >
> In my GCC built, we have suffixes like ".constprop.0", ".part.0", ".isra.0", > and ".isra.0.cold". A fresher's eye, I met sometime when try to build a livepatch module and found some mistake caused by ".constprop.0" ".part.0" which is generated by GCC. These section with such suffixes is special and sometime the symbol st_value is quite different. What is these kind of section (or symbol) use for? Regards Wardenjohn
Hi, On Wed, Aug 7, 2024 at 3:08 AM Masami Hiramatsu <mhiramat@kernel.org> wrote: > > On Wed, 7 Aug 2024 00:19:20 +0000 > Song Liu <songliubraving@meta.com> wrote: > > > Do you mean we do not want patch 3/3, but would like to keep 1/3 and part > > of 2/3 (remove the _without_suffix APIs)? If this is the case, we are > > undoing the change by Sami in [1], and thus may break some tracing tools. > > What tracing tools may be broke and why? This was a few years ago when we were first adding LTO support, but the unexpected suffixes in tracing output broke systrace in Android, presumably because the tools expected to find specific function names without suffixes. I'm not sure if systrace would still be a problem today, but other tools might still make assumptions about the function name format. At the time, we decided to filter out the suffixes in all user space visible output to avoid these issues. > For this suffix problem, I would like to add another patch to allow probing on > suffixed symbols. (It seems suffixed symbols are not available at this point) > > The problem is that the suffixed symbols maybe a "part" of the original function, > thus user has to carefully use it. > > > > > Sami, could you please share your thoughts on this? > > Sami, I would like to know what problem you have on kprobes. The reports we received back then were about registering kprobes for static functions, which obviously failed if the compiler added a suffix to the function name. This was more of a problem with ThinLTO and Clang CFI at the time because the compiler used to rename _all_ static functions, but one can obviously run into the same issue with just LTO. Sami
> On Aug 7, 2024, at 3:08 AM, Masami Hiramatsu <mhiramat@kernel.org> wrote: > > On Wed, 7 Aug 2024 00:19:20 +0000 > Song Liu <songliubraving@meta.com> wrote: > >> >> >>> On Aug 6, 2024, at 5:01 PM, Masami Hiramatsu <mhiramat@kernel.org> wrote: >>> >>> On Tue, 6 Aug 2024 20:12:55 +0000 >>> Song Liu <songliubraving@meta.com> wrote: >>> >>>> >>>> >>>>> On Aug 6, 2024, at 1:01 PM, Steven Rostedt <rostedt@goodmis.org> wrote: >>>>> >>>>> On Tue, 6 Aug 2024 16:00:49 -0400 >>>>> Steven Rostedt <rostedt@goodmis.org> wrote: >>>>> >>>>>>>>> + if (IS_ENABLED(CONFIG_LTO_CLANG) && !addr) >>>>>>>>> + addr = kallsyms_lookup_name_without_suffix(trace_kprobe_symbol(tk)); >>>>>>>>> + >>>>>>>> >>>>>>>> So you do the lookup twice if this is enabled? >>>>>>>> >>>>>>>> Why not just use "kallsyms_lookup_name_without_suffix()" the entire time, >>>>>>>> and it should work just the same as "kallsyms_lookup_name()" if it's not >>>>>>>> needed? >>>>>>> >>>>>>> We still want to give priority to full match. For example, we have: >>>>>>> >>>>>>> [root@~]# grep c_next /proc/kallsyms >>>>>>> ffffffff81419dc0 t c_next.llvm.7567888411731313343 >>>>>>> ffffffff81680600 t c_next >>>>>>> ffffffff81854380 t c_next.llvm.14337844803752139461 >>>>>>> >>>>>>> If the goal is to explicitly trace c_next.llvm.7567888411731313343, the >>>>>>> user can provide the full name. If we always match _without_suffix, all >>>>>>> of the 3 will match to the first one. >>>>>>> >>>>>>> Does this make sense? >>>>>> >>>>>> Yes. Sorry, I missed the "&& !addr)" after the "IS_ENABLED()", which looked >>>>>> like you did the command twice. >>>>> >>>>> But that said, does this only have to be for llvm? Or should we do this for >>>>> even gcc? As I believe gcc can give strange symbols too. >>>> >>>> I think most of the issue comes with LTO, as LTO promotes local static >>>> functions to global functions. IIUC, we don't have GCC built, LTO enabled >>>> kernel yet. >>>> >>>> In my GCC built, we have suffixes like ".constprop.0", ".part.0", ".isra.0", >>>> and ".isra.0.cold". We didn't do anything about these before this set. So I >>>> think we are OK not handling them now. We sure can enable it for GCC built >>>> kernel in the future. >>> >>> Hmm, I think it should be handled as it is. This means it should do as >>> livepatch does. Since I expected user will check kallsyms if gets error, >>> we should keep this as it is. (if a symbol has suffix, it should accept >>> symbol with suffix, or user will get confused because they can not find >>> which symbol is kprobed.) >>> >>> Sorry about the conclusion (so I NAK this), but this is a good discussion. >> >> Do you mean we do not want patch 3/3, but would like to keep 1/3 and part >> of 2/3 (remove the _without_suffix APIs)? If this is the case, we are >> undoing the change by Sami in [1], and thus may break some tracing tools. > > What tracing tools may be broke and why? > > For this suffix problem, I would like to add another patch to allow probing on > suffixed symbols. (It seems suffixed symbols are not available at this point) > > The problem is that the suffixed symbols maybe a "part" of the original function, > thus user has to carefully use it. It appears there are multiple APIs that may need change. For example, on gcc built kernel, /sys/kernel/tracing/available_filter_functions does not show the suffix: [root@(none)]# grep cmos_irq_enable /proc/kallsyms ffffffff81db5470 t __pfx_cmos_irq_enable.constprop.0 ffffffff81db5480 t cmos_irq_enable.constprop.0 ffffffff822dec6e t cmos_irq_enable.constprop.0.cold [root@(none)]# grep cmos_irq_enable /sys/kernel/tracing/available_filter_functions cmos_irq_enable perf-probe uses _text+<offset> for such cases: [root@(none)]# cat /sys/kernel/tracing/kprobe_events p:probe/cmos_irq_enable _text+14374016 I am not sure which APIs do we need to change here. Thanks, Song
> On Aug 7, 2024, at 7:58 AM, zhang warden <zhangwarden@gmail.com> wrote: > > >> In my GCC built, we have suffixes like ".constprop.0", ".part.0", ".isra.0", >> and ".isra.0.cold". > > A fresher's eye, I met sometime when try to build a livepatch module and found some mistake caused by ".constprop.0" ".part.0" which is generated by GCC. > > These section with such suffixes is special and sometime the symbol st_value is quite different. What is these kind of section (or symbol) use for? IIUC, constprop means const propagation. For example, function "foo(int a, int b)" that is called as "foo(a, 10)" will be come "foo(int a)" with a hard-coded b = 10 inside. .part.0 is part of the function, as the other part is inlined in the caller. With binary-diff based toolchain (kpatch-build), I think these will be handled automatically. However, if we write the livepatch manually, we need to understand these behavior with .constprop and .part. Thanks, Song
On Wed, 7 Aug 2024 19:41:11 +0000 Song Liu <songliubraving@meta.com> wrote: > It appears there are multiple APIs that may need change. For example, on gcc > built kernel, /sys/kernel/tracing/available_filter_functions does not show > the suffix: > > [root@(none)]# grep cmos_irq_enable /proc/kallsyms > ffffffff81db5470 t __pfx_cmos_irq_enable.constprop.0 > ffffffff81db5480 t cmos_irq_enable.constprop.0 > ffffffff822dec6e t cmos_irq_enable.constprop.0.cold > > [root@(none)]# grep cmos_irq_enable /sys/kernel/tracing/available_filter_functions > cmos_irq_enable Strange, I don't see that: ~# grep cmos_irq_enable /proc/kallsyms ffffffff8f4b2500 t __pfx_cmos_irq_enable.constprop.0 ffffffff8f4b2510 t cmos_irq_enable.constprop.0 ~# grep cmos_irq_enable /sys/kernel/tracing/available_filter_functions cmos_irq_enable.constprop.0 -- Steve > > perf-probe uses _text+<offset> for such cases: > > [root@(none)]# cat /sys/kernel/tracing/kprobe_events > p:probe/cmos_irq_enable _text+14374016 > > I am not sure which APIs do we need to change here. > > Thanks, > Song > >
> On Aug 7, 2024, at 1:08 PM, Steven Rostedt <rostedt@goodmis.org> wrote: > > On Wed, 7 Aug 2024 19:41:11 +0000 > Song Liu <songliubraving@meta.com> wrote: > > >> It appears there are multiple APIs that may need change. For example, on gcc >> built kernel, /sys/kernel/tracing/available_filter_functions does not show >> the suffix: >> >> [root@(none)]# grep cmos_irq_enable /proc/kallsyms >> ffffffff81db5470 t __pfx_cmos_irq_enable.constprop.0 >> ffffffff81db5480 t cmos_irq_enable.constprop.0 >> ffffffff822dec6e t cmos_irq_enable.constprop.0.cold >> >> [root@(none)]# grep cmos_irq_enable /sys/kernel/tracing/available_filter_functions >> cmos_irq_enable > > Strange, I don't see that: > > ~# grep cmos_irq_enable /proc/kallsyms > ffffffff8f4b2500 t __pfx_cmos_irq_enable.constprop.0 > ffffffff8f4b2510 t cmos_irq_enable.constprop.0 > > ~# grep cmos_irq_enable /sys/kernel/tracing/available_filter_functions > cmos_irq_enable.constprop.0 Ah, this is caused by my change. Let me fix that in the next version. Thanks, Song
> On Aug 7, 2024, at 1:40 PM, Song Liu <songliubraving@meta.com> wrote: > > > >> On Aug 7, 2024, at 1:08 PM, Steven Rostedt <rostedt@goodmis.org> wrote: >> >> On Wed, 7 Aug 2024 19:41:11 +0000 >> Song Liu <songliubraving@meta.com> wrote: >> >> >>> It appears there are multiple APIs that may need change. For example, on gcc >>> built kernel, /sys/kernel/tracing/available_filter_functions does not show >>> the suffix: >>> >>> [root@(none)]# grep cmos_irq_enable /proc/kallsyms >>> ffffffff81db5470 t __pfx_cmos_irq_enable.constprop.0 >>> ffffffff81db5480 t cmos_irq_enable.constprop.0 >>> ffffffff822dec6e t cmos_irq_enable.constprop.0.cold >>> >>> [root@(none)]# grep cmos_irq_enable /sys/kernel/tracing/available_filter_functions >>> cmos_irq_enable >> >> Strange, I don't see that: >> >> ~# grep cmos_irq_enable /proc/kallsyms >> ffffffff8f4b2500 t __pfx_cmos_irq_enable.constprop.0 >> ffffffff8f4b2510 t cmos_irq_enable.constprop.0 >> >> ~# grep cmos_irq_enable /sys/kernel/tracing/available_filter_functions >> cmos_irq_enable.constprop.0 > > Ah, this is caused by my change. Let me fix that in the next version. PS: Current code still remove .llvm.<hash> suffix from available_filter_functions. I will change kallsyms_lookup_buildid() to not do the cleanup. Thanks, Song
> On Aug 7, 2024, at 8:33 AM, Sami Tolvanen <samitolvanen@google.com> wrote: > > Hi, > > On Wed, Aug 7, 2024 at 3:08 AM Masami Hiramatsu <mhiramat@kernel.org> wrote: >> >> On Wed, 7 Aug 2024 00:19:20 +0000 >> Song Liu <songliubraving@meta.com> wrote: >> >>> Do you mean we do not want patch 3/3, but would like to keep 1/3 and part >>> of 2/3 (remove the _without_suffix APIs)? If this is the case, we are >>> undoing the change by Sami in [1], and thus may break some tracing tools. >> >> What tracing tools may be broke and why? > > This was a few years ago when we were first adding LTO support, but > the unexpected suffixes in tracing output broke systrace in Android, > presumably because the tools expected to find specific function names > without suffixes. I'm not sure if systrace would still be a problem > today, but other tools might still make assumptions about the function > name format. At the time, we decided to filter out the suffixes in all > user space visible output to avoid these issues. > >> For this suffix problem, I would like to add another patch to allow probing on >> suffixed symbols. (It seems suffixed symbols are not available at this point) >> >> The problem is that the suffixed symbols maybe a "part" of the original function, >> thus user has to carefully use it. >> >>> >>> Sami, could you please share your thoughts on this? >> >> Sami, I would like to know what problem you have on kprobes. > > The reports we received back then were about registering kprobes for > static functions, which obviously failed if the compiler added a > suffix to the function name. This was more of a problem with ThinLTO > and Clang CFI at the time because the compiler used to rename _all_ > static functions, but one can obviously run into the same issue with > just LTO. I think newer LLVM/clang no longer add suffixes to all static functions with LTO and CFI. So this may not be a real issue any more? If we still need to allow tracing without suffix, I think the approach in this patch set is correct (sort syms based on full name, remove suffixes in special APIs during lookup). Thanks, Song
On Wed, 7 Aug 2024 00:19:20 +0000 Song Liu <songliubraving@meta.com> wrote: > > > > On Aug 6, 2024, at 5:01 PM, Masami Hiramatsu <mhiramat@kernel.org> wrote: > > > > On Tue, 6 Aug 2024 20:12:55 +0000 > > Song Liu <songliubraving@meta.com> wrote: > > > >> > >> > >>> On Aug 6, 2024, at 1:01 PM, Steven Rostedt <rostedt@goodmis.org> wrote: > >>> > >>> On Tue, 6 Aug 2024 16:00:49 -0400 > >>> Steven Rostedt <rostedt@goodmis.org> wrote: > >>> > >>>>>>> + if (IS_ENABLED(CONFIG_LTO_CLANG) && !addr) > >>>>>>> + addr = kallsyms_lookup_name_without_suffix(trace_kprobe_symbol(tk)); > >>>>>>> + > >>>>>> > >>>>>> So you do the lookup twice if this is enabled? > >>>>>> > >>>>>> Why not just use "kallsyms_lookup_name_without_suffix()" the entire time, > >>>>>> and it should work just the same as "kallsyms_lookup_name()" if it's not > >>>>>> needed? > >>>>> > >>>>> We still want to give priority to full match. For example, we have: > >>>>> > >>>>> [root@~]# grep c_next /proc/kallsyms > >>>>> ffffffff81419dc0 t c_next.llvm.7567888411731313343 > >>>>> ffffffff81680600 t c_next > >>>>> ffffffff81854380 t c_next.llvm.14337844803752139461 > >>>>> > >>>>> If the goal is to explicitly trace c_next.llvm.7567888411731313343, the > >>>>> user can provide the full name. If we always match _without_suffix, all > >>>>> of the 3 will match to the first one. > >>>>> > >>>>> Does this make sense? > >>>> > >>>> Yes. Sorry, I missed the "&& !addr)" after the "IS_ENABLED()", which looked > >>>> like you did the command twice. > >>> > >>> But that said, does this only have to be for llvm? Or should we do this for > >>> even gcc? As I believe gcc can give strange symbols too. > >> > >> I think most of the issue comes with LTO, as LTO promotes local static > >> functions to global functions. IIUC, we don't have GCC built, LTO enabled > >> kernel yet. > >> > >> In my GCC built, we have suffixes like ".constprop.0", ".part.0", ".isra.0", > >> and ".isra.0.cold". We didn't do anything about these before this set. So I > >> think we are OK not handling them now. We sure can enable it for GCC built > >> kernel in the future. > > > > Hmm, I think it should be handled as it is. This means it should do as > > livepatch does. Since I expected user will check kallsyms if gets error, > > we should keep this as it is. (if a symbol has suffix, it should accept > > symbol with suffix, or user will get confused because they can not find > > which symbol is kprobed.) > > > > Sorry about the conclusion (so I NAK this), but this is a good discussion. > > Do you mean we do not want patch 3/3, but would like to keep 1/3 and part > of 2/3 (remove the _without_suffix APIs)? If this is the case, we are > undoing the change by Sami in [1], and thus may break some tracing tools. BTW, I confirmed that the PATCH 1/3 and 2/3 fixes kprobes to probe on suffixed symbols correctly. (because 1/3 allows to search suffixed symbols) /sys/kernel/tracing # cat dynamic_events p:kprobes/p_c_stop_llvm_17132674095431275852_0 c_stop.llvm.17132674095431275852 p:kprobes/p_c_stop_llvm_8011538628216713357_0 c_stop.llvm.8011538628216713357 p:kprobes/p_c_stop_0 c_stop Thank you, > > Sami, could you please share your thoughts on this? > > If this works, I will send next version with 1/3 and part of 2/3. > > Thanks, > Song > > [1] https://lore.kernel.org/all/20210408182843.1754385-8-samitolvanen@google.com/ >
> On Aug 7, 2024, at 1:55 PM, Masami Hiramatsu <mhiramat@kernel.org> wrote: > > On Wed, 7 Aug 2024 00:19:20 +0000 > Song Liu <songliubraving@meta.com> wrote: > >> >> >>> On Aug 6, 2024, at 5:01 PM, Masami Hiramatsu <mhiramat@kernel.org> wrote: >>> >>> On Tue, 6 Aug 2024 20:12:55 +0000 >>> Song Liu <songliubraving@meta.com> wrote: >>> >>>> >>>> >>>>> On Aug 6, 2024, at 1:01 PM, Steven Rostedt <rostedt@goodmis.org> wrote: >>>>> >>>>> On Tue, 6 Aug 2024 16:00:49 -0400 >>>>> Steven Rostedt <rostedt@goodmis.org> wrote: >>>>> >>>>>>>>> + if (IS_ENABLED(CONFIG_LTO_CLANG) && !addr) >>>>>>>>> + addr = kallsyms_lookup_name_without_suffix(trace_kprobe_symbol(tk)); >>>>>>>>> + >>>>>>>> >>>>>>>> So you do the lookup twice if this is enabled? >>>>>>>> >>>>>>>> Why not just use "kallsyms_lookup_name_without_suffix()" the entire time, >>>>>>>> and it should work just the same as "kallsyms_lookup_name()" if it's not >>>>>>>> needed? >>>>>>> >>>>>>> We still want to give priority to full match. For example, we have: >>>>>>> >>>>>>> [root@~]# grep c_next /proc/kallsyms >>>>>>> ffffffff81419dc0 t c_next.llvm.7567888411731313343 >>>>>>> ffffffff81680600 t c_next >>>>>>> ffffffff81854380 t c_next.llvm.14337844803752139461 >>>>>>> >>>>>>> If the goal is to explicitly trace c_next.llvm.7567888411731313343, the >>>>>>> user can provide the full name. If we always match _without_suffix, all >>>>>>> of the 3 will match to the first one. >>>>>>> >>>>>>> Does this make sense? >>>>>> >>>>>> Yes. Sorry, I missed the "&& !addr)" after the "IS_ENABLED()", which looked >>>>>> like you did the command twice. >>>>> >>>>> But that said, does this only have to be for llvm? Or should we do this for >>>>> even gcc? As I believe gcc can give strange symbols too. >>>> >>>> I think most of the issue comes with LTO, as LTO promotes local static >>>> functions to global functions. IIUC, we don't have GCC built, LTO enabled >>>> kernel yet. >>>> >>>> In my GCC built, we have suffixes like ".constprop.0", ".part.0", ".isra.0", >>>> and ".isra.0.cold". We didn't do anything about these before this set. So I >>>> think we are OK not handling them now. We sure can enable it for GCC built >>>> kernel in the future. >>> >>> Hmm, I think it should be handled as it is. This means it should do as >>> livepatch does. Since I expected user will check kallsyms if gets error, >>> we should keep this as it is. (if a symbol has suffix, it should accept >>> symbol with suffix, or user will get confused because they can not find >>> which symbol is kprobed.) >>> >>> Sorry about the conclusion (so I NAK this), but this is a good discussion. >> >> Do you mean we do not want patch 3/3, but would like to keep 1/3 and part >> of 2/3 (remove the _without_suffix APIs)? If this is the case, we are >> undoing the change by Sami in [1], and thus may break some tracing tools. > > BTW, I confirmed that the PATCH 1/3 and 2/3 fixes kprobes to probe on suffixed > symbols correctly. (because 1/3 allows to search suffixed symbols) > > /sys/kernel/tracing # cat dynamic_events > p:kprobes/p_c_stop_llvm_17132674095431275852_0 c_stop.llvm.17132674095431275852 > p:kprobes/p_c_stop_llvm_8011538628216713357_0 c_stop.llvm.8011538628216713357 > p:kprobes/p_c_stop_0 c_stop Thanks for confirming this works. I will clean up the code and send v3. I plan to remove the _without_suffix APIs in v3. We can add them back if we need them to get systrace working. Song
On Wed, 7 Aug 2024 08:33:07 -0700 Sami Tolvanen <samitolvanen@google.com> wrote: > Hi, > > On Wed, Aug 7, 2024 at 3:08 AM Masami Hiramatsu <mhiramat@kernel.org> wrote: > > > > On Wed, 7 Aug 2024 00:19:20 +0000 > > Song Liu <songliubraving@meta.com> wrote: > > > > > Do you mean we do not want patch 3/3, but would like to keep 1/3 and part > > > of 2/3 (remove the _without_suffix APIs)? If this is the case, we are > > > undoing the change by Sami in [1], and thus may break some tracing tools. > > > > What tracing tools may be broke and why? > > This was a few years ago when we were first adding LTO support, but > the unexpected suffixes in tracing output broke systrace in Android, > presumably because the tools expected to find specific function names > without suffixes. I'm not sure if systrace would still be a problem > today, but other tools might still make assumptions about the function > name format. At the time, we decided to filter out the suffixes in all > user space visible output to avoid these issues. Thanks for explanation. IMHO, those tools might change their assumptions and decide the policy that it drops suffixes (as you did) or keep the suffixes as it is. > > For this suffix problem, I would like to add another patch to allow probing on > > suffixed symbols. (It seems suffixed symbols are not available at this point) > > > > The problem is that the suffixed symbols maybe a "part" of the original function, > > thus user has to carefully use it. > > > > > > > > Sami, could you please share your thoughts on this? > > > > Sami, I would like to know what problem you have on kprobes. > > The reports we received back then were about registering kprobes for > static functions, which obviously failed if the compiler added a > suffix to the function name. This was more of a problem with ThinLTO > and Clang CFI at the time because the compiler used to rename _all_ > static functions, but one can obviously run into the same issue with > just LTO. Yeah, without 1/3 of this series, user can not specify llvm suffixed symbols on kprobe events. However, as perf-probe does, users can use the relative offset from a unique symbol too. (kprobe does not care the function boundary.) Thank you, > > Sami
> IIUC, constprop means const propagation. For example, function > "foo(int a, int b)" that is called as "foo(a, 10)" will be come > "foo(int a)" with a hard-coded b = 10 inside. > > .part.0 is part of the function, as the other part is inlined in > the caller. > > With binary-diff based toolchain (kpatch-build), I think these will be > handled automatically. > > Thanks, > Song Yep, Thanks for your explanation! I discuss here just for an interest. IMO, more people may use tools like 'kpatch-build' to build a livepatch for it will be more easy. But I think such tools should also be attention to such changes because when it trying to do section-level or symbol-level operations. Otherwise it may cause problems :) > However, if we write the livepatch manually, we > need to understand these behavior with .constprop and .part. Do you think it is wise to write a livepatch module manually? If we are trying to write a module manually, we should also take care of the klp_* function calling, while using a livepatch building tools will help handle it :) Thanks. Wardenjohn
On Wed 2024-08-07 19:46:31, Song Liu wrote: > > > > On Aug 7, 2024, at 7:58 AM, zhang warden <zhangwarden@gmail.com> wrote: > > > > > >> In my GCC built, we have suffixes like ".constprop.0", ".part.0", ".isra.0", > >> and ".isra.0.cold". > > > > A fresher's eye, I met sometime when try to build a livepatch module and found some mistake caused by ".constprop.0" ".part.0" which is generated by GCC. > > > > These section with such suffixes is special and sometime the symbol st_value is quite different. What is these kind of section (or symbol) use for? > > > IIUC, constprop means const propagation. For example, function > "foo(int a, int b)" that is called as "foo(a, 10)" will be come > "foo(int a)" with a hard-coded b = 10 inside. > > .part.0 is part of the function, as the other part is inlined in > the caller. Hmm, we should not remove the suffixes like .constprop*, .part*, .isra*. They implement a special optimized variant of the function. It is not longer the original full-featured one. This is a difference against adding a suffix for a static function. Such a symbol implements the original full-featured function. Best Regards, Petr
On Wed 2024-08-07 20:48:48, Song Liu wrote: > > > > On Aug 7, 2024, at 8:33 AM, Sami Tolvanen <samitolvanen@google.com> wrote: > > > > Hi, > > > > On Wed, Aug 7, 2024 at 3:08 AM Masami Hiramatsu <mhiramat@kernel.org> wrote: > >> > >> On Wed, 7 Aug 2024 00:19:20 +0000 > >> Song Liu <songliubraving@meta.com> wrote: > >> > >>> Do you mean we do not want patch 3/3, but would like to keep 1/3 and part > >>> of 2/3 (remove the _without_suffix APIs)? If this is the case, we are > >>> undoing the change by Sami in [1], and thus may break some tracing tools. > >> > >> What tracing tools may be broke and why? > > > > This was a few years ago when we were first adding LTO support, but > > the unexpected suffixes in tracing output broke systrace in Android, > > presumably because the tools expected to find specific function names > > without suffixes. I'm not sure if systrace would still be a problem > > today, but other tools might still make assumptions about the function > > name format. At the time, we decided to filter out the suffixes in all > > user space visible output to avoid these issues. > > > >> For this suffix problem, I would like to add another patch to allow probing on > >> suffixed symbols. (It seems suffixed symbols are not available at this point) > >> > >> The problem is that the suffixed symbols maybe a "part" of the original function, > >> thus user has to carefully use it. > >> > >>> > >>> Sami, could you please share your thoughts on this? > >> > >> Sami, I would like to know what problem you have on kprobes. > > > > The reports we received back then were about registering kprobes for > > static functions, which obviously failed if the compiler added a > > suffix to the function name. This was more of a problem with ThinLTO > > and Clang CFI at the time because the compiler used to rename _all_ > > static functions, but one can obviously run into the same issue with > > just LTO. > > I think newer LLVM/clang no longer add suffixes to all static functions > with LTO and CFI. So this may not be a real issue any more? > > If we still need to allow tracing without suffix, I think the approach > in this patch set is correct (sort syms based on full name, Yes, we should allow to find the symbols via the full name, definitely. > remove suffixes in special APIs during lookup). Just an idea. Alternative solution would be to make make an alias without the suffix when there is only one symbol with the same name. It would be complementary with the patch adding aliases for symbols with the same name, see https://lore.kernel.org/r/20231204214635.2916691-1-alessandro.carminati@gmail.com I would allow to find the symbols with and without the suffix using a single API. Best Regards, Petr
> On Aug 8, 2024, at 2:48 AM, Petr Mladek <pmladek@suse.com> wrote: > > On Wed 2024-08-07 19:46:31, Song Liu wrote: >> >> >>> On Aug 7, 2024, at 7:58 AM, zhang warden <zhangwarden@gmail.com> wrote: >>> >>> >>>> In my GCC built, we have suffixes like ".constprop.0", ".part.0", ".isra.0", >>>> and ".isra.0.cold". >>> >>> A fresher's eye, I met sometime when try to build a livepatch module and found some mistake caused by ".constprop.0" ".part.0" which is generated by GCC. >>> >>> These section with such suffixes is special and sometime the symbol st_value is quite different. What is these kind of section (or symbol) use for? >> >> >> IIUC, constprop means const propagation. For example, function >> "foo(int a, int b)" that is called as "foo(a, 10)" will be come >> "foo(int a)" with a hard-coded b = 10 inside. >> >> .part.0 is part of the function, as the other part is inlined in >> the caller. > > Hmm, we should not remove the suffixes like .constprop*, .part*, > .isra*. They implement a special optimized variant of the function. > It is not longer the original full-featured one. > > This is a difference against adding a suffix for a static function. > Such a symbol implements the original full-featured function. Allow tracing without .llvm.<hash> suffixes may target a different function with same name, i.e. func_a.llvm.1 vs. func_a.llvm.2. We can probably detect and report this in the kernel. However, I would rather we just disallow tracing without suffixes. I think Masami also agrees with this. Thanks, Song
> On Aug 8, 2024, at 2:59 AM, Petr Mladek <pmladek@suse.com> wrote: > > On Wed 2024-08-07 20:48:48, Song Liu wrote: >> >> >>> On Aug 7, 2024, at 8:33 AM, Sami Tolvanen <samitolvanen@google.com> wrote: >>> >>> Hi, >>> >>> On Wed, Aug 7, 2024 at 3:08 AM Masami Hiramatsu <mhiramat@kernel.org> wrote: >>>> >>>> On Wed, 7 Aug 2024 00:19:20 +0000 >>>> Song Liu <songliubraving@meta.com> wrote: >>>> >>>>> Do you mean we do not want patch 3/3, but would like to keep 1/3 and part >>>>> of 2/3 (remove the _without_suffix APIs)? If this is the case, we are >>>>> undoing the change by Sami in [1], and thus may break some tracing tools. >>>> >>>> What tracing tools may be broke and why? >>> >>> This was a few years ago when we were first adding LTO support, but >>> the unexpected suffixes in tracing output broke systrace in Android, >>> presumably because the tools expected to find specific function names >>> without suffixes. I'm not sure if systrace would still be a problem >>> today, but other tools might still make assumptions about the function >>> name format. At the time, we decided to filter out the suffixes in all >>> user space visible output to avoid these issues. >>> >>>> For this suffix problem, I would like to add another patch to allow probing on >>>> suffixed symbols. (It seems suffixed symbols are not available at this point) >>>> >>>> The problem is that the suffixed symbols maybe a "part" of the original function, >>>> thus user has to carefully use it. >>>> >>>>> >>>>> Sami, could you please share your thoughts on this? >>>> >>>> Sami, I would like to know what problem you have on kprobes. >>> >>> The reports we received back then were about registering kprobes for >>> static functions, which obviously failed if the compiler added a >>> suffix to the function name. This was more of a problem with ThinLTO >>> and Clang CFI at the time because the compiler used to rename _all_ >>> static functions, but one can obviously run into the same issue with >>> just LTO. >> >> I think newer LLVM/clang no longer add suffixes to all static functions >> with LTO and CFI. So this may not be a real issue any more? >> >> If we still need to allow tracing without suffix, I think the approach >> in this patch set is correct (sort syms based on full name, > > Yes, we should allow to find the symbols via the full name, definitely. > >> remove suffixes in special APIs during lookup). > > Just an idea. Alternative solution would be to make make an alias > without the suffix when there is only one symbol with the same > name. > > It would be complementary with the patch adding aliases for symbols > with the same name, see > https://lore.kernel.org/r/20231204214635.2916691-1-alessandro.carminati@gmail.com I guess v3 plus this work may work well together. > I would allow to find the symbols with and without the suffix using > a single API. Could you please describe how this API would work? I tried some idea in v1, but it turned out to be quite confusing. So I decided to leave this logic to the users of kallsyms APIs in v2. Thanks, Song
On Thu, 8 Aug 2024 11:59:00 +0200 Petr Mladek <pmladek@suse.com> wrote: > On Wed 2024-08-07 20:48:48, Song Liu wrote: > > > > > > > On Aug 7, 2024, at 8:33 AM, Sami Tolvanen <samitolvanen@google.com> wrote: > > > > > > Hi, > > > > > > On Wed, Aug 7, 2024 at 3:08 AM Masami Hiramatsu <mhiramat@kernel.org> wrote: > > >> > > >> On Wed, 7 Aug 2024 00:19:20 +0000 > > >> Song Liu <songliubraving@meta.com> wrote: > > >> > > >>> Do you mean we do not want patch 3/3, but would like to keep 1/3 and part > > >>> of 2/3 (remove the _without_suffix APIs)? If this is the case, we are > > >>> undoing the change by Sami in [1], and thus may break some tracing tools. > > >> > > >> What tracing tools may be broke and why? > > > > > > This was a few years ago when we were first adding LTO support, but > > > the unexpected suffixes in tracing output broke systrace in Android, > > > presumably because the tools expected to find specific function names > > > without suffixes. I'm not sure if systrace would still be a problem > > > today, but other tools might still make assumptions about the function > > > name format. At the time, we decided to filter out the suffixes in all > > > user space visible output to avoid these issues. > > > > > >> For this suffix problem, I would like to add another patch to allow probing on > > >> suffixed symbols. (It seems suffixed symbols are not available at this point) > > >> > > >> The problem is that the suffixed symbols maybe a "part" of the original function, > > >> thus user has to carefully use it. > > >> > > >>> > > >>> Sami, could you please share your thoughts on this? > > >> > > >> Sami, I would like to know what problem you have on kprobes. > > > > > > The reports we received back then were about registering kprobes for > > > static functions, which obviously failed if the compiler added a > > > suffix to the function name. This was more of a problem with ThinLTO > > > and Clang CFI at the time because the compiler used to rename _all_ > > > static functions, but one can obviously run into the same issue with > > > just LTO. > > > > I think newer LLVM/clang no longer add suffixes to all static functions > > with LTO and CFI. So this may not be a real issue any more? > > > > If we still need to allow tracing without suffix, I think the approach > > in this patch set is correct (sort syms based on full name, > > Yes, we should allow to find the symbols via the full name, definitely. > > > remove suffixes in special APIs during lookup). > > Just an idea. Alternative solution would be to make make an alias > without the suffix when there is only one symbol with the same > name. > > It would be complementary with the patch adding aliases for symbols > with the same name, see > https://lore.kernel.org/r/20231204214635.2916691-1-alessandro.carminati@gmail.com I think this is the best idea what we can do for tracing/stacktrace with same-name symbols. But the root cause here is a bit different, that's why I rejected the last patch. With compiler suffixes, the source line aliases should remove those suffixes and add new suffix like below. 1234 t name_show.llvm.12345678 1234 t name_show@kernel_irq_irqdesc_c_264 > I would allow to find the symbols with and without the suffix using > a single API. I wonder why we need it? for ftrace filter? The same symbol name does not mean the same function prototype. For the kprobes (and fprobes, ftraces too) user must specify which actual symbol is what you want to probe. Of course if user can say "hey kprobe, probe name_show", but that is unclear (not unique symbol) so kprobe will reject it. If there is .llvm suffix, user can specify one of them. But it is still unclear to user where in the source code the symbol is actually defined. So eventually, we need the debuginfo or this @suffix. Thank you, > > Best Regards, > Petr
Hello, sorry to join late at the party. Il giorno gio 8 ago 2024 alle ore 11:59 Petr Mladek <pmladek@suse.com> ha scritto: > > On Wed 2024-08-07 20:48:48, Song Liu wrote: > > > > > > > On Aug 7, 2024, at 8:33 AM, Sami Tolvanen <samitolvanen@google.com> wrote: > > > > > > Hi, > > > > > > On Wed, Aug 7, 2024 at 3:08 AM Masami Hiramatsu <mhiramat@kernel.org> wrote: > > >> > > >> On Wed, 7 Aug 2024 00:19:20 +0000 > > >> Song Liu <songliubraving@meta.com> wrote: > > >> > > >>> Do you mean we do not want patch 3/3, but would like to keep 1/3 and part > > >>> of 2/3 (remove the _without_suffix APIs)? If this is the case, we are > > >>> undoing the change by Sami in [1], and thus may break some tracing tools. > > >> > > >> What tracing tools may be broke and why? > > > > > > This was a few years ago when we were first adding LTO support, but > > > the unexpected suffixes in tracing output broke systrace in Android, > > > presumably because the tools expected to find specific function names > > > without suffixes. I'm not sure if systrace would still be a problem > > > today, but other tools might still make assumptions about the function > > > name format. At the time, we decided to filter out the suffixes in all > > > user space visible output to avoid these issues. > > > > > >> For this suffix problem, I would like to add another patch to allow probing on > > >> suffixed symbols. (It seems suffixed symbols are not available at this point) > > >> > > >> The problem is that the suffixed symbols maybe a "part" of the original function, > > >> thus user has to carefully use it. > > >> > > >>> > > >>> Sami, could you please share your thoughts on this? > > >> > > >> Sami, I would like to know what problem you have on kprobes. > > > > > > The reports we received back then were about registering kprobes for > > > static functions, which obviously failed if the compiler added a > > > suffix to the function name. This was more of a problem with ThinLTO > > > and Clang CFI at the time because the compiler used to rename _all_ > > > static functions, but one can obviously run into the same issue with > > > just LTO. > > > > I think newer LLVM/clang no longer add suffixes to all static functions > > with LTO and CFI. So this may not be a real issue any more? > > > > If we still need to allow tracing without suffix, I think the approach > > in this patch set is correct (sort syms based on full name, > > Yes, we should allow to find the symbols via the full name, definitely. > > > remove suffixes in special APIs during lookup). > > Just an idea. Alternative solution would be to make make an alias > without the suffix when there is only one symbol with the same > name. > > It would be complementary with the patch adding aliases for symbols > with the same name, see > https://lore.kernel.org/r/20231204214635.2916691-1-alessandro.carminati@gmail.com > > I would allow to find the symbols with and without the suffix using > a single API. kas_alias isn't handling LTO as effectively as it should. This is something I plan to address in the next patch version. Introducing aliases is the best approach I found to preserve current tools behavior while adding this new feature. While I believe it will deliver the promised benefits, there is a trade-off, particularly affecting features like live patching, which rely on handling duplicate symbols. For instance, kallsyms_lookup_names typically returns the last occurrence of a symbol when the end argument is not NULL, but introducing aliases disrupts this behavior. I'm working on a solution to manage duplicate symbols, ensuring compatibility with both LTO and kallsyms_lookup_names. thanks, Alessandro > > Best Regards, > Petr
On Thu 2024-08-08 15:20:26, Song Liu wrote: > > > > On Aug 8, 2024, at 2:59 AM, Petr Mladek <pmladek@suse.com> wrote: > > > > On Wed 2024-08-07 20:48:48, Song Liu wrote: > >> > >> > >>> On Aug 7, 2024, at 8:33 AM, Sami Tolvanen <samitolvanen@google.com> wrote: > >>> > >>> Hi, > >>> > >>> On Wed, Aug 7, 2024 at 3:08 AM Masami Hiramatsu <mhiramat@kernel.org> wrote: > >>>> > >>>> On Wed, 7 Aug 2024 00:19:20 +0000 > >>>> Song Liu <songliubraving@meta.com> wrote: > >>>> > >>>>> Do you mean we do not want patch 3/3, but would like to keep 1/3 and part > >>>>> of 2/3 (remove the _without_suffix APIs)? If this is the case, we are > >>>>> undoing the change by Sami in [1], and thus may break some tracing tools. > >>>> > >>>> What tracing tools may be broke and why? > >>> > >>> This was a few years ago when we were first adding LTO support, but > >>> the unexpected suffixes in tracing output broke systrace in Android, > >>> presumably because the tools expected to find specific function names > >>> without suffixes. I'm not sure if systrace would still be a problem > >>> today, but other tools might still make assumptions about the function > >>> name format. At the time, we decided to filter out the suffixes in all > >>> user space visible output to avoid these issues. > >>> > >>>> For this suffix problem, I would like to add another patch to allow probing on > >>>> suffixed symbols. (It seems suffixed symbols are not available at this point) > >>>> > >>>> The problem is that the suffixed symbols maybe a "part" of the original function, > >>>> thus user has to carefully use it. > >>>> > >>>>> > >>>>> Sami, could you please share your thoughts on this? > >>>> > >>>> Sami, I would like to know what problem you have on kprobes. > >>> > >>> The reports we received back then were about registering kprobes for > >>> static functions, which obviously failed if the compiler added a > >>> suffix to the function name. This was more of a problem with ThinLTO > >>> and Clang CFI at the time because the compiler used to rename _all_ > >>> static functions, but one can obviously run into the same issue with > >>> just LTO. > >> > >> I think newer LLVM/clang no longer add suffixes to all static functions > >> with LTO and CFI. So this may not be a real issue any more? > >> > >> If we still need to allow tracing without suffix, I think the approach > >> in this patch set is correct (sort syms based on full name, > > > > Yes, we should allow to find the symbols via the full name, definitely. > > > >> remove suffixes in special APIs during lookup). > > > > Just an idea. Alternative solution would be to make make an alias > > without the suffix when there is only one symbol with the same > > name. > > > > It would be complementary with the patch adding aliases for symbols > > with the same name, see > > https://lore.kernel.org/r/20231204214635.2916691-1-alessandro.carminati@gmail.com > > I guess v3 plus this work may work well together. > > > I would allow to find the symbols with and without the suffix using > > a single API. > > Could you please describe how this API would work? I tried some > idea in v1, but it turned out to be quite confusing. So I decided > to leave this logic to the users of kallsyms APIs in v2. If we create an alias without the suffix but only when there is only one symbol with such a name then we have, for example: klp_complete_transition.lwn.123456 klp_complete_transition [alias] init_once.lwn.2131221 init_once.lwn.3443243 init_once.lwn.4324322 init_once.lwn.5214121 init_once.lwn.2153121 init_once.lwn.4342343 This way, it will be possible to find the static symbol "klp_complete_transition" without the suffix via the alias. It will have the alias because it has an unique name. While "init_once" symbol must always be searched with the suffix because it is not unique. It looks like >99% of static symbols have unique name. Best Regards, Petr
> On Aug 9, 2024, at 8:40 AM, Petr Mladek <pmladek@suse.com> wrote: > > On Thu 2024-08-08 15:20:26, Song Liu wrote: >> >> >>> On Aug 8, 2024, at 2:59 AM, Petr Mladek <pmladek@suse.com> wrote: >>> >>> On Wed 2024-08-07 20:48:48, Song Liu wrote: >>>> >>>> >>>>> On Aug 7, 2024, at 8:33 AM, Sami Tolvanen <samitolvanen@google.com> wrote: >>>>> >>>>> Hi, >>>>> >>>>> On Wed, Aug 7, 2024 at 3:08 AM Masami Hiramatsu <mhiramat@kernel.org> wrote: >>>>>> >>>>>> On Wed, 7 Aug 2024 00:19:20 +0000 >>>>>> Song Liu <songliubraving@meta.com> wrote: >>>>>> >>>>>>> Do you mean we do not want patch 3/3, but would like to keep 1/3 and part >>>>>>> of 2/3 (remove the _without_suffix APIs)? If this is the case, we are >>>>>>> undoing the change by Sami in [1], and thus may break some tracing tools. >>>>>> >>>>>> What tracing tools may be broke and why? >>>>> >>>>> This was a few years ago when we were first adding LTO support, but >>>>> the unexpected suffixes in tracing output broke systrace in Android, >>>>> presumably because the tools expected to find specific function names >>>>> without suffixes. I'm not sure if systrace would still be a problem >>>>> today, but other tools might still make assumptions about the function >>>>> name format. At the time, we decided to filter out the suffixes in all >>>>> user space visible output to avoid these issues. >>>>> >>>>>> For this suffix problem, I would like to add another patch to allow probing on >>>>>> suffixed symbols. (It seems suffixed symbols are not available at this point) >>>>>> >>>>>> The problem is that the suffixed symbols maybe a "part" of the original function, >>>>>> thus user has to carefully use it. >>>>>> >>>>>>> >>>>>>> Sami, could you please share your thoughts on this? >>>>>> >>>>>> Sami, I would like to know what problem you have on kprobes. >>>>> >>>>> The reports we received back then were about registering kprobes for >>>>> static functions, which obviously failed if the compiler added a >>>>> suffix to the function name. This was more of a problem with ThinLTO >>>>> and Clang CFI at the time because the compiler used to rename _all_ >>>>> static functions, but one can obviously run into the same issue with >>>>> just LTO. >>>> >>>> I think newer LLVM/clang no longer add suffixes to all static functions >>>> with LTO and CFI. So this may not be a real issue any more? >>>> >>>> If we still need to allow tracing without suffix, I think the approach >>>> in this patch set is correct (sort syms based on full name, >>> >>> Yes, we should allow to find the symbols via the full name, definitely. >>> >>>> remove suffixes in special APIs during lookup). >>> >>> Just an idea. Alternative solution would be to make make an alias >>> without the suffix when there is only one symbol with the same >>> name. >>> >>> It would be complementary with the patch adding aliases for symbols >>> with the same name, see >>> https://lore.kernel.org/r/20231204214635.2916691-1-alessandro.carminati@gmail.com >> >> I guess v3 plus this work may work well together. >> >>> I would allow to find the symbols with and without the suffix using >>> a single API. >> >> Could you please describe how this API would work? I tried some >> idea in v1, but it turned out to be quite confusing. So I decided >> to leave this logic to the users of kallsyms APIs in v2. > > If we create an alias without the suffix but only when there is only > one symbol with such a name then we have, for example: > > klp_complete_transition.lwn.123456 > klp_complete_transition [alias] > > init_once.lwn.2131221 > init_once.lwn.3443243 > init_once.lwn.4324322 > init_once.lwn.5214121 > init_once.lwn.2153121 > init_once.lwn.4342343 > > This way, it will be possible to find the static symbol > "klp_complete_transition" without the suffix via the alias. > It will have the alias because it has an unique name. > > While "init_once" symbol must always be searched with the suffix > because it is not unique. > > It looks like >99% of static symbols have unique name. Got it. The idea is to generate the alias at boot time. I think this will indeed work. IIUC, v3 of this set with Alessandro's work (maybe with some variations) should do this. Thanks, Song
Hi Alessandro, > On Aug 8, 2024, at 11:20 PM, Alessandro Carminati <alessandro.carminati@gmail.com> wrote: > > Hello, > sorry to join late at the party. > > Il giorno gio 8 ago 2024 alle ore 11:59 Petr Mladek <pmladek@suse.com> > ha scritto: >> >> On Wed 2024-08-07 20:48:48, Song Liu wrote: >>> >>> >>>> On Aug 7, 2024, at 8:33 AM, Sami Tolvanen <samitolvanen@google.com> wrote: >>>> >>>> Hi, >>>> >>>> On Wed, Aug 7, 2024 at 3:08 AM Masami Hiramatsu <mhiramat@kernel.org> wrote: >>>>> >>>>> On Wed, 7 Aug 2024 00:19:20 +0000 >>>>> Song Liu <songliubraving@meta.com> wrote: >>>>> >>>>>> Do you mean we do not want patch 3/3, but would like to keep 1/3 and part >>>>>> of 2/3 (remove the _without_suffix APIs)? If this is the case, we are >>>>>> undoing the change by Sami in [1], and thus may break some tracing tools. >>>>> >>>>> What tracing tools may be broke and why? >>>> >>>> This was a few years ago when we were first adding LTO support, but >>>> the unexpected suffixes in tracing output broke systrace in Android, >>>> presumably because the tools expected to find specific function names >>>> without suffixes. I'm not sure if systrace would still be a problem >>>> today, but other tools might still make assumptions about the function >>>> name format. At the time, we decided to filter out the suffixes in all >>>> user space visible output to avoid these issues. >>>> >>>>> For this suffix problem, I would like to add another patch to allow probing on >>>>> suffixed symbols. (It seems suffixed symbols are not available at this point) >>>>> >>>>> The problem is that the suffixed symbols maybe a "part" of the original function, >>>>> thus user has to carefully use it. >>>>> >>>>>> >>>>>> Sami, could you please share your thoughts on this? >>>>> >>>>> Sami, I would like to know what problem you have on kprobes. >>>> >>>> The reports we received back then were about registering kprobes for >>>> static functions, which obviously failed if the compiler added a >>>> suffix to the function name. This was more of a problem with ThinLTO >>>> and Clang CFI at the time because the compiler used to rename _all_ >>>> static functions, but one can obviously run into the same issue with >>>> just LTO. >>> >>> I think newer LLVM/clang no longer add suffixes to all static functions >>> with LTO and CFI. So this may not be a real issue any more? >>> >>> If we still need to allow tracing without suffix, I think the approach >>> in this patch set is correct (sort syms based on full name, >> >> Yes, we should allow to find the symbols via the full name, definitely. >> >>> remove suffixes in special APIs during lookup). >> >> Just an idea. Alternative solution would be to make make an alias >> without the suffix when there is only one symbol with the same >> name. >> >> It would be complementary with the patch adding aliases for symbols >> with the same name, see >> https://lore.kernel.org/r/20231204214635.2916691-1-alessandro.carminati@gmail.com >> >> I would allow to find the symbols with and without the suffix using >> a single API. > > kas_alias isn't handling LTO as effectively as it should. > This is something I plan to address in the next patch version. > Introducing aliases is the best approach I found to preserve current > tools behavior while adding this new feature. > While I believe it will deliver the promised benefits, there is a trade-off, > particularly affecting features like live patching, which rely on handling > duplicate symbols. > For instance, kallsyms_lookup_names typically returns the last occurrence > of a symbol when the end argument is not NULL, but introducing aliases > disrupts this behavior. Do you think with v3 of this set [1], live patching should be fine? The idea is to let kallsyms_lookup_names() do full name match, then live patching can find the right symbol with symbol name + old_sympos. Did I miss some cases? > I'm working on a solution to manage duplicate symbols, ensuring compatibility > with both LTO and kallsyms_lookup_names. Thanks, Song [1] https://lore.kernel.org/live-patching/20240807220513.3100483-1-song@kernel.org/T/#u
diff --git a/kernel/kprobes.c b/kernel/kprobes.c index e85de37d9e1e..99102283b076 100644 --- a/kernel/kprobes.c +++ b/kernel/kprobes.c @@ -70,7 +70,11 @@ static DEFINE_PER_CPU(struct kprobe *, kprobe_instance); kprobe_opcode_t * __weak kprobe_lookup_name(const char *name, unsigned int __unused) { - return ((kprobe_opcode_t *)(kallsyms_lookup_name(name))); + unsigned long addr = kallsyms_lookup_name(name); + + if (IS_ENABLED(CONFIG_LTO_CLANG) && !addr) + addr = kallsyms_lookup_name_without_suffix(name); + return ((kprobe_opcode_t *)(addr)); } /* diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c index 61a6da808203..d2ad0c561c83 100644 --- a/kernel/trace/trace_kprobe.c +++ b/kernel/trace/trace_kprobe.c @@ -203,6 +203,10 @@ unsigned long trace_kprobe_address(struct trace_kprobe *tk) if (tk->symbol) { addr = (unsigned long) kallsyms_lookup_name(trace_kprobe_symbol(tk)); + + if (IS_ENABLED(CONFIG_LTO_CLANG) && !addr) + addr = kallsyms_lookup_name_without_suffix(trace_kprobe_symbol(tk)); + if (addr) addr += tk->rp.kp.offset; } else { @@ -766,8 +770,13 @@ static unsigned int number_of_same_symbols(const char *mod, const char *func_nam { struct sym_count_ctx ctx = { .count = 0, .name = func_name }; - if (!mod) + if (!mod) { kallsyms_on_each_match_symbol(count_symbols, func_name, &ctx.count); + if (IS_ENABLED(CONFIG_LTO_CLANG) && !ctx.count) { + kallsyms_on_each_match_symbol_without_suffix( + count_symbols, func_name, &ctx.count); + } + } module_kallsyms_on_each_symbol(mod, count_mod_symbols, &ctx);
Use the new kallsyms APIs that matches symbols name with .XXX suffix. This allows userspace tools to get kprobes on the expected function name, while the actual symbol has a .llvm.<hash> suffix. This only effects kernel compile with CONFIG_LTO_CLANG. Signed-off-by: Song Liu <song@kernel.org> --- kernel/kprobes.c | 6 +++++- kernel/trace/trace_kprobe.c | 11 ++++++++++- 2 files changed, 15 insertions(+), 2 deletions(-)