Re: [RFC PATCH v2 04/11] objtool/klp: Read symbol identities from DWARF

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] objtool/klp: broken external and source filtering in fallback_by_name()
- [Medium] objtool/klp: mishandled .data.rel.ro and aliases in get_identity_for_symbol()
- [Medium] objtool/klp: check_relocations() fails on SHN_UNDEF debug relocations
- [Medium] objtool/klp: missing .lto_priv handling in matches_name() and demangle_name()
--

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

objtool/klp: Read symbol identities from DWARF

This commit adds a libdw-based DWARF reader to extract defining translation
units and enclosing scope chains for ELF symbols in objtool KLP builds. It
indexes DIE addresses and names, resolves abstract origins and specifications,
and validates DWARF symbol identities against STT_FILE symbols.

> diff --git a/tools/objtool/klp-dwarf.c b/tools/objtool/klp-dwarf.c
> new file mode 100644
> index 0000000000000..99b098ee7904f
> --- /dev/null
> +++ b/tools/objtool/klp-dwarf.c
> @@ -0,0 +1,1181 @@

[ ... ]

> +/*
> + * 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: Medium]
As reported by Sashiko on RFC [1], won't check_relocations() fail on valid
ET_REL kernel and module objects whose .debug_info or .debug_addr sections
reference external (SHN_UNDEF) symbols?

When build_identities() reports a single offline ET_REL object via
dwfl_report_offline(), libdwfl only sets sh_size to 0 on a relocation section
when every relocation in that section is resolved.

In ET_REL objects compiled with -O2 -g, .rela.debug_info (DWARF 4) and
.rela.debug_addr (DWARF 5) can contain relocations against SHN_UNDEF symbols
(for example, optimized-out pointers initialized to the address of an external
symbol via DW_OP_addr or DW_OP_addrx with DW_OP_stack_value, or
DW_TAG_call_site targets). Because libdwfl leaves those SHN_UNDEF relocations
in place with sh.sh_size non-zero, check_relocations() will reject the object
with "unapplied relocations for .debug_info" or ".debug_addr" unless it checks
whether the remaining relocations only target SHN_UNDEF symbols.

[ ... ]

> +/* Match DW_AT_name verbatim; accept compiler suffixes only on the ELF name. */
> +static bool matches_name(const char *name, const char *identifier)
> +{
> +	size_t len = strlen(identifier);
> +
> +	if (strncmp(name, identifier, len))
> +		return false;
> +	name += len;
> +	while (*name) {
> +		if (strstarts(name, ".llvm."))
> +			name += 5;
> +		if (*name++ != '.' || !isdigit((unsigned char)*name))
> +			return false;
> +		while (isdigit((unsigned char)*name))
> +			name++;
> +	}
> +	return true;
> +}

[Severity: Medium]
As reported by Sashiko on RFC [2], how are GCC LTO .lto_priv.<N> suffixes
handled in matches_name() and in sym->identity.demangled_name?

In matches_name(), the suffix loop only skips .llvm.<digits> and .<digits>, so
an ELF symbol renamed by GCC LTO with .lto_priv.<N> will fail to match its
DW_AT_name in matches_dwarf_name().

Similarly, demangled_name_len() in tools/objtool/elf.c only strips .llvm.* and
trailing .<digits>, so foo demangles to foo while foo.lto_priv.0 demangles to
foo.lto_priv, and 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. Because identity_hash(),
same_identity(), and identity->path in tools/objtool/klp-identity.c use
identity->demangled_name directly, won't symbols whose .lto_priv.<N> suffix is
added, removed, or renumbered before a clone tag end up with mismatched
identities across builds?

[ ... ]

