[PATCH v3 0/2] Fix kallsyms with CONFIG_LTO_CLANG

[PATCH v3 0/2] 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.

Address this by sorting full symbols at build time, and let kallsyms
lookup APIs to match the symbols exactly.

Changes v2 => v3:
1. Remove the _without_suffix APIs, as kprobe will not use them.
   (Masami Hiramatsu)

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

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 (2):
  kallsyms: Do not cleanup .llvm.<hash> suffix before sorting symbols
  kallsyms: Match symbols exactly with CONFIG_LTO_CLANG

 kernel/kallsyms.c          | 55 +++++---------------------------------
 kernel/kallsyms_selftest.c | 22 +--------------
 scripts/kallsyms.c         | 31 ++-------------------
 scripts/link-vmlinux.sh    |  4 ---
 4 files changed, 9 insertions(+), 103 deletions(-)

--
2.43.5

[PATCH v3 1/2] 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.

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

[PATCH v3 2/2] kallsyms: Match symbols exactly with CONFIG_LTO_CLANG

[Song Liu]

With CONFIG_LTO_CLANG=y, the compiler may add .llvm.<hash> suffix to
function names to avoid duplication. APIs like kallsyms_lookup_name()
and kallsyms_on_each_match_symbol() tries to match these symbol names
without the .llvm.<hash> suffix, e.g., match "c_stop" with symbol
c_stop.llvm.17132674095431275852. This turned out to be problematic
for use cases that require exact match, for example, livepatch.

Fix this by making the APIs to match symbols exactly.

Also cleanup kallsyms_selftests accordingly.

Signed-off-by: Song Liu <song@kernel.org>
---
 kernel/kallsyms.c          | 55 +++++---------------------------------
 kernel/kallsyms_selftest.c | 22 +--------------
 2 files changed, 7 insertions(+), 70 deletions(-)

diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
index fb2c77368d18..a9a0ca605d4a 100644
--- a/kernel/kallsyms.c
+++ b/kernel/kallsyms.c
@@ -160,38 +160,6 @@ unsigned long kallsyms_sym_address(int idx)
 	return kallsyms_relative_base - 1 - kallsyms_offsets[idx];
 }
 
-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]+
-	 */
-	res = strstr(s, ".llvm.");
-	if (res)
-		*res = '\0';
-
-	return;
-}
-
-static int compare_symbol_name(const char *name, char *namebuf)
-{
-	/* 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.
-	 */
-	cleanup_symbol_name(namebuf);
-	return strcmp(name, namebuf);
-}
-
 static unsigned int get_symbol_seq(int index)
 {
 	unsigned int i, seq = 0;
@@ -219,7 +187,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 = strcmp(name, namebuf);
 		if (ret > 0)
 			low = mid + 1;
 		else if (ret < 0)
@@ -236,7 +204,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 (strcmp(name, namebuf))
 			break;
 		low--;
 	}
@@ -248,7 +216,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 (strcmp(name, namebuf))
 				break;
 			high++;
 		}
@@ -407,8 +375,7 @@ static int kallsyms_lookup_buildid(unsigned long addr,
 		if (modbuildid)
 			*modbuildid = NULL;
 
-		ret = strlen(namebuf);
-		goto found;
+		return strlen(namebuf);
 	}
 
 	/* See if it's in a module or a BPF JITed image. */
@@ -422,8 +389,6 @@ static int kallsyms_lookup_buildid(unsigned long addr,
 		ret = ftrace_mod_address_lookup(addr, symbolsize,
 						offset, modname, namebuf);
 
-found:
-	cleanup_symbol_name(namebuf);
 	return ret;
 }
 
@@ -450,8 +415,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 +425,10 @@ int lookup_symbol_name(unsigned long addr, char *symname)
 		/* Grab name */
 		kallsyms_expand_symbol(get_symbol_offset(pos),
 				       symname, KSYM_NAME_LEN);
-		goto found;
+		return 0;
 	}
 	/* See if it's in a module. */
