objtool/klp: Some klp-build fixes and improvements

From: Joe Lawrence <[email protected]>

For naming function-local static locals, GCC uses <var>.<id>, e.g. __already_done.15, while Clang uses <func>.<var> with optional .<id>, e.g. create_worker.__already_done.111

The existing is_uncorrelated_static_local() check only matches the GCC convention where the variable name is a prefix. Handle both cases by checking for a prefix match (GCC) and by checking after the first dot separator (Clang).

Fixes dd590d4d57eb ("objtool/klp: Introduce klp diff subcommand for diffing object files")

Signed-off-by Joe Lawrence <[email protected]>

Signed-off-by Josh Poimboeuf <[email protected]>

For naming function-local static locals, GCC uses <var>.<id>, e.g. __already_done.15, while Clang uses <func>.<var> with optional .<id>, e.g. create_worker.__already_done.111

The existing is_uncorrelated_static_local() check only matches the GCC convention where the variable name is a prefix. Handle both cases by checking for a prefix match (GCC) and by checking after the first dot separator (Clang).

[...]

While there was once a section named .data.once, it has since been renamed to .data..once with commit dbefa1f31a91 ("Rename .data.once to .data..once to fix resetting WARN*_ONCE"). Fix it.

Fixes dd590d4d57eb ("objtool/klp: Introduce klp diff subcommand for diffing object files")

Signed-off-by Josh Poimboeuf <[email protected]>

---
 tools/objtool/klp-diff.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

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

index b1b068e9b4c7..cb26c1c92a74 100644
@@ -257,7 +257,8 @@ static bool is_uncorrelated_static_local(struct symbol *sym)
        if (!is_object_sym(sym) || !is_local_sym(sym))
                return false;
 
-       if (!strcmp(sym->sec->name, ".data.once"))
+       /* WARN_ONCE, etc */
+       if (!strcmp(sym->sec->name, ".data..once"))
                return true;
 
        dot = strchr(sym->name, '.');
--

2.53.0

Quoted 5 lines · Josh Poimboeuf

While there was once a section named .data.once, it has since been renamed to .data..once with commit dbefa1f31a91 ("Rename .data.once to .data..once to fix resetting WARN*_ONCE"). Fix it.

Quoted 2 lines

Fixes dd590d4d57eb ("objtool/klp: Introduce klp diff subcommand for diffing object files")

Signed-off-by Josh Poimboeuf <[email protected]>

Acked-by Song Liu <[email protected]>

Nitpick: shall we match both ".data.once" and ".data..once", so that whoever backports klp-build to older kernels will not have a surprise.

Quoted 9 lines · Song Liu

Quoted 7 lines · Josh Poimboeuf

While there was once a section named .data.once, it has since been renamed to .data..once with commit dbefa1f31a91 ("Rename .data.once to .data..once to fix resetting WARN*_ONCE"). Fix it.

Fixes dd590d4d57eb ("objtool/klp: Introduce klp diff subcommand for diffing object files")

Signed-off-by Josh Poimboeuf <[email protected]>

Quoted 4 lines

Acked-by Song Liu <[email protected]>

Nitpick: shall we match both ".data.once" and ".data..once", so that whoever backports klp-build to older kernels will not have a surprise.

Hm, I'm a bit hesitant to do that. One of the nice things about having this code upstream is that we don't have to start collecting all the cruft for old kernels.

While there was once a section named .data.once, it has since been renamed to .data..once with commit dbefa1f31a91 ("Rename .data.once to .data..once to fix resetting WARN*_ONCE"). Fix it.

Symbols created by __ADDRESSABLE() are only used to convince the toolchain not to optimize out the referenced symbol.

Quoted 2 lines · Josh Poimboeuf

Symbols created by __ADDRESSABLE() are only used to convince the toolchain not to optimize out the referenced symbol.

Reviewed-by Miroslav Benes <[email protected]>

Quoted 5 lines · Miroslav Benes

Quoted 2 lines · Josh Poimboeuf

Symbols created by __ADDRESSABLE() are only used to convince the toolchain not to optimize out the referenced symbol.

Reviewed-by Miroslav Benes <[email protected]>

Looking at it again... wouldn't it be better to address this in is_special_section() which is looking at .discard.addressable already (only the outcome is different)?

Quoted 8 lines · Miroslav Benes

Quoted 5 lines · Miroslav Benes

Quoted 2 lines · Josh Poimboeuf

Symbols created by __ADDRESSABLE() are only used to convince the toolchain not to optimize out the referenced symbol.

Reviewed-by Miroslav Benes <[email protected]>

Quoted 3 lines

Looking at it again... wouldn't it be better to address this in is_special_section() which is looking at .discard.addressable already (only the outcome is different)?

No, I don't think so. If .discard.addressable were classified as a "special" section then klp-diff would try to extract its entries into the livepatch module, which is completely unnecessary as these are throwaway symbols.

With Clang LTO enabled, DECLARE_PCI_FIXUP_SECTION() uses __UNIQUE_ID() to generate uniquely named wrapper functions, which are being reported as new functions and unnecessarily included in the patch module:

vmlinux.o: new function: __UNIQUE_ID_quirk_f0_vpd_link_661

These stub functions only exist to make the compiler happy. Just ignore them along with any other dont_correlate() symbols. Note that dont_correlate() already includes prefix functions.

Signed-off-by Josh Poimboeuf <[email protected]>

---
 tools/objtool/klp-diff.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

