Skip to contentSkip to conversation

objtool/klp: klp-build LTO support and tests

PATCH Superseded Song Liu 2 participants 14 messages Feb 19 – Feb 24

Subsystems (3)

v2 of 4 Superseded Latest is v4 →

lore
Share & tools
lore

Song Liu

author maintainer
#0 lore raw

v2 cover objtool/klp: klp-build LTO support and tests

Add support for LTO in klp-build toolchain. The key changes are to the symbol correlation logic.Basically, we want to:

1. Match symbols with differerent .llvm.<hash> suffixes, e.g., foo.llvm.123 to foo.llvm.456.
2. Match local symbols with promoted global symbols, e.g., local foo with global foo.llvm.123.

1/8 and 2/8 are small cleanup/fix for existing code.
3/8 through 7/8 contains the core logic changes to correlate_symbols(). 8/8 contains tests for klp-build toolchain.

Changes v1 => v2:
1. Error out on ambiguous .llvm.<hash>

Song Liu (8):
objtool/klp: Remove redundent strcmp in correlate_symbols objtool/klp: Remove trailing '_' in demangle_name() objtool/klp: Use sym->demangled_name for symbol_name hash objtool/klp: Also demangle global objects objtool/klp: Remove .llvm suffix in demangle_name() objtool/klp: Match symbols based on demangled_name for global
variables
objtool/klp: Correlate locals to globals
livepatch: Add tests for klp-build toolchain

 kernel/livepatch/Kconfig                      |  20 +++
 kernel/livepatch/Makefile                     |   2 +
 kernel/livepatch/tests/Makefile               |   6 +
 kernel/livepatch/tests/klp_test_module.c      | 111 ++++++++++++++
 kernel/livepatch/tests/klp_test_module.h      |   8 +
 kernel/livepatch/tests/klp_test_vmlinux.c     | 138 ++++++++++++++++++
 kernel/livepatch/tests/klp_test_vmlinux.h     |  16 ++
 kernel/livepatch/tests/klp_test_vmlinux_aux.c |  59 ++++++++
 tools/objtool/elf.c                           |  95 +++++++++---
 tools/objtool/include/objtool/elf.h           |   3 +
 tools/objtool/klp-diff.c                      |  92 +++++++++++-
 .../selftests/livepatch/test_patches/README   |  15 ++
 .../test_patches/klp_test_hash_change.patch   |  30 ++++
 .../test_patches/klp_test_module.patch        |  18 +++
 .../klp_test_nonstatic_to_static.patch        |  40 +++++
 .../klp_test_static_to_nonstatic.patch        |  39 +++++
 .../test_patches/klp_test_vmlinux.patch       |  18 +++
 17 files changed, 688 insertions(+), 22 deletions(-)
 create mode 100644 kernel/livepatch/tests/Makefile
 create mode 100644 kernel/livepatch/tests/klp_test_module.c
 create mode 100644 kernel/livepatch/tests/klp_test_module.h
 create mode 100644 kernel/livepatch/tests/klp_test_vmlinux.c
 create mode 100644 kernel/livepatch/tests/klp_test_vmlinux.h
 create mode 100644 kernel/livepatch/tests/klp_test_vmlinux_aux.c
 create mode 100644 tools/testing/selftests/livepatch/test_patches/README
 create mode 100644 tools/testing/selftests/livepatch/test_patches/klp_test_hash_change.patch
 create mode 100644 tools/testing/selftests/livepatch/test_patches/klp_test_module.patch
 create mode 100644 tools/testing/selftests/livepatch/test_patches/klp_test_nonstatic_to_static.patch
 create mode 100644 tools/testing/selftests/livepatch/test_patches/klp_test_static_to_nonstatic.patch
 create mode 100644 tools/testing/selftests/livepatch/test_patches/klp_test_vmlinux.patch

--
2.47.3

author maintainer
#1 lore raw

PATCH v2 1/8 objtool/klp: Remove redundent strcmp in correlate_symbols

find_global_symbol_by_name() already compares names of the two symbols, so there is no need to compare them again.

---
 tools/objtool/klp-diff.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
tools/objtool/klp-diff.c +1 -1
index a3198a63c2f0..57606bc3390a 100644
@@ -454,7 +454,7 @@ static int correlate_symbols(struct elfs *e)
 
 		sym2 = find_global_symbol_by_name(e->patched, sym1->name);
 
-		if (sym2 && !sym2->twin && !strcmp(sym1->name, sym2->name)) {
+		if (sym2 && !sym2->twin) {
 			sym1->twin = sym2;
 			sym2->twin = sym1;
 		}
Signature · 1 line
2.47.3
author maintainer
#2 lore raw

PATCH v2 2/8 objtool/klp: Remove trailing '_' in demangle_name()

In reply to Song Liu

With CONFIG_LTO_CLANG_THIN, it is possible to have nested __UNIQUE_ID_, such as:

__UNIQUE_ID_addressable___UNIQUE_ID_pci_invalid_bar_694_695

To remove both trailing numbers, also remove trailing '_'.

Also add comments to demangle_name().

---
 tools/objtool/elf.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)
tools/objtool/elf.c +14 -1
index 2c02c7b49265..0d93e8496e8d 100644
@@ -441,6 +441,19 @@ static int read_sections(struct elf *elf)
 	return 0;
 }
 
