Re: [RFC PATCH 1/3] objtool/klp: Correlate symbols using DWARF identity

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [High] objtool/klp: missing second find_twin() pass after find_twin_checksum()
- [High] objtool/klp: uncorrelated symbols misclassified in mark_changed_functions()
- [High] objtool/klp: demangle_name() mismatch with same_compiler_name() and LTO
- [High] objtool/klp: get_identity_for_symbol() mishandles aliases and .data.rel.ro
- [High] objtool/klp: collect_addresses() poisons target identity on compound ops
- [High] objtool/klp: check_relocations() fails on SHN_UNDEF debug relocations
- [High] objtool/klp: local clones of external functions omit defining CU source
- [Medium] objtool/klp: compatible_symbol_name() collapses object-fragment numbers
--

commit 8e5857a74bfb5a2a01cd3d2f06cf4f1ad842d7ed
Author: Longjun Luo <luolongjuna@gmail.com>

objtool/klp: Correlate symbols using DWARF identity

This commit replaces positional and suffix-map symbol correlation with
DWARF-derived (source, scope, name) symbol identities and checksum
disambiguation. It requires libdw for building objtool's klp support and
updates klp-build and the klp test suite to compile objects with -g.

> diff --git a/tools/objtool/klp-diff.c b/tools/objtool/klp-diff.c
> index 34da55e456097..957e589b66031 100644
> --- a/tools/objtool/klp-diff.c
> +++ b/tools/objtool/klp-diff.c
[ ... ]
> -static const char *llvm_suffix(const char *name)
> +static size_t llvm_name_len(const char *name)
>  {
> -	return strstr(name, ".llvm.");
> -}
> +	const char *suffix = strstr(name, ".llvm."), *p;
>  
> -static bool is_llvm_sym(struct symbol *sym)
> -{
> -	return llvm_suffix(sym->name);
> +	if (!suffix || !isdigit((unsigned char)suffix[6]))
> +		return strlen(name);
> +	for (p = suffix + 6; isdigit((unsigned char)*p); p++)
> +		;
> +	return *p ? strlen(name) : (size_t)(suffix - name);
>  }
>  
[ ... ]
> +/* Keep object-fragment numbers, e.g. amd_northbridges.0. */
> +static bool same_llvm_name(const char *a, const char *b)
>  {
> -	if (!sym1->file || !sym2->file)
> -		return true;
> -	if (sym1->file == sym2->file)
> -		return true;
> -	return sym1->file->twin == sym2->file;
> +	size_t a_len = llvm_name_len(a), b_len = llvm_name_len(b);
> +
> +	return a_len == b_len && !strncmp(a, b, a_len);
>  }
>  
> -/*
> - * Similar to maybe_same_file(), but strict: no scope changes allowed.
> - *
> - * Works for both same-ELF (direct pointer compare) and cross-ELF
> - * (compare via file->twin) cases.
> - */
> -static bool same_file(struct symbol *sym1, struct symbol *sym2)
> +static const char *skip_lto_priv(const char *name)
>  {
> -	if (llvm_suffix(sym1->name) && llvm_suffix(sym2->name))
> -		return true;
> -	if (!sym1->file && !sym2->file)
> -		return true;
> -	if (!sym1->file || !sym2->file)
> -		return false;
> -	if (sym1->file == sym2->file)
> -		return true;
> -	return sym1->file->twin == sym2->file;
> +	if (!strstarts(name, ".lto_priv.") || !isdigit((unsigned char)name[10]))
> +		return name;
> +	name += 10;
> +	while (isdigit((unsigned char)*name))
> +		name++;
> +	return name;
>  }
>  
> -/*
> - * Is it a local symbol, or at least was it local in the translation unit
> - * before LLVM promoted it?
> - */
> -static bool is_tu_local_sym(struct symbol *sym)
> +static bool is_numeric_suffix(const char *name)
>  {
> -	return is_local_sym(sym) || is_llvm_sym(sym);
> +	if (*name++ != '.' || !isdigit((unsigned char)*name))
> +		return false;
> +	while (isdigit((unsigned char)*name))
> +		name++;
> +	return !*name;
>  }
>  
[ ... ]
> +/* Allow compiler counters and GCC partition renames to change. */
> +static bool same_compiler_name(const char *a, const char *b)
>  {
[ ... ]
> +	for (;;) {
> +		const char *next_a = skip_lto_priv(a), *next_b = skip_lto_priv(b);
>  
> -		/* Level 3 */
> -		if (!same_file(sym1, sym2))
> +		if (next_a != a || next_b != b) {
> +			a = next_a;
> +			b = next_b;
>  			continue;
> -		file_patched++;
> -		file_last = sym2;
> -
> -		/* Level 4 */
> -		if (sym1->len != sym2->len || !sym1->csum.checksum ||
> -		    sym1->csum.checksum != sym2->csum.checksum)
> -			continue;
> -		csum_patched++;
> -		csum_last = sym2;
> +		}
> +		if (*a != *b)
> +			break;
> +		if (!*a)
> +			return true;
> +		if (*a == '.' && isdigit((unsigned char)a[1])) {
> +			if (!isdigit((unsigned char)b[1]))
> +				return false;
> +			for (a++; isdigit((unsigned char)*a); a++)
> +				;
> +			for (b++; isdigit((unsigned char)*b); b++)
> +				;
> +		} else {
> +			a++;
> +			b++;
> +		}
>  	}
>  
[ ... ]
> +	/* A trailing counter may be absent on one side. */
> +	return (!*a && is_numeric_suffix(b)) || (!*b && is_numeric_suffix(a));
>  }
>  
> +/* Keep clone tags such as .cold, .isra and .part distinct. */
> +static bool compatible_symbol_name(const char *a, const char *b)
>  {
> +	if (same_llvm_name(a, b))
> +		return true;
> +	if (strchr(a, '@') || strchr(b, '@') || strstr(a, ".str1."))
> +		return !strcmp(a, b);
> +	return same_compiler_name(a, b);
>  }

