Skip to contentSkip to conversation

objtool/klp: klp-build LTO support and tests

PATCH Superseded Song Liu 5 participants 26 messages Feb 26 – Mar 6

Subsystems (3)

v3 of 4 Superseded Latest is v4 →

lore
Share & tools
lore

Song Liu

author maintainer
#0 lore raw

v3 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 v2 => v3:
1. Fix a bug in global => local correlations (patch 7/8).
2. Remove a WARN().
3. Some empty line changes.

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                      |  93 +++++++++++-
 .../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, 689 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 v3 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
maintainer
#2 lore raw

Re: [PATCH v3 1/8] objtool/klp: Remove redundent strcmp in correlate_symbols

Quoted 13 lines · Song Liu

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 +0 -0
index a3198a63c2f0..57606bc3390a 100644

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

Quoted 7 lines
tools/objtool/klp-diff.c +1 -1
@@ -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;

Subject has a typo ("redundent"), and function names are missing "()":

Remove redundant strcmp() in correlate_symbols()

Signature · 1 line
Josh
author maintainer
#3 lore raw

PATCH v3 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
#4 lore raw

PATCH v3 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
maintainer
#5 lore raw

Re: [PATCH v3 3/8] objtool/klp: Use sym->demangled_name for symbol_name hash

Quoted 11 lines · Song Liu
tools/objtool/elf.c +11 -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);
+

tools/objtool/elf.c static int read_sections(struct elf *elf) · line 471

Quoted 10 lines
tools/objtool/elf.c +10 -0
+	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;
+}

This actually returns the index of the last char rather than the length. Should "len" be renamed to "idx" and then it can return "idx + 1"?

Signature · 1 line
Josh
author maintainer
#6 lore raw

PATCH v3 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
#7 lore raw

PATCH v3 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
#8 lore raw

PATCH v3 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            | 57 +++++++++++++++++++++++++++++
 3 files changed, 73 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 +57 -0

69 lines not on this page · raw message

Signature · 1 line
2.47.3
author maintainer
#9 lore raw

PATCH v3 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 | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)
tools/objtool/klp-diff.c +34 -0
index 92043da0ed0b..5cda965807a5 100644
@@ -517,6 +517,40 @@ 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->orig, 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
#10 lore raw

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

Quoted 26 lines · 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 | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)
tools/objtool/klp-diff.c +4 -0
index 92043da0ed0b..5cda965807a5 100644
@@ -517,6 +517,40 @@ 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 533

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

I think this correlation is deterministic so there's no need for the warning?

Quoted 8 lines
tools/objtool/klp-diff.c +8 -0
+		}
+	}
+
+	/* Correlate original globals with patched locals */
+	for_each_sym(e->patched, sym2) {
+		if (sym2->twin || dont_correlate(sym2) || !is_local_sym(sym2))
+			continue;
+

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

Quoted 10 lines
tools/objtool/klp-diff.c +10 -0
+		sym1 = find_global_symbol_by_name(e->orig, sym2->name);
+		if (!sym1 && find_global_symbol_by_demangled_name(e->orig, 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);
+		}

Ditto.

Signature · 1 line
Josh
author maintainer
#11 lore raw

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

Quoted 33 lines · Josh Poimboeuf
Quoted 31 lines · 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 | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)
tools/objtool/klp-diff.c +9 -0
index 92043da0ed0b..5cda965807a5 100644
@@ -517,6 +517,40 @@ 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;
+
Quoted 8 lines

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

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

I think this correlation is deterministic so there's no need for the warning?

Yes, we can remove this.

I also fixed 1/8 and 3/8. I will send v4 of patch 1/8 to 7/8, as we discuss more with 8/8.

Thanks,
Song

author maintainer
#12 lore raw

PATCH v3 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
maintainer
#13 lore raw

Re: [PATCH v3 8/8] livepatch: Add tests for klp-build toolchain

Hi,

I have a couple of questions before reviewing the code itself. See below. I removed the code completely as it seems better to have it compact. Sorry if it is too confusing in the end and I apologize for being late to the party. We can always merge the first 7 patches when they are settled and keep this one separate.

Quoted 4 lines · 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.

Quoted 8 lines

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.

This should go to the changelog directly. See new
Documentation/process/generated-content.rst.

Quoted 20 lines
---
 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
Quoted 8 lines
 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

We store test modules in tools/testing/selftests/livepatch/test_modules/ now. Could you move klp_test_module.c there, please? You might also reuse existing ones for the purpose perhaps.

What about vmlinux? I understand that it provides a lot more flexibility to have separate functions for testing but would it be somehow sufficient to use the existing (real) kernel functions? Like cmdline_proc_show() and such which we use everywhere else? Or would it be to limited? I am fine if you find it necessary in the end. I just think that reusing as much as possible is generally a good approach.

The patch mentiones kpatch in some places. Could you replace it, please?

And a little bit of bikeshedding at the end. I think it would be more descriptive if the new config options and tests (test modules) have klp-build somewhere in the name to keep it clear. What do you think?