tools/objtool/klp-diff.c +2 -3

index 36753eeba58c..ea9ccf8c4ea9 100644
@@ -786,7 +786,7 @@ static int mark_changed_functions(struct elfs *e)
 
        /* Find changed functions */
        for_each_sym(e->orig, sym_orig) {
-               if (!is_func_sym(sym_orig) || is_prefix_func(sym_orig))
+               if (!is_func_sym(sym_orig) || dont_correlate(sym_orig))
                        continue;
 
                patched_sym = sym_orig->twin;

@@ -802,7 +802,7 @@ static int mark_changed_functions(struct elfs *e)
 
        /* Find added functions and print them */
        for_each_sym(e->patched, patched_sym) {
-               if (!is_func_sym(patched_sym) || is_prefix_func(patched_sym))
+               if (!is_func_sym(patched_sym) || dont_correlate(patched_sym))
                        continue;
 
                if (!patched_sym->twin) {
--

2.53.0

Quoted 7 lines · Josh Poimboeuf

With Clang LTO enabled, DECLARE_PCI_FIXUP_SECTION() uses __UNIQUE_ID() to generate uniquely named wrapper functions, which are being reported as new functions and unnecessarily included in the patch module:

vmlinux.o: new function: __UNIQUE_ID_quirk_f0_vpd_link_661

Quoted 5 lines

These stub functions only exist to make the compiler happy. Just ignore them along with any other dont_correlate() symbols. Note that dont_correlate() already includes prefix functions.

Signed-off-by Josh Poimboeuf <[email protected]>

The actual change appears to be much bigger than the subject line. Maybe rephrase it a bit?

Quoted 30 lines

---
 tools/objtool/klp-diff.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

tools/objtool/klp-diff.c +2 -3

index 36753eeba58c..ea9ccf8c4ea9 100644
@@ -786,7 +786,7 @@ static int mark_changed_functions(struct elfs *e)
 
        /* Find changed functions */
        for_each_sym(e->orig, sym_orig) {
-               if (!is_func_sym(sym_orig) || is_prefix_func(sym_orig))
+               if (!is_func_sym(sym_orig) || dont_correlate(sym_orig))
                        continue;
 
                patched_sym = sym_orig->twin;

@@ -802,7 +802,7 @@ static int mark_changed_functions(struct elfs *e)
 
        /* Find added functions and print them */
        for_each_sym(e->patched, patched_sym) {
-               if (!is_func_sym(patched_sym) || is_prefix_func(patched_sym))
+               if (!is_func_sym(patched_sym) || dont_correlate(patched_sym))
                        continue;
 
                if (!patched_sym->twin) {
--

2.53.0

Quoted 8 lines · Song Liu

Quoted 7 lines · Josh Poimboeuf

With Clang LTO enabled, DECLARE_PCI_FIXUP_SECTION() uses __UNIQUE_ID() to generate uniquely named wrapper functions, which are being reported as new functions and unnecessarily included in the patch module:

vmlinux.o: new function: __UNIQUE_ID_quirk_f0_vpd_link_661

Quoted 8 lines

Quoted 5 lines

These stub functions only exist to make the compiler happy. Just ignore them along with any other dont_correlate() symbols. Note that dont_correlate() already includes prefix functions.

Signed-off-by Josh Poimboeuf <[email protected]>

The actual change appears to be much bigger than the subject line. Maybe rephrase it a bit?

Hm, in fact this is a relic from a previous iteration of the patches: it longer fixes what it claims to fix, as __UNIQUE_ID_ (other than __ADDRESSABLE()) are now correlated. The claimed issue actually gets fixed later by the rewriting of the correlation algorithm.

That said, I still think the below is needed, I just need to rewrite the commit log.

Quoted 24 lines · Josh Poimboeuf

Quoted 8 lines · Song Liu

Quoted 7 lines · Josh Poimboeuf

With Clang LTO enabled, DECLARE_PCI_FIXUP_SECTION() uses __UNIQUE_ID() to generate uniquely named wrapper functions, which are being reported as new functions and unnecessarily included in the patch module:

vmlinux.o: new function: __UNIQUE_ID_quirk_f0_vpd_link_661

Quoted 8 lines

Quoted 5 lines

These stub functions only exist to make the compiler happy. Just ignore them along with any other dont_correlate() symbols. Note that dont_correlate() already includes prefix functions.

Signed-off-by Josh Poimboeuf <[email protected]>

The actual change appears to be much bigger than the subject line. Maybe rephrase it a bit?

Hm, in fact this is a relic from a previous iteration of the patches: it longer fixes what it claims to fix, as __UNIQUE_ID_ (other than __ADDRESSABLE()) are now correlated. The claimed issue actually gets fixed later by the rewriting of the correlation algorithm.

Quoted 2 lines

That said, I still think the below is needed, I just need to rewrite the commit log.

Agreed.

Quoted 25 lines · Song Liu

Quoted 24 lines · Josh Poimboeuf

Quoted 8 lines · Song Liu

Quoted 7 lines · Josh Poimboeuf

With Clang LTO enabled, DECLARE_PCI_FIXUP_SECTION() uses __UNIQUE_ID() to generate uniquely named wrapper functions, which are being reported as new functions and unnecessarily included in the patch module:

vmlinux.o: new function: __UNIQUE_ID_quirk_f0_vpd_link_661

Quoted 8 lines

Quoted 5 lines

These stub functions only exist to make the compiler happy. Just ignore them along with any other dont_correlate() symbols. Note that dont_correlate() already includes prefix functions.

Signed-off-by Josh Poimboeuf <[email protected]>

The actual change appears to be much bigger than the subject line. Maybe rephrase it a bit?

Hm, in fact this is a relic from a previous iteration of the patches: it longer fixes what it claims to fix, as __UNIQUE_ID_ (other than __ADDRESSABLE()) are now correlated. The claimed issue actually gets fixed later by the rewriting of the correlation algorithm.

Quoted 4 lines

Quoted 2 lines

That said, I still think the below is needed, I just need to rewrite the commit log.

Agreed.

From: Josh Poimboeuf <[email protected]>
Subject: [PATCH] objtool/klp: Don't report uncorrelated functions as new

Clang LTO uses __UNIQUE_ID() to generate some uniquely named wrapper functions, like initstubs. If they're uncorrelated, prevent them from being reported as new functions and included unnecessarily.

Note that dont_correlate() already includes prefix functions, so prefix functions are still being ignored here.

Signed-off-by Josh Poimboeuf <[email protected]>

Quoted 25 lines · Song Liu

Quoted 24 lines · Josh Poimboeuf

Quoted 8 lines · Song Liu

Quoted 7 lines · Josh Poimboeuf

With Clang LTO enabled, DECLARE_PCI_FIXUP_SECTION() uses __UNIQUE_ID() to generate uniquely named wrapper functions, which are being reported as new functions and unnecessarily included in the patch module:

vmlinux.o: new function: __UNIQUE_ID_quirk_f0_vpd_link_661

Quoted 8 lines

Quoted 5 lines

These stub functions only exist to make the compiler happy. Just ignore them along with any other dont_correlate() symbols. Note that dont_correlate() already includes prefix functions.

Signed-off-by Josh Poimboeuf <[email protected]>

The actual change appears to be much bigger than the subject line. Maybe rephrase it a bit?

Hm, in fact this is a relic from a previous iteration of the patches: it longer fixes what it claims to fix, as __UNIQUE_ID_ (other than __ADDRESSABLE()) are now correlated. The claimed issue actually gets fixed later by the rewriting of the correlation algorithm.

Quoted 4 lines

Quoted 2 lines

That said, I still think the below is needed, I just need to rewrite the commit log.

Agreed.

From: Josh Poimboeuf <[email protected]>
Subject: [PATCH] objtool/klp: Don't report uncorrelated functions as new

Clang LTO uses __UNIQUE_ID() to generate some uniquely named wrapper functions, like initstubs. If they're uncorrelated, prevent them from being reported as new functions and included unnecessarily.

Note that dont_correlate() already includes prefix functions, so prefix functions are still being ignored here.

Signed-off-by Josh Poimboeuf <[email protected]>

Move the sec->rodata marking from check.c to elf.c so it's set during ELF reading rather than during the check pipeline. This makes the rodata flag available to all objtool users, including klp-diff which reads ELF files directly without running check().

Add an is_rodata_sec() helper to elf.h for consistency with is_text_sec() and is_string_sec().

Signed-off-by Josh Poimboeuf <[email protected]>

---
 tools/objtool/check.c               | 11 +++--------
 tools/objtool/elf.c                 | 13 +++++++++++++
 tools/objtool/include/objtool/elf.h |  5 +++++
 3 files changed, 21 insertions(+), 8 deletions(-)

tools/objtool/check.c +3 -8

index 9b11cf3193b9..5722d4568401 100644
@@ -2566,7 +2566,6 @@ static int classify_symbols(struct objtool_file *file)
 static void mark_rodata(struct objtool_file *file)
 {
 	struct section *sec;
-	bool found = false;
 
 	/*
 	 * Search for the following rodata sections, each of which can

@@ -2579,15 +2578,11 @@ static void mark_rodata(struct objtool_file *file)
 	 * .rodata.str1.* sections are ignored; they don't contain jump tables.
 	 */
 	for_each_sec(file->elf, sec) {
-		if ((!strncmp(sec->name, ".rodata", 7) &&
-		     !strstr(sec->name, ".str1.")) ||
-		    !strncmp(sec->name, ".data.rel.ro", 12)) {
-			sec->rodata = true;
-			found = true;
+		if (is_rodata_sec(sec)) {
+			file->rodata = true;
+			return;
 		}
 	}
-
-	file->rodata = found;
 }
 
 static void mark_holes(struct objtool_file *file)

tools/objtool/elf.c +13 -0

index f3df2bde119f..ac9da81a7a2f 100644
@@ -1172,6 +1172,17 @@ static int read_relocs(struct elf *elf)
 	return 0;
 }
 
+static void mark_rodata(struct elf *elf)
+{
+	struct section *sec;
+
+	for_each_sec(elf, sec) {
+		if ((strstarts(sec->name, ".rodata") && !strstr(sec->name, ".str1.")) ||
+		    strstarts(sec->name, ".data.rel.ro"))
+			sec->rodata = true;
+	}
+}
+
 struct elf *elf_open_read(const char *name, int flags)
 {
 	struct elf *elf;

@@ -1222,6 +1233,8 @@ struct elf *elf_open_read(const char *name, int flags)
 	if (read_sections(elf))
 		goto err;
 
+	mark_rodata(elf);
+
 	if (read_symbols(elf))
 		goto err;
 

tools/objtool/include/objtool/elf.h +5 -0

index 25573e5af76e..c61bd57767f9 100644
@@ -296,6 +296,11 @@ static inline bool is_text_sec(struct section *sec)
 	return sec->sh.sh_flags & SHF_EXECINSTR;
 }
 
+static inline bool is_rodata_sec(struct section *sec)
+{
+	return sec->rodata;
+}
+
 static inline bool sec_changed(struct section *sec)
 {
 	return sec->_changed;

Signature · 1 line

2.53.0

Move the sec->rodata marking from check.c to elf.c so it's set during ELF reading rather than during the check pipeline. This makes the rodata flag available to all objtool users, including klp-diff which reads ELF files directly without running check().

Add an is_rodata_sec() helper to elf.h for consistency with is_text_sec() and is_string_sec().

[...]

Move the sec->rodata marking from check.c to elf.c so it's set during ELF reading rather than during the check pipeline. This makes the rodata flag available to all objtool users, including klp-diff which reads ELF files directly without running check().

Add an is_rodata_sec() helper to elf.h for consistency with is_text_sec() and is_string_sec().

Signed-off-by Josh Poimboeuf <[email protected]>

tools/objtool/klp-diff.c static bool dont_correlate(struct symbol *sym) · line 377

Quoted 9 lines · Josh Poimboeuf

tools/objtool/klp-diff.c +1 -0

index ea9ccf8c4ea9..f6597015b33b 100644
@@ -374,6 +374,7 @@ static bool dont_correlate(struct symbol *sym)
 	       is_uncorrelated_static_local(sym) ||
 	       is_clang_tmp_label(sym) ||
 	       is_string_sec(sym->sec) ||
+	       is_rodata_sec(sym->sec) ||

Sashiko comments here [1] that the check is suddenly to broad and it covers also global rodata symbols which might then be skipped in klp_reloc_needed(). I think it has a point.

[1] https://sashiko.dev/#/patchset/cover.1776916871.git.jpoimboe%40kernel.org?part=6

Some arch/x86/crypto/*.S files define local .set/.equ constants that get duplicated in vmlinux.o. This causes klp-diff to fail with "Multiple correlation candidates" errors since it can't uniquely match these between orig and patched builds.

Skip ABS symbols in dont_correlate(). They're purely compile-time assembly constants that are never referenced by relocations, so they don't need correlation.

[...]

Some arch/x86/crypto/*.S files define local .set/.equ constants that get duplicated in vmlinux.o. This causes klp-diff to fail with "Multiple correlation candidates" errors since it can't uniquely match these between orig and patched builds.

Skip ABS symbols in dont_correlate(). They're purely compile-time assembly constants that are never referenced by relocations, so they don't need correlation.

Signed-off-by Josh Poimboeuf <[email protected]>

With LTO, the initcall infrastructure generates __initstub__kmod_* wrapper functions in .init.text. These are the LTO equivalent of __initcall__kmod_* data pointers, which are already excluded from correlation.

These are __init functions whose memory is freed after boot, so there's no reason to include or reference them in a livepatch module.

[...]

With LTO, the initcall infrastructure generates __initstub__kmod_* wrapper functions in .init.text. These are the LTO equivalent of __initcall__kmod_* data pointers, which are already excluded from correlation.

These are __init functions whose memory is freed after boot, so there's no reason to include or reference them in a livepatch module.

Signed-off-by Josh Poimboeuf <[email protected]>

From: Joe Lawrence <[email protected]>

create_fake_symbols() has two phases: creating symbols from ANNOTATE_DATA_SPECIAL entries, and a fallback that uses sh_entsize for special sections like .static_call_sites.

When .discard.annotate_data is absent, the function returns early, skipping the entsize fallback and silently allowing unsupported module-local static call keys through.

Fix it by jumping to the entsize phase instead of returning early.

Fixes dd590d4d57eb ("objtool/klp: Introduce klp diff subcommand for diffing object files")

Assisted-by Claude:claude-4-opus

Signed-off-by Joe Lawrence <[email protected]>

Signed-off-by Josh Poimboeuf <[email protected]>

create_fake_symbols() has two phases: creating symbols from ANNOTATE_DATA_SPECIAL entries, and a fallback that uses sh_entsize for special sections like .static_call_sites.

When .discard.annotate_data is absent, the function returns early, skipping the entsize fallback and silently allowing unsupported module-local static call keys through.

[...]

find_symbol_by_name() only returns the first match, so --debug-checksum=<func> silently ignores any subsequent duplicately named functions after the first.

Add a new iterate_sym_by_name() to fix that.

Signed-off-by Josh Poimboeuf <[email protected]>

find_symbol_by_name() only returns the first match, so --debug-checksum=<func> silently ignores any subsequent duplicately named functions after the first.

Add a new iterate_sym_by_name() to fix that.

Quoted 5 lines · Josh Poimboeuf

find_symbol_by_name() only returns the first match, so --debug-checksum=<func> silently ignores any subsequent duplicately named functions after the first.

Add a new iterate_sym_by_name() to fix that.

The patch is ok but is it worth it when you remove it again in 39/48 with a better implementation? Can 39/48 be moved here in the series instead?

When a section is empty (e.g. only zero-length alternative replacements), there are no symbols to convert a section symbol reference to. Skip the reloc instead of erroring out.

Fixes dd590d4d57eb ("objtool/klp: Introduce klp diff subcommand for diffing object files")

Signed-off-by Josh Poimboeuf <[email protected]>

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

@@ -1293,12 +1301,15 @@ static int clone_sym_relocs(struct elfs *e, struct symbol *patched_sym)
                    !strcmp(patched_reloc->sym->sec->name, ".altinstr_aux"))
                        continue;
 

Quoted 7 lines · Josh Poimboeuf

When a section is empty (e.g. only zero-length alternative replacements), there are no symbols to convert a section symbol reference to. Skip the reloc instead of erroring out.

Fixes dd590d4d57eb ("objtool/klp: Introduce klp diff subcommand for diffing object files")

Signed-off-by Josh Poimboeuf <[email protected]>

Acked-by Song Liu <[email protected]>

With a nitpick below:

[...]

Quoted 4 lines

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

@@ -1293,12 +1301,15 @@ static int clone_sym_relocs(struct elfs *e, struct symbol *patched_sym)
                    !strcmp(patched_reloc->sym->sec->name, ".altinstr_aux"))
                        continue;
 

tools/objtool/klp-diff.c static int clone_sym_relocs(struct elfs *e, struct symbol *patched_sym) · line 1312

Quoted 10 lines

tools/objtool/klp-diff.c +4 -1

-               if (convert_reloc_sym(e->patched, patched_reloc)) {
+               ret = convert_reloc_sym(e->patched, patched_reloc);
+               if (ret < 0) {
                        ERROR_FUNC(patched_rsec->base, reloc_offset(patched_reloc),
                                   "failed to convert reloc sym '%s' to its proper format",
                                   patched_reloc->sym->name);
                        return -1;
                }
+               if (ret > 0)
+                       continue;

Functions that return -1, 0, 1 are usually more confusing. Shall we add more comments for convert_reloc_sym()?

When a section is empty (e.g. only zero-length alternative replacements), there are no symbols to convert a section symbol reference to. Skip the reloc instead of erroring out.

Fix NULL dereference when cloning a symbol from an empty section. sec->data is only populated for sections with non-zero size.

Fixes dd590d4d57eb ("objtool/klp: Introduce klp diff subcommand for diffing object files")

Signed-off-by Josh Poimboeuf <[email protected]>

Fix NULL dereference when cloning a symbol from an empty section. sec->data is only populated for sections with non-zero size.

The XXH3 state allocated in checksum_init() is never freed. Free it in checksum_finish().

Signed-off-by Josh Poimboeuf <[email protected]>

The XXH3 state allocated in checksum_init() is never freed. Free it in checksum_finish().

Objtool is failing to extract text annotations which reference .altinstr_replacement instructions:

1) Alternative replacement fake symbols are NOTYPE rather than FUNC,

     and they don't have sym->included set, thus they aren't recognized
     by should_keep_special_sym().

2) .discard.annotate_insn gets processed before .altinstr_replacement, so the referenced (fake) symbols don't have clones yet.

Fix the first issue by checking for a valid clone instead of sym->included and by accepting NOTYPE symbols when processing .discard.annotate_insn.

Fix the second issue by deferring text annotation processing until after the other special sections have been cloned.

Fixes dd590d4d57eb ("objtool/klp: Introduce klp diff subcommand for diffing object files")

Signed-off-by Josh Poimboeuf <[email protected]>

Objtool is failing to extract text annotations which reference .altinstr_replacement instructions:

1) Alternative replacement fake symbols are NOTYPE rather than FUNC,

     and they don't have sym->included set, thus they aren't recognized
     by should_keep_special_sym().

[...]

.kcfi_traps contains references to kCFI trap instruction locations. When a KCFI type check fails at an indirect call, the trap handler looks up the faulting address in this section.

Add it to the special sections list so the entries get extracted for the changed functions they reference.

Signed-off-by Josh Poimboeuf <[email protected]>

.kcfi_traps contains references to kCFI trap instruction locations. When a KCFI type check fails at an indirect call, the trap handler looks up the faulting address in this section.

Add it to the special sections list so the entries get extracted for the changed functions they reference.

[...]

Objtool has some hacks which NOP out certain calls/jumps and replace their relocations with R_X86_64_NONE. The klp-diff relocation extraction code will error out when trying to copy these relocations due to their negative addend, which would only makes sense for a PC-relative branch instruction. Just ignore them.

Fixes dd590d4d57eb ("objtool/klp: Introduce klp diff subcommand for diffing object files")

Signed-off-by Josh Poimboeuf <[email protected]>

Objtool has some hacks which NOP out certain calls/jumps and replace their relocations with R_X86_64_NONE. The klp-diff relocation extraction code will error out when trying to copy these relocations due to their negative addend, which would only makes sense for a PC-relative branch instruction. Just ignore them.

[...]

In find_reloc_by_dest_range(), hash collisions can cause a high-offset relocation to appear when probing a low-offset hash bucket.

Only return early when the best match found so far genuinely belongs to the current bucket (its offset is within the bucket's stride range). Otherwise, continue scanning later buckets which may contain lower-offset matches.

Fixes 74b873e49d92 ("objtool: Optimize find_rela_by_dest_range()")

Signed-off-by Josh Poimboeuf <[email protected]>

---
 tools/objtool/elf.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

tools/objtool/elf.c +2 -2

index a5486e172e5c..c4cb371e72b2 100644
@@ -370,11 +370,11 @@ struct reloc *find_reloc_by_dest_range(const struct elf *elf, struct section *se
 					r = reloc;
 			}
 		}
-		if (r)
+		if (r && (reloc_offset(r) & OFFSET_STRIDE_MASK) == o)
 			return r;
 	}
 
-	return NULL;
+	return r;
 }
 
 struct reloc *find_reloc_by_dest(const struct elf *elf, struct section *sec, unsigned long offset)

Signature · 1 line

2.53.0

Quoted 7 lines · Josh Poimboeuf

In find_reloc_by_dest_range(), hash collisions can cause a high-offset relocation to appear when probing a low-offset hash bucket.

Only return early when the best match found so far genuinely belongs to the current bucket (its offset is within the bucket's stride range). Otherwise, continue scanning later buckets which may contain lower-offset matches.

Maybe mention (and or add a comment to the function) that in case of multiple matches in the given range, it will return the lowest address one.

Acked-by Peter Zijlstra (Intel) <[email protected]>

Quoted 14 lines · Peter Zijlstra

Quoted 7 lines · Josh Poimboeuf

In find_reloc_by_dest_range(), hash collisions can cause a high-offset relocation to appear when probing a low-offset hash bucket.

Only return early when the best match found so far genuinely belongs to the current bucket (its offset is within the bucket's stride range). Otherwise, continue scanning later buckets which may contain lower-offset matches.

Maybe mention (and or add a comment to the function) that in case of multiple matches in the given range, it will return the lowest address one.

Acked-by Peter Zijlstra (Intel) <[email protected]>

tools/objtool/elf.c +0 -0

index c4cb371e72b2..af2841b8e095 100644

In find_reloc_by_dest_range(), hash collisions can cause a high-offset relocation to appear when probing a low-offset hash bucket.

Only return early when the best match found so far genuinely belongs to the current bucket (its offset is within the bucket's stride range). Otherwise, continue scanning later buckets which may contain lower-offset matches.

[...]

If .config is out of date with the kernel source, 'make syncconfig' hangs while waiting for user input on new config options. Detect the mismatch and return an error.

Fixes 6f93f7b06810 ("livepatch/klp-build: Fix inconsistent kernel version")

Signed-off-by Josh Poimboeuf <[email protected]>

---
 scripts/livepatch/klp-build | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

scripts/livepatch/klp-build +0 -0

index 0ad7e6631314..81b35fc10877 100755

scripts/livepatch/klp-build +0 -1

        [[ -z "$kernelrelease" ]] && die "failed to get kernel version"
 
        sed -i "2i echo $kernelrelease; exit 0" scripts/setlocalversion
--

2.53.0

Quoted 19 lines · Josh Poimboeuf

If .config is out of date with the kernel source, 'make syncconfig' hangs while waiting for user input on new config options. Detect the mismatch and return an error.

Fixes 6f93f7b06810 ("livepatch/klp-build: Fix inconsistent kernel version")

Signed-off-by Josh Poimboeuf <[email protected]>

---
 scripts/livepatch/klp-build | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

scripts/livepatch/klp-build +0 -0

index 0ad7e6631314..81b35fc10877 100755
@@ -306,7 +306,12 @@ set_kernelversion() {
 
        stash_file "$file"
 

scripts/livepatch/klp-build set_kernelversion() { · line 314

Quoted 7 lines

scripts/livepatch/klp-build +6 -1

-       kernelrelease="$(cd "$SRC" && make syncconfig &>/dev/null && make -s kernelrelease)"
+       if [[ -n "$(make -s listnewconfig 2>/dev/null)" ]]; then
+               die ".config mismatch, check your .config or run 'make olddefconfig'"
+       fi
+       make syncconfig &>/dev/null || die "make syncconfig failed"
+
+       kernelrelease="$(cd "$SRC" && make -s kernelrelease)"

Do we really need cd "$SRC" here? If so, we need it before all the make commands, right?

The errtrace option (combined with the ERR trap) already serves the same function (and more) as errexit, so errexit is redundant. And it has more pitfalls. Remove it.

Signed-off-by Josh Poimboeuf <[email protected]>

The errtrace option (combined with the ERR trap) already serves the same function (and more) as errexit, so errexit is redundant. And it has more pitfalls. Remove it.

Make sure all patch files actually exist. Otherwise there can be confusing errors later.

Signed-off-by Josh Poimboeuf <[email protected]>

Make sure all patch files actually exist. Otherwise there can be confusing errors later.

When a patch applies with fuzz, the detailed output from the patch tool can be very noisy, especially for big patches.

Suppress the fuzz details by default, while keeping the "applied with fuzz" warning. The noise can be restored with '--verbose'.

Signed-off-by Josh Poimboeuf <[email protected]>

When a patch applies with fuzz, the detailed output from the patch tool can be very noisy, especially for big patches.

Suppress the fuzz details by default, while keeping the "applied with fuzz" warning. The noise can be restored with '--verbose'.

Signed-off-by Josh Poimboeuf <[email protected]>

If a build error occurs and the user hits Ctrl-C while a large patch is being reverted during cleanup, the cleanup EXIT trap gets re-triggered and tries to re-revert the already partially-reverted patch. That causes 'patch -R' to repeatedly prompt

"Unreversed patch detected! Ignore -R? [n]"

for each already-reverted hunk, with no way to break out.

Fix it by adding '--force' to the patch revert command in revert_patch(), which causes it to silently ignore already-reverted hunks. And ignore errors, as the cleanup is always best-effort.

For similar reasons, add to APPLIED_PATCHES before (rather than after) applying the patch in apply_patch() so an interrupted apply will also get cleaned up.

Fixes d36a7343f4ba ("livepatch/klp-build: switch to GNU patch and recountdiff")

Signed-off-by Josh Poimboeuf <[email protected]>

vDSO code runs in userspace and can't be livepatched. Such patches also cause spurious "new function" errors due to generated files like vdso*-image.c having unstable line numbers across builds.

Signed-off-by Josh Poimboeuf <[email protected]>

Realmode code is compiled as a separate 16-bit binary and embedded into the kernel image via rmpiggy.S. It can't be livepatched.

Signed-off-by Josh Poimboeuf <[email protected]>

Section symbols aren't grouped after their corresponding FILE symbols. Their sym->file should really be NULL rather than whatever random FILE happened to be last.

Signed-off-by Josh Poimboeuf <[email protected]>

---
 tools/objtool/elf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

tools/objtool/elf.c +1 -1

index c4cb371e72b2..00c2389f345f 100644
@@ -680,7 +680,7 @@ static int read_symbols(struct elf *elf)
 
 		if (is_file_sym(sym))
 			file = sym;
-		else if (sym->bind == STB_LOCAL)
+		else if (sym->bind == STB_LOCAL && !is_sec_sym(sym))
 			sym->file = file;
 	}
 

Signature · 1 line

2.53.0

Quoted 5 lines · Josh Poimboeuf

Section symbols aren't grouped after their corresponding FILE symbols. Their sym->file should really be NULL rather than whatever random FILE happened to be last.

Signed-off-by Josh Poimboeuf <[email protected]>

Acked-by Peter Zijlstra (Intel) <[email protected]>

Instead of installing libsubcmd headers to a build output directory and including from there, include directly from tools/lib/ where they already exist. This fixes clangd indexing which otherwise can't find libsubcmd headers.

Signed-off-by Josh Poimboeuf <[email protected]>

If an object file has no functions, objtool has nothing to checksum, so it doesn't create the .discard.sym_checksum symbol.

Then when 'objtool klp diff' reads symbol checksums, it errors out due to the missing .discard.sym_checksum section.

Instead, just create an empty checksum section to signal to read_sym_checksums() that the file has been processed.

Signed-off-by Josh Poimboeuf <[email protected]>

Print the full objtool command line when '--verbose' is given to help with debugging.

Signed-off-by Josh Poimboeuf <[email protected]>

Clang generates anonymous data sections named .data..Lanon.<hash>. These need section-symbol references in the same way as .data..Lubsan (GCC) and .data..L__unnamed_ (Clang UBSAN) sections. Without this, convert_reloc_sym() fails when processing relocations that reference these sections.

Signed-off-by Josh Poimboeuf <[email protected]>

Improve readability with a new is_alias_sym() helper.

No functional changes intended.

Signed-off-by Josh Poimboeuf <[email protected]>

---
 tools/objtool/check.c               | 6 +++---
 tools/objtool/include/objtool/elf.h | 5 +++++
 2 files changed, 8 insertions(+), 3 deletions(-)

tools/objtool/check.c +3 -3

index 54ceac857979..4c18d6e7f6c3 100644
@@ -491,7 +491,7 @@ static int decode_instructions(struct objtool_file *file)
 				return -1;
 			}
 
-			if (func->embedded_insn || func->alias != func)
+			if (func->embedded_insn || is_alias_sym(func))
 				continue;
 
 			if (!find_insn(file, sec, func->offset)) {

@@ -2229,7 +2229,7 @@ static int add_jump_table_alts(struct objtool_file *file)
 		return 0;
 
 	for_each_sym(file->elf, func) {
-		if (!is_func_sym(func) || func->alias != func)
+		if (!is_func_sym(func) || is_alias_sym(func))
 			continue;
 
 		mark_func_jump_tables(file, func);

@@ -4527,7 +4527,7 @@ static int validate_symbol(struct objtool_file *file, struct section *sec,
 		return 1;
 	}
 
-	if (sym->pfunc != sym || sym->alias != sym)
+	if (sym->pfunc != sym || is_alias_sym(sym))
 		return 0;
 
 	insn = find_insn(file, sec, sym->offset);

tools/objtool/include/objtool/elf.h +5 -0

index cd5844c7b4e2..3abe4cbc584c 100644
@@ -279,6 +279,11 @@ static inline bool is_local_sym(struct symbol *sym)
 	return sym->bind == STB_LOCAL;
 }
 
+static inline bool is_alias_sym(struct symbol *sym)
+{
+	return sym->alias != sym;
+}
+
 static inline bool is_prefix_func(struct symbol *sym)
 {
 	return sym->prefix;

Signature · 1 line

2.53.0

Quoted 5 lines · Josh Poimboeuf

Improve readability with a new is_alias_sym() helper.

No functional changes intended.

Signed-off-by Josh Poimboeuf <[email protected]>

Acked-by Peter Zijlstra (Intel) <[email protected]>

tools/objtool/elf.c +4 -1

index 00c2389f345f..8a6e1338af97 100644
@@ -586,8 +586,11 @@ static int elf_add_symbol(struct elf *elf, struct symbol *sym)
 	if (strstarts(sym->name, ".klp.sym"))
 		sym->klp = 1;
 
+	sym->pfunc = sym->cfunc = sym;
+
 	if (!sym->klp && !is_sec_sym(sym) && strstr(sym->name, ".cold")) {
-		sym->cold = 1;
+		/* Tell read_symbols() this is a cold subfunction */
+		sym->pfunc = NULL;
 
 		/*
 		 * Clang doesn't mark cold subfunctions as STT_FUNC, which

Quoted 18 lines · Josh Poimboeuf

tools/objtool/elf.c +4 -1

index 00c2389f345f..8a6e1338af97 100644
@@ -586,8 +586,11 @@ static int elf_add_symbol(struct elf *elf, struct symbol *sym)
 	if (strstarts(sym->name, ".klp.sym"))
 		sym->klp = 1;
 
+	sym->pfunc = sym->cfunc = sym;
+
 	if (!sym->klp && !is_sec_sym(sym) && strstr(sym->name, ".cold")) {
-		sym->cold = 1;
+		/* Tell read_symbols() this is a cold subfunction */
+		sym->pfunc = NULL;
 
 		/*
 		 * Clang doesn't mark cold subfunctions as STT_FUNC, which

tools/objtool/elf.c static int elf_add_symbol(struct elf *elf, struct symbol *sym) · line 603

Quoted 7 lines

tools/objtool/elf.c +0 -2

@@ -596,8 +599,6 @@ static int elf_add_symbol(struct elf *elf, struct symbol *sym)
 		sym->type = STT_FUNC;
 	}
 
-	sym->pfunc = sym->cfunc = sym;
-
 	return 0;
 }

So now the cold subfunction has a NULL parent-function and a child-function that points to the parent?

I'm confused.

Quoted 31 lines · Peter Zijlstra

Quoted 18 lines · Josh Poimboeuf

tools/objtool/elf.c +4 -1

index 00c2389f345f..8a6e1338af97 100644
@@ -586,8 +586,11 @@ static int elf_add_symbol(struct elf *elf, struct symbol *sym)
 	if (strstarts(sym->name, ".klp.sym"))
 		sym->klp = 1;
 
+	sym->pfunc = sym->cfunc = sym;
+
 	if (!sym->klp && !is_sec_sym(sym) && strstr(sym->name, ".cold")) {
-		sym->cold = 1;
+		/* Tell read_symbols() this is a cold subfunction */
+		sym->pfunc = NULL;
 
 		/*
 		 * Clang doesn't mark cold subfunctions as STT_FUNC, which

tools/objtool/elf.c static int elf_add_symbol(struct elf *elf, struct symbol *sym) · line 603

Quoted 7 lines

tools/objtool/elf.c +0 -2

@@ -596,8 +599,6 @@ static int elf_add_symbol(struct elf *elf, struct symbol *sym)
 		sym->type = STT_FUNC;
 	}
 
-	sym->pfunc = sym->cfunc = sym;
-
 	return 0;
 }

So now the cold subfunction has a NULL parent-function and a child-function that points to the parent?

Quoted 1 line

I'm confused.

It's a bit clunky. As the comment implies, 'sym->pfunc = NULL' is a signal to it caller read_symbols() that this is a .cold function. Then, after all the symbols have been added, read_symbols() goes and finds the parent.

I think I did it this way because klp-diff.c calls elf_add_symbol() (via elf_create_symbol()) and later needs to call is_cold_func() on it. In that case, even though the parent isn't set, it still works because is_cold_func() returns true for sym->pfunc != sym;

Quoted 39 lines · Josh Poimboeuf

Quoted 31 lines · Peter Zijlstra

Quoted 18 lines · Josh Poimboeuf

tools/objtool/elf.c +4 -1

index 00c2389f345f..8a6e1338af97 100644
@@ -586,8 +586,11 @@ static int elf_add_symbol(struct elf *elf, struct symbol *sym)
 	if (strstarts(sym->name, ".klp.sym"))
 		sym->klp = 1;
 
+	sym->pfunc = sym->cfunc = sym;
+
 	if (!sym->klp && !is_sec_sym(sym) && strstr(sym->name, ".cold")) {
-		sym->cold = 1;
+		/* Tell read_symbols() this is a cold subfunction */
+		sym->pfunc = NULL;
 
 		/*
 		 * Clang doesn't mark cold subfunctions as STT_FUNC, which

tools/objtool/elf.c static int elf_add_symbol(struct elf *elf, struct symbol *sym) · line 603

Quoted 7 lines

tools/objtool/elf.c +0 -2

@@ -596,8 +599,6 @@ static int elf_add_symbol(struct elf *elf, struct symbol *sym)
 		sym->type = STT_FUNC;
 	}
 
-	sym->pfunc = sym->cfunc = sym;
-
 	return 0;
 }

So now the cold subfunction has a NULL parent-function and a child-function that points to the parent?

Quoted 1 line

I'm confused.

It's a bit clunky. As the comment implies, 'sym->pfunc = NULL' is a signal to it caller read_symbols() that this is a .cold function. Then, after all the symbols have been added, read_symbols() goes and finds the parent.

Quoted 4 lines

I think I did it this way because klp-diff.c calls elf_add_symbol() (via elf_create_symbol()) and later needs to call is_cold_func() on it. In that case, even though the parent isn't set, it still works because is_cold_func() returns true for sym->pfunc != sym;

I'm thinking this needs more comments if it stays like this. Is most confusing.

In preparation for porting the checksum code to other arches, make its functionality independent from the CFG reverse engineering code.

Move it into a standalone calculate_checksums() function which iterates all functions and instructions directly, rather than being called inline from do_validate_branch().

Since checksum_update_insn() is no longer called during CFG traversal, it needs to manually iterate the alternatives.

Signed-off-by Josh Poimboeuf <[email protected]>