+/*
+ * Remove number suffix of a symbol.
+ *
+ * Specifically, remove trailing numbers for "__UNIQUE_ID_" symbols and
+ * symbols with '.'.
+ *
+ * With CONFIG_LTO_CLANG_THIN, it is possible to have nested __UNIQUE_ID_,
+ * such as
+ *
+ *   __UNIQUE_ID_addressable___UNIQUE_ID_pci_invalid_bar_694_695
+ *
+ * to remove both trailing numbers, also remove trailing '_'.
+ */
 static const char *demangle_name(struct symbol *sym)
 {
 	char *str;
@@ -463,7 +476,7 @@ static const char *demangle_name(struct symbol *sym)
 	for (int i = strlen(str) - 1; i >= 0; i--) {
 		char c = str[i];
 
-		if (!isdigit(c) && c != '.') {
+		if (!isdigit(c) && c != '.' && c != '_') {
 			str[i + 1] = '\0';
 			break;
 		}
Signature · 1 line
2.47.3
author maintainer
#3 lore raw

PATCH v2 3/8 objtool/klp: Use sym->demangled_name for symbol_name hash

In reply to Song Liu

For klp-build with LTO, it is necessary to correlate demangled symbols, e.g., correlate foo.llvm.<num 1> and foo.llvm.<num 2>. However, these two symbols do not have the same str_hash(name). To be able to correlate the two symbols, calculate hash based on demanged_name, so that these two symbols have the same hash.

No functional changes intended.

---
 tools/objtool/elf.c | 58 +++++++++++++++++++++++++++++++--------------
 1 file changed, 40 insertions(+), 18 deletions(-)
tools/objtool/elf.c +40 -18
index 0d93e8496e8d..c784a0484270 100644
@@ -26,11 +26,18 @@
 #include <objtool/elf.h>
 #include <objtool/warn.h>
 
+static ssize_t demangled_name_len(const char *name);
+
 static inline u32 str_hash(const char *str)
 {
 	return jhash(str, strlen(str), 0);
 }
 
+static inline u32 str_hash_demangled(const char *str)
+{
+	return jhash(str, demangled_name_len(str), 0);
+}
+
 #define __elf_table(name)	(elf->name##_hash)
 #define __elf_bits(name)	(elf->name##_bits)
 
@@ -294,7 +301,7 @@ static struct symbol *find_local_symbol_by_file_and_name(const struct elf *elf,
 {
 	struct symbol *sym;
 
-	elf_hash_for_each_possible(symbol_name, sym, name_hash, str_hash(name)) {
+	elf_hash_for_each_possible(symbol_name, sym, name_hash, str_hash_demangled(name)) {
 		if (sym->bind == STB_LOCAL && sym->file == file &&
 		    !strcmp(sym->name, name)) {
 			return sym;
@@ -308,7 +315,7 @@ struct symbol *find_global_symbol_by_name(const struct elf *elf, const char *nam
 {
 	struct symbol *sym;
 
-	elf_hash_for_each_possible(symbol_name, sym, name_hash, str_hash(name)) {
+	elf_hash_for_each_possible(symbol_name, sym, name_hash, str_hash_demangled(name)) {
 		if (!strcmp(sym->name, name) && !is_local_sym(sym))
 			return sym;
 	}
@@ -441,6 +448,28 @@ static int read_sections(struct elf *elf)
 	return 0;
 }
 
+/*
+ * Returns desired length of the demangled name.
+ * If name doesn't need demangling, return strlen(name).
+ */
+static ssize_t demangled_name_len(const char *name)
+{
+	ssize_t len;
+
+	if (!strstarts(name, "__UNIQUE_ID_") && !strchr(name, '.'))
+		return strlen(name);
+
+	for (len = strlen(name) - 1; len >= 0; len--) {
+		char c = name[len];
+
+		if (!isdigit(c) && c != '.' && c != '_')
+			break;
+	}
+	if (len <= 0)
+		return strlen(name);
+	return len;
+}
+
 /*
  * Remove number suffix of a symbol.
  *
@@ -457,6 +486,7 @@ static int read_sections(struct elf *elf)
 static const char *demangle_name(struct symbol *sym)
 {
 	char *str;
+	ssize_t len;
 
 	if (!is_local_sym(sym))
 		return sym->name;
@@ -464,24 +494,16 @@ static const char *demangle_name(struct symbol *sym)
 	if (!is_func_sym(sym) && !is_object_sym(sym))
 		return sym->name;
 
-	if (!strstarts(sym->name, "__UNIQUE_ID_") && !strchr(sym->name, '.'))
+	len = demangled_name_len(sym->name);
+	if (len == strlen(sym->name))
 		return sym->name;
 
-	str = strdup(sym->name);
+	str = strndup(sym->name, len);
 	if (!str) {
 		ERROR_GLIBC("strdup");
 		return NULL;
 	}
 
-	for (int i = strlen(str) - 1; i >= 0; i--) {
-		char c = str[i];
-
-		if (!isdigit(c) && c != '.' && c != '_') {
-			str[i + 1] = '\0';
-			break;
-		}
-	}
-
 	return str;
 }
 
@@ -517,9 +539,13 @@ static int elf_add_symbol(struct elf *elf, struct symbol *sym)
 		entry = &sym->sec->symbol_list;
 	list_add(&sym->list, entry);
 
+	sym->demangled_name = demangle_name(sym);
+	if (!sym->demangled_name)
+		return -1;
+
 	list_add_tail(&sym->global_list, &elf->symbols);
 	elf_hash_add(symbol, &sym->hash, sym->idx);
-	elf_hash_add(symbol_name, &sym->name_hash, str_hash(sym->name));
+	elf_hash_add(symbol_name, &sym->name_hash, str_hash(sym->demangled_name));
 
 	if (is_func_sym(sym) &&
 	    (strstarts(sym->name, "__pfx_") ||
@@ -543,10 +569,6 @@ static int elf_add_symbol(struct elf *elf, struct symbol *sym)
 
 	sym->pfunc = sym->cfunc = sym;
 
-	sym->demangled_name = demangle_name(sym);
-	if (!sym->demangled_name)
-		return -1;
-
 	return 0;
 }
 
Signature · 1 line
2.47.3
author maintainer
#4 lore raw

PATCH v2 4/8 objtool/klp: Also demangle global objects

In reply to Song Liu

With CONFIG_LTO_CLANG_THIN, it is possible to have global __UNIQUE_ID, such as:

FUNC GLOBAL HIDDEN 19745 __UNIQUE_ID_quirk_amd_nb_node_458

Also demangle global objects.

---
 tools/objtool/elf.c | 3 ---
 1 file changed, 3 deletions(-)
tools/objtool/elf.c +0 -3
index c784a0484270..d66452d66fb4 100644
@@ -488,9 +488,6 @@ static const char *demangle_name(struct symbol *sym)
 	char *str;
 	ssize_t len;
 
-	if (!is_local_sym(sym))
-		return sym->name;
-
 	if (!is_func_sym(sym) && !is_object_sym(sym))
 		return sym->name;
 
Signature · 1 line
2.47.3
author maintainer
#5 lore raw

PATCH v2 5/8 objtool/klp: Remove .llvm suffix in demangle_name()

In reply to Song Liu

Remove .llvm suffix, so that we can correlate foo.llvm.<hash 1> and foo.llvm.<hash 2>.

---
 tools/objtool/elf.c | 8 ++++++++
 1 file changed, 8 insertions(+)
tools/objtool/elf.c +8 -0
index d66452d66fb4..efb13ec0a89d 100644
@@ -455,10 +455,15 @@ static int read_sections(struct elf *elf)
 static ssize_t demangled_name_len(const char *name)
 {
 	ssize_t len;
+	const char *p;
 
 	if (!strstarts(name, "__UNIQUE_ID_") && !strchr(name, '.'))
 		return strlen(name);
 
+	p = strstr(name, ".llvm.");
+	if (p)
+		return p - name;
+
 	for (len = strlen(name) - 1; len >= 0; len--) {
 		char c = name[len];
 
@@ -482,6 +487,9 @@ static ssize_t demangled_name_len(const char *name)
  *   __UNIQUE_ID_addressable___UNIQUE_ID_pci_invalid_bar_694_695
  *
  * to remove both trailing numbers, also remove trailing '_'.
+ *
+ * For symbols with llvm suffix, i.e., foo.llvm.<hash>, remove the
+ * .llvm.<hash> part.
  */
 static const char *demangle_name(struct symbol *sym)
 {
Signature · 1 line
2.47.3
author maintainer
#6 lore raw

PATCH v2 6/8 objtool/klp: Match symbols based on demangled_name for global variables

In reply to Song Liu

correlate_symbols will always try to match full name first. If there is no match, try match only demangled_name.

In very rare cases, it is possible to have multiple foo.llvm.<hash> in the same kernel. Whenever there is ambiguity like this, fail the klp diff.

---
 tools/objtool/elf.c                 | 13 +++++++
 tools/objtool/include/objtool/elf.h |  3 ++
 tools/objtool/klp-diff.c            | 59 +++++++++++++++++++++++++++++
 3 files changed, 75 insertions(+)
tools/objtool/elf.c +13 -0
index efb13ec0a89d..5ddbfa8f8701 100644
@@ -323,6 +323,19 @@ struct symbol *find_global_symbol_by_name(const struct elf *elf, const char *nam
 	return NULL;
 }
 
+void iterate_global_symbol_by_demangled_name(const struct elf *elf,
+					     const char *demangled_name,
+					     void (*process)(struct symbol *sym, void *data),
+					     void *data)
+{
+	struct symbol *sym;
+
+	elf_hash_for_each_possible(symbol_name, sym, name_hash, str_hash(demangled_name)) {
+		if (!strcmp(sym->demangled_name, demangled_name) && !is_local_sym(sym))
+			process(sym, data);
+	}
+}
+
 struct reloc *find_reloc_by_dest_range(const struct elf *elf, struct section *sec,
 				     unsigned long offset, unsigned int len)
 {
tools/objtool/include/objtool/elf.h +3 -0
index e12c516bd320..25573e5af76e 100644
@@ -186,6 +186,9 @@ struct symbol *find_func_by_offset(struct section *sec, unsigned long offset);
 struct symbol *find_symbol_by_offset(struct section *sec, unsigned long offset);
 struct symbol *find_symbol_by_name(const struct elf *elf, const char *name);
 struct symbol *find_global_symbol_by_name(const struct elf *elf, const char *name);
+void iterate_global_symbol_by_demangled_name(const struct elf *elf, const char *demangled_name,
+					     void (*process)(struct symbol *sym, void *data),
+					     void *data);
 struct symbol *find_symbol_containing(const struct section *sec, unsigned long offset);
 int find_symbol_hole_containing(const struct section *sec, unsigned long offset);
 struct reloc *find_reloc_by_dest(const struct elf *elf, struct section *sec, unsigned long offset);
tools/objtool/klp-diff.c +59 -0

73 lines not on this page · raw message

Signature · 1 line
2.47.3
maintainer
#7 lore raw

Re: [PATCH v2 6/8] objtool/klp: Match symbols based on demangled_name for global variables

Quoted 2 lines · Song Liu

correlate_symbols will always try to match full name first. If there is no match, try match only demangled_name.

In commit logs, please add "()" to function names, like correlate_symbols().

Quoted 8 lines

+++ b/tools/objtool/elf.c

tools/objtool/elf.c +3 -0
@@ -323,6 +323,19 @@ struct symbol *find_global_symbol_by_name(const struct elf *elf, const char *nam
 	return NULL;
 }
 
+void iterate_global_symbol_by_demangled_name(const struct elf *elf,
+					     const char *demangled_name,
+					     void (*process)(struct symbol *sym, void *data),

tools/objtool/elf.c struct symbol *find_global_symbol_by_name(const struct elf *elf, const char *nam · line 338

Quoted 10 lines
tools/objtool/elf.c +10 -0
+					     void *data)
+{
+	struct symbol *sym;
+
+	elf_hash_for_each_possible(symbol_name, sym, name_hash, str_hash(demangled_name)) {
+		if (!strcmp(sym->demangled_name, demangled_name) && !is_local_sym(sym))
+			process(sym, data);
+	}
+}
+

I think a saner interface would be something like:

struct symbol *find_global_demangled_symbol(const struct elf *elf, const char *demangled_name)

{
	struct symbol *ret = NULL;

	elf_hash_for_each_possible(symbol_name, sym, name_hash, str_hash(demangled_name)) {
		if (!is_local_sym(sym) && !strcmp(sym->demangled_name, demangled_name)) {
			if (ret)
				return ERR_PTR(-EEXIST);
			ret = sym;
		}
	}

	return ret;
}
Quoted 15 lines
tools/objtool/klp-diff.c +11 -0
@@ -453,8 +493,27 @@ static int correlate_symbols(struct elfs *e)
 			continue;
 
 		sym2 = find_global_symbol_by_name(e->patched, sym1->name);
+		if (sym2 && !sym2->twin) {
+			sym1->twin = sym2;
+			sym2->twin = sym1;
+		}
+	}
+
+	/*
+	 * Correlate globals with demangled_name.
+	 * A separate loop is needed because we want to finish all the
+	 * full name correlations first.
+	 */

tools/objtool/klp-diff.c static int correlate_symbols(struct elfs *e) · line 516

Quoted 10 lines
tools/objtool/klp-diff.c +8 -0
+	for_each_sym(e->orig, sym1) {
+		if (sym1->bind == STB_LOCAL || sym1->twin)
+			continue;
+
+		if (find_global_symbol_by_demangled_name(e->patched, sym1, &sym2))
+			return -1;
 
 		if (sym2 && !sym2->twin) {
+			WARN("correlate %s (original) to %s (patched)",
+			     sym1->name, sym2->name);

Since there's not actually any ambiguity at this point, do we actually need a warning?

Signature · 1 line
Josh
author maintainer
#8 lore raw

Re: [PATCH v2 6/8] objtool/klp: Match symbols based on demangled_name for global variables

Hi Josh,

Thanks for the review!

Quoted 7 lines · Josh Poimboeuf
Quoted 2 lines · Song Liu

correlate_symbols will always try to match full name first. If there is no match, try match only demangled_name.

In commit logs, please add "()" to function names, like correlate_symbols().

Noted.

Quoted 23 lines
Quoted 9 lines

+++ b/tools/objtool/elf.c

tools/objtool/elf.c +3 -0
@@ -323,6 +323,19 @@ struct symbol *find_global_symbol_by_name(const struct elf *elf, const char *nam
      return NULL;
 }
 
+void iterate_global_symbol_by_demangled_name(const struct elf *elf,
+                                          const char *demangled_name,
+                                          void (*process)(struct symbol *sym, void *data),

tools/objtool/elf.c struct symbol *find_global_symbol_by_name(const struct elf *elf, const char *nam · line 338

Quoted 10 lines
tools/objtool/elf.c +10 -0
+                                          void *data)
+{
+     struct symbol *sym;
+
+     elf_hash_for_each_possible(symbol_name, sym, name_hash, str_hash(demangled_name)) {
+             if (!strcmp(sym->demangled_name, demangled_name) && !is_local_sym(sym))
+                     process(sym, data);
+     }
+}
+

I think a saner interface would be something like:

struct symbol *find_global_demangled_symbol(const struct elf *elf, const char *demangled_name)

Quoted 13 lines
{
        struct symbol *ret = NULL;

        elf_hash_for_each_possible(symbol_name, sym, name_hash, str_hash(demangled_name)) {
                if (!is_local_sym(sym) && !strcmp(sym->demangled_name, demangled_name)) {
                        if (ret)
                                return ERR_PTR(-EEXIST);
                        ret = sym;
                }
        }

        return ret;
}

I had something similar to this initially. However, we need to check sym->twin, and skip symbols that already have correlations. For example, if we have foo.llvm.123 and foo.llvm.456 in the original kernel, and foo.llvm.123 and foo.llvm.789 in the patched kernel, we will match foo.llvm.456 to foo.llvm.789 without ambiguity. Since elf.c doesn't touch sym->twin at all, I think it is cleaner to keep this logic in klp-diff.c. If you think it is OK to have elf.c handle this, we can do something like:

struct symbol *find_global_demangled_symbol(const struct elf *elf, const char *demangled_name)
{
struct symbol *ret = NULL;

elf_hash_for_each_possible(symbol_name, sym, name_hash,

str_hash(demangled_name)) {
                if (!is_local_sym(sym) &&
                    !strcmp(sym->demangled_name, demangled_name) &&
                    !sym->twin) {  /* We need to add this */
                        if (ret)
                                return ERR_PTR(-EEXIST);
                        ret = sym;
                }
        }

        return ret;
}

I personally like v2 patch better. But I wouldn't mind changing to the above version in v3.

Quoted 22 lines
Quoted 22 lines
tools/objtool/klp-diff.c +17 -0
@@ -453,8 +493,27 @@ static int correlate_symbols(struct elfs *e)
                      continue;
 
              sym2 = find_global_symbol_by_name(e->patched, sym1->name);
+             if (sym2 && !sym2->twin) {
+                     sym1->twin = sym2;
+                     sym2->twin = sym1;
+             }
+     }
+
+     /*
+      * Correlate globals with demangled_name.
+      * A separate loop is needed because we want to finish all the
+      * full name correlations first.
+      */
+     for_each_sym(e->orig, sym1) {
+             if (sym1->bind == STB_LOCAL || sym1->twin)
+                     continue;
+
+             if (find_global_symbol_by_demangled_name(e->patched, sym1, &sym2))
+                     return -1;
 
Quoted 6 lines

tools/objtool/klp-diff.c static int correlate_symbols(struct elfs *e) · line 516

Quoted 3 lines
tools/objtool/klp-diff.c +2 -0
              if (sym2 && !sym2->twin) {
+                     WARN("correlate %s (original) to %s (patched)",
+                          sym1->name, sym2->name);

Since there's not actually any ambiguity at this point, do we actually need a warning?

I cannot think of a case where this match is ambiguous, so yes, we can remove this warning.

Thanks,
Song

maintainer
#9 lore raw

Re: [PATCH v2 6/8] objtool/klp: Match symbols based on demangled_name for global variables

Quoted 59 lines · Song Liu
Quoted 17 lines

tools/objtool/elf.c struct symbol *find_global_symbol_by_name(const struct elf *elf, const char *nam · line 338

Quoted 13 lines
tools/objtool/elf.c +13 -0
+void iterate_global_symbol_by_demangled_name(const struct elf *elf,
+                                          const char *demangled_name,
+                                          void (*process)(struct symbol *sym, void *data),
+                                          void *data)
+{
+     struct symbol *sym;
+
+     elf_hash_for_each_possible(symbol_name, sym, name_hash, str_hash(demangled_name)) {
+             if (!strcmp(sym->demangled_name, demangled_name) && !is_local_sym(sym))
+                     process(sym, data);
+     }
+}
+

I think a saner interface would be something like:

struct symbol *find_global_demangled_symbol(const struct elf *elf, const char *demangled_name)

Quoted 13 lines
{
        struct symbol *ret = NULL;

        elf_hash_for_each_possible(symbol_name, sym, name_hash, str_hash(demangled_name)) {
                if (!is_local_sym(sym) && !strcmp(sym->demangled_name, demangled_name)) {
                        if (ret)
                                return ERR_PTR(-EEXIST);
                        ret = sym;
                }
        }

        return ret;
}

I had something similar to this initially. However, we need to check sym->twin, and skip symbols that already have correlations. For example, if we have foo.llvm.123 and foo.llvm.456 in the original kernel, and foo.llvm.123 and foo.llvm.789 in the patched kernel, we will match foo.llvm.456 to foo.llvm.789 without ambiguity. Since elf.c doesn't touch sym->twin at all, I think it is cleaner to keep this logic in klp-diff.c. If you think it is OK to have elf.c handle this, we can do something like:

struct symbol *find_global_demangled_symbol(const struct elf *elf, const char *demangled_name)
{
struct symbol *ret = NULL;

elf_hash_for_each_possible(symbol_name, sym, name_hash,

str_hash(demangled_name)) {
                if (!is_local_sym(sym) &&
                    !strcmp(sym->demangled_name, demangled_name) &&
                    !sym->twin) {  /* We need to add this */
                        if (ret)
                                return ERR_PTR(-EEXIST);
                        ret = sym;
                }
        }

        return ret;
}
Quoted 2 lines

I personally like v2 patch better. But I wouldn't mind changing to the above version in v3.

Ah, I see. Yeah, the v2 implementation seems ok.

Signature · 1 line
Josh
author maintainer
#10 lore raw

PATCH v2 7/8 objtool/klp: Correlate locals to globals

In reply to Song Liu

Allow correlating original locals to patched globals, and vice versa. This is needed when:

1. User adds/removes "static" for a function.
2. CONFIG_LTO_CLANG_THIN promotes local functions and objects to global and add .llvm.<hash> suffix.

Given this is a less common scenario, show warnings when this is needed.

---
 tools/objtool/klp-diff.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)
tools/objtool/klp-diff.c +31 -0
index 147e7e7176fb..07fcaca46160 100644
@@ -519,6 +519,37 @@ static int correlate_symbols(struct elfs *e)
 		}
 	}
 
+	/* Correlate original locals with patched globals */
+	for_each_sym(e->orig, sym1) {
+		if (sym1->twin || dont_correlate(sym1) || !is_local_sym(sym1))
+			continue;
+		sym2 = find_global_symbol_by_name(e->patched, sym1->name);
+		if (!sym2 && find_global_symbol_by_demangled_name(e->patched, sym1, &sym2))
+			return -1;
+		if (sym2 && !sym2->twin) {
+			sym1->twin = sym2;
+			sym2->twin = sym1;
+			WARN("correlate LOCAL %s (original) to GLOBAL %s (patched)",
+			     sym1->name, sym2->name);
+		}
+	}
+
+	/* Correlate original globals with patched locals */
+	for_each_sym(e->patched, sym2) {
+		if (sym2->twin || dont_correlate(sym2) || !is_local_sym(sym2))
+			continue;
+		sym1 = find_global_symbol_by_name(e->orig, sym2->name);
+		if (!sym1 && find_global_symbol_by_demangled_name(e->patched, sym2, &sym1))
+			return -1;
+
+		if (sym1 && !sym1->twin) {
+			sym2->twin = sym1;
+			sym1->twin = sym2;
+			WARN("correlate GLOBAL %s (origial) to LOCAL %s (patched)",
+			     sym1->name, sym2->name);
+		}
+	}
+
 	for_each_sym(e->orig, sym1) {
 		if (sym1->twin || dont_correlate(sym1))
 			continue;
Signature · 1 line
2.47.3
maintainer
#11 lore raw

Re: [PATCH v2 7/8] objtool/klp: Correlate locals to globals

Quoted 9 lines · Song Liu

+++ b/tools/objtool/klp-diff.c

tools/objtool/klp-diff.c +4 -0
@@ -519,6 +519,37 @@ static int correlate_symbols(struct elfs *e)
 		}
 	}
 
+	/* Correlate original locals with patched globals */
+	for_each_sym(e->orig, sym1) {
+		if (sym1->twin || dont_correlate(sym1) || !is_local_sym(sym1))
+			continue;

tools/objtool/klp-diff.c static int correlate_symbols(struct elfs *e) · line 535

Quoted 10 lines
tools/objtool/klp-diff.c +10 -0
+		sym2 = find_global_symbol_by_name(e->patched, sym1->name);
+		if (!sym2 && find_global_symbol_by_demangled_name(e->patched, sym1, &sym2))
+			return -1;
+		if (sym2 && !sym2->twin) {
+			sym1->twin = sym2;
+			sym2->twin = sym1;
+			WARN("correlate LOCAL %s (original) to GLOBAL %s (patched)",
+			     sym1->name, sym2->name);
+		}
+	}

Try to follow the existing newline conventions which break the code into "paragraphs", like:

	for_each_sym(e->orig, sym1) {
		if (sym1->twin || dont_correlate(sym1) || !is_local_sym(sym1))
			continue;

		sym2 = find_global_symbol_by_name(e->patched, sym1->name);
		if (!sym2 && find_global_symbol_by_demangled_name(e->patched, sym1, &sym2))
			return -1;

		if (sym2 && !sym2->twin) {
			sym1->twin = sym2;
			sym2->twin = sym1;
			WARN("correlate LOCAL %s (original) to GLOBAL %s (patched)",
			     sym1->name, sym2->name);
		}
	}

tools/objtool/klp-diff.c static int correlate_symbols(struct elfs *e) · line 542

Quoted 7 lines
tools/objtool/klp-diff.c +7 -0
+
+	/* Correlate original globals with patched locals */
+	for_each_sym(e->patched, sym2) {
+		if (sym2->twin || dont_correlate(sym2) || !is_local_sym(sym2))
+			continue;
+		sym1 = find_global_symbol_by_name(e->orig, sym2->name);
+		if (!sym1 && find_global_symbol_by_demangled_name(e->patched, sym2, &sym1))
								  ^^^^^^^^^^

should be e->orig?

Signature · 1 line
Josh
author maintainer
#12 lore raw

Re: [PATCH v2 7/8] objtool/klp: Correlate locals to globals

Quoted 48 lines · Josh Poimboeuf
Quoted 9 lines · Song Liu

+++ b/tools/objtool/klp-diff.c

tools/objtool/klp-diff.c +4 -0
@@ -519,6 +519,37 @@ static int correlate_symbols(struct elfs *e)
              }
      }
 
+     /* Correlate original locals with patched globals */
+     for_each_sym(e->orig, sym1) {
+             if (sym1->twin || dont_correlate(sym1) || !is_local_sym(sym1))
+                     continue;

tools/objtool/klp-diff.c static int correlate_symbols(struct elfs *e) · line 535

Quoted 10 lines
tools/objtool/klp-diff.c +10 -0
+             sym2 = find_global_symbol_by_name(e->patched, sym1->name);
+             if (!sym2 && find_global_symbol_by_demangled_name(e->patched, sym1, &sym2))
+                     return -1;
+             if (sym2 && !sym2->twin) {
+                     sym1->twin = sym2;
+                     sym2->twin = sym1;
+                     WARN("correlate LOCAL %s (original) to GLOBAL %s (patched)",
+                          sym1->name, sym2->name);
+             }
+     }

Try to follow the existing newline conventions which break the code into "paragraphs", like:

        for_each_sym(e->orig, sym1) {
                if (sym1->twin || dont_correlate(sym1) || !is_local_sym(sym1))
                        continue;

                sym2 = find_global_symbol_by_name(e->patched, sym1->name);
                if (!sym2 && find_global_symbol_by_demangled_name(e->patched, sym1, &sym2))
                        return -1;

                if (sym2 && !sym2->twin) {
                        sym1->twin = sym2;
                        sym2->twin = sym1;
                        WARN("correlate LOCAL %s (original) to GLOBAL %s (patched)",
                             sym1->name, sym2->name);
                }
        }

tools/objtool/klp-diff.c static int correlate_symbols(struct elfs *e) · line 542

Quoted 7 lines
tools/objtool/klp-diff.c +7 -0
+
+     /* Correlate original globals with patched locals */
+     for_each_sym(e->patched, sym2) {
+             if (sym2->twin || dont_correlate(sym2) || !is_local_sym(sym2))
+                     continue;
+             sym1 = find_global_symbol_by_name(e->orig, sym2->name);
+             if (!sym1 && find_global_symbol_by_demangled_name(e->patched, sym2, &sym1))
Quoted 3 lines
                                                                  ^^^^^^^^^^

should be e->orig?

Yes, exactly...

Thanks for catching this bug!
Song

author maintainer
#13 lore raw

PATCH v2 8/8 livepatch: Add tests for klp-build toolchain

In reply to Song Liu

Add selftests for the klp-build toolchain. This includes kernel side test code and .patch files. The tests cover both livepatch to vmlinux and kernel modules.

Check tools/testing/selftests/livepatch/test_patches/README for instructions to run these tests.

---

AI was used to wrote the test code and .patch files in this.

---
 kernel/livepatch/Kconfig                      |  20 +++
 kernel/livepatch/Makefile                     |   2 +
 kernel/livepatch/tests/Makefile               |   6 +
 kernel/livepatch/tests/klp_test_module.c      | 111 ++++++++++++++
 kernel/livepatch/tests/klp_test_module.h      |   8 +
 kernel/livepatch/tests/klp_test_vmlinux.c     | 138 ++++++++++++++++++
 kernel/livepatch/tests/klp_test_vmlinux.h     |  16 ++
 kernel/livepatch/tests/klp_test_vmlinux_aux.c |  59 ++++++++
 .../selftests/livepatch/test_patches/README   |  15 ++
 .../test_patches/klp_test_hash_change.patch   |  30 ++++
 .../test_patches/klp_test_module.patch        |  18 +++
 .../klp_test_nonstatic_to_static.patch        |  40 +++++
 .../klp_test_static_to_nonstatic.patch        |  39 +++++
 .../test_patches/klp_test_vmlinux.patch       |  18 +++
 14 files changed, 520 insertions(+)
 create mode 100644 kernel/livepatch/tests/Makefile
 create mode 100644 kernel/livepatch/tests/klp_test_module.c
 create mode 100644 kernel/livepatch/tests/klp_test_module.h
 create mode 100644 kernel/livepatch/tests/klp_test_vmlinux.c
 create mode 100644 kernel/livepatch/tests/klp_test_vmlinux.h
 create mode 100644 kernel/livepatch/tests/klp_test_vmlinux_aux.c
 create mode 100644 tools/testing/selftests/livepatch/test_patches/README
 create mode 100644 tools/testing/selftests/livepatch/test_patches/klp_test_hash_change.patch
 create mode 100644 tools/testing/selftests/livepatch/test_patches/klp_test_module.patch
 create mode 100644 tools/testing/selftests/livepatch/test_patches/klp_test_nonstatic_to_static.patch
 create mode 100644 tools/testing/selftests/livepatch/test_patches/klp_test_static_to_nonstatic.patch
 create mode 100644 tools/testing/selftests/livepatch/test_patches/klp_test_vmlinux.patch
kernel/livepatch/Kconfig +20 -0
index 4c0a9c18d0b2..852049601389 100644
@@ -30,3 +30,23 @@ config KLP_BUILD
 	select OBJTOOL
 	help
 	  Enable klp-build support
+
+config KLP_TEST
+	bool "Livepatch test code"
+	depends on LIVEPATCH
+	help
+	  Dummy kernel code for testing the klp-build livepatch toolchain.
+	  Provides built-in vmlinux functions with sysfs interfaces for
+	  verifying livepatches.
+
+	  If unsure, say N.
+
+config KLP_TEST_MODULE
+	tristate "Livepatch test module (klp_test_module)"
+	depends on KLP_TEST && m
+	help
+	  Test module for livepatch testing. Dummy kernel module for
+	  testing the klp-build toolchain. Provides sysfs interfaces for
+	  verifying livepatches.
+
+	  If unsure, say N.
kernel/livepatch/Makefile +2 -0
index cf03d4bdfc66..751080a62cec 100644
@@ -2,3 +2,5 @@
 obj-$(CONFIG_LIVEPATCH) += livepatch.o
 
 livepatch-objs := core.o patch.o shadow.o state.o transition.o
+
+obj-$(CONFIG_KLP_TEST) += tests/
kernel/livepatch/tests/Makefile new file +6 -0
new file mode 100644
index 000000000000..82ae48f54abe
@@ -0,0 +1,6 @@
+# SPDX-License-Identifier: GPL-2.0
+obj-y                           += klp_test_vmlinux_all.o
+obj-$(CONFIG_KLP_TEST_MODULE)   += klp_test_module.o
+
+klp_test_vmlinux_all-y := klp_test_vmlinux.o \
+			   klp_test_vmlinux_aux.o
kernel/livepatch/tests/klp_test_module.c new file +111 -0

111 lines not on this page · raw message

kernel/livepatch/tests/klp_test_module.h new file +8 -0
new file mode 100644
index 000000000000..56a766f4744b
@@ -0,0 +1,8 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+#ifndef _KLP_TEST_MODULE_H
+#define _KLP_TEST_MODULE_H
+
+ssize_t klp_test_module_func1(char *buf, int len);
+ssize_t klp_test_module_func2(char *buf, int len);
+
+#endif /* _KLP_TEST_MODULE_H */
kernel/livepatch/tests/klp_test_vmlinux.c new file +138 -0

138 lines not on this page · raw message

kernel/livepatch/tests/klp_test_vmlinux.h new file +16 -0
new file mode 100644
index 000000000000..56d9f7b6d350
@@ -0,0 +1,16 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+#ifndef _KLP_TEST_VMLINUX_H
+#define _KLP_TEST_VMLINUX_H
+
+#include <linux/kobject.h>
+
+extern struct kobject *klp_test_kobj;
+
+ssize_t klp_test_vmlinux_func1(char *buf, int len);
+ssize_t klp_test_vmlinux_func2(char *buf, int len);
+ssize_t klp_test_vmlinux_func3(char *buf, int len);
+
+int klp_test_vmlinux_aux_init(struct kobject *parent);
+void klp_test_vmlinux_aux_exit(struct kobject *parent);
+
+#endif /* _KLP_TEST_VMLINUX_H */
kernel/livepatch/tests/klp_test_vmlinux_aux.c new file +59 -0

59 lines not on this page · raw message

tools/testing/selftests/livepatch/test_patches/README new file +15 -0
new file mode 100644
index 000000000000..8266348aab57
@@ -0,0 +1,15 @@
+This is folder contains patches to test the klp-build toolchain.
+
+To run the test:
+
+1. Enable CONFIG_KLP_TEST and CONFIG_KLP_TEST_MODULE, and build the kernel.
+
+2. Build these patches with:
+
+  ./scripts/livepatch/klp-build tools/testing/selftests/livepatch/test_patches/*.patch
+
+3. Verify the correctness with:
+
+  modprobe klp_test_module
+  kpatch load livepatch-patch.ko
+  grep -q unpatched /sys/kernel/klp_test/*/* && echo FAIL || echo PASS
tools/testing/selftests/livepatch/test_patches/klp_test_hash_change.patch new file +30 -0
new file mode 100644
index 000000000000..609d54d6d6f6
@@ -0,0 +1,30 @@
+Test ThinLTO .llvm.<hash> suffix handling.
+
+Modify a static __helper() function whose body change causes its
+.llvm.<hash> suffix to change under ThinLTO. Both klp_test_vmlinux.c
+and klp_test_vmlinux_aux.c define static __helper() with different
+bodies, so ThinLTO promotes both to globals with different hashes.
+This patch changes the __helper() in the aux file, which changes its
+hash, and klp-build must correctly match the old and new symbols.
+
+diff --git i/kernel/livepatch/tests/klp_test_vmlinux_aux.c w/kernel/livepatch/tests/klp_test_vmlinux_aux.c
+--- i/kernel/livepatch/tests/klp_test_vmlinux_aux.c
++++ w/kernel/livepatch/tests/klp_test_vmlinux_aux.c
+@@ -20,7 +20,7 @@
+ 	int i, sum = x;
+ 
+ 	for (i = 0; i < len; i++)
+-		sum += i + 10;
++		sum += i * 2 + 10;
+ 	if (sum > 1000)
+ 		sum = 0;
+ 	return sum;
+@@ -30,7 +30,7 @@
+ {
+ 	klp_test_vmlinux_var3 = __helper(klp_test_vmlinux_var3, len);
+ 
+-	return sysfs_emit(buf, "klp_test_vmlinux_func3 unpatched %d\n",
++	return sysfs_emit(buf, "klp_test_vmlinux_func3 hash_patched %d\n",
+ 			 klp_test_vmlinux_var3);
+ }
+ 
tools/testing/selftests/livepatch/test_patches/klp_test_module.patch new file +18 -0
new file mode 100644
index 000000000000..d86e75618136
@@ -0,0 +1,18 @@
+Test basic module patching.
+
+Patch a loadable module function to verify that klp-build can generate
+a livepatch for module code. Changes __klp_test_module_func1() output
+from "unpatched" to "patched".
+
+diff --git i/kernel/livepatch/tests/klp_test_module.c w/kernel/livepatch/tests/klp_test_module.c
+--- i/kernel/livepatch/tests/klp_test_module.c
++++ w/kernel/livepatch/tests/klp_test_module.c
+@@ -27,7 +27,7 @@
+ 	if (klp_test_module_var1 > 1000)
+ 		klp_test_module_var1 = 0;
+ 
+-	ret = sysfs_emit(buf, "klp_test_module_func1 unpatched %d\n",
++	ret = sysfs_emit(buf, "klp_test_module_func1 patched %d\n",
+ 			 klp_test_module_var1);
+ 	return ret;
+ }
tools/testing/selftests/livepatch/test_patches/klp_test_nonstatic_to_static.patch new file +40 -0
new file mode 100644
index 000000000000..f26711c6bfac
@@ -0,0 +1,40 @@
+Test nonstatic-to-static symbol change.
+
+Change klp_test_module_func2() from nonstatic (global) to static and
+remove its EXPORT_SYMBOL_GPL. Also remove its declaration from the
+header file. This tests klp-build's ability to handle symbol visibility
+changes where a function that was originally global becomes static in
+the patched kernel.
+
+diff --git i/kernel/livepatch/tests/klp_test_module.c w/kernel/livepatch/tests/klp_test_module.c
+--- i/kernel/livepatch/tests/klp_test_module.c
++++ w/kernel/livepatch/tests/klp_test_module.c
+@@ -49,16 +49,15 @@
+ 	if (klp_test_module_var2 > 1000)
+ 		klp_test_module_var2 = 0;
+
+-	ret = sysfs_emit(buf, "klp_test_module_func2 unpatched %d\n",
++	ret = sysfs_emit(buf, "klp_test_module_func2 patched_nts %d\n",
+ 			 klp_test_module_var2);
+ 	return ret;
+ }
+
+-ssize_t klp_test_module_func2(char *buf, int len)
++static noinline ssize_t klp_test_module_func2(char *buf, int len)
+ {
+ 	return __klp_test_module_func2(buf, len);
+ }
+-EXPORT_SYMBOL_GPL(klp_test_module_func2);
+
+ static ssize_t func1_show(struct kobject *kobj,
+ 			   struct kobj_attribute *attr, char *buf)
+diff --git i/kernel/livepatch/tests/klp_test_module.h w/kernel/livepatch/tests/klp_test_module.h
+--- i/kernel/livepatch/tests/klp_test_module.h
++++ w/kernel/livepatch/tests/klp_test_module.h
+@@ -3,6 +3,5 @@
+ #define _KLP_TEST_MODULE_H
+
+ ssize_t klp_test_module_func1(char *buf, int len);
+-ssize_t klp_test_module_func2(char *buf, int len);
+
+ #endif /* _KLP_TEST_MODULE_H */
tools/testing/selftests/livepatch/test_patches/klp_test_static_to_nonstatic.patch new file +39 -0
new file mode 100644
index 000000000000..673f6c42f698
@@ -0,0 +1,39 @@
+Test static-to-nonstatic symbol change.
+
+Change __klp_test_vmlinux_func2() from static to nonstatic (global).
+This tests klp-build's ability to handle symbol visibility changes
+where a function that was originally static becomes globally visible
+in the patched kernel.
+
+diff --git i/kernel/livepatch/tests/klp_test_vmlinux.c w/kernel/livepatch/tests/klp_test_vmlinux.c
+--- i/kernel/livepatch/tests/klp_test_vmlinux.c
++++ w/kernel/livepatch/tests/klp_test_vmlinux.c
+@@ -44,7 +44,7 @@
+ }
+ EXPORT_SYMBOL_GPL(klp_test_vmlinux_func1);
+
+-static noinline ssize_t __klp_test_vmlinux_func2(char *buf, int len)
++noinline ssize_t __klp_test_vmlinux_func2(char *buf, int len)
+ {
+ 	ssize_t ret = 0;
+ 	int i;
+@@ -55,7 +55,7 @@
+ 	if (klp_test_vmlinux_var2 > 1000)
+ 		klp_test_vmlinux_var2 = 0;
+
+-	ret = sysfs_emit(buf, "klp_test_vmlinux_func2 unpatched %d\n",
++	ret = sysfs_emit(buf, "klp_test_vmlinux_func2 patched_stn %d\n",
+ 			 klp_test_vmlinux_var2);
+ 	return ret;
+ }
+diff --git i/kernel/livepatch/tests/klp_test_vmlinux.h w/kernel/livepatch/tests/klp_test_vmlinux.h
+--- i/kernel/livepatch/tests/klp_test_vmlinux.h
++++ w/kernel/livepatch/tests/klp_test_vmlinux.h
+@@ -9,6 +9,7 @@
+ ssize_t klp_test_vmlinux_func1(char *buf, int len);
+ ssize_t klp_test_vmlinux_func2(char *buf, int len);
+ ssize_t klp_test_vmlinux_func3(char *buf, int len);
++ssize_t __klp_test_vmlinux_func2(char *buf, int len);
+
+ int klp_test_vmlinux_aux_init(struct kobject *parent);
+ void klp_test_vmlinux_aux_exit(struct kobject *parent);
tools/testing/selftests/livepatch/test_patches/klp_test_vmlinux.patch new file +18 -0
new file mode 100644
index 000000000000..8b1d91381728
@@ -0,0 +1,18 @@
+Test basic vmlinux patching.
+
+Patch a built-in vmlinux function to verify that klp-build can generate
+a livepatch for vmlinux code. Changes __klp_test_vmlinux_func1() output
+from "unpatched" to "patched".
+
+diff --git i/kernel/livepatch/tests/klp_test_vmlinux.c w/kernel/livepatch/tests/klp_test_vmlinux.c
+--- i/kernel/livepatch/tests/klp_test_vmlinux.c
++++ w/kernel/livepatch/tests/klp_test_vmlinux.c
+@@ -33,7 +33,7 @@
+ 
+ 	klp_test_vmlinux_var1 = __helper(klp_test_vmlinux_var1, len);
+ 
+-	ret = sysfs_emit(buf, "klp_test_vmlinux_func1 unpatched %d\n",
++	ret = sysfs_emit(buf, "klp_test_vmlinux_func1 patched %d\n",
+ 			 klp_test_vmlinux_var1);
+ 	return ret;
+ }
Signature · 1 line
2.47.3