Thanks for the patches!

Miroslav

author maintainer
#14 lore raw

Re: [PATCH v3 8/8] livepatch: Add tests for klp-build toolchain

Hi Miroslav,

Quoted 8 lines · Miroslav Benes

Hi,

I have a couple of questions before reviewing the code itself. See below. I removed the code completely as it seems better to have it compact. Sorry if it is too confusing in the end and I apologize for being late to the party. We can always merge the first 7 patches when they are settled and keep this one separate.

Yes, I was also thinking this patch will need more discussions than the rest of the set.

Quoted 9 lines · Song Liu
Quoted 8 lines

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.

Quoted 8 lines
Quoted 5 lines

---

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

This should go to the changelog directly. See new
Documentation/process/generated-content.rst.

Thanks for pointing this out. I didn't know we had this guidance.

Quoted 30 lines
Quoted 21 lines
---
 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
Quoted 8 lines
 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
Quoted 3 lines

We store test modules in tools/testing/selftests/livepatch/test_modules/ now. Could you move klp_test_module.c there, please? You might also reuse existing ones for the purpose perhaps.

IIUC, tools/testing/selftests/livepatch/test_modules/ is more like an out of tree module. In the case of testing klp-build, we prefer to have it to work the same as in-tree modules. This is important because klp-build is a toolchain, and any changes of in-tree Makefiles may cause issues with klp-build. Current version can catch these issues easily. If we build the test module as an OOT module, we may miss some of these issues. In the longer term, we should consider adding klp-build support to build livepatch for OOT modules. But for now, good test coverage for in-tree modules are more important.

Quoted 7 lines

What about vmlinux? I understand that it provides a lot more flexibility to have separate functions for testing but would it be somehow sufficient to use the existing (real) kernel functions? Like cmdline_proc_show() and such which we use everywhere else? Or would it be to limited? I am fine if you find it necessary in the end. I just think that reusing as much as possible is generally a good approach.

I think using existing functions would be too limited, and Joe seems to agree with this based on his experience. To be able to test corner cases of the compiler/linker, such as LTO, we need special code patterns. OTOH, if we want to use an existing kernel function for testing, it needs to be relatively stable, i.e., not being changed very often. It is not always easy to find some known to be stable code that follows specific patterns. If we add dedicated code as test targets, things will be much easier down the road.

CC Joe to chime in here.

Quoted 2 lines

The patch mentiones kpatch in some places. Could you replace it, please?

I was using kpatch for testing. I can replace it with insmod.

Quoted 3 lines

And a little bit of bikeshedding at the end. I think it would be more descriptive if the new config options and tests (test modules) have klp-build somewhere in the name to keep it clear. What do you think?

Technically, we can also use these tests to test other toolchains, for example, kpatch-build. I don't know ksplice or kGraft enough to tell whether they can benefit from these tests or not. OTOH, I am OK changing the name/description of these config options.

Thanks,
Song

maintainer
#15 lore raw

Re: [PATCH v3 8/8] livepatch: Add tests for klp-build toolchain

Him

Quoted 4 lines
Quoted 3 lines

We store test modules in tools/testing/selftests/livepatch/test_modules/ now. Could you move klp_test_module.c there, please? You might also reuse existing ones for the purpose perhaps.

Quoted 9 lines

IIUC, tools/testing/selftests/livepatch/test_modules/ is more like an out of tree module. In the case of testing klp-build, we prefer to have it to work the same as in-tree modules. This is important because klp-build is a toolchain, and any changes of in-tree Makefiles may cause issues with klp-build. Current version can catch these issues easily. If we build the test module as an OOT module, we may miss some of these issues. In the longer term, we should consider adding klp-build support to build livepatch for OOT modules. But for now, good test coverage for in-tree modules are more important.

Ok. I thought it would not matter but it is a fair point.

Quoted 7 lines
Quoted 6 lines

What about vmlinux? I understand that it provides a lot more flexibility to have separate functions for testing but would it be somehow sufficient to use the existing (real) kernel functions? Like cmdline_proc_show() and such which we use everywhere else? Or would it be to limited? I am fine if you find it necessary in the end. I just think that reusing as much as possible is generally a good approach.

Quoted 8 lines

I think using existing functions would be too limited, and Joe seems to agree with this based on his experience. To be able to test corner cases of the compiler/linker, such as LTO, we need special code patterns. OTOH, if we want to use an existing kernel function for testing, it needs to be relatively stable, i.e., not being changed very often. It is not always easy to find some known to be stable code that follows specific patterns. If we add dedicated code as test targets, things will be much easier down the road.

Fair enough.

Quoted 8 lines
Quoted 3 lines

And a little bit of bikeshedding at the end. I think it would be more descriptive if the new config options and tests (test modules) have klp-build somewhere in the name to keep it clear. What do you think?

Technically, we can also use these tests to test other toolchains, for example, kpatch-build. I don't know ksplice or kGraft enough to tell whether they can benefit from these tests or not. OTOH, I am OK changing the name/description of these config options.

