Re: [Patch v8 4/5] perf regs: Enable dumping of SIMD registers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Low] Misleading post-increment of `idx` in `simd_regs_dump__printf` inner loop.

Pre-existing issues:
- [High] Integer underflow in manual `sample_size` calculation in `perf_event__convert_sample_callchain` corrupts the event stream if registers are missing.
--

commit 7e4b22be95dca28326d1271ce91f25c99e231f74
Author: Dapeng Mi <dapeng1.mi@linux.intel.com>

perf regs: Enable dumping of SIMD registers

This patch adds support for dumping SIMD registers using the new
PERF_SAMPLE_REGS_ABI_SIMD ABI.

> diff --git a/tools/perf/builtin-inject.c b/tools/perf/builtin-inject.c
> --- a/tools/perf/builtin-inject.c
> +++ b/tools/perf/builtin-inject.c
> @@ -457,8 +457,13 @@ static int perf_event__convert_sample_callchain(const struct perf_tool *tool,
>  	/* adjust sample size for stack and regs */
>  	sample_size -= sample->user_stack.size;
>  	sample_size -= (hweight64(evsel->core.attr.sample_regs_user) + 1) * sizeof(u64);

[Severity: High]
This is a pre-existing issue, but does unconditionally subtracting the full
expected size of the register mask cause an integer underflow if the perf
sample didn't capture user registers?

If the sample didn't capture user registers (e.g., when sampling a kernel
thread), the ABI is PERF_SAMPLE_REGS_ABI_NONE (0). It seems subtracting the
full mask size would underflow sample_size (a u32), resulting in a massive
size being written to event_copy->header.size.

Could this corrupt the event stream or cause out-of-bounds reads when
perf_event__repipe_synth writes to the pipe?

> -	if (sample->user_regs && sample->user_regs->abi & PERF_SAMPLE_REGS_ABI_SIMD)
> -		sample_size -= 4 * sizeof(u64);	/* Reduce SIMD regs header size */
> +	if (sample->user_regs && sample->user_regs->abi & PERF_SAMPLE_REGS_ABI_SIMD) {
> +		sample_size -= 4 * sizeof(u64);
> +		sample_size -= (sample->user_regs->nr_vectors *
> +				sample->user_regs->vector_qwords +
> +				sample->user_regs->nr_pred *
> +				sample->user_regs->pred_qwords) * sizeof(u64);
> +	}
>  	sample_size += (sample->callchain->nr + 1) * sizeof(u64);

[ ... ]

> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> --- a/tools/perf/util/session.c
> +++ b/tools/perf/util/session.c

[ ... ]

> +	for (i = 0; i < regs->nr_vectors; i++) {
> +		for (j = 0; j < regs->vector_qwords; j++) {
> +			idx = i * regs->vector_qwords + j;
> +			if (regs->vector_qwords > 1) {
> +				printf(".... %3s[%d][%d] 0x%016" PRIx64 "\n",
> +				       name, i, j, regs->simd_data[idx++]);

[Severity: Low]
Is the post-increment of idx necessary here?

Since idx is re-initialized at the start of each inner loop iteration
(idx = i * regs->vector_qwords + j), the incremented value seems to be
discarded on the next iteration.

> +			} else {
> +				printf(".... %3s[%d] 0x%016" PRIx64 "\n",
> +				       name, i, regs->simd_data[idx++]);
> +			}
> +		}
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260529082451.591783-1-dapeng1.mi@linux.intel.com?part=4