Re: [PATCH 3/3] perf cs-etm: Check frontend instruction reads

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 2 related pre-existing issues it does not
introduce:
- [Medium] perf cs-etm: missing perf_sample__exit() in sample synthesis
  error paths [1]
- [Medium] perf cs-etm: infinite loop in cs_etm__sample() when period is
  0 [2]
--

commit 1c4da6763b4506cd44f0609ef42fba0e4bd574ea
Author: Leo Yan <leo.yan@arm.com>

perf cs-etm: Check frontend instruction reads

This commit updates cs_etm__frontend_mem_access() to return 0 on a complete
read and -EIO otherwise, propagating read errors through instruction size,
address calculation, instruction copying, and SVC detection.

> diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c
> index 312d4a734e84f..303e16a54445b 100644
> --- a/tools/perf/util/cs-etm.c
> +++ b/tools/perf/util/cs-etm.c

[ ... ]

> @@ -2011,11 +2021,13 @@ static int cs_etm__sample(struct cs_etm_queue *etmq,
>  			 */
>  			addr = cs_etm__instr_addr(etmq, tidq, tidq->packet,
>  						  offset - 1);
> -			ret = cs_etm__synth_instruction_sample(
> -				etmq, tidq, tidq->packet, addr,
> -				etm->instructions_sample_period);
> -			if (ret)
> -				return ret;
> +			if (addr != CS_ETM_INVAL_ADDR) {
> +				ret = cs_etm__synth_instruction_sample(
> +					etmq, tidq, tidq->packet, addr,
> +					etm->instructions_sample_period);
> +				if (ret)
> +					return ret;
> +			}

[Severity: Medium]
This problem wasn't introduced by this patch, but can
cs_etm__synth_instruction_sample() and cs_etm__synth_branch_sample() leak
sample resources when event injection fails [1]?

Both functions initialize sample with perf_sample__init(&sample, true),
which allocates sub-structures, and then return early before
perf_sample__exit(&sample) if cs_etm__inject_event() returns an error:

tools/perf/util/cs-etm.c:cs_etm__synth_instruction_sample() {
	...
	perf_sample__init(&sample, /*all=*/true);
	...
	if (etm->synth_opts.inject) {
		ret = cs_etm__inject_event(etm, event, &sample,
					   etm->instructions_sample_type);
		if (ret)
			return ret;
	}
	...
	perf_sample__exit(&sample);
	return ret;
}

>  
>  			offset += etm->instructions_sample_period;
>  			tidq->period_instructions -=

[Severity: Medium]
This problem wasn't introduced by this patch, but can cs_etm__sample() spin
forever when the user specifies an instruction sample period of 0 (for
example, --itrace=i0i) [2]?

The loop condition is:

tools/perf/util/cs-etm.c:cs_etm__sample() {
	...
		while (tidq->period_instructions >=
				etm->instructions_sample_period) {
			...
			offset += etm->instructions_sample_period;
			tidq->period_instructions -=
				etm->instructions_sample_period;
		}
}

Because both operands are unsigned, tidq->period_instructions >= 0 is
always true and subtracting 0 never reduces tidq->period_instructions.

[1] https://sashiko.dev/#/bug/linux-313ce3d0-12d6-4671-8007-6e3925af60fc
[2] https://sashiko.dev/#/bug/linux-88c200a9-e322-487b-904a-28d581ccee8b

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113-perf_fix_gcc15_failure-v1-0-eae592c5ff64@arm.com?part=3