[PATCH v2 0/3] Fix kallsyms with CONFIG_LTO_CLANG

[PATCH v2 0/3] Fix kallsyms with CONFIG_LTO_CLANG

[Song Liu]

With CONFIG_LTO_CLANG, the compiler/linker adds .llvm.<hash> suffix to
local symbols to avoid duplications. Existing scripts/kallsyms sorts
symbols without .llvm.<hash> suffix. However, this causes quite some
issues later on. Some users of kallsyms, such as livepatch, have to match
symbols exactly; while other users, such as kprobe, would match symbols
without the suffix.

Address this by sorting full symbols at build time, and split kallsyms
APIs to explicitly match full symbols or without suffix. Specifically,
exiting APIs will match symbols exactly. Two new APIs are added to match
symbols with suffix. Use the new APIs in tracing/kprobes.


Changes v1 => v2:
1. Update the APIs to remove all .XXX suffixes (v1 only removes .llvm.*).
2. Rename the APIs as *_without_suffix. (Masami Hiramatsu)
3. Fix another user from kprobe. (Masami Hiramatsu)
4. Add tests for the new APIs in kallsyms_selftests.

v1: https://lore.kernel.org/live-patching/20240730005433.3559731-1-song@kernel.org/T/#u

Song Liu (3):
  kallsyms: Do not cleanup .llvm.<hash> suffix before sorting symbols
  kallsyms: Add APIs to match symbol without .XXXX suffix.
  tracing/kprobes: Use APIs that matches symbols without .XXX suffix

 include/linux/kallsyms.h    | 14 ++++++
 kernel/kallsyms.c           | 88 +++++++++++++++++++++++++------------
 kernel/kallsyms_selftest.c  | 75 ++++++++++++++++++++++---------
 kernel/kprobes.c            |  6 ++-
 kernel/trace/trace_kprobe.c | 11 ++++-
 scripts/kallsyms.c          | 31 +------------
 scripts/link-vmlinux.sh     |  4 --
 7 files changed, 145 insertions(+), 84 deletions(-)

--
2.43.0

[PATCH v2 1/3] kallsyms: Do not cleanup .llvm.<hash> suffix before sorting symbols

[Song Liu]

Cleaning up the symbols causes various issues afterwards. Let's sort
the list based on original name and handle suffix later during the
symbol lookup.

Signed-off-by: Song Liu <song@kernel.org>
---
 scripts/kallsyms.c      | 31 ++-----------------------------
 scripts/link-vmlinux.sh |  4 ----
 2 files changed, 2 insertions(+), 33 deletions(-)

diff --git a/scripts/kallsyms.c b/scripts/kallsyms.c
index 0ed873491bf5..123dab0572f8 100644
--- a/scripts/kallsyms.c
+++ b/scripts/kallsyms.c
@@ -5,8 +5,7 @@
  * This software may be used and distributed according to the terms
  * of the GNU General Public License, incorporated herein by reference.
  *
- * Usage: kallsyms [--all-symbols] [--absolute-percpu]
- *                         [--lto-clang] in.map > out.S
+ * Usage: kallsyms [--all-symbols] [--absolute-percpu]  in.map > out.S
  *
  *      Table compression uses all the unused char codes on the symbols and
  *  maps these to the most used substrings (tokens). For instance, it might
@@ -62,7 +61,6 @@ static struct sym_entry **table;
 static unsigned int table_size, table_cnt;
 static int all_symbols;
 static int absolute_percpu;
-static int lto_clang;
 
 static int token_profit[0x10000];
 
@@ -73,8 +71,7 @@ static unsigned char best_table_len[256];
 
 static void usage(void)
 {
-	fprintf(stderr, "Usage: kallsyms [--all-symbols] [--absolute-percpu] "
-			"[--lto-clang] in.map > out.S\n");
+	fprintf(stderr, "Usage: kallsyms [--all-symbols] [--absolute-percpu] in.map > out.S\n");
 	exit(1);
 }
 
@@ -344,25 +341,6 @@ static bool symbol_absolute(const struct sym_entry *s)
 	return s->percpu_absolute;
 }
 
