kallsyms: let kallsyms_on_each_match_symbol match symbols exactly

With CONFIG_LTO_CLANG, kallsyms.c:cleanup_symbol_name() removes symbols suffixes during comparison. This is problematic for livepatch, as kallsyms_on_each_match_symbol may find multiple matches for the same symbol, and fail with:

livepatch: unresolvable ambiguity for symbol 'xxx' in object 'yyy'

Make kallsyms_on_each_match_symbol() to match symbols exactly. Since livepatch is the only user of kallsyms_on_each_match_symbol(), this change is safe.

Signed-off-by Song Liu <[email protected]>

---
 kernel/kallsyms.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

kernel/kallsyms.c +9 -8

index 77747391f49b..2ab459b43084 100644
@@ -187,7 +187,7 @@ static bool cleanup_symbol_name(char *s)
 	return false;
 }
 
-static int compare_symbol_name(const char *name, char *namebuf)
+static int compare_symbol_name(const char *name, char *namebuf, bool match_exactly)
 {
 	int ret;
 

@@ -195,7 +195,7 @@ static int compare_symbol_name(const char *name, char *namebuf)
 	if (!ret)
 		return ret;
 
-	if (cleanup_symbol_name(namebuf) && !strcmp(name, namebuf))
+	if (!match_exactly && cleanup_symbol_name(namebuf) && !strcmp(name, namebuf))
 		return 0;
 
 	return ret;

@@ -213,7 +213,8 @@ 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 match_exactly)
 {
 	int ret;
 	int low, mid, high;

@@ -228,7 +229,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, match_exactly);
 		if (ret > 0)
 			low = mid + 1;
 		else if (ret < 0)

@@ -245,7 +246,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, match_exactly))
 			break;
 		low--;
 	}

@@ -257,7 +258,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, match_exactly))
 				break;
 			high++;
 		}

@@ -277,7 +278,7 @@ 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, false);
 	if (!ret)
 		return kallsyms_sym_address(get_symbol_seq(i));
 

@@ -312,7 +313,7 @@ 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;
 

Quoted 5 lines · Song Liu

With CONFIG_LTO_CLANG, kallsyms.c:cleanup_symbol_name() removes symbols suffixes during comparison. This is problematic for livepatch, as kallsyms_on_each_match_symbol may find multiple matches for the same symbol, and fail with:

Quoted 1 line

livepatch: unresolvable ambiguity for symbol 'xxx' in object 'yyy'

Did you forget to specify 'old_sympos'? When there are multiple symbols with the same name, we need to specify the sequence number of the symbols to be matched.

Quoted 10 lines · Leizhen (ThunderTown)

Quoted 5 lines · Song Liu

With CONFIG_LTO_CLANG, kallsyms.c:cleanup_symbol_name() removes symbols suffixes during comparison. This is problematic for livepatch, as kallsyms_on_each_match_symbol may find multiple matches for the same symbol, and fail with:

Quoted 5 lines

Quoted 1 line

livepatch: unresolvable ambiguity for symbol 'xxx' in object 'yyy'

Did you forget to specify 'old_sympos'? When there are multiple symbols with the same name, we need to specify the sequence number of the symbols to be matched.

old_sympos is indeed 0 here. However, the issue with CONFIG_LTO_CLANG is different. Here is an example:

$ grep bpf_verifier_vlog /proc/kallsyms
ffffffff81549f60 t bpf_verifier_vlog
ffffffff8268b430 d bpf_verifier_vlog._entry
ffffffff8282a958 d bpf_verifier_vlog._entry_ptr
ffffffff82e12a1f d bpf_verifier_vlog.__already_done

kallsyms_on_each_match_symbol matches "bpf_verifier_vlog" to all of these because of cleanup_symbol_name(). IOW, we only have one function called bpf_verifier_vlog, but kallsyms_on_each_match_symbol() matches it to bpf_verifier_vlog.*.

Does this make sense?

Thanks,
Song

.

Quoted 31 lines · Song Liu

Quoted 10 lines · Leizhen (ThunderTown)

Quoted 5 lines · Song Liu

With CONFIG_LTO_CLANG, kallsyms.c:cleanup_symbol_name() removes symbols suffixes during comparison. This is problematic for livepatch, as kallsyms_on_each_match_symbol may find multiple matches for the same symbol, and fail with:

Quoted 5 lines

Quoted 1 line

livepatch: unresolvable ambiguity for symbol 'xxx' in object 'yyy'

Did you forget to specify 'old_sympos'? When there are multiple symbols with the same name, we need to specify the sequence number of the symbols to be matched.

old_sympos is indeed 0 here. However, the issue with CONFIG_LTO_CLANG is different. Here is an example:

$ grep bpf_verifier_vlog /proc/kallsyms
ffffffff81549f60 t bpf_verifier_vlog
ffffffff8268b430 d bpf_verifier_vlog._entry
ffffffff8282a958 d bpf_verifier_vlog._entry_ptr
ffffffff82e12a1f d bpf_verifier_vlog.__already_done

kallsyms_on_each_match_symbol matches "bpf_verifier_vlog" to all of these because of cleanup_symbol_name(). IOW, we only have one function called bpf_verifier_vlog, but kallsyms_on_each_match_symbol() matches it to bpf_verifier_vlog.*.

Quoted 1 line

Does this make sense?

Sorry. I mistakenly thought you were operating a static function.

These suffixes are not mentioned in the comments in the function cleanup_symbol_name(). So I didn't notice it.

Quoted 8 lines

Thanks,
Song

.

Quoted 34 lines · Leizhen (ThunderTown)

Quoted 31 lines · Song Liu

Quoted 10 lines · Leizhen (ThunderTown)

Quoted 5 lines · Song Liu

With CONFIG_LTO_CLANG, kallsyms.c:cleanup_symbol_name() removes symbols suffixes during comparison. This is problematic for livepatch, as kallsyms_on_each_match_symbol may find multiple matches for the same symbol, and fail with:

Quoted 5 lines

Quoted 1 line

livepatch: unresolvable ambiguity for symbol 'xxx' in object 'yyy'

Did you forget to specify 'old_sympos'? When there are multiple symbols with the same name, we need to specify the sequence number of the symbols to be matched.

old_sympos is indeed 0 here. However, the issue with CONFIG_LTO_CLANG is different. Here is an example:

$ grep bpf_verifier_vlog /proc/kallsyms
ffffffff81549f60 t bpf_verifier_vlog
ffffffff8268b430 d bpf_verifier_vlog._entry
ffffffff8282a958 d bpf_verifier_vlog._entry_ptr
ffffffff82e12a1f d bpf_verifier_vlog.__already_done

