objtool/klp: klp-build LTO support and tests
Subsystems (3)
Song Liu
maintainerv2 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
Song Liu
maintainerPATCH 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
@@ -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
Song Liu
maintainerPATCH v2 2/8 objtool/klp: Remove trailing '_' in demangle_name()
In reply to Song LiuWith 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
@@ -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
Song Liu
maintainerPATCH v2 3/8 objtool/klp: Use sym->demangled_name for symbol_name hash
In reply to Song LiuFor 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
@@ -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
Song Liu
maintainerPATCH v2 4/8 objtool/klp: Also demangle global objects
In reply to Song LiuWith 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
@@ -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
Song Liu
maintainerPATCH v2 5/8 objtool/klp: Remove .llvm suffix in demangle_name()
In reply to Song LiuRemove .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
@@ -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
Song Liu
maintainerPATCH v2 6/8 objtool/klp: Match symbols based on demangled_name for global variables
In reply to Song Liucorrelate_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
@@ -323,6 +323,19 @@ struct symbol *find_global_symbol_by_name(const struct elf *elf, const char *namreturn 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
@@ -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
Josh Poimboeuf
maintainerRe: [PATCH v2 6/8] objtool/klp: Match symbols based on demangled_name for global variables
Quoted 2 lines · Song Liu
correlate_symbolswill always try to match full name first. If there is no match, try match onlydemangled_name.
In commit logs, please add "()" to function names, like correlate_symbols().
Quoted 8 lines
+++
b/tools/objtool/elf.ctools/objtool/elf.c +3
-0@@ -323,6 +323,19 @@ struct symbol *find_global_symbol_by_name(const struct elf *elf, const char *namreturn 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
Song Liu
maintainerRe: [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_symbolswill always try to match full name first. If there is no match, try match onlydemangled_name.In commit logs, please add "()" to function names, like
correlate_symbols().
Noted.
Quoted 23 lines
Quoted 9 lines
+++
b/tools/objtool/elf.ctools/objtool/elf.c +3
-0@@ -323,6 +323,19 @@ struct symbol *find_global_symbol_by_name(const struct elf *elf, const char *namreturn 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
-0if (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
Josh Poimboeuf
maintainerRe: [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. Sinceelf.cdoesn't touchsym->twinat all, I think it is cleaner to keep this logic inklp-diff.c. If you think it is OK to haveelf.chandle 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
Song Liu
maintainerPATCH v2 7/8 objtool/klp: Correlate locals to globals
In reply to Song LiuAllow 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
@@ -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
Josh Poimboeuf
maintainerRe: [PATCH v2 7/8] objtool/klp: Correlate locals to globals
Quoted 9 lines · Song Liu
+++
b/tools/objtool/klp-diff.ctools/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
Song Liu
maintainerRe: [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.ctools/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
Song Liu
maintainerPATCH v2 8/8 livepatch: Add tests for klp-build toolchain
In reply to Song LiuAdd 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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