I would prefer it, thank you. Unless someone else objects of course.

Regards,
Miroslav

maintainer
#16 lore raw

Re: [PATCH v3 8/8] livepatch: Add tests for klp-build toolchain

Quoted 34 lines · Miroslav Benes

Him

Quoted 4 lines
Quoted 3 lines

We store test modules in tools/testing/selftests/livepatch/test_modules/ now. Could you move klp_test_module.c there, please? You might also reuse existing ones for the purpose perhaps.

Quoted 9 lines

IIUC, tools/testing/selftests/livepatch/test_modules/ is more like an out of tree module. In the case of testing klp-build, we prefer to have it to work the same as in-tree modules. This is important because klp-build is a toolchain, and any changes of in-tree Makefiles may cause issues with klp-build. Current version can catch these issues easily. If we build the test module as an OOT module, we may miss some of these issues. In the longer term, we should consider adding klp-build support to build livepatch for OOT modules. But for now, good test coverage for in-tree modules are more important.

Ok. I thought it would not matter but it is a fair point.

Quoted 7 lines
Quoted 6 lines

What about vmlinux? I understand that it provides a lot more flexibility to have separate functions for testing but would it be somehow sufficient to use the existing (real) kernel functions? Like cmdline_proc_show() and such which we use everywhere else? Or would it be to limited? I am fine if you find it necessary in the end. I just think that reusing as much as possible is generally a good approach.

Quoted 8 lines

I think using existing functions would be too limited, and Joe seems to agree with this based on his experience. To be able to test corner cases of the compiler/linker, such as LTO, we need special code patterns. OTOH, if we want to use an existing kernel function for testing, it needs to be relatively stable, i.e., not being changed very often. It is not always easy to find some known to be stable code that follows specific patterns. If we add dedicated code as test targets, things will be much easier down the road.

Quoted 2 lines

Fair enough.

I've been tinkering with ideas in this space, though I took it in a very different direction.

(First a disclaimer, this effort is largely the result of vibe coding with Claude to prototype testing concepts, so I don't believe any of it is reliable or upstream-worthy at this point.)

From a top-down perspective, I might start with the generated test reports:

- https://file.rdu.redhat.com/~jolawren/artifacts/report.html
- https://file.rdu.redhat.com/~jolawren/artifacts/report.txt

and then in my own words:

1- I'm interested in testing several kernel configurations (distros, debug, thinLTO) as well as toolchains (gcc, llvm) against the same source tree and machine. I call these config/toolchain pairs a testing "profile". In the report examples, these are combos like "fedora-43 + virtme-ng" and "virtme-ng + thin-lto".

2- For test cases, a few possible results:

PASS - should build / load / run
e.g. cmdline-string.patch
FAIL* - unexpected failure to build / load / run
e.g. some new bug in klp-build
XFAIL - expected to build / load / run failure
e.g. "no changed detected" patch
XPASS* - unexpected build / load / run success
e.g. "no changed detected" patch actually created a .ko

* These would be considered interesting to look at. Did we find a new bug, or maybe an existing bug is now fixed?

3- Test cases and makefile target workflows are split into build and runtime parts.

4- Based on kpatch-build experience, test cases are further divided into "quick" and "long" sets with the understanding that klp-build testing takes a non-trivial amount of time.

5- Two patch targets:

a) current-tree - target the user's current source tree
b) patched-tree - (temporarily) patch the user's tree to *exactly* what we need to target

Why? In the kpatch-build project, patching the current-tree meant we had to rebase patches for every release. We also had to hunt and find precise scenarios across the kernel tree to test, hoping they wouldn't go away in future versions. In other cases, the kernel or compiler changed and we weren't testing the original problem any longer.

That said, patching a dummy patched-tree isn't be perfect either, particularly in the runtime sense. You're not testing a release kernel, but something slightly different.

(Tangent: kpatch-build implemented a unit test scheme that cached object files for even greater speed and fixed testing. I haven't thought about how a similar idea might work for klp-build.)

6- Two patch models:

a) static .patch files
b) scripted .patch generation

Why? Sometimes a test like cmdline-string.patch is sufficient and stable. Other times it's not. For example, the recount-many-file test in this branch is implemented via a script. This allows the test to be dynamic and potentially avoid the rebasing problem mentioned above.

7- Build verification including ELF analysis. Not very mature in this branch, but it would be nice to be able to build on it:

======================================================================
BUILD VERIFICATION
======================================================================

klp-build exit code is 0
Module exists: livepatch-cmdline-string.ko verify_diff_log_contains('changed function: cmdline_proc_show'): OK

ELF Analysis:
klp_object[0]:
.name = NULL (vmlinux)
VERIFIED: klp_object.name = NULL (vmlinux)

      klp_func[0]:
        .old_name = "cmdline_proc_show"  [-> .rodata+0x15d]
        .new_func -> cmdline_proc_show
        .old_sympos = 0
      VERIFIED: klp_func.old_name = 'cmdline_proc_show'
      VERIFIED: klp_func.new_func -> cmdline_proc_show