kallsyms_on_each_match_symbol matches "bpf_verifier_vlog" to all of these because of cleanup_symbol_name(). IOW, we only have one function called bpf_verifier_vlog, but kallsyms_on_each_match_symbol() matches it to bpf_verifier_vlog.*.

Quoted 6 lines

Quoted 1 line

Does this make sense?

Sorry. I mistakenly thought you were operating a static function.

These suffixes are not mentioned in the comments in the function cleanup_symbol_name(). So I didn't notice it.

We can keep these three suffixes on the kallsyms tool end.

Quoted 10 lines

Quoted 9 lines

Thanks,
Song

.

Quoted 39 lines · Leizhen (ThunderTown)

Quoted 36 lines · Leizhen (ThunderTown)

Quoted 32 lines · Song Liu

Quoted 10 lines · Leizhen (ThunderTown)

Quoted 5 lines · Song Liu

With CONFIG_LTO_CLANG, kallsyms.c:cleanup_symbol_name() removes symbols suffixes during comparison. This is problematic for livepatch, as kallsyms_on_each_match_symbol may find multiple matches for the same symbol, and fail with:

Quoted 5 lines

Quoted 1 line

livepatch: unresolvable ambiguity for symbol 'xxx' in object 'yyy'

Did you forget to specify 'old_sympos'? When there are multiple symbols with the same name, we need to specify the sequence number of the symbols to be matched.

old_sympos is indeed 0 here. However, the issue with CONFIG_LTO_CLANG is different. Here is an example:

$ grep bpf_verifier_vlog /proc/kallsyms
ffffffff81549f60 t bpf_verifier_vlog
ffffffff8268b430 d bpf_verifier_vlog._entry
ffffffff8282a958 d bpf_verifier_vlog._entry_ptr
ffffffff82e12a1f d bpf_verifier_vlog.__already_done

kallsyms_on_each_match_symbol matches "bpf_verifier_vlog" to all of these because of cleanup_symbol_name(). IOW, we only have one function called bpf_verifier_vlog, but kallsyms_on_each_match_symbol() matches it to bpf_verifier_vlog.*.

Does this make sense?

Quoted 5 lines

Quoted 4 lines

Sorry. I mistakenly thought you were operating a static function.

These suffixes are not mentioned in the comments in the function cleanup_symbol_name(). So I didn't notice it.

We can keep these three suffixes on the kallsyms tool end.

And modify cleanup_symbol_name() not to cleanup these three suffixes.

With CONFIG_LTO_CLANG, kallsyms.c:cleanup_symbol_name() removes symbols suffixes during comparison. This is problematic for livepatch, as kallsyms_on_each_match_symbol may find multiple matches for the same symbol, and fail with:

livepatch: unresolvable ambiguity for symbol 'xxx' in object 'yyy'

Make kallsyms_on_each_match_symbol() to match symbols exactly. Since livepatch is the only user of kallsyms_on_each_match_symbol(), this change is safe.

Signed-off-by Song Liu <[email protected]>

---
 kernel/kallsyms.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

kernel/kallsyms.c +1 -1

index 77747391f49b..2ab459b43084 100644
@@ -187,7 +187,7 @@ static bool cleanup_symbol_name(char *s)
 	return false;
 }
 
-static int compare_symbol_name(const char *name, char *namebuf)
+static int compare_symbol_name(const char *name, char *namebuf, bool match_exactly)
 {
 	int ret;
 

kernel/kallsyms.c +7 -6

 		return 0;
 
 	return ret;

@@ -213,7 +213,8 @@ 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 match_exactly)
 {
 	int ret;
 	int low, mid, high;

@@ -228,7 +229,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, match_exactly);
 		if (ret > 0)
 			low = mid + 1;
 		else if (ret < 0)

@@ -245,7 +246,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, match_exactly))
 			break;
 		low--;
 	}

@@ -257,7 +258,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, match_exactly))
 				break;
 			high++;
 		}

@@ -277,7 +278,7 @@ 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, false);
 	if (!ret)
 		return kallsyms_sym_address(get_symbol_seq(i));
 

@@ -312,7 +313,7 @@ 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;
 

Quoted 29 lines · Song Liu

With CONFIG_LTO_CLANG, kallsyms.c:cleanup_symbol_name() removes symbols suffixes during comparison. This is problematic for livepatch, as kallsyms_on_each_match_symbol may find multiple matches for the same symbol, and fail with:

livepatch: unresolvable ambiguity for symbol 'xxx' in object 'yyy'

Make kallsyms_on_each_match_symbol() to match symbols exactly. Since livepatch is the only user of kallsyms_on_each_match_symbol(), this change is safe.

Signed-off-by Song Liu <[email protected]>

---
kernel/kallsyms.c | 17 +++++++++--------

1 file changed, 9 insertions(+), 8 deletions(-)

kernel/kallsyms.c +1 -1

index 77747391f49b..2ab459b43084 100644
@@ -187,7 +187,7 @@ static bool cleanup_symbol_name(char *s)
 return false;
 }
 
-static int compare_symbol_name(const char *name, char *namebuf)
+static int compare_symbol_name(const char *name, char *namebuf, bool match_exactly)
 {
 int ret;
 

kernel/kallsyms.c static int compare_symbol_name(const char *name, char *namebuf) · line 198

Quoted 6 lines

kernel/kallsyms.c +1 -1

@@ -195,7 +195,7 @@ static int compare_symbol_name(const char *name, char *namebuf)
 if (!ret)
 return ret;
 
- if (cleanup_symbol_name(namebuf) && !strcmp(name, namebuf))
+ if (!match_exactly && cleanup_symbol_name(namebuf) && !strcmp(name, namebuf))

This may affect the lookup of static functions.

Quoted 40 lines · Leizhen (ThunderTown)

Quoted 33 lines · Song Liu

With CONFIG_LTO_CLANG, kallsyms.c:cleanup_symbol_name() removes symbols suffixes during comparison. This is problematic for livepatch, as kallsyms_on_each_match_symbol may find multiple matches for the same symbol, and fail with:

livepatch: unresolvable ambiguity for symbol 'xxx' in object 'yyy'

Make kallsyms_on_each_match_symbol() to match symbols exactly. Since livepatch is the only user of kallsyms_on_each_match_symbol(), this change is safe.

Signed-off-by Song Liu <[email protected]>

---
kernel/kallsyms.c | 17 +++++++++--------

1 file changed, 9 insertions(+), 8 deletions(-)

kernel/kallsyms.c +1 -1

index 77747391f49b..2ab459b43084 100644
@@ -187,7 +187,7 @@ static bool cleanup_symbol_name(char *s)
 return false;
 }
 