-static void cleanup_symbol_name(char *s)
-{
-	char *p;
-
-	/*
-	 * ASCII[.]   = 2e
-	 * ASCII[0-9] = 30,39
-	 * ASCII[A-Z] = 41,5a
-	 * ASCII[_]   = 5f
-	 * ASCII[a-z] = 61,7a
-	 *
-	 * As above, replacing the first '.' in ".llvm." with '\0' does not
-	 * affect the main sorting, but it helps us with subsorting.
-	 */
-	p = strstr(s, ".llvm.");
-	if (p)
-		*p = '\0';
-}
-
 static int compare_names(const void *a, const void *b)
 {
 	int ret;
@@ -526,10 +504,6 @@ static void write_src(void)
 	output_address(relative_base);
 	printf("\n");
 
-	if (lto_clang)
-		for (i = 0; i < table_cnt; i++)
-			cleanup_symbol_name((char *)table[i]->sym);
-
 	sort_symbols_by_name();
 	output_label("kallsyms_seqs_of_names");
 	for (i = 0; i < table_cnt; i++)
@@ -807,7 +781,6 @@ int main(int argc, char **argv)
 		static const struct option long_options[] = {
 			{"all-symbols",     no_argument, &all_symbols,     1},
 			{"absolute-percpu", no_argument, &absolute_percpu, 1},
-			{"lto-clang",       no_argument, &lto_clang,       1},
 			{},
 		};
 
diff --git a/scripts/link-vmlinux.sh b/scripts/link-vmlinux.sh
index f7b2503cdba9..22d0bc843986 100755
--- a/scripts/link-vmlinux.sh
+++ b/scripts/link-vmlinux.sh
@@ -156,10 +156,6 @@ kallsyms()
 		kallsymopt="${kallsymopt} --absolute-percpu"
 	fi
 
-	if is_enabled CONFIG_LTO_CLANG; then
-		kallsymopt="${kallsymopt} --lto-clang"
-	fi
-
 	info KSYMS "${2}.S"
 	scripts/kallsyms ${kallsymopt} "${1}" > "${2}.S"
 
-- 
2.43.0

[PATCH v2 2/3] kallsyms: Add APIs to match symbol without .XXXX suffix.

[Song Liu]

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 match only the part
without .XXXX suffix. Specifically, the following two APIs are added.

1. kallsyms_lookup_name_without_suffix()
2. kallsyms_on_each_match_symbol_without_suffix()

These APIs will be used by kprobe.

Also cleanup some code and update kallsyms_selftests accordingly.

Signed-off-by: Song Liu <song@kernel.org>
---
 include/linux/kallsyms.h   | 14 ++++++
 kernel/kallsyms.c          | 88 ++++++++++++++++++++++++++------------
 kernel/kallsyms_selftest.c | 75 +++++++++++++++++++++++---------
 3 files changed, 128 insertions(+), 49 deletions(-)

diff --git a/include/linux/kallsyms.h b/include/linux/kallsyms.h
index c3f075e8f60c..9ef2a944a993 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_without_suffix(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_without_suffix(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_without_suffix(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_without_suffix(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..64fdff6cde85 100644
--- a/kernel/kallsyms.c
+++ b/kernel/kallsyms.c
@@ -164,30 +164,27 @@ 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
 	 * hooking of static functions with kprobes. '.' is not a valid
-	 * character in an identifier in C. Suffixes only in LLVM LTO observed:
-	 * - foo.llvm.[0-9a-f]+
+	 * character in an identifier in C, so we can just remove the
+	 * suffix.
 	 */
-	res = strstr(s, ".llvm.");
+	res = strstr(s, ".");
 	if (res)
 		*res = '\0';
 
 	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,7 +269,28 @@ 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.
+ * Remove .XXX suffix from the symbole before comparing against
+ * the name to lookup.
+ */
+unsigned long kallsyms_lookup_name_without_suffix(const char *name)
+{
+	int ret;
+	unsigned int i;
+
+	/* Skip the search for empty string. */
+	if (!*name)
+		return 0;
+
+	ret = kallsyms_lookup_names(name, &i, NULL, false);
 	if (!ret)
 		return kallsyms_sym_address(get_symbol_seq(i));
 
@@ -303,7 +325,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_without_suffix(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 +490,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 +500,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..929270f4ed55 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;
 
@@ -327,12 +307,28 @@ static int test_kallsyms_basic_function(void)
 		}
 	}
 
+	prefix = "kallsyms_on_each_match_symbol() for";
+	for (i = 0; i < ARRAY_SIZE(test_items); i++) {
+		memset(stat, 0, sizeof(*stat));
+		stat->max = INT_MAX;
+		stat->name = test_items[i].name;
+		kallsyms_on_each_match_symbol(match_symbol, test_items[i].name, stat);
+		if (stat->addr != test_items[i].addr || stat->real_cnt != 1) {
+			nr_failed++;
+			pr_info("%s %s failed: count=%d, addr=%lx, expect %lx\n",
+				prefix, test_items[i].name,
+				stat->real_cnt, stat->addr, test_items[i].addr);
+		}
+	}
+
 	if (nr_failed) {
 		kfree(stat);
 		return -ESRCH;
 	}
 
 	for (i = 0; i < kallsyms_num_syms; i++) {
+		char *p;
+
 		addr = kallsyms_sym_address(i);
 		if (!is_ksym_addr(addr))
 			continue;
@@ -415,6 +411,43 @@ static int test_kallsyms_basic_function(void)
 				goto failed;
 			}
 		}
+
+		if (IS_ENABLED(CONFIG_LTO_CLANG))
+			continue;
+
+		p = strstr(namebuf, ".");
+
+		/*
+		 * If the symbol contains a "." and it not started with
+		 * ".", test the no suffix lookup.
+		 */
+		if (!p || namebuf[0] != '.')
+			continue;
+
+		*p = '\0';
+
+		/*
+		 * kallsyms_lookup_name_without_suffix should return a
+		 * value, but it may not match the address of this symbol,
+		 * as the name without suffix may not be unique.
+		 */
+		addr = kallsyms_lookup_name_without_suffix(namebuf);
+		if (!addr) {
+			pr_info("%s: kallsyms_lookup_name_without_suffix cannot find the symbol\n",
+				namebuf);
+			goto failed;
+		}
+
+		memset(stat, 0, sizeof(*stat));
+		stat->max = INT_MAX;
+		stat->name = namebuf;
+		kallsyms_on_each_match_symbol_without_suffix(
+			match_symbol, namebuf, stat);
+		if (!stat->real_cnt) {
+			pr_info("%s: kallsyms_on_each_match_symbol_without_suffix cannot find the symbol\n",
+				namebuf);
+			goto failed;
+		}
 	}
 
 	kfree(stat);
-- 
2.43.0

[PATCH v2 3/3] tracing/kprobes: Use APIs that matches symbols without .XXX suffix

[Song Liu]

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));
+
 		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);
 