Perhaps even extending this to the intermediate klp-tmp/ files? This would aid in greater sanity checking of what's produced, but also in verifying that our test is still testing what it originally set out to. (i.e. Is the thinLTO suffix test still generating two common symbols with a different hash suffix?)

8- Probably more I've already forgotten about :) Cross-compilation may
be interesting for build testing in the future. For the full AI created commentary, there's https://github.com/joe-lawrence/linux/blob/klp-build-selftests/README.md

Quoted 2 lines
Quoted 1 line

I was using kpatch for testing. I can replace it with insmod.

Do the helpers in functions.sh for safely loading and unloading livepatches (that wait for the transition, etc.) aid here?

Quoted 11 lines
Quoted 8 lines
Quoted 3 lines

And a little bit of bikeshedding at the end. I think it would be more descriptive if the new config options and tests (test modules) have klp-build somewhere in the name to keep it clear. What do you think?

Technically, we can also use these tests to test other toolchains, for example, kpatch-build. I don't know ksplice or kGraft enough to tell whether they can benefit from these tests or not. OTOH, I am OK changing the name/description of these config options.

I would prefer it, thank you. Unless someone else objects of course.

To continue the bike shedding, in my branch, I had dumped this all under a new tools/testing/klp-build subdirectory as my focus was to put klp-build through the paces. It does load the generated livepatches in the runtime testing, but as only as a sanity check. With that, it didn't touch CONFIG or intermix testing with the livepatch/ set.

If we do end up supplementing the livepatch/ with klp-build tests, then I agree that naming them (either filename prefix or subdirectory) would be nice.

But first, is it goal for klp-build to be the build tool (rather than simple module kbuild) for the livepatching .ko selftests?

--
Joe

author maintainer
#17 lore raw

Re: [PATCH v3 8/8] livepatch: Add tests for klp-build toolchain

On Wed, Mar 4, 2026 at 11:33 AM Joe Lawrence <[email protected]> wrote:
[...]

Quoted 7 lines

I've been tinkering with ideas in this space, though I took it in a very different direction.

(First a disclaimer, this effort is largely the result of vibe coding with Claude to prototype testing concepts, so I don't believe any of it is reliable or upstream-worthy at this point.)

Quoted 5 lines

From a top-down perspective, I might start with the generated test reports:

- https://file.rdu.redhat.com/~jolawren/artifacts/report.html
- https://file.rdu.redhat.com/~jolawren/artifacts/report.txt

I cannot access these two links.

Quoted 2 lines

and then in my own words:

[...]

Quoted 7 lines

5- Two patch targets:

a) current-tree - target the user's current source tree
b) patched-tree - (temporarily) patch the user's tree to *exactly* what we need to target

Quoted 5 lines

Why? In the kpatch-build project, patching the current-tree meant we had to rebase patches for every release. We also had to hunt and find precise scenarios across the kernel tree to test, hoping they wouldn't go away in future versions. In other cases, the kernel or compiler changed and we weren't testing the original problem any longer.

I would prefer making patched-tree as upstream + some different CONFIG_s. Otherwise, we will need to carry .patch files for the patched-tree in selftests, which seems weird.

Quoted 3 lines

That said, patching a dummy patched-tree isn't be perfect either, particularly in the runtime sense. You're not testing a release kernel, but something slightly different.

This should not be a problem. The goal is to test the klp-build toolchain.

Quoted 3 lines

(Tangent: kpatch-build implemented a unit test scheme that cached object files for even greater speed and fixed testing. I haven't thought about how a similar idea might work for klp-build.)

I think it is a good idea to have similar .o file tests for klp-diff in klp-build.

Quoted 10 lines

6- Two patch models:

a) static .patch files
b) scripted .patch generation

Why? Sometimes a test like cmdline-string.patch is sufficient and stable. Other times it's not. For example, the recount-many-file test in this branch is implemented via a script. This allows the test to be dynamic and potentially avoid the rebasing problem mentioned above.

I think we can have both a) and b).

Quoted 2 lines

7- Build verification including ELF analysis. Not very mature in this branch, but it would be nice to be able to build on it:

If we test the behavior after loading the patches, ELF analysis might be optional. But we can also do both.

[...]

Quoted 4 lines

8- Probably more I've already forgotten about :) Cross-compilation may
be interesting for build testing in the future. For the full AI created commentary, there's https://github.com/joe-lawrence/linux/blob/klp-build-selftests/README.md

Yes, cross-compilation can be really useful.

Quoted 5 lines
Quoted 2 lines
Quoted 1 line

I was using kpatch for testing. I can replace it with insmod.

Do the helpers in functions.sh for safely loading and unloading livepatches (that wait for the transition, etc.) aid here?

Yes, we can reuse those.
[...]

Quoted 6 lines

To continue the bike shedding, in my branch, I had dumped this all under a new tools/testing/klp-build subdirectory as my focus was to put klp-build through the paces. It does load the generated livepatches in the runtime testing, but as only as a sanity check. With that, it didn't touch CONFIG or intermix testing with the livepatch/ set.