-static int compare_symbol_name(const char *name, char *namebuf)
+static int compare_symbol_name(const char *name, char *namebuf, bool match_exactly)
 {
 int ret;
 

@@ -195,7 +195,7 @@ static int compare_symbol_name(const char *name, char *namebuf)
 if (!ret)
 return ret;
 

Quoted 4 lines

kernel/kallsyms.c static int compare_symbol_name(const char *name, char *namebuf) · line 198

Quoted 2 lines

kernel/kallsyms.c +1 -1

- if (cleanup_symbol_name(namebuf) && !strcmp(name, namebuf))
+ if (!match_exactly && cleanup_symbol_name(namebuf) && !strcmp(name, namebuf))

This may affect the lookup of static functions.

I am not following why would this be a problem. Could you give an example of it?

Thanks,
Song

.

Quoted 40 lines · Song Liu

Quoted 40 lines · Leizhen (ThunderTown)

Quoted 33 lines · Song Liu

With CONFIG_LTO_CLANG, kallsyms.c:cleanup_symbol_name() removes symbols suffixes during comparison. This is problematic for livepatch, as kallsyms_on_each_match_symbol may find multiple matches for the same symbol, and fail with:

livepatch: unresolvable ambiguity for symbol 'xxx' in object 'yyy'

Make kallsyms_on_each_match_symbol() to match symbols exactly. Since livepatch is the only user of kallsyms_on_each_match_symbol(), this change is safe.

Signed-off-by Song Liu <[email protected]>

---
kernel/kallsyms.c | 17 +++++++++--------

1 file changed, 9 insertions(+), 8 deletions(-)

kernel/kallsyms.c +1 -1

index 77747391f49b..2ab459b43084 100644
@@ -187,7 +187,7 @@ static bool cleanup_symbol_name(char *s)
 return false;
 }
 
-static int compare_symbol_name(const char *name, char *namebuf)
+static int compare_symbol_name(const char *name, char *namebuf, bool match_exactly)
 {
 int ret;
 

@@ -195,7 +195,7 @@ static int compare_symbol_name(const char *name, char *namebuf)
 if (!ret)
 return ret;
 

Quoted 7 lines

Quoted 4 lines

kernel/kallsyms.c static int compare_symbol_name(const char *name, char *namebuf) · line 198

Quoted 2 lines

kernel/kallsyms.c +1 -1

- if (cleanup_symbol_name(namebuf) && !strcmp(name, namebuf))
+ if (!match_exactly && cleanup_symbol_name(namebuf) && !strcmp(name, namebuf))

This may affect the lookup of static functions.

I am not following why would this be a problem. Could you give an example of it?

Here are the comments in cleanup_symbol_name(). If the compiler adds a suffix to the static function, but we do not remove the suffix, will the symbol match fail?

/*
* 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 observed:
* - foo.llvm.[0-9a-f]+
* - foo.[0-9a-f]+
*/

Quoted 55 lines · Leizhen (ThunderTown)

Quoted 40 lines · Song Liu

Quoted 40 lines · Leizhen (ThunderTown)

Quoted 33 lines · Song Liu

With CONFIG_LTO_CLANG, kallsyms.c:cleanup_symbol_name() removes symbols suffixes during comparison. This is problematic for livepatch, as kallsyms_on_each_match_symbol may find multiple matches for the same symbol, and fail with:

livepatch: unresolvable ambiguity for symbol 'xxx' in object 'yyy'

Make kallsyms_on_each_match_symbol() to match symbols exactly. Since livepatch is the only user of kallsyms_on_each_match_symbol(), this change is safe.

Signed-off-by Song Liu <[email protected]>

---
kernel/kallsyms.c | 17 +++++++++--------

1 file changed, 9 insertions(+), 8 deletions(-)

kernel/kallsyms.c +1 -1

index 77747391f49b..2ab459b43084 100644
@@ -187,7 +187,7 @@ static bool cleanup_symbol_name(char *s)
 return false;
 }
 
