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

[Namhyung Kim <namhyung@kernel.org>]

On Wed, Jul 01, 2026 at 04:56:06PM +0800, Tengda Wu wrote:
> 
> 
> On 2026/7/1 12:16, sashiko-bot@kernel.org wrote:
> > Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> > - [Medium] Memory leak of the dynamically allocated `type_name` string obtained from `strbuf_detach()`.
> > - [Medium] Cross-DSO contamination and potential type confusion due to globally caching a DWARF offset (`task_struct_dieoff`).
> > --
> > 
> > commit 0f7d6f3c161448b3cb967b47ee3887be3d974dc1
> > Author: Tengda Wu <wutengda@huaweicloud.com>
> > 
> > perf annotate-arm64: Support 'mrs' instruction to track 'current' pointer
> > 
> > This commit adds support for parsing the 'mrs' instruction on arm64 in
> > 'perf annotate' to track the 'current' task pointer. It dynamically queries
> > the DWARF debug information for the 'get_current()' inline function's
> > return type to find 'struct task_struct', caching the DWARF offset for
> > subsequent lookups.
> > 
> >> diff --git a/tools/perf/util/annotate-arch/annotate-arm64.c b/tools/perf/util/annotate-arch/annotate-arm64.c
> >> index ec6fd59d51a27..89e3997f6721b 100644
> >> --- a/tools/perf/util/annotate-arch/annotate-arm64.c
> >> +++ b/tools/perf/util/annotate-arch/annotate-arm64.c
> > [ ... ]
> >> @@ -787,6 +789,81 @@ static void update_adrp_insn_state(struct type_state *state,
> >>  		     insn_offset, tsr->addr, dreg);
> >>  }
> >>  
> >> +static Dwarf_Off task_struct_dieoff;
> >> +
> >> +static void update_mrs_insn_state(struct type_state *state,
> >> +				  struct data_loc_info *dloc, Dwarf_Die *cu_die,
> >> +				  struct disasm_line *dl,
> >> +				  struct annotated_op_loc *dst)
> >> +{
> > [ ... ]
> >> +	/* 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")) {
> >> +		invalidate_reg_state(tsr);
> >> +		return;
> >> +	}
> >> +
> >> +	ip = dloc->ms->sym->start + dl->al.offset;
> >> +	pc = map__rip_2objdump(dloc->ms->map, ip);
> >> +
> >> +	if (!task_struct_dieoff ||
> >> +	    !dwarf_offdie(dloc->di->dbg, task_struct_dieoff, &type_die)) {
> > 
> > [Severity: Medium]
> > Since DWARF offsets are local to their .debug_info section, if perf annotate
> > processes a symbol in a different kernel module (where dso__kernel() is still
> > true), couldn't this global task_struct_dieoff be applied to the module's
> > Dwarf * handle?
> > 
> > Would this result in type confusion or returning an arbitrary, unrelated DIE
> > across different DSOs?

Right, you need to invalidate the cache when the current debug info is
closed.

> > 
> >> +		struct strbuf sb;
> >> +		char *type_name;
> >> +		/*
> >> +		 * Find the inline function 'get_current()' Dwarf_Die
> >> +		 * and obtain its return value data type, which should
> >> +		 * be 'struct task_struct*'.
> >> +		 */
> >> +		if (!die_find_inlinefunc(cu_die, pc, &func_die) ||

Probably better to check the name of the function.

Thanks,
Namhyung


> >> +			!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);
> >> +			return;
> >> +		}
> > 
> > [Severity: Medium]
> > Does this code leak the type_name string?
> > 
> > strbuf_detach() allocates and transfers ownership of the string to type_name,
> > but it appears to never be freed on either the success path or the error path
> > returning here.
> > 
> 
> Agreed. Will fix it in next version.
> 
> >> +
> >> +		/*
> >> +		 * 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_dieoff = dwarf_dieoffset(&type_die);
> >> +	}
> > 
>