Quoted 6 lines

If we do end up supplementing the livepatch/ with klp-build tests, then I agree that naming them (either filename prefix or subdirectory) would be nice.

But first, is it goal for klp-build to be the build tool (rather than simple module kbuild) for the livepatching .ko selftests?

I think we don't have to use klp-build for livepatch selftests. Existing tests work well as-is.

Thanks,
Song

maintainer
#18 lore raw

Re: [PATCH v3 8/8] livepatch: Add tests for klp-build toolchain

Quoted 10 lines · Song Liu

On Wed, Mar 4, 2026 at 11:33 AM Joe Lawrence <[email protected]> wrote:
[...]

Quoted 7 lines

I've been tinkering with ideas in this space, though I took it in a very different direction.

(First a disclaimer, this effort is largely the result of vibe coding with Claude to prototype testing concepts, so I don't believe any of it is reliable or upstream-worthy at this point.)

Quoted 8 lines
Quoted 5 lines

From a top-down perspective, I might start with the generated test reports:

- https://file.rdu.redhat.com/~jolawren/artifacts/report.html
- https://file.rdu.redhat.com/~jolawren/artifacts/report.txt

I cannot access these two links.

Gah, sorry about those internal links.

Try:

https://joe-lawrence.github.io/klp-build-selftest-artifacts/report.html
https://joe-lawrence.github.io/klp-build-selftest-artifacts/report.txt

Quoted 16 lines
Quoted 2 lines

and then in my own words:

[...]

Quoted 7 lines

5- Two patch targets:

a) current-tree - target the user's current source tree
b) patched-tree - (temporarily) patch the user's tree to *exactly* what we need to target

Quoted 5 lines

Why? In the kpatch-build project, patching the current-tree meant we had to rebase patches for every release. We also had to hunt and find precise scenarios across the kernel tree to test, hoping they wouldn't go away in future versions. In other cases, the kernel or compiler changed and we weren't testing the original problem any longer.

Quoted 4 lines

I would prefer making patched-tree as upstream + some different CONFIG_s. Otherwise, we will need to carry .patch files for the patched-tree in selftests, which seems weird.

It is strange, but for my experiment, I wanted minimal disruption to the tree. For the "real" changeset, upstream + some testing CONFIG_ sounds good to me.

Quoted 6 lines
Quoted 3 lines

That said, patching a dummy patched-tree isn't be perfect either, particularly in the runtime sense. You're not testing a release kernel, but something slightly different.

This should not be a problem. The goal is to test the klp-build toolchain.

Right, perhaps klp-build testing always targets a slightly modified kernel (or at least CONFIG_) while livepatching testing operates against the stock tree?

Quoted 7 lines
Quoted 3 lines

(Tangent: kpatch-build implemented a unit test scheme that cached object files for even greater speed and fixed testing. I haven't thought about how a similar idea might work for klp-build.)

I think it is a good idea to have similar .o file tests for klp-diff in klp-build.

kpatch-build uses a git submodule (a joy to work with /s), but maybe upstream tree can fetch the binary objects from some external (github/etc.) source? I wonder if there is any kselftest precident for this, we'll need to investigate that.

Quoted 11 lines
Quoted 10 lines

6- Two patch models:

a) static .patch files
b) scripted .patch generation

Why? Sometimes a test like cmdline-string.patch is sufficient and stable. Other times it's not. For example, the recount-many-file test in this branch is implemented via a script. This allows the test to be dynamic and potentially avoid the rebasing problem mentioned above.

Quoted 8 lines

I think we can have both a) and b).

Quoted 2 lines

7- Build verification including ELF analysis. Not very mature in this branch, but it would be nice to be able to build on it:

If we test the behavior after loading the patches, ELF analysis might be optional. But we can also do both.

Maybe, though doing so during the build test would give us that analysis for future cross-compiled test cases without having to actually boot and load them somewhere.

Quoted 9 lines

[...]

Quoted 4 lines

8- Probably more I've already forgotten about :) Cross-compilation may
be interesting for build testing in the future. For the full AI created commentary, there's https://github.com/joe-lawrence/linux/blob/klp-build-selftests/README.md

Yes, cross-compilation can be really useful.

Agreed (I think Josh may be working on arm64 klp-build?) how many dimensions of testing are we up to now :)

Quoted 19 lines
Quoted 5 lines
Quoted 2 lines
Quoted 1 line

I was using kpatch for testing. I can replace it with insmod.

Do the helpers in functions.sh for safely loading and unloading livepatches (that wait for the transition, etc.) aid here?

Yes, we can reuse those.
[...]

Quoted 10 lines

To continue the bike shedding, in my branch, I had dumped this all under a new tools/testing/klp-build subdirectory as my focus was to put klp-build through the paces. It does load the generated livepatches in the runtime testing, but as only as a sanity check. With that, it didn't touch CONFIG or intermix testing with the livepatch/ set.

