Re: [PATCH v4 23/23] perf annotate-arm64: Support 'mrs' instruction to track 'current' pointer

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] The global `task_struct_cache` may permanently cache a DWARF offset for an incomplete forward declaration of `struct task_struct`, breaking task tracking globally.
- [High] The globally static `task_struct_cache` caches a raw `Dwarf *` pointer as a validation key, which is susceptible to an ABA problem when DWARF handles are recycled, leading to corrupt DWARF parsing.
- [Medium] Calling `die_find_inlinefunc()` with a Compilation Unit DIE (`cu_die`) instead of a Subprogram DIE (`sp_die`) causes a severe O(N) recursive DWARF tree traversal.
- [Medium] Caching the DWARF offset of `struct task_struct *` relies on a brittle non-deterministic order dependency, silently failing for compiler-generated code like stack canaries.
- [Low] Missing explicit inclusion of `<strings.h>` for `strcasecmp()`, which violates subsystem guidelines and causes build failures on musl libc.
--

--- Patch [23]: [PATCH v4 23/23] perf annotate-arm64: Support 'mrs' instruction to track 'current' pointer ---
commit 9ea98019ec4e55f722077c51c12557f82e4d5f60
Author: Tengda Wu <[email protected]>

perf annotate-arm64: Support 'mrs' instruction to track 'current' pointer

This commit extends the arm64 instruction state tracker to handle mrs
instructions reading from sp_el0. It attempts to track the current
task pointer by locating the get_current() inline function and caching
the struct task_struct DWARF offset for subsequent resolution.

> diff --git a/tools/perf/util/annotate-arch/annotate-arm64.c b/tools/perf/util/annotate-arch/annotate-arm64.c
> index b01fa6cf865ff..9d1f315dcb280 100644
> --- a/tools/perf/util/annotate-arch/annotate-arm64.c
> +++ b/tools/perf/util/annotate-arch/annotate-arm64.c

[ ... ]

> @@ -889,6 +890,96 @@ static void update_adrp_insn_state(struct type_state *state,

[ ... ]

> +	/* Handle case difference: LLVM (SP_EL0) vs objdump (sp_el0) */
> +	if (!dso__kernel(map__dso(dloc->ms->map)) ||
> +	    strcasecmp(dl->ops.source.raw, "sp_el0")) {

[Severity: Low]
Is the <strings.h> header missing for strcasecmp()?

Building perf with musl libc requires explicit inclusion of POSIX headers,
and omitting <strings.h> can cause musl build failures.

> +		invalidate_reg_state(tsr);
> +		return;
> +	}

[ ... ]

> +	/*
> +	 * Invalidate cached DIE offset if the debug info handle changed
> +	 * (e.g. switching between vmlinux and a kernel module).
> +	 */
> +	if (task_struct_cache.dbg != dloc->di->dbg) {
> +		task_struct_cache.dieoff = 0;
> +		task_struct_cache.dbg = dloc->di->dbg;
> +	}

[Severity: High]
Does checking the raw Dwarf pointer expose an ABA problem?

If debug info handles are freed and reallocated for different DSOs during
analysis, the memory address could be recycled. This would bypass the cache
invalidation and cause perf to use a stale DWARF offset from the old DSO
on the new DSO, potentially leading to out-of-bounds reads in libdw.

> +
> +	if (!task_struct_cache.dieoff ||
> +	    !dwarf_offdie(dloc->di->dbg, task_struct_cache.dieoff, &type_die)) {
> +		struct strbuf sb;
> +		char *type_name;
> +		/*
> +		 * Find the inline function 'get_current()' Dwarf_Die

[Severity: Medium]
Can this cache mechanism create a non-deterministic order dependency?

If the first mrs instruction processed is a stack canary, which does not
belong to get_current(), the search will fail and the state is invalidated.

However, if a true get_current() is processed first, the cache is populated
and the function check is bypassed for subsequent stack canaries, allowing
them to succeed.

> +		 * and obtain its return value data type, which should
> +		 * be 'struct task_struct*'.
> +		 */
> +		if (!die_find_inlinefunc(cu_die, pc, &func_die) ||

[Severity: Medium]
Will passing the Compilation Unit DIE (cu_die) instead of a Subprogram
DIE cause a severe performance regression?

Because the underlying callback returns DIE_FIND_CB_CONTINUE, passing
the entire compilation unit forces die_find_inlinefunc() to perform a
full, inefficient O(N) recursive DWARF AST walk across every node in the
file when the cache is empty.

> +		    !die_compare_name(&func_die, "get_current") ||
> +		    !dwarf_attr_integrate(&func_die, DW_AT_type, &attr) ||
> +		    !dwarf_formref_die(&attr, &type_die)) {
> +			invalidate_reg_state(tsr);
> +			return;
> +		}
> +
> +		strbuf_init(&sb, 32);
> +		die_get_typename_from_type(&type_die, &sb);
> +		type_name = strbuf_detach(&sb, NULL);
> +
> +		if (!type_name || strcmp(type_name, "struct task_struct*")) {
> +			invalidate_reg_state(tsr);
> +			free(type_name);
> +			return;
> +		}
> +
> +		/*
> +		 * Cache the 'struct task_struct*' die offset globally.
> +		 * This allows us to resolve stack canary accesses even
> +		 * in CUs that lack a full task_struct definition (e.g.,
> +		 * compiler-generated entry/exit code).
> +		 */
> +		task_struct_cache.dieoff = dwarf_dieoffset(&type_die);

[Severity: High]
Could this code permanently cache incomplete forward declarations of
struct task_struct?

The string comparison matches "struct task_struct*", but there is no check
to verify if the DIE is a complete definition (e.g., by checking
DW_AT_declaration or dwarf_aggregate_size()).

If the first processed CU only has a forward declaration, its incomplete
offset will be cached globally, causing all subsequent member offset
lookups to fail since an incomplete type has no children.

> +		free(type_name);
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=23