> +static bool needs_identity_source(struct symbol *sym, bool external)
> +{
> +	return !external || (!is_global_sym(sym) && !is_weak_sym(sym));
> +}
> +
> +static void get_die_identity(struct identity_die *entry, struct symbol *sym,
> +			     struct identity *identity)
> +{
> +	const char *source = entry->source;
> +
> +	identity->scope = entry->scope;
> +	identity->external = entry->external;
> +	if (entry->cu->assembly) {
> +		source = entry->cu->path;
> +		identity->scope = "/";
> +		/* Assembly linkage is not subject to LTO promotion/internalization. */
> +		identity->external = sym->bind != STB_LOCAL;
> +	}
> +	identity->source = needs_identity_source(sym, identity->external) ? source : "";
> +}

[ ... ]

> +/* GCC may omit address attributes for IPA-ICF wrappers such as msr_event_del. */
> +static int fallback_by_name(struct elf *elf, struct symbol *sym, struct identity_cu **matched_cu)
> +{
> +	const char *name = sym->pfunc->identity.demangled_name;
> +	const char *file = sym->file ? basename(sym->file->name) : NULL;
> +	struct identity_die *entry, *match = NULL;
> +	struct identity candidate, matched;
> +
> +	if (!is_func_sym(sym) && !(is_object_sym(sym) && is_rodata_sec(sym->sec)))
> +		return 1;
> +	elf_hash_for_each_possible(elf, dwarf_name, entry, name_hash, str_hash(name)) {
> +		if (entry->type != sym->type || strcmp(name, entry->name))
> +			continue;
> +		/* Different CUs can have the same FILE basename and function name. */
> +		if (is_func_sym(sym) && !cu_contains_symbol(entry->cu, sym))
> +			continue;
> +		get_die_identity(entry, sym, &candidate);
> +		if (!candidate.source)
> +			return 1;
> +		if (!entry->cu->assembly && !candidate.external && file &&
> +		    strcmp(file, basename(candidate.source)) &&
> +		    !known_file_name_mismatch(file, basename(candidate.source)))
> +			continue;
> +		if (match) {
> +			get_die_identity(match, sym, &matched);
> +			if (strcmp(matched.source, candidate.source) ||
> +			    strcmp(matched.scope, candidate.scope) ||
> +			    matched.external != candidate.external)
> +				return 1;
> +		}
> +		match = entry;
> +	}
> +	if (!match)
> +		return 1;
> +	*matched_cu = match->cu;
> +	get_die_identity(match, sym, &sym->identity);
> +	return 0;
> +}

[Severity: Medium]
Can the candidate.external and candidate.source checks in fallback_by_name()
cause unrelated DIEs from other translation units to match or collide?

In read_symbols(), sym->file is only populated when sym->bind == STB_LOCAL.
For an STB_LOCAL symbol, needs_identity_source() always returns true even when
entry->external is true, so candidate.source is set to entry->source.

However, the STT_FILE comparison in fallback_by_name() is guarded by
!candidate.external:

	if (!entry->cu->assembly && !candidate.external && file &&
	    strcmp(file, basename(candidate.source)) &&
	    !known_file_name_mismatch(file, basename(candidate.source)))
		continue;

Because of this:

1. When looking up an STB_LOCAL symbol (such as a read-only object where
   cu_contains_symbol() is not called, or a local function in a shared
   section), any DW_AT_external DIE with the same name in another CU has
   candidate.external == true and bypasses the file check. It will then either
   collide with the local symbol's own DIE at the match comparison (causing
   fallback_by_name() to return 1) or be chosen as the match for the local
   symbol.

2. Conversely, when sym is STB_GLOBAL or STB_WEAK (where file is NULL),
   fallback_by_name() does not filter out non-external (!candidate.external)
   static DIEs from other CUs, allowing a static DIE in another CU to collide
   with the global DIE.

3. In addition, index_identities() adds external DIEs to dwarf_name even when
   entry->source is NULL. Checking if (!candidate.source) return 1; inside the
   loop before filtering out unrelated CUs aborts the entire search on the
   first sourceless external DIE from another CU rather than continuing to
   remaining candidates.

