Re: [PATCH v6 21/26] perf annotate-arm64: Support stack variable tracking

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] perf: stale stack state due to bypassed invalidation in update_store_insn_state
--

commit 2ddb0f384d44eeb70844d8c3adc06f8d55101f68
Author: Tengda Wu <wutengda@huaweicloud.com>

perf annotate-arm64: Support stack variable tracking

Extend update_insn_state_arm64() to track data types stored on the stack. The
implementation handles stack loads and stores to follow data types as they
move between registers and memory.

> diff --git a/tools/perf/util/annotate-arch/annotate-arm64.c b/tools/perf/util/annotate-arch/annotate-arm64.c
> index a72c7703dd6b3..8339aab0d314f 100644
> --- a/tools/perf/util/annotate-arch/annotate-arm64.c
> +++ b/tools/perf/util/annotate-arch/annotate-arm64.c
[ ... ]
> +/*
> + * For store insns: propagate type from @sreg to the memory location referenced
> + * by @dst, applying @mem_spacing to account for multi-regs stores (e.g. stp).
> + */
> +static void propagate_store_reg_state(struct type_state *state,
> +				      struct data_loc_info *dloc,
> +				      struct disasm_line *dl, int sreg,
> +				      struct annotated_op_loc *dst,
> +				      int mem_spacing)
> +{
[ ... ]
> +		if (!has_reg_type(state, sreg) || !state->regs[sreg].ok) {
> +			stack = find_stack_state(state, offset);
> +			/* Preserve compound states when only a member is overwritten. */
> +			if (stack && !stack->compound)
> +				delete_stack_state(state, offset);
> +
> +			return;
> +		}
[ ... ]
> +static void update_store_insn_state(struct type_state *state,
> +				    struct data_loc_info *dloc,
> +				    struct disasm_line *dl,
> +				    struct annotated_op_loc *src,
> +				    struct annotated_op_loc *dst)
> +{
> +	int mem_spacing = 0;
> +
> +	if (!has_reg_type(state, dst->reg1))
> +		return;
> +
> +	/* Handle the first source register */
> +	if (has_reg_type(state, src->reg1))
> +		propagate_store_reg_state(state, dloc, dl, src->reg1,
> +					  dst, mem_spacing);
> +
> +	if (!src->multi_regs || !has_reg_type(state, src->reg2))
> +		goto out_adjust;

[Severity: Medium]
Does the has_reg_type() guard here inadvertently skip the stack state
invalidation logic?

Looking at propagate_store_reg_state() above, there is logic to delete the
stack state if the source register is untracked. However, because
update_store_insn_state() completely bypasses the call to
propagate_store_reg_state() when the source register is untracked, this
cleanup is never reached.

If an untracked register is stored into a stack slot previously used for a
tracked variable, could this leave stale tracking state on the stack, causing
subsequent loads from that slot to inherit the wrong type?

> +
> +	/* Handle the second source register */
> +	mem_spacing = arm64__reg_size(dl->ops.source.raw);
> +
> +	if (mem_spacing >= 0)
> +		propagate_store_reg_state(state, dloc, dl, src->reg2,
> +					  dst, mem_spacing);
> +
> +out_adjust:
> +	adjust_reg_index_state(state, dloc, dl, dst);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1789521520.git.wutengda@huaweicloud.com?part=21