-- 
2.43.0

Re: [PATCH v2 2/3] kallsyms: Add APIs to match symbol without .XXXX suffix.

[Masami Hiramatsu]

On Fri,  2 Aug 2024 14:08:34 -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 match only the part
> without .XXXX suffix. Specifically, the following two APIs are added.
> 
> 1. kallsyms_lookup_name_without_suffix()
> 2. kallsyms_on_each_match_symbol_without_suffix()
> 
> These APIs will be used by kprobe.
> 
> Also cleanup some code and update kallsyms_selftests accordingly.
> 
> Signed-off-by: Song Liu <song@kernel.org>

Looks good to me, but I have a nitpick. 


> --- a/kernel/kallsyms.c
> +++ b/kernel/kallsyms.c
> @@ -164,30 +164,27 @@ 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
>  	 * hooking of static functions with kprobes. '.' is not a valid
> -	 * character in an identifier in C. Suffixes only in LLVM LTO observed:
> -	 * - foo.llvm.[0-9a-f]+
> +	 * character in an identifier in C, so we can just remove the
> +	 * suffix.
>  	 */
> -	res = strstr(s, ".llvm.");
> +	res = strstr(s, ".");

nit: "strchr(s, '.')" ?

Anyway,

Reviewed-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>

Thank you,


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: [PATCH v2 2/3] kallsyms: Add APIs to match symbol without .XXXX suffix.

[Song Liu]

Hi Masami,

