Re: [RFC PATCH v2 08/11] objtool/klp: Include symbol identities in reference checksums

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] objtool/klp: incomplete .lto_priv suffix handling in checksum_name()
- [Medium] objtool/klp: alias checksum mismatch in checksum_object() and klp-diff
- [Medium] objtool/klp: broken .rodata and cross-CU matching in fallback_by_name()
- [Medium] objtool/klp: missing alias and TU checks in check_linkage_changes()
--

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

objtool/klp: Include symbol identities in reference checksums

This commit updates livepatch checksum calculation to prepare DWARF
identities and hash symbol identity paths for named references instead of
relying solely on suffix-stripped ELF names.

> diff --git a/tools/objtool/klp-checksum.c b/tools/objtool/klp-checksum.c
> index 1a5f2fa89baa5..44c0eb784e570 100644
> --- a/tools/objtool/klp-checksum.c
> +++ b/tools/objtool/klp-checksum.c
> @@ -77,6 +77,12 @@ static bool is_const_reference(struct symbol *sym)
>  static int checksum_object(struct objtool_file *file, struct symbol *sym,
>  			   unsigned int depth);
>  
> +/* Use suffix-stripped ELF names for references without DWARF identities. */
> +static const char *checksum_name(struct symbol *sym)
> +{
> +	return sym->identity.path ?: sym->identity.demangled_name;
> +}

[Severity: Medium]
When building with GCC LTO, GCC appends .lto_priv.<num> suffixes to
privatized symbols. In tools/objtool/klp-diff.c, same_compiler_name() calls
skip_lto_priv() so that symbols whose .lto_priv.<num> suffix is added or
removed between original and patched builds can still correlate:

tools/objtool/klp-diff.c:skip_lto_priv() {
	if (!strstarts(name, ".lto_priv.") || !isdigit((unsigned char)name[10]))
		return name;
	name += 10;
	while (isdigit((unsigned char)*name))
		name++;
	return name;
}

However, demangled_name_len() in tools/objtool/elf.c only strips .llvm.<hash>
and trailing .<digits> suffixes:

tools/objtool/elf.c:demangled_name_len() {
	...
	while (len && isdigit(name[len - 1])) {
		p = name + len - 1;
		while (p > name && isdigit(*p))
			p--;

		if (p == name || *p != '.')
			break;
		len = p - name;
	}
	return len;
}

As a result, a symbol like foo.lto_priv.0 has sym->identity.demangled_name
truncated to foo.lto_priv instead of foo, which also leaves .lto_priv in
sym->identity.path. In addition, matches_name() in tools/objtool/klp-dwarf.c
only skips .llvm. and .<digits>, rejecting .lto_priv.<num> when matching ELF
names against DWARF DW_AT_name, and find_twin() in tools/objtool/klp-diff.c
keys hash lookups and same_identity() on sym->identity.demangled_name before
calling is_twin_candidate() -> compatible_symbol_name() ->
same_compiler_name() -> skip_lto_priv().

If a referenced symbol gains or loses a .lto_priv.<num> suffix between
builds, won't checksum_name() hash different strings and cause false checksum
mismatches as well as failed symbol correlation?

> @@ -170,8 +176,8 @@ static int checksum_update_insn(struct objtool_file *file, struct symbol *func,
>  		__checksum_update_insn(func, insn, &sym->csum.checksum,
>  				       sizeof(sym->csum.checksum));
>  	} else {
> -		__checksum_update_insn(func, insn, sym->identity.demangled_name,
> -				       strlen(sym->identity.demangled_name));
> +		__checksum_update_insn(func, insn, checksum_name(sym),
> +				       strlen(checksum_name(sym)));
>  	}
>  	__checksum_update_insn(func, insn, &offset, sizeof(offset));

[Severity: Medium]
In calculate_checksums(), symbols where is_alias_sym(sym) is true are skipped
so that only the representative symbol sym->alias gets a checksum in
.discard.sym_checksum.

However, in checksum_update_insn() and checksum_update_object(), compound
constants are hashed by calling checksum_object() directly on sym without
resolving sym->alias or checking whether the checksum was already computed:

tools/objtool/klp-checksum.c:checksum_update_insn() {
	...
	if (is_compound_constant(sym)) {
		if (checksum_object(file, sym, 1))
			return -1;
		__checksum_update_insn(func, insn, &sym->csum.checksum,
				       sizeof(sym->csum.checksum));
	}
}