-	res = lookup_module_symbol_name(addr, symname);
-	if (res)
-		return res;
-
-found:
-	cleanup_symbol_name(symname);
-	return 0;
+	return lookup_module_symbol_name(addr, symname);
 }
 
 /* Look up a kernel symbol and return it in a text buffer. */
diff --git a/kernel/kallsyms_selftest.c b/kernel/kallsyms_selftest.c
index 2f84896a7bcb..873f7c445488 100644
--- a/kernel/kallsyms_selftest.c
+++ b/kernel/kallsyms_selftest.c
@@ -187,31 +187,11 @@ static void test_perf_kallsyms_lookup_name(void)
 		stat.min, stat.max, div_u64(stat.sum, stat.real_cnt));
 }
 
-static bool match_cleanup_name(const char *s, const char *name)
-{
-	char *p;
-	int len;
-
-	if (!IS_ENABLED(CONFIG_LTO_CLANG))
-		return false;
-
-	p = strstr(s, ".llvm.");
-	if (!p)
-		return false;
-
-	len = strlen(name);
-	if (p - s != len)
-		return false;
-
-	return !strncmp(s, name, len);
-}
-
 static int find_symbol(void *data, const char *name, unsigned long addr)
 {
 	struct test_stat *stat = (struct test_stat *)data;
 
-	if (strcmp(name, stat->name) == 0 ||
-	    (!stat->perf && match_cleanup_name(name, stat->name))) {
+	if (!strcmp(name, stat->name)) {
 		stat->real_cnt++;
 		stat->addr = addr;
 
-- 
2.43.5

Re: [PATCH v3 0/2] Fix kallsyms with CONFIG_LTO_CLANG

[Song Liu]

Hi folks,

Do we have more concerns and/or suggestions with this set? If not,
what would be the next step for it?

Thanks,
Song

On Wed, Aug 7, 2024 at 3:05 PM Song Liu <song@kernel.org> wrote:
>
> 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.
>
> Address this by sorting full symbols at build time, and let kallsyms
> lookup APIs to match the symbols exactly.
>
> Changes v2 => v3:
> 1. Remove the _without_suffix APIs, as kprobe will not use them.
>    (Masami Hiramatsu)
>
> v2: https://lore.kernel.org/live-patching/20240802210836.2210140-1-song@kernel.org/T/#u
>
> 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 (2):
>   kallsyms: Do not cleanup .llvm.<hash> suffix before sorting symbols
>   kallsyms: Match symbols exactly with CONFIG_LTO_CLANG
>
>  kernel/kallsyms.c          | 55 +++++---------------------------------
>  kernel/kallsyms_selftest.c | 22 +--------------
>  scripts/kallsyms.c         | 31 ++-------------------
>  scripts/link-vmlinux.sh    |  4 ---
>  4 files changed, 9 insertions(+), 103 deletions(-)
>
> --
> 2.43.5

Re: [PATCH v3 0/2] Fix kallsyms with CONFIG_LTO_CLANG

[Luis Chamberlain]

On Mon, Aug 12, 2024 at 09:21:02AM -0700, Song Liu wrote:
> Hi folks,
> 
> Do we have more concerns and/or suggestions with this set? If not,
> what would be the next step for it?

I'm all for simplifying things, and this does just that, however,
I'm not the one you need to convince, the folks who added the original
hacks should provide their Reviewed-by / Tested-by not just for CONFIG_LTO_CLANG
but also given this provides an alternative fix, don't we want to invert
the order so we don't regress CONFIG_LTO_CLANG ? And shouldn't the patches
also have their respective Fixes tag?

Provided the commit logs are extended with Fixes and order is maintained
to be able to bisect correctly:

Reviewed-by: Luis Chamberlain <mcgrof@kernel.org>

  Luis

Re: [PATCH v3 0/2] Fix kallsyms with CONFIG_LTO_CLANG

[Song Liu]

Hi Luis,

> On Aug 12, 2024, at 9:57 AM, Luis Chamberlain <mcgrof@kernel.org> wrote:
> 
> On Mon, Aug 12, 2024 at 09:21:02AM -0700, Song Liu wrote:
>> Hi folks,
>> 
>> Do we have more concerns and/or suggestions with this set? If not,
>> what would be the next step for it?
> 
> I'm all for simplifying things, and this does just that, however,
> I'm not the one you need to convince, the folks who added the original
> hacks should provide their Reviewed-by / Tested-by not just for CONFIG_LTO_CLANG
> but also given this provides an alternative fix, don't we want to invert
> the order so we don't regress CONFIG_LTO_CLANG ? And shouldn't the patches
> also have their respective Fixes tag?

kallsyms has got quite a few changes/improvements in the past few years:

1. Sami added logic to trim LTO hash in 2021 [1];
2. Zhen added logic to sort kallsyms in 2022 [2];
3. Yonghong changed cleanup_symbol_name() in 2023 [3]. 

In this set, we are undoing 1 and 3, but we keep 2. Shall we point Fixes
tag to [1] or [3]? The patch won't apply to a kernel with only [1] 
(without [2] and [3]); while this set is not just fixing [3]. So I think
it is not accurate either way. OTOH, the combination of CONFIG_LTO_CLANG
and livepatching is probably not used by a lot of users, so I guess we 
are OK without Fixes tags? I personally don't have a strong preference 
either way. 

It is not necessary to invert the order of the two patches. Only applying
one of the two patches won't cause more issues than what we have today. 

Thanks,
Song


> 
> Provided the commit logs are extended with Fixes and order is maintained
> to be able to bisect correctly:
> 
> Reviewed-by: Luis Chamberlain <mcgrof@kernel.org>
> 
>  Luis


[1] 8b8e6b5d3b01 ("kallsyms: strip ThinLTO hashes from static functions")
[2] 60443c88f3a8 ("kallsyms: Improve the performance of kallsyms_lookup_name()")
[3] 8cc32a9bbf29 ("kallsyms: strip LTO-only suffixes from promoted global functions")

Re: [PATCH v3 0/2] Fix kallsyms with CONFIG_LTO_CLANG

[Masami Hiramatsu]

On Wed,  7 Aug 2024 15:05:11 -0700
Song Liu <song@kernel.org> wrote:

> 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.
> 
> Address this by sorting full symbols at build time, and let kallsyms
> lookup APIs to match the symbols exactly.
> 

I've tested this series and confirmed it makes kprobes work with llvm suffixed
symbols.

/sys/kernel/tracing # echo "p c_start.llvm.8011538628216713357" >> kprobe_events
 /sys/kernel/tracing # cat kprobe_events 
p:kprobes/p_c_start_llvm_8011538628216713357_0 c_start.llvm.8011538628216713357
/sys/kernel/tracing # echo "p c_start" >> kprobe_events 
/sys/kernel/tracing # cat kprobe_events 
p:kprobes/p_c_start_llvm_8011538628216713357_0 c_start.llvm.8011538628216713357
p:kprobes/p_c_start_0 c_start

And ftrace too.

/sys/kernel/tracing # grep ^c_start available_filter_functions
c_start.llvm.8011538628216713357
c_start
c_start.llvm.17132674095431275852

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

for this series.

> Changes v2 => v3:
> 1. Remove the _without_suffix APIs, as kprobe will not use them.
>    (Masami Hiramatsu)
> 
> v2: https://lore.kernel.org/live-patching/20240802210836.2210140-1-song@kernel.org/T/#u
> 
> 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 (2):
>   kallsyms: Do not cleanup .llvm.<hash> suffix before sorting symbols
>   kallsyms: Match symbols exactly with CONFIG_LTO_CLANG
> 
>  kernel/kallsyms.c          | 55 +++++---------------------------------
>  kernel/kallsyms_selftest.c | 22 +--------------
>  scripts/kallsyms.c         | 31 ++-------------------
>  scripts/link-vmlinux.sh    |  4 ---
>  4 files changed, 9 insertions(+), 103 deletions(-)
> 
> --
> 2.43.5


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

Re: [PATCH v3 1/2] kallsyms: Do not cleanup .llvm.<hash> suffix before sorting symbols

[Song Liu]

> On Aug 7, 2024, at 3:05 PM, Song Liu <song@kernel.org> wrote:
> 
> Cleaning up the symbols causes various issues afterwards. Let's sort
> the list based on original name.
> 
> Signed-off-by: Song Liu <song@kernel.org>

Fixes: 8cc32a9bbf29 ("kallsyms: strip LTO-only suffixes from promoted global functions")

Re: [PATCH v3 2/2] kallsyms: Match symbols exactly with CONFIG_LTO_CLANG

[Song Liu]

> On Aug 7, 2024, at 3:05 PM, Song Liu <song@kernel.org> wrote:
> 
> With CONFIG_LTO_CLANG=y, the compiler may add .llvm.<hash> suffix to
> function names to avoid duplication. APIs like kallsyms_lookup_name()
> and kallsyms_on_each_match_symbol() tries to match these symbol names
> without the .llvm.<hash> suffix, e.g., match "c_stop" with symbol
> c_stop.llvm.17132674095431275852. This turned out to be problematic
> for use cases that require exact match, for example, livepatch.
> 
> Fix this by making the APIs to match symbols exactly.
> 
> Also cleanup kallsyms_selftests accordingly.
> 
> Signed-off-by: Song Liu <song@kernel.org>

Fixes: 8cc32a9bbf29 ("kallsyms: strip LTO-only suffixes from promoted global functions")

Re: [PATCH v3 0/2] Fix kallsyms with CONFIG_LTO_CLANG

[Song Liu]

Hi Masami, 

Thanks for your review and test!

@Sami, could you please also review the set?

@Luis, I replied to 1/2 and 2/2 with Fixes tags that I think make most 
sense. Please let me know if we need changes to the set or more reviews
and tests. 

Thanks,
Song

> On Aug 12, 2024, at 9:29 PM, Masami Hiramatsu <mhiramat@kernel.org> wrote:
> 
> On Wed,  7 Aug 2024 15:05:11 -0700
> Song Liu <song@kernel.org> wrote:
> 
>> 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.
>> 
>> Address this by sorting full symbols at build time, and let kallsyms
>> lookup APIs to match the symbols exactly.
>> 
> 
> I've tested this series and confirmed it makes kprobes work with llvm suffixed
> symbols.
> 
> /sys/kernel/tracing # echo "p c_start.llvm.8011538628216713357" >> kprobe_events
> /sys/kernel/tracing # cat kprobe_events 
> p:kprobes/p_c_start_llvm_8011538628216713357_0 c_start.llvm.8011538628216713357
> /sys/kernel/tracing # echo "p c_start" >> kprobe_events 
> /sys/kernel/tracing # cat kprobe_events 
> p:kprobes/p_c_start_llvm_8011538628216713357_0 c_start.llvm.8011538628216713357
> p:kprobes/p_c_start_0 c_start
> 
> And ftrace too.
> 
> /sys/kernel/tracing # grep ^c_start available_filter_functions
> c_start.llvm.8011538628216713357
> c_start
> c_start.llvm.17132674095431275852
> 
> Tested-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> Reviewed-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> 
> for this series.
> 
>> Changes v2 => v3:
>> 1. Remove the _without_suffix APIs, as kprobe will not use them.
>>   (Masami Hiramatsu)
>> 
>> v2: https://lore.kernel.org/live-patching/20240802210836.2210140-1-song@kernel.org/T/#u
>> 
>> 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 (2):
>>  kallsyms: Do not cleanup .llvm.<hash> suffix before sorting symbols
>>  kallsyms: Match symbols exactly with CONFIG_LTO_CLANG
>> 
>> kernel/kallsyms.c          | 55 +++++---------------------------------
>> kernel/kallsyms_selftest.c | 22 +--------------
>> scripts/kallsyms.c         | 31 ++-------------------
>> scripts/link-vmlinux.sh    |  4 ---
>> 4 files changed, 9 insertions(+), 103 deletions(-)
>> 
>> --
>> 2.43.5
> 
> 
> -- 
> Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: [PATCH v3 0/2] Fix kallsyms with CONFIG_LTO_CLANG

[Sami Tolvanen]

Hi,

On Tue, Aug 13, 2024 at 2:20 PM Song Liu <songliubraving@meta.com> wrote:
>
> Hi Masami,
>
> Thanks for your review and test!
>
> @Sami, could you please also review the set?

As the kernel no longer uses the Clang feature combination that was
the primary motivation for adding these kallsyms changes in the first
place, this series looks reasonable to me. Thanks for cleaning this
up!

Reviewed-by: Sami Tolvanen <samitolvanen@google.com>

Sami

Re: [PATCH v3 0/2] Fix kallsyms with CONFIG_LTO_CLANG

[Petr Mladek]

On Wed 2024-08-07 15:05:11, Song Liu wrote:
> 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.
> 
> Address this by sorting full symbols at build time, and let kallsyms
> lookup APIs to match the symbols exactly.

The changes look good from the livepatching POV. For both patches,
feel free to use:

Acked-by: Petr Mladek <pmladek@suse.com>

I made a quick glance over the code changes. They look sane. But I did
not check them deep enough to provide a valuable Reviewed-by ;-)

Best Regards,
Petr

Re: [PATCH v3 0/2] Fix kallsyms with CONFIG_LTO_CLANG

[Kees Cook]

On Mon, Aug 12, 2024 at 06:13:22PM +0000, Song Liu wrote:
> Hi Luis,
> 
> > On Aug 12, 2024, at 9:57 AM, Luis Chamberlain <mcgrof@kernel.org> wrote:
> > 
> > On Mon, Aug 12, 2024 at 09:21:02AM -0700, Song Liu wrote:
> >> Hi folks,
> >> 
> >> Do we have more concerns and/or suggestions with this set? If not,
> >> what would be the next step for it?
> > 
> > I'm all for simplifying things, and this does just that, however,
> > I'm not the one you need to convince, the folks who added the original
> > hacks should provide their Reviewed-by / Tested-by not just for CONFIG_LTO_CLANG
> > but also given this provides an alternative fix, don't we want to invert
> > the order so we don't regress CONFIG_LTO_CLANG ? And shouldn't the patches
> > also have their respective Fixes tag?
> 
> kallsyms has got quite a few changes/improvements in the past few years:
> 
> 1. Sami added logic to trim LTO hash in 2021 [1];
> 2. Zhen added logic to sort kallsyms in 2022 [2];
> 3. Yonghong changed cleanup_symbol_name() in 2023 [3]. 
> 
> In this set, we are undoing 1 and 3, but we keep 2. Shall we point Fixes
> tag to [1] or [3]? The patch won't apply to a kernel with only [1] 
> (without [2] and [3]); while this set is not just fixing [3]. So I think
> it is not accurate either way. OTOH, the combination of CONFIG_LTO_CLANG
> and livepatching is probably not used by a lot of users, so I guess we 
> are OK without Fixes tags? I personally don't have a strong preference 
> either way. 
> 
> It is not necessary to invert the order of the two patches. Only applying
> one of the two patches won't cause more issues than what we have today. 

Which tree should carry this series?

-- 
Kees Cook

Re: [PATCH v3 0/2] Fix kallsyms with CONFIG_LTO_CLANG

[Song Liu]

Hi Kees,

> On Aug 15, 2024, at 9:05 AM, Kees Cook <kees@kernel.org> wrote:
> 
> On Mon, Aug 12, 2024 at 06:13:22PM +0000, Song Liu wrote:
>> Hi Luis,
>> 
>>> On Aug 12, 2024, at 9:57 AM, Luis Chamberlain <mcgrof@kernel.org> wrote:
>>> 
>>> On Mon, Aug 12, 2024 at 09:21:02AM -0700, Song Liu wrote:
>>>> Hi folks,
>>>> 
>>>> Do we have more concerns and/or suggestions with this set? If not,
>>>> what would be the next step for it?
>>> 
>>> I'm all for simplifying things, and this does just that, however,
>>> I'm not the one you need to convince, the folks who added the original
>>> hacks should provide their Reviewed-by / Tested-by not just for CONFIG_LTO_CLANG
>>> but also given this provides an alternative fix, don't we want to invert
>>> the order so we don't regress CONFIG_LTO_CLANG ? And shouldn't the patches
>>> also have their respective Fixes tag?
>> 
>> kallsyms has got quite a few changes/improvements in the past few years:
>> 
>> 1. Sami added logic to trim LTO hash in 2021 [1];
>> 2. Zhen added logic to sort kallsyms in 2022 [2];
>> 3. Yonghong changed cleanup_symbol_name() in 2023 [3]. 
>> 
>> In this set, we are undoing 1 and 3, but we keep 2. Shall we point Fixes
>> tag to [1] or [3]? The patch won't apply to a kernel with only [1] 
>> (without [2] and [3]); while this set is not just fixing [3]. So I think
>> it is not accurate either way. OTOH, the combination of CONFIG_LTO_CLANG
>> and livepatching is probably not used by a lot of users, so I guess we 
>> are OK without Fixes tags? I personally don't have a strong preference 
>> either way. 
>> 
>> It is not necessary to invert the order of the two patches. Only applying
>> one of the two patches won't cause more issues than what we have today.
> 
> Which tree should carry this series?

I am looking through the commit log on kernel/kallsyms.c _just now_, and  
found you took most of recent patches for kallsyms. Could you please take
this set as well?

Thanks,
Song

Re: [PATCH v3 0/2] Fix kallsyms with CONFIG_LTO_CLANG

[Kees Cook]

On Wed, 07 Aug 2024 15:05:11 -0700, Song Liu wrote:
> 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.
> 
> Address this by sorting full symbols at build time, and let kallsyms
> lookup APIs to match the symbols exactly.
> 
> [...]

Applied to for-linus/hardening, thanks!

[1/2] kallsyms: Do not cleanup .llvm.<hash> suffix before sorting symbols
      https://git.kernel.org/kees/c/020925ce9299
[2/2] kallsyms: Match symbols exactly with CONFIG_LTO_CLANG
      https://git.kernel.org/kees/c/fb6a421fb615

Take care,

-- 
Kees Cook

Re: [PATCH v3 0/2] Fix kallsyms with CONFIG_LTO_CLANG

[Miroslav Benes]

Hi,

On Wed, 7 Aug 2024, Song Liu wrote:

> 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.
> 
> Address this by sorting full symbols at build time, and let kallsyms
> lookup APIs to match the symbols exactly.
> 
> Changes v2 => v3:
> 1. Remove the _without_suffix APIs, as kprobe will not use them.
>    (Masami Hiramatsu)
> 
> v2: https://lore.kernel.org/live-patching/20240802210836.2210140-1-song@kernel.org/T/#u
> 
> 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 (2):
>   kallsyms: Do not cleanup .llvm.<hash> suffix before sorting symbols
>   kallsyms: Match symbols exactly with CONFIG_LTO_CLANG
> 
>  kernel/kallsyms.c          | 55 +++++---------------------------------
>  kernel/kallsyms_selftest.c | 22 +--------------
>  scripts/kallsyms.c         | 31 ++-------------------
>  scripts/link-vmlinux.sh    |  4 ---
>  4 files changed, 9 insertions(+), 103 deletions(-)

I was on holiday most of August and the patch set has been merged but let 
me at least add

Acked-by: Miroslav Benes <mbenes@suse.cz>

here since I participated in the discussion at the beginning.

Thank you for cleaning it up!

Miroslav