If we do end up supplementing the livepatch/ with klp-build tests, then I agree that naming them (either filename prefix or subdirectory) would be nice.

Quoted 6 lines
Quoted 2 lines

But first, is it goal for klp-build to be the build tool (rather than simple module kbuild) for the livepatching .ko selftests?

I think we don't have to use klp-build for livepatch selftests. Existing tests work well as-is.

--
Joe

author maintainer
#19 lore raw

Re: [PATCH v3 8/8] livepatch: Add tests for klp-build toolchain

On Wed, Mar 4, 2026 at 5:39 PM Joe Lawrence <[email protected]> wrote:
[...]

Quoted 8 lines
Quoted 1 line

Gah, sorry about those internal links.

Try:

https://joe-lawrence.github.io/klp-build-selftest-artifacts/report.html
https://joe-lawrence.github.io/klp-build-selftest-artifacts/report.txt

Thanks! These look pretty good!

[...]

Quoted 8 lines

It is strange, but for my experiment, I wanted minimal disruption to the tree. For the "real" changeset, upstream + some testing CONFIG_ sounds good to me.

Quoted 4 lines
Quoted 3 lines

That said, patching a dummy patched-tree isn't be perfect either, particularly in the runtime sense. You're not testing a release kernel, but something slightly different.

Quoted 6 lines
Quoted 2 lines

This should not be a problem. The goal is to test the klp-build toolchain.

Right, perhaps klp-build testing always targets a slightly modified kernel (or at least CONFIG_) while livepatching testing operates against the stock tree?

Agreed.

Quoted 4 lines
Quoted 4 lines
Quoted 3 lines

(Tangent: kpatch-build implemented a unit test scheme that cached object files for even greater speed and fixed testing. I haven't thought about how a similar idea might work for klp-build.)

Quoted 8 lines
Quoted 3 lines

I think it is a good idea to have similar .o file tests for klp-diff in klp-build.

kpatch-build uses a git submodule (a joy to work with /s), but maybe upstream tree can fetch the binary objects from some external (github/etc.) source? I wonder if there is any kselftest precident for this, we'll need to investigate that.

Ah, right. I forgot that carrying .o files in the upstream kernel is a bit weird. That may indeed be a blocker.

[...]

Quoted 10 lines
Quoted 7 lines
Quoted 4 lines

8- Probably more I've already forgotten about :) Cross-compilation may
be interesting for build testing in the future. For the full AI created commentary, there's https://github.com/joe-lawrence/linux/blob/klp-build-selftests/README.md

Yes, cross-compilation can be really useful.

Agreed (I think Josh may be working on arm64 klp-build?) how many dimensions of testing are we up to now :)

We currently cross compile arm64 livepatch on x86_64 hosts with kpatch-build. We don't have to do the same with klp-build. But it is good to have the option. :)

Thanks,
Song

maintainer
#20 lore raw

Re: [PATCH v3 8/8] livepatch: Add tests for klp-build toolchain

Quoted 9 lines · Song Liu
Quoted 8 lines · Joe Lawrence
Quoted 7 lines
Quoted 3 lines