On Mon, Aug 5, 2024 at 6:45 AM Masami Hiramatsu <mhiramat@kernel.org> wrote:
>
> On Fri,  2 Aug 2024 14:08:34 -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 match only the part
> > without .XXXX suffix. Specifically, the following two APIs are added.
> >
> > 1. kallsyms_lookup_name_without_suffix()
> > 2. kallsyms_on_each_match_symbol_without_suffix()
> >
> > These APIs will be used by kprobe.
> >
> > Also cleanup some code and update kallsyms_selftests accordingly.
> >
> > Signed-off-by: Song Liu <song@kernel.org>
>
> Looks good to me, but I have a nitpick.
>
>
> > --- a/kernel/kallsyms.c
> > +++ b/kernel/kallsyms.c
> > @@ -164,30 +164,27 @@ 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
> >        * hooking of static functions with kprobes. '.' is not a valid
> > -      * character in an identifier in C. Suffixes only in LLVM LTO observed:
> > -      * - foo.llvm.[0-9a-f]+
> > +      * character in an identifier in C, so we can just remove the
> > +      * suffix.
> >        */
> > -     res = strstr(s, ".llvm.");
> > +     res = strstr(s, ".");
>
> nit: "strchr(s, '.')" ?
>
> Anyway,
>
> Reviewed-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>

Thanks for your kind review!

If we have another version, I will fold this change (and the one in
kallsyms_selftest.c) in.

Song

Re: [PATCH v2 3/3] tracing/kprobes: Use APIs that matches symbols without .XXX suffix

[Steven Rostedt]

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);
>  

Re: [PATCH v2 3/3] tracing/kprobes: Use APIs that matches symbols without .XXX suffix

[Song Liu]

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

Re: [PATCH v2 3/3] tracing/kprobes: Use APIs that matches symbols without .XXX suffix

[Steven Rostedt]

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

Re: [PATCH v2 3/3] tracing/kprobes: Use APIs that matches symbols without .XXX suffix

[Steven Rostedt]

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

Re: [PATCH v2 3/3] tracing/kprobes: Use APIs that matches symbols without .XXX suffix

[Song Liu]

> 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

Re: [PATCH v2 3/3] tracing/kprobes: Use APIs that matches symbols without .XXX suffix

[Masami Hiramatsu]

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
> 
> 
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: [PATCH v2 3/3] tracing/kprobes: Use APIs that matches symbols without .XXX suffix

[Song Liu]

> 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/

Re: [PATCH v2 3/3] tracing/kprobes: Use APIs that matches symbols without .XXX suffix

[Masami Hiramatsu]

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/
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: [PATCH v2 3/3] tracing/kprobes: Use APIs that matches symbols without .XXX suffix

[zhang warden]

> 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

Re: [PATCH v2 3/3] tracing/kprobes: Use APIs that matches symbols without .XXX suffix

[Sami Tolvanen]

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

Re: [PATCH v2 3/3] tracing/kprobes: Use APIs that matches symbols without .XXX suffix

[Song Liu]

> 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

Re: [PATCH v2 3/3] tracing/kprobes: Use APIs that matches symbols without .XXX suffix

[Song Liu]

> 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

Re: [PATCH v2 3/3] tracing/kprobes: Use APIs that matches symbols without .XXX suffix

[Steven Rostedt]

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
> 
> 

Re: [PATCH v2 3/3] tracing/kprobes: Use APIs that matches symbols without .XXX suffix

[Song Liu]

> 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

Re: [PATCH v2 3/3] tracing/kprobes: Use APIs that matches symbols without .XXX suffix

[Song Liu]

> 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

Re: [PATCH v2 3/3] tracing/kprobes: Use APIs that matches symbols without .XXX suffix

[Song Liu]

> 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

Re: [PATCH v2 3/3] tracing/kprobes: Use APIs that matches symbols without .XXX suffix

[Masami Hiramatsu]

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/
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: [PATCH v2 3/3] tracing/kprobes: Use APIs that matches symbols without .XXX suffix

[Song Liu]

> 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

Re: [PATCH v2 3/3] tracing/kprobes: Use APIs that matches symbols without .XXX suffix

[Masami Hiramatsu]

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


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: [PATCH v2 3/3] tracing/kprobes: Use APIs that matches symbols without .XXX suffix

[zhang warden]

> 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 

Re: [PATCH v2 3/3] tracing/kprobes: Use APIs that matches symbols without .XXX suffix

[Petr Mladek]

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

Re: [PATCH v2 3/3] tracing/kprobes: Use APIs that matches symbols without .XXX suffix