And on the diff side in tools/objtool/klp-diff.c, read_sym_checksums() only
populates csum.checksum on sym->alias (leaving sym->csum.checksum == 0 on
aliases), whereas correlate_by_identity() pairs aliased static objects
directly via orig_sym->twin = patched_sym, and mark_changed_functions() and
find_twin_by_identity_and_checksum() inspect sym->csum.checksum directly
rather than sym->alias->csum.checksum:

tools/objtool/klp-diff.c:mark_changed_functions() {
	...
		if (orig_sym->csum.checksum != patched_sym->csum.checksum) {
			if (!is_func_sym(orig_sym)) {
				ERROR("changed data: %s", orig_sym->name);
				return -1;
			}
}

Doesn't this emit duplicate .discard.sym_checksum entries for compound
constant aliases, and cause mark_changed_functions() to falsely report
changed data when constant merging or symbol table ordering changes which
symbol is the representative alias?

> @@ -411,6 +418,12 @@ int cmd_klp_checksum(int argc, const char **argv)
>  	if (ret)
>  		goto out;
>  
> +	for_each_sym(file->elf, sym)
> +		sym->dont_correlate = dont_correlate(sym);
> +	ret = prepare_identities(file->elf);
> +	if (ret)
> +		goto out;
> +
>  	ret = calculate_checksums(file);
>  	if (ret)
>  		goto out;

[Severity: Medium]
Now that cmd_klp_checksum() calls prepare_identities(),
get_identity_for_symbol() invokes fallback_by_name() in
tools/objtool/klp-dwarf.c when no matching address DIE is found:

tools/objtool/klp-dwarf.c:fallback_by_name() {
	const char *name = sym->pfunc->identity.demangled_name;
	const char *file = sym->file ? basename(sym->file->name) : NULL;
	...
	elf_hash_for_each_possible(elf, dwarf_name, entry, name_hash, str_hash(name)) {
		if (entry->type != sym->type || strcmp(name, entry->name))
			continue;
		...
		if (!entry->cu->assembly && !candidate.external && file &&
		    strcmp(file, basename(candidate.source)) &&
		    !known_file_name_mismatch(file, basename(candidate.source)))
			continue;
}

Could this fail in two cases for .rodata symbols?

First, under Clang, function-local static const objects in .rodata have ELF
names of the form <func>.<var>[.<suffix>], so
sym->pfunc->identity.demangled_name is <func>.<var>, whereas dwarf_name is
indexed by DW_AT_name (<var>), so strcmp(name, entry->name) never matches.

Second, for top-level .rodata symbols in multi-TU objects where two CUs share
the same basename, fallback_by_name() does not exclude DIEs that already have
concrete addresses (entry->nr_addresses > 0), skips the file check when
candidate.external is true, and only compares basename(sym->file->name)
against basename(candidate.source) rather than checking
sym->file->identity.source. Can a .rodata symbol without a location attribute
in one CU match a same-named DIE in another CU and cause
complete_file_source() to fail with conflicting DWARF sources?

[Severity: Medium]
Also related to identity handling in tools/objtool/klp-diff.c,
check_linkage_changes() checks orig_sym->twin and patched_sym->twin directly
when scanning for linkage changes across symbols with the same demangled
name:

tools/objtool/klp-diff.c:check_linkage_changes() {
	for_each_sym(e->orig, orig_sym) {
		if (orig_sym->twin || orig_sym->dont_correlate ||
		    !has_dwarf_identity(orig_sym) || is_undef_sym(orig_sym) || !orig_sym->len)
			continue;

		for_each_sym_by_demangled_name(e->patched, orig_sym->identity.demangled_name,
					       patched_sym) {
			if (patched_sym->twin || patched_sym->dont_correlate ||
			    !has_dwarf_identity(patched_sym) ||
			    is_undef_sym(patched_sym) || !patched_sym->len)
				continue;

			if (orig_sym->type != patched_sym->type ||
			    strcmp(orig_sym->identity.scope, patched_sym->identity.scope) ||
			    orig_sym->identity.external == patched_sym->identity.external)
				continue;

			ERROR("unsupported linkage change for %s",
			      orig_sym->identity.demangled_name);
			return -1;
		}
	}
}

Unlike correlate_symbols() and validate_symbol_twins(), which use
reference_twin(sym) so data aliases whose representative symbol is correlated
are treated as correlated, check_linkage_changes() checks sym->twin directly
and does not check whether orig_sym and patched_sym belong to the same
translation unit.

In a multi-TU object or an object with correlated data aliases, won't an
uncorrelated static symbol in one TU matching the demangled name of an
uncorrelated global symbol in a different TU (or an uncorrelated alias of a
correlated symbol) trigger a false "unsupported linkage change" error?

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