| Message ID | 20240730005433.3559731-3-song@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fix kallsyms with CONFIG_LTO_CLANG | expand |
On Mon, 29 Jul 2024 17:54:32 -0700 Song Liu <song@kernel.org> wrote: > With CONFIG_LTO_CLANG=y, the compiler may add suffix to function names > to avoid duplication. This causes confusion with users of kallsyms. > On one hand, users like livepatch are required to match the symbols > exactly. On the other hand, users like kprobe would like to match to > original function names. > > Solve this by splitting kallsyms APIs. Specifically, existing APIs now > should match the symbols exactly. Add two APIs that matches the full > symbol, or only the part without .llvm.suffix. Specifically, the following > two APIs are added: > > 1. kallsyms_lookup_name_or_prefix() > 2. kallsyms_on_each_match_symbol_or_prefix() Since this API only removes the suffix, "match prefix" is a bit confusing. (this sounds like matching "foo" with "foo" and "foo_bar", but in reality, it only matches "foo" and "foo.llvm.*") What about the name below? kallsyms_lookup_name_without_suffix() kallsyms_on_each_match_symbol_without_suffix() > > These APIs will be used by kprobe. No other user need this? Thank you, > > Also cleanup some code and adjust kallsyms_selftests accordingly. > > Signed-off-by: Song Liu <song@kernel.org> > --- > include/linux/kallsyms.h | 14 +++++++ > kernel/kallsyms.c | 83 ++++++++++++++++++++++++++------------ > kernel/kallsyms_selftest.c | 22 +--------- > 3 files changed, 73 insertions(+), 46 deletions(-) > > diff --git a/include/linux/kallsyms.h b/include/linux/kallsyms.h > index c3f075e8f60c..09b2d2099107 100644 > --- a/include/linux/kallsyms.h > +++ b/include/linux/kallsyms.h > @@ -74,9 +74,12 @@ int kallsyms_on_each_symbol(int (*fn)(void *, const char *, unsigned long), > void *data); > int kallsyms_on_each_match_symbol(int (*fn)(void *, unsigned long), > const char *name, void *data); > +int kallsyms_on_each_match_symbol_or_prefix(int (*fn)(void *, unsigned long), > + const char *name, void *data); > > /* Lookup the address for a symbol. Returns 0 if not found. */ > unsigned long kallsyms_lookup_name(const char *name); > +unsigned long kallsyms_lookup_name_or_prefix(const char *name); > > extern int kallsyms_lookup_size_offset(unsigned long addr, > unsigned long *symbolsize, > @@ -104,6 +107,11 @@ static inline unsigned long kallsyms_lookup_name(const char *name) > return 0; > } > > +static inline unsigned long kallsyms_lookup_name_or_prefix(const char *name) > +{ > + return 0; > +} > + > static inline int kallsyms_lookup_size_offset(unsigned long addr, > unsigned long *symbolsize, > unsigned long *offset) > @@ -165,6 +173,12 @@ static inline int kallsyms_on_each_match_symbol(int (*fn)(void *, unsigned long) > { > return -EOPNOTSUPP; > } > + > +static inline int kallsyms_on_each_match_symbol_or_prefix(int (*fn)(void *, unsigned long), > + const char *name, void *data) > +{ > + return -EOPNOTSUPP; > +} > #endif /*CONFIG_KALLSYMS*/ > > static inline void print_ip_sym(const char *loglvl, unsigned long ip) > diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c > index fb2c77368d18..4285dd85d814 100644 > --- a/kernel/kallsyms.c > +++ b/kernel/kallsyms.c > @@ -164,9 +164,6 @@ static void cleanup_symbol_name(char *s) > { > char *res; > > - if (!IS_ENABLED(CONFIG_LTO_CLANG)) > - return; > - > /* > * LLVM appends various suffixes for local functions and variables that > * must be promoted to global scope as part of LTO. This can break > @@ -181,13 +178,13 @@ static void cleanup_symbol_name(char *s) > return; > } > > -static int compare_symbol_name(const char *name, char *namebuf) > +static int compare_symbol_name(const char *name, char *namebuf, bool exact_match) > { > - /* The kallsyms_seqs_of_names is sorted based on names after > - * cleanup_symbol_name() (see scripts/kallsyms.c) if clang lto is enabled. > - * To ensure correct bisection in kallsyms_lookup_names(), do > - * cleanup_symbol_name(namebuf) before comparing name and namebuf. > - */ > + int ret = strcmp(name, namebuf); > + > + if (exact_match || !ret) > + return ret; > + > cleanup_symbol_name(namebuf); > return strcmp(name, namebuf); > } > @@ -204,13 +201,17 @@ static unsigned int get_symbol_seq(int index) > > static int kallsyms_lookup_names(const char *name, > unsigned int *start, > - unsigned int *end) > + unsigned int *end, > + bool exact_match) > { > int ret; > int low, mid, high; > unsigned int seq, off; > char namebuf[KSYM_NAME_LEN]; > > + if (!IS_ENABLED(CONFIG_LTO_CLANG)) > + exact_match = true; > + > low = 0; > high = kallsyms_num_syms - 1; > > @@ -219,7 +220,7 @@ static int kallsyms_lookup_names(const char *name, > seq = get_symbol_seq(mid); > off = get_symbol_offset(seq); > kallsyms_expand_symbol(off, namebuf, ARRAY_SIZE(namebuf)); > - ret = compare_symbol_name(name, namebuf); > + ret = compare_symbol_name(name, namebuf, exact_match); > if (ret > 0) > low = mid + 1; > else if (ret < 0) > @@ -236,7 +237,7 @@ static int kallsyms_lookup_names(const char *name, > seq = get_symbol_seq(low - 1); > off = get_symbol_offset(seq); > kallsyms_expand_symbol(off, namebuf, ARRAY_SIZE(namebuf)); > - if (compare_symbol_name(name, namebuf)) > + if (compare_symbol_name(name, namebuf, exact_match)) > break; > low--; > } > @@ -248,7 +249,7 @@ static int kallsyms_lookup_names(const char *name, > seq = get_symbol_seq(high + 1); > off = get_symbol_offset(seq); > kallsyms_expand_symbol(off, namebuf, ARRAY_SIZE(namebuf)); > - if (compare_symbol_name(name, namebuf)) > + if (compare_symbol_name(name, namebuf, exact_match)) > break; > high++; > } > @@ -268,13 +269,35 @@ unsigned long kallsyms_lookup_name(const char *name) > if (!*name) > return 0; > > - ret = kallsyms_lookup_names(name, &i, NULL); > + ret = kallsyms_lookup_names(name, &i, NULL, true); > if (!ret) > return kallsyms_sym_address(get_symbol_seq(i)); > > return module_kallsyms_lookup_name(name); > } > > +/* > + * Lookup the address for this symbol. > + * > + * With CONFIG_LTO_CLANG=y, if there is no exact match, also try lookup > + * symbol.llvm.<hash>. > + */ > +unsigned long kallsyms_lookup_name_or_prefix(const char *name) > +{ > + unsigned long addr; > + > + addr = kallsyms_lookup_name(name); > + > + if (!addr && IS_ENABLED(CONFIG_LTO_CLANG)) { > + int ret, i; > + > + ret = kallsyms_lookup_names(name, &i, NULL, false); > + if (!ret) > + addr = kallsyms_sym_address(get_symbol_seq(i)); > + } > + return addr; > +} > + > /* > * Iterate over all symbols in vmlinux. For symbols from modules use > * module_kallsyms_on_each_symbol instead. > @@ -303,7 +326,25 @@ int kallsyms_on_each_match_symbol(int (*fn)(void *, unsigned long), > int ret; > unsigned int i, start, end; > > - ret = kallsyms_lookup_names(name, &start, &end); > + ret = kallsyms_lookup_names(name, &start, &end, true); > + if (ret) > + return 0; > + > + for (i = start; !ret && i <= end; i++) { > + ret = fn(data, kallsyms_sym_address(get_symbol_seq(i))); > + cond_resched(); > + } > + > + return ret; > +} > + > +int kallsyms_on_each_match_symbol_or_prefix(int (*fn)(void *, unsigned long), > + const char *name, void *data) > +{ > + int ret; > + unsigned int i, start, end; > + > + ret = kallsyms_lookup_names(name, &start, &end, false); > if (ret) > return 0; > > @@ -450,8 +491,6 @@ const char *kallsyms_lookup(unsigned long addr, > > int lookup_symbol_name(unsigned long addr, char *symname) > { > - int res; > - > symname[0] = '\0'; > symname[KSYM_NAME_LEN - 1] = '\0'; > > @@ -462,16 +501,10 @@ int lookup_symbol_name(unsigned long addr, char *symname) > /* Grab name */ > kallsyms_expand_symbol(get_symbol_offset(pos), > symname, KSYM_NAME_LEN); > - goto found; > + return 0; > } > /* See if it's in a module. */ > - res = lookup_module_symbol_name(addr, symname); > - if (res) > - return res; > - > -found: > - cleanup_symbol_name(symname); > - return 0; > + return lookup_module_symbol_name(addr, symname); > } > > /* Look up a kernel symbol and return it in a text buffer. */ > diff --git a/kernel/kallsyms_selftest.c b/kernel/kallsyms_selftest.c > index 2f84896a7bcb..873f7c445488 100644 > --- a/kernel/kallsyms_selftest.c > +++ b/kernel/kallsyms_selftest.c > @@ -187,31 +187,11 @@ static void test_perf_kallsyms_lookup_name(void) > stat.min, stat.max, div_u64(stat.sum, stat.real_cnt)); > } > > -static bool match_cleanup_name(const char *s, const char *name) > -{ > - char *p; > - int len; > - > - if (!IS_ENABLED(CONFIG_LTO_CLANG)) > - return false; > - > - p = strstr(s, ".llvm."); > - if (!p) > - return false; > - > - len = strlen(name); > - if (p - s != len) > - return false; > - > - return !strncmp(s, name, len); > -} > - > static int find_symbol(void *data, const char *name, unsigned long addr) > { > struct test_stat *stat = (struct test_stat *)data; > > - if (strcmp(name, stat->name) == 0 || > - (!stat->perf && match_cleanup_name(name, stat->name))) { > + if (!strcmp(name, stat->name)) { > stat->real_cnt++; > stat->addr = addr; > > -- > 2.43.0 >
Hi Masami, > On Jul 30, 2024, at 6:03 AM, Masami Hiramatsu <mhiramat@kernel.org> wrote: > > On Mon, 29 Jul 2024 17:54:32 -0700 > Song Liu <song@kernel.org> wrote: > >> With CONFIG_LTO_CLANG=y, the compiler may add suffix to function names >> to avoid duplication. This causes confusion with users of kallsyms. >> On one hand, users like livepatch are required to match the symbols >> exactly. On the other hand, users like kprobe would like to match to >> original function names. >> >> Solve this by splitting kallsyms APIs. Specifically, existing APIs now >> should match the symbols exactly. Add two APIs that matches the full >> symbol, or only the part without .llvm.suffix. Specifically, the following >> two APIs are added: >> >> 1. kallsyms_lookup_name_or_prefix() >> 2. kallsyms_on_each_match_symbol_or_prefix() > > Since this API only removes the suffix, "match prefix" is a bit confusing. > (this sounds like matching "foo" with "foo" and "foo_bar", but in reality, > it only matches "foo" and "foo.llvm.*") > What about the name below? > > kallsyms_lookup_name_without_suffix() > kallsyms_on_each_match_symbol_without_suffix() I am open to name suggestions. I named it as xx or prefix to highlight that these two APIs will try match full name first, and they only match the symbol without suffix when there is no full name match. Maybe we can call them: - kallsyms_lookup_name_or_without_suffix() - kallsyms_on_each_match_symbol_or_without_suffix() Again, I am open to any name selections here. > >> >> These APIs will be used by kprobe. > > No other user need this? AFACIT, kprobe is the only use case here. Sami, please correct me if I missed any users. More thoughts on this: I actually hope we don't need these two new APIs, as they are confusing. Modern compilers can do many things to the code (inlining, etc.). So when we are tracing a function, we are not really tracing "function in the source code". Instead, we are tracing "function in the binary". If a function is inlined, it will not show up in the binary. If a function is _partially_ inlined (inlined by some callers, but not by others), it will show up in the binary, but we won't be tracing it as it appears in the source code. Therefore, tracing functions by their names in the source code only works under certain assumptions. And these assumptions may not hold with modern compilers. Ideally, I think we cannot promise the user can use name "ping_table" to trace function "ping_table.llvm.15394922576589127018" Does this make sense? Thanks, Song [...]
On 2024/7/31 9:00, Song Liu wrote: > Hi Masami, > >> On Jul 30, 2024, at 6:03 AM, Masami Hiramatsu <mhiramat@kernel.org> wrote: >> >> On Mon, 29 Jul 2024 17:54:32 -0700 >> Song Liu <song@kernel.org> wrote: >> >>> With CONFIG_LTO_CLANG=y, the compiler may add suffix to function names >>> to avoid duplication. This causes confusion with users of kallsyms. >>> On one hand, users like livepatch are required to match the symbols >>> exactly. On the other hand, users like kprobe would like to match to >>> original function names. >>> >>> Solve this by splitting kallsyms APIs. Specifically, existing APIs now >>> should match the symbols exactly. Add two APIs that matches the full >>> symbol, or only the part without .llvm.suffix. Specifically, the following >>> two APIs are added: >>> >>> 1. kallsyms_lookup_name_or_prefix() >>> 2. kallsyms_on_each_match_symbol_or_prefix() >> >> Since this API only removes the suffix, "match prefix" is a bit confusing. >> (this sounds like matching "foo" with "foo" and "foo_bar", but in reality, >> it only matches "foo" and "foo.llvm.*") >> What about the name below? >> >> kallsyms_lookup_name_without_suffix() >> kallsyms_on_each_match_symbol_without_suffix() > > I am open to name suggestions. I named it as xx or prefix to highlight > that these two APIs will try match full name first, and they only match > the symbol without suffix when there is no full name match. > > Maybe we can call them: > - kallsyms_lookup_name_or_without_suffix() > - kallsyms_on_each_match_symbol_or_without_suffix() > > Again, I am open to any name selections here. Only static functions have suffixes. In my opinion, explicitly marking static might be a little clearer. kallsyms_lookup_static_name() kallsyms_on_each_match_static_symbol() > >> >>> >>> These APIs will be used by kprobe. >> >> No other user need this? > > AFACIT, kprobe is the only use case here. Sami, please correct > me if I missed any users. > > > More thoughts on this: > > I actually hope we don't need these two new APIs, as they are > confusing. Modern compilers can do many things to the code > (inlining, etc.). So when we are tracing a function, we are not > really tracing "function in the source code". Instead, we are > tracing "function in the binary". If a function is inlined, it > will not show up in the binary. If a function is _partially_ > inlined (inlined by some callers, but not by others), it will > show up in the binary, but we won't be tracing it as it appears > in the source code. Therefore, tracing functions by their names > in the source code only works under certain assumptions. And > these assumptions may not hold with modern compilers. Ideally, > I think we cannot promise the user can use name "ping_table" to > trace function "ping_table.llvm.15394922576589127018" > > Does this make sense? > > Thanks, > Song > > > [...] >
> On Aug 1, 2024, at 6:18 PM, Leizhen (ThunderTown) <thunder.leizhen@huaweicloud.com> wrote: > > On 2024/7/31 9:00, Song Liu wrote: >> Hi Masami, >> >>> On Jul 30, 2024, at 6:03 AM, Masami Hiramatsu <mhiramat@kernel.org> wrote: >>> >>> On Mon, 29 Jul 2024 17:54:32 -0700 >>> Song Liu <song@kernel.org> wrote: >>> >>>> With CONFIG_LTO_CLANG=y, the compiler may add suffix to function names >>>> to avoid duplication. This causes confusion with users of kallsyms. >>>> On one hand, users like livepatch are required to match the symbols >>>> exactly. On the other hand, users like kprobe would like to match to >>>> original function names. >>>> >>>> Solve this by splitting kallsyms APIs. Specifically, existing APIs now >>>> should match the symbols exactly. Add two APIs that matches the full >>>> symbol, or only the part without .llvm.suffix. Specifically, the following >>>> two APIs are added: >>>> >>>> 1. kallsyms_lookup_name_or_prefix() >>>> 2. kallsyms_on_each_match_symbol_or_prefix() >>> >>> Since this API only removes the suffix, "match prefix" is a bit confusing. >>> (this sounds like matching "foo" with "foo" and "foo_bar", but in reality, >>> it only matches "foo" and "foo.llvm.*") >>> What about the name below? >>> >>> kallsyms_lookup_name_without_suffix() >>> kallsyms_on_each_match_symbol_without_suffix() >> >> I am open to name suggestions. I named it as xx or prefix to highlight >> that these two APIs will try match full name first, and they only match >> the symbol without suffix when there is no full name match. >> >> Maybe we can call them: >> - kallsyms_lookup_name_or_without_suffix() >> - kallsyms_on_each_match_symbol_or_without_suffix() >> >> Again, I am open to any name selections here. > > Only static functions have suffixes. In my opinion, explicitly marking static > might be a little clearer. > kallsyms_lookup_static_name() > kallsyms_on_each_match_static_symbol() While these names are shorter, I think they are more confusing. Not all folks know that only static functions can have suffixes. Maybe we should not hide the "try match full name first first" in the API, and let the users handle it. Then, we can safely call the new APIs *_without_suffix(), as Masami suggested. If there is no objections, I will send v2 based on this direction. Thanks, Song
On 2024/8/2 11:45, Song Liu wrote: > > >> On Aug 1, 2024, at 6:18 PM, Leizhen (ThunderTown) <thunder.leizhen@huaweicloud.com> wrote: >> >> On 2024/7/31 9:00, Song Liu wrote: >>> Hi Masami, >>> >>>> On Jul 30, 2024, at 6:03 AM, Masami Hiramatsu <mhiramat@kernel.org> wrote: >>>> >>>> On Mon, 29 Jul 2024 17:54:32 -0700 >>>> Song Liu <song@kernel.org> wrote: >>>> >>>>> With CONFIG_LTO_CLANG=y, the compiler may add suffix to function names >>>>> to avoid duplication. This causes confusion with users of kallsyms. >>>>> On one hand, users like livepatch are required to match the symbols >>>>> exactly. On the other hand, users like kprobe would like to match to >>>>> original function names. >>>>> >>>>> Solve this by splitting kallsyms APIs. Specifically, existing APIs now >>>>> should match the symbols exactly. Add two APIs that matches the full >>>>> symbol, or only the part without .llvm.suffix. Specifically, the following >>>>> two APIs are added: >>>>> >>>>> 1. kallsyms_lookup_name_or_prefix() >>>>> 2. kallsyms_on_each_match_symbol_or_prefix() >>>> >>>> Since this API only removes the suffix, "match prefix" is a bit confusing. >>>> (this sounds like matching "foo" with "foo" and "foo_bar", but in reality, >>>> it only matches "foo" and "foo.llvm.*") >>>> What about the name below? >>>> >>>> kallsyms_lookup_name_without_suffix() >>>> kallsyms_on_each_match_symbol_without_suffix() >>> >>> I am open to name suggestions. I named it as xx or prefix to highlight >>> that these two APIs will try match full name first, and they only match >>> the symbol without suffix when there is no full name match. >>> >>> Maybe we can call them: >>> - kallsyms_lookup_name_or_without_suffix() >>> - kallsyms_on_each_match_symbol_or_without_suffix() >>> >>> Again, I am open to any name selections here. >> >> Only static functions have suffixes. In my opinion, explicitly marking static >> might be a little clearer. >> kallsyms_lookup_static_name() >> kallsyms_on_each_match_static_symbol() > > While these names are shorter, I think they are more confusing. Not all > folks know that only static functions can have suffixes. > > Maybe we should not hide the "try match full name first first" in the > API, and let the users handle it. Then, we can safely call the new APIs > *_without_suffix(), as Masami suggested. Yes, that would be clearer. > > If there is no objections, I will send v2 based on this direction. > > Thanks, > Song >
Hi, Sorry for late reply. On Fri, 2 Aug 2024 03:45:42 +0000 Song Liu <songliubraving@meta.com> wrote: > > > > On Aug 1, 2024, at 6:18 PM, Leizhen (ThunderTown) <thunder.leizhen@huaweicloud.com> wrote: > > > > On 2024/7/31 9:00, Song Liu wrote: > >> Hi Masami, > >> > >>> On Jul 30, 2024, at 6:03 AM, Masami Hiramatsu <mhiramat@kernel.org> wrote: > >>> > >>> On Mon, 29 Jul 2024 17:54:32 -0700 > >>> Song Liu <song@kernel.org> wrote: > >>> > >>>> With CONFIG_LTO_CLANG=y, the compiler may add suffix to function names > >>>> to avoid duplication. This causes confusion with users of kallsyms. > >>>> On one hand, users like livepatch are required to match the symbols > >>>> exactly. On the other hand, users like kprobe would like to match to > >>>> original function names. > >>>> > >>>> Solve this by splitting kallsyms APIs. Specifically, existing APIs now > >>>> should match the symbols exactly. Add two APIs that matches the full > >>>> symbol, or only the part without .llvm.suffix. Specifically, the following > >>>> two APIs are added: > >>>> > >>>> 1. kallsyms_lookup_name_or_prefix() > >>>> 2. kallsyms_on_each_match_symbol_or_prefix() > >>> > >>> Since this API only removes the suffix, "match prefix" is a bit confusing. > >>> (this sounds like matching "foo" with "foo" and "foo_bar", but in reality, > >>> it only matches "foo" and "foo.llvm.*") > >>> What about the name below? > >>> > >>> kallsyms_lookup_name_without_suffix() > >>> kallsyms_on_each_match_symbol_without_suffix() > >> > >> I am open to name suggestions. I named it as xx or prefix to highlight > >> that these two APIs will try match full name first, and they only match > >> the symbol without suffix when there is no full name match. > >> > >> Maybe we can call them: > >> - kallsyms_lookup_name_or_without_suffix() > >> - kallsyms_on_each_match_symbol_or_without_suffix() No, I think "_or_without_suffix" is a bit complicated. If we have 0x1234 "foo.llvm.XXX" 0x2356 "bar" and user calls kallsyms_lookup_name_without_suffix("bar"); This returns "bar"'s address(0x2356). Also, kallsyms_lookup_name_without_suffix("foo"); this will returns 0x1234. These commonly just ignore the suffix. The difference of kallsyms_lookup_name() is that point. > >> > >> Again, I am open to any name selections here. > > > > Only static functions have suffixes. In my opinion, explicitly marking static > > might be a little clearer. > > kallsyms_lookup_static_name() > > kallsyms_on_each_match_static_symbol() But this is not correctly check the symbol is static or not. If you check the symbol is really static, it is OK. (return NULL if the symbol is global.) Thank you, > > While these names are shorter, I think they are more confusing. Not all > folks know that only static functions can have suffixes. > > Maybe we should not hide the "try match full name first first" in the > API, and let the users handle it. Then, we can safely call the new APIs > *_without_suffix(), as Masami suggested. > > If there is no objections, I will send v2 based on this direction. > > Thanks, > Song >
On Wed 2024-07-31 01:00:34, Song Liu wrote: > Hi Masami, > > > On Jul 30, 2024, at 6:03 AM, Masami Hiramatsu <mhiramat@kernel.org> wrote: > > > > On Mon, 29 Jul 2024 17:54:32 -0700 > > Song Liu <song@kernel.org> wrote: > > > >> With CONFIG_LTO_CLANG=y, the compiler may add suffix to function names > >> to avoid duplication. This causes confusion with users of kallsyms. > >> On one hand, users like livepatch are required to match the symbols > >> exactly. On the other hand, users like kprobe would like to match to > >> original function names. > >> > >> Solve this by splitting kallsyms APIs. Specifically, existing APIs now > >> should match the symbols exactly. Add two APIs that matches the full > >> symbol, or only the part without .llvm.suffix. Specifically, the following > >> two APIs are added: > >> > >> 1. kallsyms_lookup_name_or_prefix() > >> 2. kallsyms_on_each_match_symbol_or_prefix() > > > > Since this API only removes the suffix, "match prefix" is a bit confusing. > > (this sounds like matching "foo" with "foo" and "foo_bar", but in reality, > > it only matches "foo" and "foo.llvm.*") > > What about the name below? > > > > kallsyms_lookup_name_without_suffix() > > kallsyms_on_each_match_symbol_without_suffix() This looks like the best variant to me. A reasonable compromise. > >> These APIs will be used by kprobe. > > > > No other user need this? > > AFACIT, kprobe is the only use case here. Sami, please correct > me if I missed any users. > > > More thoughts on this: > > I actually hope we don't need these two new APIs, as they are > confusing. Modern compilers can do many things to the code > (inlining, etc.). So when we are tracing a function, we are not > really tracing "function in the source code". Instead, we are > tracing "function in the binary". If a function is inlined, it > will not show up in the binary. If a function is _partially_ > inlined (inlined by some callers, but not by others), it will > show up in the binary, but we won't be tracing it as it appears > in the source code. Therefore, tracing functions by their names > in the source code only works under certain assumptions. And > these assumptions may not hold with modern compilers. Ideally, > I think we cannot promise the user can use name "ping_table" to > trace function "ping_table.llvm.15394922576589127018" > > Does this make sense? IMHO, it depends on the use case. Let's keep "ping_table/" as an example. Why people would want to trace this function? There might be various reasons, for example: 1. ping_table.llvm.15394922576589127018 appeared in a backtrace 2. ping_table.llvm.15394922576589127018 appeared in a histogram 3. ping_table looks interesting when reading code sources 4. ping_table need to be monitored on all systems because of security/performance. The full name "ping_table.llvm.15394922576589127018" is perfectly fine in the 1st and 2nd scenario. People knew this name already before they start thinking about tracing. The short name is more practical in 3rd and 4th scenario. Especially, when there is only one static symbol with this short name. Otherwise, the user would need an extra step to find the full name. The full name is even more problematic for system monitors. These applications might need to probe particular symbols. They might have hard times when the symbol is: <symbol_name_from_sources>.<random_suffix_generated_by_compiler> They will have to deal with it. But it means that every such tool would need an extra (non-trivial) code for this. Every tool would try its own approach => a lot of problems. IMHO, the two APIs could make the life easier. Well, even kprobe might need two APIs to allow probing by full name or without the suffix. Best Regards, Petr
Hi Petr, > On Aug 2, 2024, at 8:45 AM, Petr Mladek <pmladek@suse.com> wrote: [...] > > IMHO, it depends on the use case. Let's keep "ping_table/" > as an example. Why people would want to trace this function? > There might be various reasons, for example: > > 1. ping_table.llvm.15394922576589127018 appeared in a backtrace > > 2. ping_table.llvm.15394922576589127018 appeared in a histogram > > 3. ping_table looks interesting when reading code sources > > 4. ping_table need to be monitored on all systems because > of security/performance. > > The full name "ping_table.llvm.15394922576589127018" is perfectly > fine in the 1st and 2nd scenario. People knew this name already > before they start thinking about tracing. > > The short name is more practical in 3rd and 4th scenario. Especially, > when there is only one static symbol with this short name. Otherwise, > the user would need an extra step to find the full name. > > The full name is even more problematic for system monitors. These > applications might need to probe particular symbols. They might > have hard times when the symbol is: > > <symbol_name_from_sources>.<random_suffix_generated_by_compiler> > > They will have to deal with it. But it means that every such tool > would need an extra (non-trivial) code for this. Every tool would > try its own approach => a lot of problems. > > IMHO, the two APIs could make the life easier. > > Well, even kprobe might need two APIs to allow probing by > full name or without the suffix. The problem is, with potential partial inlining by modern compilers, tracing "symbol name from sources" is not accurate. In our production kernels, we have to add some explicit "noline" to make sure we can trace these functions reliably. Of course, this issue exists without random suffix: any function could be partially inlined. However, allowing tracing without the suffix seems to hint that tracing with "symbol name from sources" is valid, which is not really true. At the moment, I have no objections to keep the _without_suffix APIs. But for long term, I still think we need to set clear expectations for the users: tracing symbols from sources is not reliable. Thanks, Song
> On Jul 29, 2024, at 5:54 PM, Song Liu <song@kernel.org> wrote: > > With CONFIG_LTO_CLANG=y, the compiler may add suffix to function names > to avoid duplication. This causes confusion with users of kallsyms. > On one hand, users like livepatch are required to match the symbols > exactly. On the other hand, users like kprobe would like to match to > original function names. > > Solve this by splitting kallsyms APIs. Specifically, existing APIs now > should match the symbols exactly. Add two APIs that matches the full > symbol, or only the part without .llvm.suffix. Specifically, the following > two APIs are added: > > 1. kallsyms_lookup_name_or_prefix() > 2. kallsyms_on_each_match_symbol_or_prefix() > > These APIs will be used by kprobe. > > Also cleanup some code and adjust kallsyms_selftests accordingly. > > Signed-off-by: Song Liu <song@kernel.org> Actually, if we only remove .llvm.<hash> suffix, but keep other .XXX suffix, the *_without_suffx APIs will have the same issue Yonghong tried to fix in commit 33f0467fe06934d5e4ea6e24ce2b9c65ce618e26: binary search with symbols - .llvm.<hash> suffix is not correct. (Please see the commit log of 33f0467fe06934d5e4ea6e24ce2b9c65ce618e26 for more details.) I am updating the code to remove all .XXX suffix. This design will not have this issue. Thanks, Song
On Fri, 2 Aug 2024 17:09:12 +0000 Song Liu <songliubraving@meta.com> wrote: > Hi Petr, > > > On Aug 2, 2024, at 8:45 AM, Petr Mladek <pmladek@suse.com> wrote: > > [...] > > > > > IMHO, it depends on the use case. Let's keep "ping_table/" > > as an example. Why people would want to trace this function? > > There might be various reasons, for example: > > > > 1. ping_table.llvm.15394922576589127018 appeared in a backtrace > > > > 2. ping_table.llvm.15394922576589127018 appeared in a histogram > > > > 3. ping_table looks interesting when reading code sources > > > > 4. ping_table need to be monitored on all systems because > > of security/performance. > > > > The full name "ping_table.llvm.15394922576589127018" is perfectly > > fine in the 1st and 2nd scenario. People knew this name already > > before they start thinking about tracing. > > > > The short name is more practical in 3rd and 4th scenario. Especially, > > when there is only one static symbol with this short name. Otherwise, > > the user would need an extra step to find the full name. > > > > The full name is even more problematic for system monitors. These > > applications might need to probe particular symbols. They might > > have hard times when the symbol is: > > > > <symbol_name_from_sources>.<random_suffix_generated_by_compiler> > > > > They will have to deal with it. But it means that every such tool > > would need an extra (non-trivial) code for this. Every tool would > > try its own approach => a lot of problems. > > > > IMHO, the two APIs could make the life easier. > > > > Well, even kprobe might need two APIs to allow probing by > > full name or without the suffix. > > The problem is, with potential partial inlining by modern compilers, > tracing "symbol name from sources" is not accurate. In our production > kernels, we have to add some explicit "noline" to make sure we can > trace these functions reliably. > > Of course, this issue exists without random suffix: any function > could be partially inlined. However, allowing tracing without the > suffix seems to hint that tracing with "symbol name from sources" > is valid, which is not really true. > > At the moment, I have no objections to keep the _without_suffix > APIs. But for long term, I still think we need to set clear > expectations for the users: tracing symbols from sources is not > reliable. OK, I understand this part. I agree the problem. Even if the symbol is unique on kallsyms (without suffix), it may have a suffix and is not correct function entry. I think to solve this issue, we need a better DWARF, or add a symbol suffix like; https://lkml.org/lkml/2023/12/4/1535 Thank you, > > Thanks, > Song >
diff --git a/include/linux/kallsyms.h b/include/linux/kallsyms.h index c3f075e8f60c..09b2d2099107 100644 --- a/include/linux/kallsyms.h +++ b/include/linux/kallsyms.h @@ -74,9 +74,12 @@ int kallsyms_on_each_symbol(int (*fn)(void *, const char *, unsigned long), void *data); int kallsyms_on_each_match_symbol(int (*fn)(void *, unsigned long), const char *name, void *data); +int kallsyms_on_each_match_symbol_or_prefix(int (*fn)(void *, unsigned long), + const char *name, void *data); /* Lookup the address for a symbol. Returns 0 if not found. */ unsigned long kallsyms_lookup_name(const char *name); +unsigned long kallsyms_lookup_name_or_prefix(const char *name); extern int kallsyms_lookup_size_offset(unsigned long addr, unsigned long *symbolsize, @@ -104,6 +107,11 @@ static inline unsigned long kallsyms_lookup_name(const char *name) return 0; } +static inline unsigned long kallsyms_lookup_name_or_prefix(const char *name) +{ + return 0; +} + static inline int kallsyms_lookup_size_offset(unsigned long addr, unsigned long *symbolsize, unsigned long *offset) @@ -165,6 +173,12 @@ static inline int kallsyms_on_each_match_symbol(int (*fn)(void *, unsigned long) { return -EOPNOTSUPP; } + +static inline int kallsyms_on_each_match_symbol_or_prefix(int (*fn)(void *, unsigned long), + const char *name, void *data) +{ + return -EOPNOTSUPP; +} #endif /*CONFIG_KALLSYMS*/ static inline void print_ip_sym(const char *loglvl, unsigned long ip) diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c index fb2c77368d18..4285dd85d814 100644 --- a/kernel/kallsyms.c +++ b/kernel/kallsyms.c @@ -164,9 +164,6 @@ static void cleanup_symbol_name(char *s) { char *res; - if (!IS_ENABLED(CONFIG_LTO_CLANG)) - return; - /* * LLVM appends various suffixes for local functions and variables that * must be promoted to global scope as part of LTO. This can break @@ -181,13 +178,13 @@ static void cleanup_symbol_name(char *s) return; } -static int compare_symbol_name(const char *name, char *namebuf) +static int compare_symbol_name(const char *name, char *namebuf, bool exact_match) { - /* The kallsyms_seqs_of_names is sorted based on names after - * cleanup_symbol_name() (see scripts/kallsyms.c) if clang lto is enabled. - * To ensure correct bisection in kallsyms_lookup_names(), do - * cleanup_symbol_name(namebuf) before comparing name and namebuf. - */ + int ret = strcmp(name, namebuf); + + if (exact_match || !ret) + return ret; + cleanup_symbol_name(namebuf); return strcmp(name, namebuf); } @@ -204,13 +201,17 @@ static unsigned int get_symbol_seq(int index) static int kallsyms_lookup_names(const char *name, unsigned int *start, - unsigned int *end) + unsigned int *end, + bool exact_match) { int ret; int low, mid, high; unsigned int seq, off; char namebuf[KSYM_NAME_LEN]; + if (!IS_ENABLED(CONFIG_LTO_CLANG)) + exact_match = true; + low = 0; high = kallsyms_num_syms - 1; @@ -219,7 +220,7 @@ static int kallsyms_lookup_names(const char *name, seq = get_symbol_seq(mid); off = get_symbol_offset(seq); kallsyms_expand_symbol(off, namebuf, ARRAY_SIZE(namebuf)); - ret = compare_symbol_name(name, namebuf); + ret = compare_symbol_name(name, namebuf, exact_match); if (ret > 0) low = mid + 1; else if (ret < 0) @@ -236,7 +237,7 @@ static int kallsyms_lookup_names(const char *name, seq = get_symbol_seq(low - 1); off = get_symbol_offset(seq); kallsyms_expand_symbol(off, namebuf, ARRAY_SIZE(namebuf)); - if (compare_symbol_name(name, namebuf)) + if (compare_symbol_name(name, namebuf, exact_match)) break; low--; } @@ -248,7 +249,7 @@ static int kallsyms_lookup_names(const char *name, seq = get_symbol_seq(high + 1); off = get_symbol_offset(seq); kallsyms_expand_symbol(off, namebuf, ARRAY_SIZE(namebuf)); - if (compare_symbol_name(name, namebuf)) + if (compare_symbol_name(name, namebuf, exact_match)) break; high++; } @@ -268,13 +269,35 @@ unsigned long kallsyms_lookup_name(const char *name) if (!*name) return 0; - ret = kallsyms_lookup_names(name, &i, NULL); + ret = kallsyms_lookup_names(name, &i, NULL, true); if (!ret) return kallsyms_sym_address(get_symbol_seq(i)); return module_kallsyms_lookup_name(name); } +/* + * Lookup the address for this symbol. + * + * With CONFIG_LTO_CLANG=y, if there is no exact match, also try lookup + * symbol.llvm.<hash>. + */ +unsigned long kallsyms_lookup_name_or_prefix(const char *name) +{ + unsigned long addr; + + addr = kallsyms_lookup_name(name); + + if (!addr && IS_ENABLED(CONFIG_LTO_CLANG)) { + int ret, i; + + ret = kallsyms_lookup_names(name, &i, NULL, false); + if (!ret) + addr = kallsyms_sym_address(get_symbol_seq(i)); + } + return addr; +} + /* * Iterate over all symbols in vmlinux. For symbols from modules use * module_kallsyms_on_each_symbol instead. @@ -303,7 +326,25 @@ int kallsyms_on_each_match_symbol(int (*fn)(void *, unsigned long), int ret; unsigned int i, start, end; - ret = kallsyms_lookup_names(name, &start, &end); + ret = kallsyms_lookup_names(name, &start, &end, true); + if (ret) + return 0; + + for (i = start; !ret && i <= end; i++) { + ret = fn(data, kallsyms_sym_address(get_symbol_seq(i))); + cond_resched(); + } + + return ret; +} + +int kallsyms_on_each_match_symbol_or_prefix(int (*fn)(void *, unsigned long), + const char *name, void *data) +{ + int ret; + unsigned int i, start, end; + + ret = kallsyms_lookup_names(name, &start, &end, false); if (ret) return 0; @@ -450,8 +491,6 @@ const char *kallsyms_lookup(unsigned long addr, int lookup_symbol_name(unsigned long addr, char *symname) { - int res; - symname[0] = '\0'; symname[KSYM_NAME_LEN - 1] = '\0'; @@ -462,16 +501,10 @@ int lookup_symbol_name(unsigned long addr, char *symname) /* Grab name */ kallsyms_expand_symbol(get_symbol_offset(pos), symname, KSYM_NAME_LEN); - goto found; + return 0; } /* See if it's in a module. */ - res = lookup_module_symbol_name(addr, symname); - if (res) - return res; - -found: - cleanup_symbol_name(symname); - return 0; + return lookup_module_symbol_name(addr, symname); } /* Look up a kernel symbol and return it in a text buffer. */ diff --git a/kernel/kallsyms_selftest.c b/kernel/kallsyms_selftest.c index 2f84896a7bcb..873f7c445488 100644 --- a/kernel/kallsyms_selftest.c +++ b/kernel/kallsyms_selftest.c @@ -187,31 +187,11 @@ static void test_perf_kallsyms_lookup_name(void) stat.min, stat.max, div_u64(stat.sum, stat.real_cnt)); } -static bool match_cleanup_name(const char *s, const char *name) -{ - char *p; - int len; - - if (!IS_ENABLED(CONFIG_LTO_CLANG)) - return false; - - p = strstr(s, ".llvm."); - if (!p) - return false; - - len = strlen(name); - if (p - s != len) - return false; - - return !strncmp(s, name, len); -} - static int find_symbol(void *data, const char *name, unsigned long addr) { struct test_stat *stat = (struct test_stat *)data; - if (strcmp(name, stat->name) == 0 || - (!stat->perf && match_cleanup_name(name, stat->name))) { + if (!strcmp(name, stat->name)) { stat->real_cnt++; stat->addr = addr;
With CONFIG_LTO_CLANG=y, the compiler may add suffix to function names to avoid duplication. This causes confusion with users of kallsyms. On one hand, users like livepatch are required to match the symbols exactly. On the other hand, users like kprobe would like to match to original function names. Solve this by splitting kallsyms APIs. Specifically, existing APIs now should match the symbols exactly. Add two APIs that matches the full symbol, or only the part without .llvm.suffix. Specifically, the following two APIs are added: 1. kallsyms_lookup_name_or_prefix() 2. kallsyms_on_each_match_symbol_or_prefix() These APIs will be used by kprobe. Also cleanup some code and adjust kallsyms_selftests accordingly. Signed-off-by: Song Liu <song@kernel.org> --- include/linux/kallsyms.h | 14 +++++++ kernel/kallsyms.c | 83 ++++++++++++++++++++++++++------------ kernel/kallsyms_selftest.c | 22 +--------- 3 files changed, 73 insertions(+), 46 deletions(-)