[Petr Mladek]

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

Re: [PATCH v2 2/3] kallsyms: Add APIs to match symbol without .XXXX suffix.

[Petr Mladek]

On Fri 2024-08-02 14:08:34, Song Liu 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 match only the part
> without .XXXX suffix. Specifically, the following two APIs are added.
> 
> 1. kallsyms_lookup_name_without_suffix()
> 2. kallsyms_on_each_match_symbol_without_suffix()
> 
> These APIs will be used by kprobe.
> 
> Also cleanup some code and update kallsyms_selftests accordingly.
> 
> --- a/kernel/kallsyms.c
> +++ b/kernel/kallsyms.c
> @@ -164,30 +164,27 @@ 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
>  	 * hooking of static functions with kprobes. '.' is not a valid
> -	 * character in an identifier in C. Suffixes only in LLVM LTO observed:
> -	 * - foo.llvm.[0-9a-f]+
> +	 * character in an identifier in C, so we can just remove the
> +	 * suffix.
>  	 */
> -	res = strstr(s, ".llvm.");
> +	res = strstr(s, ".");

IMHO, 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.

>  	if (res)
>  		*res = '\0';
>  
>  	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;

IMHO, this is very a bad design. It causes that

     kallsyms_on_each_match_symbol_without_suffix(,,, false);

does not longer work as expected. It creates a hard to maintain
code. The code does not do what it looks like.

The caller should decide whether it wants to ignore the suffix or no.
And this function should always respect the @exact_match parameter value.

> +
>  	low = 0;
>  	high = kallsyms_num_syms - 1;
>  

Best Regards,
Petr

Re: [PATCH v2 2/3] kallsyms: Add APIs to match symbol without .XXXX suffix.

[Song Liu]

> On Aug 8, 2024, at 3:20 AM, Petr Mladek <pmladek@suse.com> wrote:
> 
> On Fri 2024-08-02 14:08:34, Song Liu 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 match only the part
>> without .XXXX suffix. Specifically, the following two APIs are added.
>> 
>> 1. kallsyms_lookup_name_without_suffix()
>> 2. kallsyms_on_each_match_symbol_without_suffix()
>> 
>> These APIs will be used by kprobe.
>> 
>> Also cleanup some code and update kallsyms_selftests accordingly.
>> 
>> --- a/kernel/kallsyms.c
>> +++ b/kernel/kallsyms.c
>> @@ -164,30 +164,27 @@ 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
>> * hooking of static functions with kprobes. '.' is not a valid
>> - * character in an identifier in C. Suffixes only in LLVM LTO observed:
>> - * - foo.llvm.[0-9a-f]+
>> + * character in an identifier in C, so we can just remove the
>> + * suffix.
>> */
>> - res = strstr(s, ".llvm.");
>> + res = strstr(s, ".");
> 
> IMHO, 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.
> 
>> if (res)
>> *res = '\0';
>> 
>> 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;
> 
> IMHO, this is very a bad design. It causes that
> 
>     kallsyms_on_each_match_symbol_without_suffix(,,, false);
> 
> does not longer work as expected. It creates a hard to maintain
> code. The code does not do what it looks like.

Indeed. It actually caused issue with GCC-built kernel in my 
tests after submitting v2. 

Thanks,
Song

Re: [PATCH v2 3/3] tracing/kprobes: Use APIs that matches symbols without .XXX suffix

[Song Liu]

> 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 

Re: [PATCH v2 3/3] tracing/kprobes: Use APIs that matches symbols without .XXX suffix

[Song Liu]

> 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

Re: [PATCH v2 3/3] tracing/kprobes: Use APIs that matches symbols without .XXX suffix

[Masami Hiramatsu]

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


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: [PATCH v2 3/3] tracing/kprobes: Use APIs that matches symbols without .XXX suffix

[Alessandro Carminati]

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

Re: [PATCH v2 3/3] tracing/kprobes: Use APIs that matches symbols without .XXX suffix

[Petr Mladek]

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

Re: [PATCH v2 3/3] tracing/kprobes: Use APIs that matches symbols without .XXX suffix

[Song Liu]

> 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

Re: [PATCH v2 3/3] tracing/kprobes: Use APIs that matches symbols without .XXX suffix

[Song Liu]

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