> +
> +int get_identity_for_symbol(struct elf *elf, struct symbol *sym)
> +{
> +	struct identity *identity = &sym->identity, candidate;
> +	struct section *sec = sym->sec;
> +	struct identity_cu *cu = NULL;
> +	struct die_address *address;
> +	struct identity_die *entry;
> +	unsigned int nr = 0, nr_both_matched = 0;
> +	bool 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, address, hash, sym->offset ^ sec->idx) {
> +		if (!address_matches_symbol(address, sym))
> +			continue;
> +		entry = address->die;
> +		nr++;
> +		if (matches_dwarf_name(entry, sym->name))
> +			nr_both_matched++;
> +	}
> +	if (!nr) {
> +		int ret = fallback_by_name(elf, sym, &cu);
> +
> +		if (ret == 1)
> +			ret = check_if_assembly(elf, sym, &cu);
> +		if (ret)
> +			return ret;
> +		goto found;
> +	}
> +
> +	/*
> +	 * ICF/constant merging can leave several definitions after name
> +	 * filtering. FIXME: Folded objects can remain uncorrelated when
> +	 * their checksums change.
> +	 */
> +	if (nr_both_matched > 1 && strstarts(sec->name, ".rodata"))
> +		return 1;
> +
> +	/*
> +	 * Prefer address and name. Compiler clones, syscall/weak aliases and
> +	 * objcopy's __pi_ prefix can differ from DW_AT_name. If no name matches,
> +	 * accept address candidates only when their identities agree below.
> +	 */
> +	objname = elf->name;
> +	elf_hash_for_each_possible(elf, dwarf_address, address, hash, sym->offset ^ sec->idx) {
> +		if (!address_matches_symbol(address, sym))
> +			continue;
> +		entry = address->die;
> +		if (nr_both_matched && !matches_dwarf_name(entry, sym->name))
> +			continue;
> +		get_die_identity(entry, sym, &candidate);
> +		error = "conflicting declarations";
> +		if (identity->scope && strcmp(identity->scope, candidate.scope))
> +			goto failed;
> +		error = "conflicting external declarations";
> +		if (identity->scope && identity->external != candidate.external)
> +			goto failed;

[Severity: Medium]
As reported by Sashiko on RFC [3], there appear to be two issues in
get_identity_for_symbol() when handling folded read-only objects and aliases:

1. The constant-merging check tests strstarts(sec->name, ".rodata") instead of
   is_rodata_sec(sec) (which fallback_by_name() uses above). Because
   mark_rodata() marks both .rodata* and .data.rel.ro* sections as rodata,
   won't folded static const objects containing relocated pointers in
   .data.rel.ro or .data.rel.ro.local (or __pi_-prefixed merged read-only
   objects where nr > 1 and nr_both_matched == 0) fall through into the loop
   and fail with a fatal "conflicting declarations" or "conflicting source
   CUs" error?

2. When an STB_GLOBAL or STB_WEAK symbol is an alias of a static (STB_LOCAL)
   symbol (for example, a __x64_sys_* syscall stub aliasing a static
   __do_sys_* function), the alias itself often has no concrete DWARF DIE at
   that address, leaving only the static target DIE (nr == 1,
   nr_both_matched == 0). In that case, the loop accepts the static target's
   DIE and get_die_identity() assigns identity->external = false and a
   non-empty CU source path to the global or weak alias symbol (or fails at
   "conflicting external declarations" if both the static target DIE and an
   external alias/weak DIE exist at the same address). Should a global or weak
   symbol matched to a non-external DIE at the same address still be treated
   as external?

[1] https://lore.kernel.org/r/sashiko-outbox-185077@kernel.org
[2] https://lore.kernel.org/r/sashiko-outbox-185077@kernel.org
[3] https://lore.kernel.org/r/sashiko-outbox-185077@kernel.org

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