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

[Tengda Wu <wutengda@huaweicloud.com>]

On 2026/8/8 21:20, sashiko-bot@kernel.org wrote:
> 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 <wutengda@huaweicloud.com>
> 
> 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);
>> +	}
> 

The cache was originally introduced to address stack_canary data type
identification, but it still has many issues at this point, so I are removing
it and will not support it for now.

Thanks,
Tengda