[Severity: Medium]
In compatible_symbol_name(), if same_llvm_name(a, b) returns false for two
SRA-split object fragments with different fragment numbers (such as
amd_northbridges.0 vs amd_northbridges.1, or amd_northbridges.0.llvm.1 vs
amd_northbridges.1.llvm.2), doesn't execution fall through to
same_compiler_name(a, b)?

Because same_compiler_name() treats any .<digits> suffix as a mutable
compiler counter, it returns true for .0 vs .1. Since SRA-split fragments
use multi-op DW_OP_piece DWARF location expressions,
get_identity_for_symbol() returns unavailable and both fragments share the
same fallback identity (demangled_name = "amd_northbridges"). Won't
compatible_symbol_name() then treat the distinct fragments as ambiguous
candidates for each other and fail to correlate them?

> +static bool is_twin_candidate(struct symbol *sym1, struct symbol *sym2)
> +{
> +	if (sym1->type != sym2->type || !same_identity(&sym1->identity, &sym2->identity))
> +		return false;
> +	/* Don't match zero-length labels with sized symbols. */
> +	if (!is_undef_sym(sym1) && !is_undef_sym(sym2) &&
> +	    (!sym1->len != !sym2->len))
> +		return false;
> +	return compatible_symbol_name(sym1->name, sym2->name);
> +}

[Severity: High]
Can same_compiler_name() ever be reached when .lto_priv.<N> suffixes,
interior clone counters, or one-sided numeric suffixes differ between
original and patched symbols?

In find_twin() and find_twin_checksum(), candidates are looked up in
symbol_identity_hash by identity_hash(&sym1->identity) and checked with
is_twin_candidate() -> same_identity(), which requires exact equality of
identity.demangled_name before calling compatible_symbol_name() ->
same_compiler_name().

However, demangled_name_len() in tools/objtool/elf.c computes
sym->identity.demangled_name by stripping .llvm.* or trailing [0-9._]+
characters:

tools/objtool/elf.c:demangled_name_len() {
	p = strstr(name, ".llvm.");
	if (p)
		return p - name;

	if (!strstarts(name, "__UNIQUE_ID_") && !strchr(name, '.'))
		return strlen(name);

	for (idx = strlen(name) - 1; idx >= 0; idx--) {
		char c = name[idx];

		if (!isdigit(c) && c != '.' && c != '_')
			break;
	}
}