(Tangent: kpatch-build implemented a unit test scheme that cached object files for even greater speed and fixed testing. I haven't thought about how a similar idea might work for klp-build.)

I think it is a good idea to have similar .o file tests for klp-diff in klp-build.

Quoted 7 lines
Quoted 4 lines

kpatch-build uses a git submodule (a joy to work with /s), but maybe upstream tree can fetch the binary objects from some external (github/etc.) source? I wonder if there is any kselftest precident for this, we'll need to investigate that.

Ah, right. I forgot that carrying .o files in the upstream kernel is a bit weird. That may indeed be a blocker.

I am afraind that caching .o files in the upstream git tree can't work in priniple. The upstream tree is generic. But .o files are comparable only when using the same toolchain, ...

Or do I miss anything, please?

Best Regards,
Petr

maintainer
#21 lore raw

Re: [PATCH v3 8/8] livepatch: Add tests for klp-build toolchain

Quoted 15 lines · Petr Mladek
Quoted 14 lines · Song Liu
Quoted 12 lines · Joe Lawrence
Quoted 7 lines
Quoted 3 lines

(Tangent: kpatch-build implemented a unit test scheme that cached object files for even greater speed and fixed testing. I haven't thought about how a similar idea might work for klp-build.)

I think it is a good idea to have similar .o file tests for klp-diff in klp-build.

kpatch-build uses a git submodule (a joy to work with /s), but maybe upstream tree can fetch the binary objects from some external (github/etc.) source? I wonder if there is any kselftest precident for this, we'll need to investigate that.

Quoted 9 lines
Quoted 2 lines

Ah, right. I forgot that carrying .o files in the upstream kernel is a bit weird. That may indeed be a blocker.

I am afraind that caching .o files in the upstream git tree can't work in priniple. The upstream tree is generic. But .o files are comparable only when using the same toolchain, ...

Or do I miss anything, please?

Hi Petr,

For kpatch-build, the cached object files were placed in a separate repo away from the source code:

https://github.com/dynup/kpatch-unit-test-objs/commits/master/

and its purpose is mostly to avoid regressions against known code sequences (i.e. gcc omitting symbol section symbols, clang unnamed sections, static call tests, etc.) It only exercises the binary comparison code and not full livepatch generation, so it never creates or loads any .ko.

But you are right that the toolchain, optimization levels, etc. can all contribute to generating unique code sequences. The unit-test-objs were
a (huge time) optimization to avoid building kernels just to check
things that should be generated 99% of the time -- or perhaps never, given the toolchains we had on hand. For example, we've converted a few user bug reports into unit tests as we didn't have a particular compiler version or kernel config at the ready.

It's a nice tool to be able to run a make unit test against your development branch and have an answer in a few minutes. The full make integration test takes at least a few coffee cups and dog walks to complete :D

Hope that clarifies things?

Regards,

Signature · 1 line
Joe
maintainer
#22 lore raw

Re: [PATCH v3 8/8] livepatch: Add tests for klp-build toolchain

In reply to Joe Lawrence
Quoted 49 lines · Joe Lawrence
Quoted 34 lines · Miroslav Benes

Him

Quoted 4 lines
Quoted 3 lines

We store test modules in tools/testing/selftests/livepatch/test_modules/ now. Could you move klp_test_module.c there, please? You might also reuse existing ones for the purpose perhaps.

Quoted 9 lines

IIUC, tools/testing/selftests/livepatch/test_modules/ is more like an out of tree module. In the case of testing klp-build, we prefer to have it to work the same as in-tree modules. This is important because klp-build is a toolchain, and any changes of in-tree Makefiles may cause issues with klp-build. Current version can catch these issues easily. If we build the test module as an OOT module, we may miss some of these issues. In the longer term, we should consider adding klp-build support to build livepatch for OOT modules. But for now, good test coverage for in-tree modules are more important.

Ok. I thought it would not matter but it is a fair point.

Quoted 7 lines
Quoted 6 lines

What about vmlinux? I understand that it provides a lot more flexibility to have separate functions for testing but would it be somehow sufficient to use the existing (real) kernel functions? Like cmdline_proc_show() and such which we use everywhere else? Or would it be to limited? I am fine if you find it necessary in the end. I just think that reusing as much as possible is generally a good approach.

Quoted 8 lines

I think using existing functions would be too limited, and Joe seems to agree with this based on his experience. To be able to test corner cases of the compiler/linker, such as LTO, we need special code patterns. OTOH, if we want to use an existing kernel function for testing, it needs to be relatively stable, i.e., not being changed very often. It is not always easy to find some known to be stable code that follows specific patterns. If we add dedicated code as test targets, things will be much easier down the road.

Quoted 2 lines

Fair enough.

Do the helpers in functions.sh for safely loading and unloading livepatches (that wait for the transition, etc.) aid here?

Quoted 9 lines
Quoted 8 lines
Quoted 3 lines

And a little bit of bikeshedding at the end. I think it would be more descriptive if the new config options and tests (test modules) have klp-build somewhere in the name to keep it clear. What do you think?

Technically, we can also use these tests to test other toolchains, for example, kpatch-build. I don't know ksplice or kGraft enough to tell whether they can benefit from these tests or not. OTOH, I am OK changing the name/description of these config options.

Quoted 8 lines
Quoted 2 lines

I would prefer it, thank you. Unless someone else objects of course.

To continue the bike shedding, in my branch, I had dumped this all under a new tools/testing/klp-build subdirectory as my focus was to put klp-build through the paces. It does load the generated livepatches in the runtime testing, but as only as a sanity check. With that, it didn't touch CONFIG or intermix testing with the livepatch/ set.

The question is what you expect from the klp-build testing.

My understanding is that the current test primary checks whether:

+ klp-build machinery is able to put together a working livepatch
+ "objtool klp diff" is able to find and match the right symbols

I assume this because the README contains a very simple steps:

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

A separate directory would be perfectly fine in this case.

We could keep the existing selftests/livepatch as is for testing the kernel/livepatch features, e.g. transition, shadow variables, callbacks, cooperation with ftrace/kprobe.

An integration with the existing selftests/livepatch would be better if you wanted to do more complicated tests. You might want to reuse the existing framework for checking the dmesg output.

Honestly, I guess that we would want to integrate both tests sooner or later anyway. You might want to test how klp-build handles the extra features, e.g. shadow variables, ...

But the integration might be non-trivial.

Quoted 6 lines

If we do end up supplementing the livepatch/ with klp-build tests, then I agree that naming them (either filename prefix or subdirectory) would be nice.

But first, is it goal for klp-build to be the build tool (rather than simple module kbuild) for the livepatching .ko selftests?

Best Regards,
Petr

PS: Sigh, I hope that I'll find time to send v2 of the patchset reworking
the callbacks, shadow variables, and states. It modifies/reworks the existing selftests modules a lot, see v1 at
https://lore.kernel.org/all/[email protected]/

maintainer
#23 lore raw

Re: [PATCH v3 8/8] livepatch: Add tests for klp-build toolchain

In reply to Joe Lawrence
Quoted 41 lines · Joe Lawrence
Quoted 34 lines · Miroslav Benes

Him

Quoted 4 lines
Quoted 3 lines

We store test modules in tools/testing/selftests/livepatch/test_modules/ now. Could you move klp_test_module.c there, please? You might also reuse existing ones for the purpose perhaps.

Quoted 9 lines

IIUC, tools/testing/selftests/livepatch/test_modules/ is more like an out of tree module. In the case of testing klp-build, we prefer to have it to work the same as in-tree modules. This is important because klp-build is a toolchain, and any changes of in-tree Makefiles may cause issues with klp-build. Current version can catch these issues easily. If we build the test module as an OOT module, we may miss some of these issues. In the longer term, we should consider adding klp-build support to build livepatch for OOT modules. But for now, good test coverage for in-tree modules are more important.

Ok. I thought it would not matter but it is a fair point.

Quoted 7 lines
Quoted 6 lines

What about vmlinux? I understand that it provides a lot more flexibility to have separate functions for testing but would it be somehow sufficient to use the existing (real) kernel functions? Like cmdline_proc_show() and such which we use everywhere else? Or would it be to limited? I am fine if you find it necessary in the end. I just think that reusing as much as possible is generally a good approach.

Quoted 8 lines

I think using existing functions would be too limited, and Joe seems to agree with this based on his experience. To be able to test corner cases of the compiler/linker, such as LTO, we need special code patterns. OTOH, if we want to use an existing kernel function for testing, it needs to be relatively stable, i.e., not being changed very often. It is not always easy to find some known to be stable code that follows specific patterns. If we add dedicated code as test targets, things will be much easier down the road.

Quoted 2 lines

Fair enough.

I've been tinkering with ideas in this space, though I took it in a very different direction.

Quoted 6 lines

(First a disclaimer, this effort is largely the result of vibe coding with Claude to prototype testing concepts, so I don't believe any of it is reliable or upstream-worthy at this point.)

From a top-down perspective, I might start with the generated test reports:

Thanks Song and Joe, these are some great ideas. Testing will be extremely important for the success of klp-build.

Below are some back of napkin thoughts based on my experiences with kpatch testing. In my opinion the unit/integration test split for kpatch was successful and we should try to continue that tradition with klp-build.

- unit testing (.o diff)
- great for preventing past regressions
- nice and fast
- tests the most fragile bits
- catches 99% of bugs (almost everything except for new kernel/compiler features)
- try to enforce a rule: one test for every regression
- if we're storing binaries we may need to host the test suite elsewhere
- extracting .o's could be more of a challenge now that vmlinux.o diff is the norm
- the making of regression tests needs to be as easy as possible
- or maybe they can be built on demand with .c and/or .S?
- that *might* be more difficult
- we'd need meta-tests to ensure the tests continue to test what they're supposed to

- integration testing:
- good for testing "this complex end-to-end operation still works"
- good for finding issues with new toolchain versions and new kernel features
- can be used for testing certain runtime-critical features like static branches/calls
- not necessarily good for testing regressions, the underlying code is free to change at any time
- I like Song's idea of adding a fake (yet stable) test component in vmlinux for which we can create .patch files to test
- I also like Joe's idea of scripted patch generation (e.g., automatically patching meminfo_proc_show() somehow)

And in general, we may want to look to porting as many of the kpatch unit/integration tests as possible so we can take advantage of all those accumulated testing experiences.

Signature · 1 line
Josh
maintainer
#24 lore raw

Re: [PATCH v3 8/8] livepatch: Add tests for klp-build toolchain

Quoted 5 lines · Josh Poimboeuf

On Wed, Mar 04, 2026 at 02:33:03PM -0500, Joe Lawrence wrote:
Thanks Song and Joe, these are some great ideas. Testing will be extremely important for the success of klp-build.

- I like Song's idea of adding a fake (yet stable) test component in vmlinux for which we can create .patch files to test

Yeah, it simplifies writing and maintaining the test cases.

Also it helps to isolate the tests at runtime. I mean that it might help to integrate the tests into some higher level testing framework. I always wondered whether changing some interface might confuse monitoring or debug output on machines used by misc QA teams.

Best Regards,
Petr

maintainer
#25 lore raw

Re: [PATCH v3 8/8] livepatch: Add tests for klp-build toolchain

In reply to Song Liu
Quoted 8 lines · 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.

This patch has some whitespace errors:

/tmp/a:505: space before tab in indent.
int i, sum = x;
/tmp/a:506: trailing whitespace.

/tmp/a:507: space before tab in indent.
for (i = 0; i < len; i++)
/tmp/a:510: space before tab in indent.
if (sum > 1000)
/tmp/a:511: space before tab in indent.
sum = 0;
warning: squelched 28 whitespace errors
warning: 33 lines add whitespace errors.

Signature · 1 line
Josh