-static int compare_symbol_name(const char *name, char *namebuf)
+static int compare_symbol_name(const char *name, char *namebuf, bool match_exactly)
 {
 int ret;
 

@@ -195,7 +195,7 @@ static int compare_symbol_name(const char *name, char *namebuf)
 if (!ret)
 return ret;
 

Quoted 7 lines

Quoted 4 lines

kernel/kallsyms.c static int compare_symbol_name(const char *name, char *namebuf) · line 198

Quoted 2 lines

kernel/kallsyms.c +1 -1

- if (cleanup_symbol_name(namebuf) && !strcmp(name, namebuf))
+ if (!match_exactly && cleanup_symbol_name(namebuf) && !strcmp(name, namebuf))

This may affect the lookup of static functions.

I am not following why would this be a problem. Could you give an example of it?

Here are the comments in cleanup_symbol_name(). If the compiler adds a suffix to the static function, but we do not remove the suffix, will the symbol match fail?

Quoted 8 lines

/*
* 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 observed:
* - foo.llvm.[0-9a-f]+
* - foo.[0-9a-f]+
*/

I think livepatch will not fail, as the tool chain should already match the suffix for the function being patched. If the tool chain failed to do so, livepatch can fail for other reasons (missing symbols, etc.)

Quoted 2 lines · Song Liu

On Sun, Jun 18, 2023 at 8:32 PM Leizhen (ThunderTown) <[email protected]> wrote:

Quoted 25 lines

Quoted 16 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

kernel/kallsyms.c +0 -0

@@ -195,7 +195,7 @@ static int compare_symbol_name(const char *name, char *namebuf)
 if (!ret)
 return ret;
 

Quoted 7 lines

Quoted 4 lines

kernel/kallsyms.c static int compare_symbol_name(const char *name, char *namebuf) · line 198

Quoted 2 lines

kernel/kallsyms.c +1 -1

- if (cleanup_symbol_name(namebuf) && !strcmp(name, namebuf))
+ if (!match_exactly && cleanup_symbol_name(namebuf) && !strcmp(name, namebuf))

This may affect the lookup of static functions.

I am not following why would this be a problem. Could you give an example of it?

Here are the comments in cleanup_symbol_name(). If the compiler adds a suffix to the static function, but we do not remove the suffix, will the symbol match fail?

Quoted 8 lines

/*
* 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 observed:
* - foo.llvm.[0-9a-f]+
* - foo.[0-9a-f]+
*/

Quoted 3 lines

I think livepatch will not fail, as the tool chain should already match the suffix for the function being patched. If the tool chain failed to do so, livepatch can fail for other reasons (missing symbols, etc.)

cleanup_symbol_name() has been added by the commit 8b8e6b5d3b013b0bd8 ("kallsyms: strip ThinLTO hashes from static functions"). The motivation is that user space tools pass the symbol names found in sources. They do not know about the "random" suffix added by the "random" compiler.

While livepatching might want to work with the full symbol names. It helps to locate avoid duplication and find the right symbol.

At least, this should be beneficial for kpatch tool which works directly with the generated symbols.

Well, in theory, the cleaned symbol names might be useful for source-based livepatches. But there might be problem to distinguish different symbols with the same name and symbols duplicated because of inlining. Well, we tend to livepatch the caller anyway.

Quoted 5 lines · Petr Mladek

Quoted 2 lines · Song Liu

On Sun, Jun 18, 2023 at 8:32 PM Leizhen (ThunderTown) <[email protected]> wrote:

[...]

Quoted 29 lines

Quoted 25 lines

Quoted 16 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

(quoted diff) +0 -0

@@ -195,7 +195,7 @@ static int compare_symbol_name(const char *name, char *namebuf)
 if (!ret)
 return ret;
 

Quoted 7 lines

Quoted 4 lines

Review static int compare_symbol_name(const char *name, char *namebuf) · line 198

Quoted 2 lines

(quoted diff) +1 -1

- if (cleanup_symbol_name(namebuf) && !strcmp(name, namebuf))
+ if (!match_exactly && cleanup_symbol_name(namebuf) && !strcmp(name, namebuf))

This may affect the lookup of static functions.

I am not following why would this be a problem. Could you give an example of it?

Here are the comments in cleanup_symbol_name(). If the compiler adds a suffix to the static function, but we do not remove the suffix, will the symbol match fail?

Quoted 8 lines

/*
* 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 observed:
* - foo.llvm.[0-9a-f]+
* - foo.[0-9a-f]+
*/

Quoted 3 lines

I think livepatch will not fail, as the tool chain should already match the suffix for the function being patched. If the tool chain failed to do so, livepatch can fail for other reasons (missing symbols, etc.)

Quoted 5 lines

cleanup_symbol_name() has been added by the commit 8b8e6b5d3b013b0bd8 ("kallsyms: strip ThinLTO hashes from static functions"). The motivation is that user space tools pass the symbol names found in sources. They do not know about the "random" suffix added by the "random" compiler.

I am not quite sure how tracing tools would work without knowing about what the compiler did to the code. But I guess we are not addressing that part here.

Quoted 6 lines

While livepatching might want to work with the full symbol names. It helps to locate avoid duplication and find the right symbol.

At least, this should be beneficial for kpatch tool which works directly with the generated symbols.

Quoted 5 lines

Well, in theory, the cleaned symbol names might be useful for source-based livepatches. But there might be problem to distinguish different symbols with the same name and symbols duplicated because of inlining. Well, we tend to livepatch the caller anyway.

I am not quite following the direction here. Do we need more work for this patch?

Quoted 7 lines · Song Liu

Quoted 5 lines · Petr Mladek

Quoted 2 lines · Song Liu

On Sun, Jun 18, 2023 at 8:32 PM Leizhen (ThunderTown) <[email protected]> wrote:

[...]

Quoted 3 lines

I am not quite following the direction here. Do we need more work for this patch?

Good question. I primary tried to add more details so that we better understand the problem.

Honestly, I do not know the answer. I am neither familiar with kpatch nor with clang. Let me think loudly.

kpatch produce livepatches by comparing binaries of the original and fixed kernel. It adds a symbol into the livepatch when the same symbol has different code in the two binaries.

So one important question is how clang generates the suffix. Is the suffix the same in every build? Is it the same even after the function gets modified by a fix?

If the suffix is always the same then then the full symbol name would be better for kpatch. kpatch would get it for free. And kpatch would not longer need to use "old_sympos".

But if the suffix is different then kpatch has a problem. kpatch would need to match symbols with different suffixes. It would be easy for symbols which are unique after removing the suffix. But it would be tricky for comparing symbols which do not have an unique name. kpatch would need to find which suffix in the original binary matches an other suffix in the fixed binary. In this case, it might be easier to use the stripped symbol names.

And the suffix might be problematic also for source based livepatches. They define struct klp_func in sources so they would need to hardcode the suffix there. It might be easy to keep using the stripped name and "old_sympos".

I expect that this patch actually breaks the livepatch selftests when the kernel is compiled with clang LTO.

Quoted 12 lines · Petr Mladek

Quoted 7 lines · Song Liu

Quoted 5 lines · Petr Mladek

Quoted 2 lines · Song Liu

On Sun, Jun 18, 2023 at 8:32 PM Leizhen (ThunderTown) <[email protected]> wrote:

[...]

[...]

Quoted 14 lines

Quoted 3 lines

I am not quite following the direction here. Do we need more work for this patch?

Good question. I primary tried to add more details so that we better understand the problem.

Honestly, I do not know the answer. I am neither familiar with kpatch nor with clang. Let me think loudly.

kpatch produce livepatches by comparing binaries of the original and fixed kernel. It adds a symbol into the livepatch when the same symbol has different code in the two binaries.

Quoted 7 lines

So one important question is how clang generates the suffix. Is the suffix the same in every build? Is it the same even after the function gets modified by a fix?

If the suffix is always the same then then the full symbol name would be better for kpatch. kpatch would get it for free. And kpatch would not longer need to use "old_sympos".

This is pretty complicated.

1. clang with LTO does not use the suffix to eliminated duplicated kallsyms, so old_sympos is still needed. Here is an example:

# grep init_once /proc/kallsyms
ffffffff8120ba80 t init_once.llvm.14172910296636650566
ffffffff8120ba90 t inode_init_once
ffffffff813ea5d0 t bpf_user_rnd_init_once
ffffffff813fd5b0 t init_once.llvm.17912494158778303782
ffffffff813ffbf0 t init_once
ffffffff813ffc60 t init_once
ffffffff813ffc70 t init_once
ffffffff813ffcd0 t init_once
ffffffff813ffce0 t init_once

There are two "init_once" with suffix, but there are also ones without them.

2. kpatch-build does "Building original source", "Building patched source", and then do binary diff of the two. From our experiments, the suffix doesn't change between the two builds. However, we need to match the build environment (path of kernel source, etc.) to make sure suffix from kpatch matches the kernel.

3. The goal of this patch is NOT to resolve different suffix by llvm (.llvm.[0-9]+). Instead, we are trying fix issues like:

# grep bpf_verifier_vlog /proc/kallsyms ffffffff81549f60 t bpf_verifier_vlog ffffffff8268b430 d bpf_verifier_vlog._entry ffffffff8282a958 d bpf_verifier_vlog._entry_ptr ffffffff82e12a1f d bpf_verifier_vlog.__already_done

With existing code, compare_symbol_name() will match bpf_verifier_vlog to all these with CONFIG_LTO_CLANG.

We can probably teach compare_symbol_name() to not to match these suffix, as Zhen suggested.

If this is not ideal, I am open to suggestions that can solve the problem.

Quoted 9 lines

But if the suffix is different then kpatch has a problem. kpatch would need to match symbols with different suffixes. It would be easy for symbols which are unique after removing the suffix. But it would be tricky for comparing symbols which do not have an unique name. kpatch would need to find which suffix in the original binary matches an other suffix in the fixed binary. In this case, it might be easier to use the stripped symbol names.

Quoted 7 lines

And the suffix might be problematic also for source based livepatches. They define struct klp_func in sources so they would need to hardcode the suffix there. It might be easy to keep using the stripped name and "old_sympos".

I expect that this patch actually breaks the livepatch selftests when the kernel is compiled with clang LTO.

Not really. This patch passes livepatch selftests.

Thanks,
Song

Quoted 31 lines · Song Liu

Quoted 6 lines · Petr Mladek

Quoted 4 lines · Song Liu

Quoted 4 lines · Petr Mladek

Quoted 2 lines · Song Liu

On Sun, Jun 18, 2023 at 8:32 PM Leizhen (ThunderTown) <[email protected]> wrote:

Quoted 8 lines

Quoted 2 lines

I am not quite following the direction here. Do we need more work for this patch?

Good question. I primary tried to add more details so that we better understand the problem.

Honestly, I do not know the answer. I am neither familiar with kpatch nor with clang.

This is pretty complicated.

1. clang with LTO does not use the suffix to eliminated duplicated kallsyms, so old_sympos is still needed. Here is an example:

# grep init_once /proc/kallsyms
ffffffff8120ba80 t init_once.llvm.14172910296636650566
ffffffff8120ba90 t inode_init_once
ffffffff813ea5d0 t bpf_user_rnd_init_once
ffffffff813fd5b0 t init_once.llvm.17912494158778303782
ffffffff813ffbf0 t init_once
ffffffff813ffc60 t init_once
ffffffff813ffc70 t init_once
ffffffff813ffcd0 t init_once
ffffffff813ffce0 t init_once

Quoted 2 lines

There are two "init_once" with suffix, but there are also ones without them.

This is correct. At LTO mode, when a static function/variable is promoted to the global. The '.llvm.<hash>' is added to the static function/variable name to form a global function name. The '<hash>' here is computed based on IR of the compiled file before LTO. So if one file is not modified, then <hash> won't change after additional code change in other files.

Quoted 5 lines

2. kpatch-build does "Building original source", "Building patched source", and then do binary diff of the two. From our experiments, the suffix doesn't change between the two builds. However, we need to match the build environment (path of kernel source, etc.) to make sure suffix from kpatch matches the kernel.

The goal here is to generate the same IR (hence <hash>) if file content is not changed. This way, <hash> value will change only for those changed files.

Quoted 8 lines

3. The goal of this patch is NOT to resolve different suffix by llvm (.llvm.[0-9]+). Instead, we are trying fix issues like:

# grep bpf_verifier_vlog /proc/kallsyms ffffffff81549f60 t bpf_verifier_vlog ffffffff8268b430 d bpf_verifier_vlog._entry ffffffff8282a958 d bpf_verifier_vlog._entry_ptr ffffffff82e12a1f d bpf_verifier_vlog.__already_done

Hi,

I have added people mentioned in commits which modified cleanup_symbol_name() in kallsyms.c.

I think that stripping ".*" suffix does not work for static variables defined locally from symbol does always work, see below.

Quoted 35 lines · Yonghong Song

Quoted 31 lines · Song Liu

Quoted 6 lines · Petr Mladek

Quoted 4 lines · Song Liu

Quoted 4 lines · Petr Mladek

Quoted 2 lines · Song Liu

On Sun, Jun 18, 2023 at 8:32 PM Leizhen (ThunderTown) <[email protected]> wrote:

Quoted 8 lines

Quoted 2 lines

I am not quite following the direction here. Do we need more work for this patch?

Good question. I primary tried to add more details so that we better understand the problem.

Honestly, I do not know the answer. I am neither familiar with kpatch nor with clang.

This is pretty complicated.

1. clang with LTO does not use the suffix to eliminated duplicated kallsyms, so old_sympos is still needed. Here is an example:

# grep init_once /proc/kallsyms
ffffffff8120ba80 t init_once.llvm.14172910296636650566
ffffffff8120ba90 t inode_init_once
ffffffff813ea5d0 t bpf_user_rnd_init_once
ffffffff813fd5b0 t init_once.llvm.17912494158778303782
ffffffff813ffbf0 t init_once
ffffffff813ffc60 t init_once
ffffffff813ffc70 t init_once
ffffffff813ffcd0 t init_once
ffffffff813ffce0 t init_once

Quoted 2 lines

There are two "init_once" with suffix, but there are also ones without them.

Quoted 6 lines

This is correct. At LTO mode, when a static function/variable is promoted to the global. The '.llvm.<hash>' is added to the static function/variable name to form a global function name. The '<hash>' here is computed based on IR of the compiled file before LTO. So if one file is not modified, then <hash> won't change after additional code change in other files.

OK, so the ".llvm.<hash>" suffix is added when a symbol is promoted from static to global. Right?

Quoted 7 lines

Quoted 6 lines

2. kpatch-build does "Building original source", "Building patched source", and then do binary diff of the two. From our experiments, the suffix doesn't change between the two builds. However, we need to match the build environment (path of kernel source, etc.) to make sure suffix from kpatch matches the kernel.

Quoted 3 lines

The goal here is to generate the same IR (hence <hash>) if file content is not changed. This way, <hash> value will change only for those changed files.

Hmm, how does kpatch match the fixed functions? They are modified so that they should get different IR (hash). Or do I miss anything, please?

Quoted 4 lines

Quoted 4 lines

3. The goal of this patch is NOT to resolve different suffix by llvm (.llvm.[0-9]+). Instead, we are trying fix issues like:

Quoted 5 lines

Quoted 5 lines

# grep bpf_verifier_vlog /proc/kallsyms ffffffff81549f60 t bpf_verifier_vlog ffffffff8268b430 d bpf_verifier_vlog._entry ffffffff8282a958 d bpf_verifier_vlog._entry_ptr ffffffff82e12a1f d bpf_verifier_vlog.__already_done

And <function>.<symbol> notation seems to be used for static symbols defined inside a function.

I guess that it is used when the symbols stay statics. It would probably get additional ".llvm.<hash>" when it got promoted from static to global. But this probably never happens.

Do I get it correctly?

It means that we have two different types of name changes:

1. .llvm.<hash> suffix

If we remove this suffix then we will not longer distinguish symbols which stayed static and which were promoted to global ones.

The result should be basically the same as without LTO. Some symbols might have duplicated name. But most symbols would have an unique one.

   2. <function>.<symbol> name

      In this case, <symbol> is _not_ suffix. It is actually
      the original symbol name.

      The extra thing is the <function>. prefix.

      These static variables seem to have special handling even
      with gcc without LTO. gcc adds an extra id instead,
      for example:

	$> nm vmlinux | grep " _entry_ptr" | head
	ffffffff82a2e800 d _entry_ptr.100135
	ffffffff82a2e7f8 d _entry_ptr.100178
	ffffffff82a32e70 d _entry_ptr.100798
	ffffffff82a1e240 d _entry_ptr.10342
	ffffffff82a33930 d _entry_ptr.104764

	ffffffff82a339c8 d _entry_ptr.104830
	ffffffff82a33928 d _entry_ptr.104871
	ffffffff82a33920 d _entry_ptr.104877
	ffffffff82a33918 d _entry_ptr.104893
	ffffffff82a339c0 d _entry_ptr.104905

	$> nm vmlinux | grep panic_console_dropped
	ffffffff853618e0 b panic_console_dropped.54158

Effect from the tracers POV?

1. .llvm.<hash> suffix

The names without the .llvm.<hash> suffix are the same as without LTO. This is probably why commit 8b8e6b5d3b013b0b ("kallsyms: strip ThinLTO hashes from static functions") worked. The tracers probably wanted to access only the symbols with uniqueue names

2. <function>.<symbol> name

The name without the .<symbol> suffix is the same as the function name. The result are duplicated function names.

I do not understand why this was not a problem for tracers. Note that this is pretty common. _entry and _entry_ptr are added into any function calling printk().

It seems to be working only by chance. Maybe, the tracers always take the first matched symbol. And the function name, without any suffix, is always the first one in the sorted list.

Effect from livepatching POV:

1. .llvm.<hash> suffix

Comparing the full symbol name looks fragile to me because the <hash> might change.

IMHO, it would be better to compare the names without the .llvm.<hash> suffix even for livepatches.

2. <function>.<symbol> name

The removal of <.symbol> suffix is a bad idea. The livepatch code is not able to distinguish the symbol of the <function> and static variables defined in this function.

IMHO, it would be better to compare the full
<function>.<symbol> name.

Result:

IMHO, cleanup_symbol_name() should remove only .llwn.* suffix. And it should be used for both tracers and livepatching.

Does this makes any sense?

Best Regards,
Petr

Quoted 3 lines · Yonghong Song

[...]

Quoted 17 lines

Quoted 16 lines

Effect from the tracers POV?
1. .llvm.<hash> suffix
The names without the .llvm.<hash> suffix are the same as without LTO. This is probably why commit 8b8e6b5d3b013b0b ("kallsyms: strip ThinLTO hashes from static functions") worked. The tracers probably wanted to access only the symbols with uniqueue names
2. <function>.<symbol> name
The name without the .<symbol> suffix is the same as the function name. The result are duplicated function names.
I do not understand why this was not a problem for tracers. Note that this is pretty common. _entry and _entry_ptr are added into any function calling printk().
It seems to be working only by chance. Maybe, the tracers always take the first matched symbol. And the function name, without any suffix, is always the first one in the sorted list.

Quoted 2 lines

Note this only happens in LTO mode. Maybe lto kernel is not used wide enough to discover this issue?

I think this is because all these <function>.<symbol> are data, while tracers are looking for functions.

Quoted 18 lines

Quoted 17 lines

Effect from livepatching POV:
1. .llvm.<hash> suffix
Comparing the full symbol name looks fragile to me because the <hash> might change.
IMHO, it would be better to compare the names without the .llvm.<hash> suffix even for livepatches.
2. <function>.<symbol> name
The removal of <.symbol> suffix is a bad idea. The livepatch code is not able to distinguish the symbol of the <function> and static variables defined in this function. IMHO, it would be better to compare the full
<function>.<symbol> name.
Result:
IMHO, cleanup_symbol_name() should remove only .llwn.* suffix. And it should be used for both tracers and livepatching. Does this makes any sense?

Quoted 1 line

Song, does this fix the problem?

I think this should work. We also see .str.<num.>llvm.<hash>. But those should not matter.

Thanks,
Song

Quoted 7 lines

I only checked llvm15 and llvm17, not sure what kind of suffix'es used for early llvm (>= llvm11).
Nick, could you help answer this question? What kind of suffix are used for lto when promoting a local symbol to a global one, considering all versions of llvm >= 11 since llvm 11 is the minimum supported version for kernel build.

Quoted 54 lines · Petr Mladek

Hi,

I have added people mentioned in commits which modified cleanup_symbol_name() in kallsyms.c.

I think that stripping ".*" suffix does not work for static variables defined locally from symbol does always work, see below.

Quoted 35 lines · Yonghong Song

Quoted 31 lines · Song Liu

Quoted 6 lines · Petr Mladek

Quoted 4 lines · Song Liu

Quoted 4 lines · Petr Mladek

Quoted 2 lines · Song Liu

On Sun, Jun 18, 2023 at 8:32 PM Leizhen (ThunderTown) <[email protected]> wrote:

Quoted 8 lines

Quoted 2 lines

I am not quite following the direction here. Do we need more work for this patch?

Good question. I primary tried to add more details so that we better understand the problem.

Honestly, I do not know the answer. I am neither familiar with kpatch nor with clang.

This is pretty complicated.

1. clang with LTO does not use the suffix to eliminated duplicated kallsyms, so old_sympos is still needed. Here is an example:

# grep init_once /proc/kallsyms
ffffffff8120ba80 t init_once.llvm.14172910296636650566
ffffffff8120ba90 t inode_init_once
ffffffff813ea5d0 t bpf_user_rnd_init_once
ffffffff813fd5b0 t init_once.llvm.17912494158778303782
ffffffff813ffbf0 t init_once
ffffffff813ffc60 t init_once
ffffffff813ffc70 t init_once
ffffffff813ffcd0 t init_once
ffffffff813ffce0 t init_once

Quoted 2 lines

There are two "init_once" with suffix, but there are also ones without them.

Quoted 6 lines

This is correct. At LTO mode, when a static function/variable is promoted to the global. The '.llvm.<hash>' is added to the static function/variable name to form a global function name. The '<hash>' here is computed based on IR of the compiled file before LTO. So if one file is not modified, then <hash> won't change after additional code change in other files.

Quoted 2 lines

OK, so the ".llvm.<hash>" suffix is added when a symbol is promoted from static to global. Right?

Right at lest for llvm >= 15.
There are an alternative format '.llvm.<file_path>' suffix with a more involved compilation process.

https://github.com/llvm/llvm-project/blob/main/llvm/test/ThinLTO/X86/promote-local-name.ll

But for kernel lto build, yes, only '.llvm.<hash>'.

Quoted 7 lines

Quoted 7 lines

Quoted 6 lines

2. kpatch-build does "Building original source", "Building patched source", and then do binary diff of the two. From our experiments, the suffix doesn't change between the two builds. However, we need to match the build environment (path of kernel source, etc.) to make sure suffix from kpatch matches the kernel.

Quoted 7 lines

Quoted 3 lines

The goal here is to generate the same IR (hence <hash>) if file content is not changed. This way, <hash> value will change only for those changed files.

Hmm, how does kpatch match the fixed functions? They are modified so that they should get different IR (hash). Or do I miss anything, please?

If the static function are promoted to global function and the file containing static function changed, then that modified static function will appear to be a *new* function. Live change should be able to do it, right?

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

3. The goal of this patch is NOT to resolve different suffix by llvm (.llvm.[0-9]+). Instead, we are trying fix issues like:

Quoted 8 lines

Quoted 5 lines

Quoted 5 lines

# grep bpf_verifier_vlog /proc/kallsyms ffffffff81549f60 t bpf_verifier_vlog ffffffff8268b430 d bpf_verifier_vlog._entry ffffffff8282a958 d bpf_verifier_vlog._entry_ptr ffffffff82e12a1f d bpf_verifier_vlog.__already_done

And <function>.<symbol> notation seems to be used for static symbols defined inside a function.

That is correct.

Quoted 4 lines

I guess that it is used when the symbols stay statics. It would probably get additional ".llvm.<hash>" when it got promoted from static to global. But this probably never happens.

I have not see a case like this yet.

Quoted 2 lines

Do I get it correctly?

yes, that is correct.

Quoted 31 lines

It means that we have two different types of name changes:

1. .llvm.<hash> suffix

If we remove this suffix then we will not longer distinguish symbols which stayed static and which were promoted to global ones.

The result should be basically the same as without LTO. Some symbols might have duplicated name. But most symbols would have an unique one.

   2. <function>.<symbol> name

      In this case, <symbol> is _not_ suffix. It is actually
      the original symbol name.

      The extra thing is the <function>. prefix.

      These static variables seem to have special handling even
      with gcc without LTO. gcc adds an extra id instead,
      for example:

      $> nm vmlinux | grep " _entry_ptr" | head
      ffffffff82a2e800 d _entry_ptr.100135
      ffffffff82a2e7f8 d _entry_ptr.100178
      ffffffff82a32e70 d _entry_ptr.100798
      ffffffff82a1e240 d _entry_ptr.10342
      ffffffff82a33930 d _entry_ptr.104764

Quoted 8 lines

      ffffffff82a339c8 d _entry_ptr.104830
      ffffffff82a33928 d _entry_ptr.104871
      ffffffff82a33920 d _entry_ptr.104877
      ffffffff82a33918 d _entry_ptr.104893
      ffffffff82a339c0 d _entry_ptr.104905

      $> nm vmlinux | grep panic_console_dropped
      ffffffff853618e0 b panic_console_dropped.54158

IIRC, yes, these 'id' might change if source code changed.

Quoted 21 lines

Effect from the tracers POV?

1. .llvm.<hash> suffix

The names without the .llvm.<hash> suffix are the same as without LTO. This is probably why commit 8b8e6b5d3b013b0b ("kallsyms: strip ThinLTO hashes from static functions") worked. The tracers probably wanted to access only the symbols with uniqueue names

2. <function>.<symbol> name

The name without the .<symbol> suffix is the same as the function name. The result are duplicated function names.

I do not understand why this was not a problem for tracers. Note that this is pretty common. _entry and _entry_ptr are added into any function calling printk().

Quoted 3 lines

It seems to be working only by chance. Maybe, the tracers always take the first matched symbol. And the function name, without any suffix, is always the first one in the sorted list.

Note this only happens in LTO mode. Maybe lto kernel is not used wide enough to discover this issue?

Quoted 24 lines

Effect from livepatching POV:

1. .llvm.<hash> suffix

Comparing the full symbol name looks fragile to me because the <hash> might change.

IMHO, it would be better to compare the names without the .llvm.<hash> suffix even for livepatches.

2. <function>.<symbol> name

The removal of <.symbol> suffix is a bad idea. The livepatch code is not able to distinguish the symbol of the <function> and static variables defined in this function.

IMHO, it would be better to compare the full
<function>.<symbol> name.

Quoted 6 lines

Result:

IMHO, cleanup_symbol_name() should remove only .llwn.* suffix. And it should be used for both tracers and livepatching.

Does this makes any sense?

Song, does this fix the problem?

I only checked llvm15 and llvm17, not sure what kind of suffix'es used for early llvm (>= llvm11).
Nick, could you help answer this question? What kind of suffix are used for lto when promoting a local symbol to a global one, considering all versions of llvm >= 11 since llvm 11 is the minimum supported version for kernel build.

Quoted 4 lines

Best Regards,
Petr

Quoted 179 lines · Yonghong Song

Quoted 54 lines · Petr Mladek

Hi,

I have added people mentioned in commits which modified cleanup_symbol_name() in kallsyms.c.

I think that stripping ".*" suffix does not work for static variables defined locally from symbol does always work, see below.

Quoted 35 lines · Yonghong Song

Quoted 31 lines · Song Liu

Quoted 6 lines · Petr Mladek

Quoted 4 lines · Song Liu

Quoted 4 lines · Petr Mladek

Quoted 2 lines · Song Liu

On Sun, Jun 18, 2023 at 8:32 PM Leizhen (ThunderTown) <[email protected]> wrote:

Quoted 8 lines

Quoted 2 lines

I am not quite following the direction here. Do we need more work for this patch?

Good question. I primary tried to add more details so that we better understand the problem.

Honestly, I do not know the answer. I am neither familiar with kpatch nor with clang.

This is pretty complicated.

1. clang with LTO does not use the suffix to eliminated duplicated kallsyms, so old_sympos is still needed. Here is an example:

# grep init_once /proc/kallsyms
ffffffff8120ba80 t init_once.llvm.14172910296636650566
ffffffff8120ba90 t inode_init_once
ffffffff813ea5d0 t bpf_user_rnd_init_once
ffffffff813fd5b0 t init_once.llvm.17912494158778303782
ffffffff813ffbf0 t init_once
ffffffff813ffc60 t init_once
ffffffff813ffc70 t init_once
ffffffff813ffcd0 t init_once
ffffffff813ffce0 t init_once

Quoted 2 lines

There are two "init_once" with suffix, but there are also ones without them.

Quoted 6 lines

This is correct. At LTO mode, when a static function/variable is promoted to the global. The '.llvm.<hash>' is added to the static function/variable name to form a global function name. The '<hash>' here is computed based on IR of the compiled file before LTO. So if one file is not modified, then <hash> won't change after additional code change in other files.

Quoted 2 lines

OK, so the ".llvm.<hash>" suffix is added when a symbol is promoted from static to global. Right?

Right at lest for llvm >= 15.
There are an alternative format '.llvm.<file_path>' suffix with a more involved compilation process.

https://github.com/llvm/llvm-project/blob/main/llvm/test/ThinLTO/X86/promote-local-name.ll

But for kernel lto build, yes, only '.llvm.<hash>'.

Quoted 7 lines

Quoted 7 lines

Quoted 6 lines

2. kpatch-build does "Building original source", "Building patched source", and then do binary diff of the two. From our experiments, the suffix doesn't change between the two builds. However, we need to match the build environment (path of kernel source, etc.) to make sure suffix from kpatch matches the kernel.

Quoted 7 lines

Quoted 3 lines

The goal here is to generate the same IR (hence <hash>) if file content is not changed. This way, <hash> value will change only for those changed files.

Hmm, how does kpatch match the fixed functions? They are modified so that they should get different IR (hash). Or do I miss anything, please?

If the static function are promoted to global function and the file containing static function changed, then that modified static function will appear to be a *new* function. Live change should be able to do it, right?

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

3. The goal of this patch is NOT to resolve different suffix by llvm (.llvm.[0-9]+). Instead, we are trying fix issues like:

Quoted 8 lines

Quoted 5 lines

Quoted 5 lines

# grep bpf_verifier_vlog /proc/kallsyms ffffffff81549f60 t bpf_verifier_vlog ffffffff8268b430 d bpf_verifier_vlog._entry ffffffff8282a958 d bpf_verifier_vlog._entry_ptr ffffffff82e12a1f d bpf_verifier_vlog.__already_done

And <function>.<symbol> notation seems to be used for static symbols defined inside a function.

That is correct.

Quoted 4 lines

I guess that it is used when the symbols stay statics. It would probably get additional ".llvm.<hash>" when it got promoted from static to global. But this probably never happens.

I have not see a case like this yet.

Quoted 2 lines

Do I get it correctly?

yes, that is correct.

Quoted 37 lines

It means that we have two different types of name changes:

1. .llvm.<hash> suffix

If we remove this suffix then we will not longer distinguish symbols which stayed static and which were promoted to global ones.

The result should be basically the same as without LTO. Some symbols might have duplicated name. But most symbols would have an unique one.

2. <function>.<symbol> name

In this case, <symbol> is _not_ suffix. It is actually the original symbol name.

The extra thing is the <function>. prefix.

These static variables seem to have special handling even with gcc without LTO. gcc adds an extra id instead, for example:

$> nm vmlinux | grep " _entry_ptr" | head
ffffffff82a2e800 d _entry_ptr.100135
ffffffff82a2e7f8 d _entry_ptr.100178
ffffffff82a32e70 d _entry_ptr.100798
ffffffff82a1e240 d _entry_ptr.10342
ffffffff82a33930 d _entry_ptr.104764
ffffffff82a339c8 d _entry_ptr.104830
ffffffff82a33928 d _entry_ptr.104871
ffffffff82a33920 d _entry_ptr.104877
ffffffff82a33918 d _entry_ptr.104893
ffffffff82a339c0 d _entry_ptr.104905

Quoted 2 lines

$> nm vmlinux | grep panic_console_dropped ffffffff853618e0 b panic_console_dropped.54158

IIRC, yes, these 'id' might change if source code changed.

Quoted 21 lines

Effect from the tracers POV?

1. .llvm.<hash> suffix

The names without the .llvm.<hash> suffix are the same as without LTO. This is probably why commit 8b8e6b5d3b013b0b ("kallsyms: strip ThinLTO hashes from static functions") worked. The tracers probably wanted to access only the symbols with uniqueue names

2. <function>.<symbol> name

The name without the .<symbol> suffix is the same as the function name. The result are duplicated function names.

I do not understand why this was not a problem for tracers. Note that this is pretty common. _entry and _entry_ptr are added into any function calling printk().

Quoted 6 lines

Quoted 3 lines

It seems to be working only by chance. Maybe, the tracers always take the first matched symbol. And the function name, without any suffix, is always the first one in the sorted list.

Note this only happens in LTO mode. Maybe lto kernel is not used wide enough to discover this issue?

Likely.

Quoted 31 lines

Quoted 24 lines

Effect from livepatching POV:

1. .llvm.<hash> suffix

Comparing the full symbol name looks fragile to me because the <hash> might change.

IMHO, it would be better to compare the names without the .llvm.<hash> suffix even for livepatches.

2. <function>.<symbol> name

The removal of <.symbol> suffix is a bad idea. The livepatch code is not able to distinguish the symbol of the <function> and static variables defined in this function.

IMHO, it would be better to compare the full
<function>.<symbol> name.

Quoted 6 lines

Result:

IMHO, cleanup_symbol_name() should remove only .llwn.* suffix. And it should be used for both tracers and livepatching.

Does this makes any sense?

Quoted 8 lines

Song, does this fix the problem?

I only checked llvm15 and llvm17, not sure what kind of suffix'es used for early llvm (>= llvm11).
Nick, could you help answer this question? What kind of suffix are used for lto when promoting a local symbol to a global one, considering all versions of llvm >= 11 since llvm 11 is the minimum supported version for kernel build.

For ToT for this case, the call chain backtrace looks like:

ModuleSummaryIndex::getGlobalNameForLocal FunctionImportGlobalProcessing::getPromotedName FunctionImportGlobalProcessing::processGlobalForThinLTO

This has been the case since release/11.x.

LLVM uses different mangling schemes for different places. For example, function specialization (that occurs when sinking a constant into a function) may rename a function from foo to something like foo.42 where a dot followed by a monotonically increasing counter is used. Numbers before may be missing from other symbols (where's .41?) if they are DCE'd (perhaps because they were inlined and have no more callers). That is done by:

ValueSymbolTable::makeUniqueName which is eventually called by FunctionSpecializer::createSpecialization.

That is the cause of common warnings from modpost.

There are likely numerous other special manglings done through llvm. The above two are slightly more generic, but other passes may do something more ad-hoc.

Quoted 5 lines

Quoted 5 lines

Best Regards,
Petr