As a result:

1. "foo" demangles to "foo", while "foo.lto_priv.0" stops at 'v' and
   demangles to "foo.lto_priv" (and "var.0.lto_priv.0" vs
   "var.1.lto_priv.0" demangle to "var.0.lto_priv" vs "var.1.lto_priv").

2. "foo.lto_priv.0.part.0" vs "foo.lto_priv.1.part.0" demangle to
   "foo.lto_priv.0.part" vs "foo.lto_priv.1.part", and "foo.isra.0.cold"
   vs "foo.isra.1.cold" demangle to "foo.isra.0.cold" vs
   "foo.isra.1.cold".

3. A symbol whose base name ends in digits or _<digits> with a one-sided
   counter (such as "var_1" vs "var_1.0", or "crc32" vs "crc32.0")
   demangles to "var_1" vs "var", or "crc32" vs "crc".

Since identity.demangled_name differs in all of these cases,
identity_hash() places them in different hash buckets and same_identity()
returns false before same_compiler_name() is called. Also, matches_name()
in tools/objtool/klp-dwarf.c only skips .llvm. and .<digits>, so it will
reject .lto_priv.<digits> suffixes when matching ELF names against DWARF
DW_AT_name in folded .rodata sections.

Should demangled_name_len() and matches_name() be updated to normalize
these suffixes consistently with same_compiler_name()?

[ ... ]
>  static int correlate_symbols(struct elfs *e)
>  {
> +	struct symbol *sym;
>  
> +	for_each_sym(e->orig, sym)
> +		sym->dont_correlate = dont_correlate(sym);
> +	for_each_sym(e->patched, sym)
> +		sym->dont_correlate = dont_correlate(sym);
>  
> +	if (prepare_identities(e->orig) || prepare_identities(e->patched))
> +		return -1;
>  
> +	correlate_pass(e, find_twin);
> +	correlate_pass(e, find_twin_checksum);

[Severity: High]
Does correlate_symbols() need a second correlate_pass(e, find_twin) after
correlate_pass(e, find_twin_checksum)?

Suppose two symbols share the same (source, scope, demangled_name)
identity and compatible symbol names (for example, the two ThinLTO
promoted helper.llvm.<hash> functions in
tools/objtool/tests/generic/test-thinlto-ambiguity.sh, which are both
compiled from thinlto_ambiguity.c), and a patch modifies one of them while
leaving the other unchanged:

1. The first correlate_pass(e, find_twin) sees candidates == 2 for both
   symbols and returns NULL.

2. correlate_pass(e, find_twin_checksum) pairs the unchanged symbol by
   checksum, setting sym->twin = match and match->twin = sym.

3. Even though the modified symbol is now the sole remaining unpaired
   candidate on both sides (and find_twin() skips already-paired symbols
   via sym2->twin), find_twin() is never run again.

Consequently, the modified symbol is left with twin == NULL, misclassified
by mark_changed_functions() as a newly added function rather than a
changed function, and omitted from .init.klp_funcs.

>  
> +	for_each_sym(e->orig, sym)
> +		if (!sym->twin && !sym->dont_correlate)
> +			WARN("no correlation: %s", sym->name);
>  
> +	return 0;
> +}
[ ... ]
> @@ -1079,7 +817,8 @@ 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) || patched_sym->dont_correlate)
> +		if (!is_func_sym(patched_sym) || patched_sym->dont_correlate ||
> +		    is_undef_sym(patched_sym))
>  			continue;
>  
>  		if (!patched_sym->twin) {
> @@ -1206,6 +945,9 @@ static bool klp_reloc_needed(struct reloc *patched_reloc)
>  		 * patch.  The symbol could be defined in this .o or in another
>  		 * .o in the patch module.
>  		 *
> +		 * FIXME: Failed correlation can duplicate existing data, breaking
> +		 * shared state or pointer identity.
> +		 *

[Severity: High]
When ambiguous symbols fail to correlate, correlate_symbols() emits a
warning and returns 0 with twin == NULL. However, don't the subsequent
passes in cmd_klp_diff() treat every symbol with twin == NULL and
!dont_correlate as a newly added symbol?

Specifically:

1. In mark_changed_functions(), the first loop skips any orig_sym with
   !orig_sym->twin (bypassing the "changed data" error check if an
   uncorrelated data object was modified), and the second loop marks any
   patched_sym with !patched_sym->twin as a "new function" (setting
   included = 1, changed = 0). In create_klp_sections(), only functions
   with sym->clone->changed get a struct klp_func_ext entry in
   .init.klp_funcs, so an uncorrelated modified function is cloned into
   the output object as dead code without livepatching the target
   function.

2. In klp_reloc_needed() and clone_reloc(), any uncorrelated non-exported
   symbol (!patched_sym->twin) referenced by a patched function returns
   klp_reloc_needed() == false and is cloned with its data into the
   livepatch module as a private copy instead of referencing the original
   kernel symbol via a KLP relocation. This also happens for unchanged
   static local variables when a patched function contains two same-named
   static locals in different unnamed lexical blocks with the same size
   and initial value, since resolve_parent_scope() ignores
   DW_TAG_lexical_block.

Should correlation failure for symbols that exist in e->orig (or
ambiguous candidates in e->patched that match an uncorrelated symbol's
identity) abort cmd_klp_diff() instead of silently generating a broken
livepatch module?

> diff --git a/tools/objtool/klp-dwarf.c b/tools/objtool/klp-dwarf.c
> new file mode 100644
> index 0000000000000..eff054f2864b6
> --- /dev/null
> +++ b/tools/objtool/klp-dwarf.c
[ ... ]
> +static int collect_addresses(struct identity_reader *reader, struct identity_die *entry)
> +{
> +	Dwarf_Die *die = &entry->die, type;
> +	Dwarf_Attribute attr, address_attr;
> +	Dwarf_Addr base, start, end, address;
> +	Dwarf_Op *ops;
> +	const char *source, *scope;
> +	size_t count;
> +	ptrdiff_t off = 0;
> +	bool external = false;
> +	int tag = dwarf_tag(die);
> +
> +	if (!entry->definition || (tag != DW_TAG_subprogram && tag != DW_TAG_variable))
> +		return 0;
> +	if (dwarf_attr_integrate(die, DW_AT_external, &attr) && dwarf_formflag(&attr, &external))
> +		return die_error(die, "invalid external flag");
> +	/*
> +	 * External identities do not need source. DWARF relocations for overridden
> +	 * weak definitions can bind to the selected strong definition, giving the
> +	 * same address conflicting source CUs (GDB PR 34741):
> +	 * https://sourceware.org/bugzilla/show_bug.cgi?id=34741
> +	 */
> +	source = external ? NULL : resolve_definition_source(entry);

[Severity: High]
For an external function (DW_AT_external = true), its cold subfunction
(foo.cold) is emitted as a range on foo's DW_TAG_subprogram, and its
compiler clones (such as foo.part.0, foo.isra.0, or foo.constprop.0) have
DW_AT_abstract_origin pointing to foo, inheriting external = true via
dwarf_attr_integrate().

Because collect_addresses() sets source = NULL whenever external is true,
these STB_LOCAL clone symbols receive identity.external = true and
identity.source = "".

When vmlinux.o links a translation unit containing a __weak function foo
and another translation unit containing a strong override foo, ld -r
merges the global foo symbol, but retains both translation units'
STB_LOCAL foo.cold (or foo.part.0 / foo.isra.0) symbols in .symtab. With
both local symbols assigned identity ("", "/", "foo.cold") and identical
sym->name, won't validate_dwarf_identity() fail with a "duplicate symbol
identity" error?

> +	if (tag == DW_TAG_subprogram) {
[ ... ]
> +	}
> +	if (!dwarf_attr(die, DW_AT_location, &attr))
> +		return 0;
> +	/* Location lists may vary with the PC; index only fixed storage. */
> +	if (dwarf_whatform(&attr) == DW_FORM_sec_offset ||
> +	    dwarf_whatform(&attr) == DW_FORM_loclistx)
> +		return 0;
> +	if (dwarf_getlocation(&attr, &ops, &count))
> +		return die_error(die, "invalid location expression");
> +	if (!count)
> +		return 0;
> +	if (dwarf_attr_integrate(die, DW_AT_type, &address_attr)) {
> +		if (!dwarf_formref_die(&address_attr, &type))
> +			return die_error(die, "invalid variable type reference");
> +		if (is_empty_type(&type))
> +			return 0;
> +	}
> +	if (resolve_definition_scope(reader, entry, &scope))
> +		return -1;
> +	if (ops[0].atom == DW_OP_addr) {
> +		address = ops[0].number;
> +	} else if (ops[0].atom == DW_OP_addrx || ops[0].atom == DW_OP_GNU_addr_index) {
> +		if (dwarf_getlocation_attr(&attr, ops, &address_attr) ||
> +		    dwarf_formaddr(&address_attr, &address))
> +			return die_error(die, "invalid location address");
> +	} else {
> +		return 0;
> +	}
> +	return add_address(reader, address + reader->bias, source,
> +			   count == 1 ? scope : NULL, entry->name, external);
> +}

[Severity: High]
When a local or static const pointer variable holds the address of another
symbol X (for example, "int *p = &counter;" or "fn_t cb = helper;"),
compilers at -O2 -g frequently emit a 2-op DW_AT_location expression
[DW_OP_addr(X), DW_OP_stack_value] (or DW_OP_addrx) on p's DIE to describe
p's value when p itself has no storage.

Because collect_addresses() inspects every DW_TAG_variable DIE and records
ops[0] with scope = (count == 1 ? scope : NULL), won't p's DIE insert an
address_identity entry at X's address with entry->scope == NULL?

Then, when get_identity_for_symbol() looks up X, it encounters that
entry->scope == NULL record, sets unavailable = true, and wipes out X's
own valid DWARF identity (resetting identity->source = "",
identity->scope = NULL, and identity->external = false). If a patch adds
or removes such a local pointer variable in a function, X's identity will
change between orig and patched, causing correlation failures or false
"unsupported linkage change" errors.

> +
> +/*
> + * libdwfl permits partial relocation, leaving unapplied entries in place.
> + * Check only sections read for identities; unused indexes may retain relocations.
> + */
> +static int check_relocations(Dwarf *dwarf)
> +{
> +	static const char * const sections[] = {
> +		".debug_info", ".debug_types", ".debug_abbrev",
> +		".debug_str", ".debug_line_str", ".debug_str_offsets",
> +		".debug_addr", ".debug_ranges", ".debug_rnglists",
> +	};
> +	Elf *elf = dwarf_getelf(dwarf);
> +	Elf_Scn *scn = NULL;
> +	GElf_Shdr sh, target;
> +	const char *name;
> +	size_t strings, i;
> +
> +	if (!elf || elf_getshdrstrndx(elf, &strings))
> +		return -1;
> +	while ((scn = elf_nextscn(elf, scn))) {
> +		if (!gelf_getshdr(scn, &sh))
> +			return -1;
> +		if (!sh.sh_size || (sh.sh_type != SHT_REL && sh.sh_type != SHT_RELA))
> +			continue;
> +		if (!gelf_getshdr(elf_getscn(elf, sh.sh_info), &target))
> +			return -1;
> +		name = elf_strptr(elf, strings, target.sh_name);
> +		if (!name)
> +			return -1;
> +		for (i = 0; i < ARRAY_SIZE(sections); i++)
> +			if (!strcmp(name, sections[i])) {
> +				ERROR("DWARF identity: unapplied relocations for %s", name);
> +				return -1;
> +			}
> +	}
> +	return 0;
> +}

[Severity: High]
Won't check_relocations() fail on valid ET_REL kernel objects and modules
that reference external (SHN_UNDEF) symbols in .debug_info or .debug_addr?

In elfutils (libdwfl/relocate.c), libdwfl only sets sh_size = 0 on a
relocation section when every relocation in that section has been
resolved. If any relocation targets an SHN_UNDEF symbol (such as an
optimized-out pointer initialized to an external symbol via DW_OP_addr /
DW_OP_stack_value, a DW_TAG_call_site target, or a DWARF 5 .debug_addr
entry referenced from .debug_loclists), libdwfl leaves that relocation in
place and keeps sh.sh_size non-zero, causing check_relocations() to abort
with "unapplied relocations for .debug_info" or ".debug_addr".

[ ... ]
> +int get_identity_for_symbol(struct elf *elf, struct symbol *sym)
> +{
> +	struct identity *identity = &sym->identity;
> +	struct section *sec = sym->sec;
> +	struct address_identity *entry;
> +	unsigned int nr = 0, nr_named = 0;
> +	bool filter_name = false, unavailable = false, source_unavailable = false;
> +	const char *error, *saved_objname = objname;
> +
> +	identity->source = "";
> +	identity->scope = NULL;
> +	identity->external = false;
> +	elf_hash_for_each_possible(elf, dwarf_address, entry, hash, sym->offset ^ sec->idx) {
> +		if (entry->section != sec->idx || entry->offset != sym->offset)
> +			continue;
> +		nr++;
> +		if (is_rodata_sec(sec) && matches_dwarf_name(entry, sym->name))
> +			nr_named++;
> +	}
> +	if (!nr)
> +		return 1;
> +
> +	/* Only folded read-only storage needs name-based disambiguation. */
> +	if (nr > 1 && is_rodata_sec(sec)) {
> +		filter_name = true;
> +		if (!nr_named)
> +			return 1;
> +		/*
> +		 * ICF/constant merging can leave several definitions after name
> +		 * filtering. FIXME: Folded objects can remain uncorrelated when
> +		 * their checksums change.
> +		 */
> +		if (nr_named > 1 && strstarts(sec->name, ".rodata"))
> +			return 1;
> +	}
> +
> +	objname = elf->name;
> +	elf_hash_for_each_possible(elf, dwarf_address, entry, hash, sym->offset ^ sec->idx) {
> +		if (entry->section != sec->idx || entry->offset != sym->offset)
> +			continue;
> +		if (filter_name && !matches_dwarf_name(entry, sym->name))
> +			continue;
> +		/* Scope cannot be resolved from the first address of a compound expression. */
> +		if (!entry->scope) {
> +			unavailable = true;
> +			/* Keep scanning: later entries may conflict. */
> +			continue;
> +		}
> +		error = "conflicting declarations";
> +		if (identity->scope && strcmp(identity->scope, entry->scope))
> +			goto failed;
> +		error = "conflicting external declarations";
> +		if (identity->scope && identity->external != entry->external)
> +			goto failed;

[Severity: High]
There seem to be a few issues with how get_identity_for_symbol() and
prepare_identities() handle aliases, .data.rel.ro sections, and fallback
symbols:

1. Because filter_name is only enabled when nr > 1 && is_rodata_sec(sec),
   what happens when a global or weak symbol is an __alias of a static
   function or variable in .text or .data (such as SYSCALL_DEFINE0 stubs
   __x64_sys_* aliasing static __do_sys_*, or static efi_runtime_lock and
   its global alias __efi_uv_runtime_lock)?

   - If only the static target has a DWARF DIE at that address (nr == 1),
     get_identity_for_symbol() does not check matches_dwarf_name() and
     assigns the static target's identity (external = false, source = CU
     path) to the global alias symbol.

   - If both the static target (external = false) and the global alias or
     an overridden weak COND_SYSCALL DIE (external = true) have DWARF DIEs
     at the same address in .text or .data (nr > 1, !is_rodata_sec(sec)),
     filter_name is false and get_identity_for_symbol() aborts at
     "conflicting external declarations".

2. In the nr > 1 && is_rodata_sec(sec) block above, the early return checks
   strstarts(sec->name, ".rodata") rather than is_rodata_sec(sec). Since
   mark_rodata() marks both .rodata* and .data.rel.ro* as rodata, won't
   merged objects in .data.rel.ro* with nr_named > 1 fall through into the
   loop and fail with "conflicting declarations" or "conflicting source
   CUs"?

3. In prepare_identities() in tools/objtool/klp-identity.c, when DWARF is
   unavailable (!identity->scope), identity->external is left as false
   even when the ELF symbol has STB_GLOBAL or STB_WEAK binding, which can
   then trigger false "unsupported linkage change" errors in
   check_linkage_changes().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026101916.3823942-1-luolongjuna@gmail.com?part=1