Re: [PATCH v3 10/21] perf annotate: Deduplicate overlapping ARM SPE events for data type profiling

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Invalid union access causes severe type punning, false positive event dropping, and logic failure.
- [Medium] Unprotected dereference of potentially NULL `evsel->name` causing a segmentation fault.
--

commit 21f6f127e80c94aececac4065deebfce02da27b3
Author: Tengda Wu <wutengda@huaweicloud.com>

perf annotate: Deduplicate overlapping ARM SPE events for data type profiling

When data type profiling is enabled on ARM SPE, multiple overlapping
events are synthesized for a single sampled instruction. This patch
implements automatic deduplication in process_sample_event() by retaining
only the "instructions" event when data type annotation is enabled and no
explicit itrace options are configured.

> diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
> index 69cb72b2082a6..46b2bd3fb2013 100644
> --- a/tools/perf/builtin-annotate.c
> +++ b/tools/perf/builtin-annotate.c
> @@ -283,9 +283,22 @@ static int process_sample_event(const struct perf_tool *tool,
>  				struct machine *machine)
>  {
>  	struct perf_annotate *ann = container_of(tool, struct perf_annotate, tool);
> +	struct evsel *evsel = sample->evsel;
>  	struct addr_location al;
>  	int ret = 0;
>  
> +	/*
> +	 * ARM SPE synthesizes multiple overlapping events (e.g., l1d-miss,
> +	 * tlb-access) for a single sampled instruction. When data type is
> +	 * enabled, counting all of these overlapping events would inflate
> +	 * the profile and distort the data type statistics. To avoid this,
> +	 * deduplicate by retaining only the comprehensive "instructions" event.
> +	 */
> +	if (ann->data_type && !ann->session->itrace_synth_opts->set &&
> +	    event->auxtrace_info.type == PERF_AUXTRACE_ARM_SPE &&

[Severity: Medium]
Since event is a union of all perf events, does checking
event->auxtrace_info.type on a PERF_RECORD_SAMPLE event read the sample data
payload as if it were an auxtrace type?

If a normal event's first 32-bit payload (like PID 4 when PERF_SAMPLE_TID
is the first element) equals 4 (PERF_AUXTRACE_ARM_SPE), could this falsely
match and silently drop the event?

> +	    strcmp(evsel->name, "instructions") != 0)

[Severity: Medium]
Could evsel->name be NULL here if the event is unnamed or if we are
processing a perf.data file missing the HEADER_EVENT_DESC feature?

Passing NULL to strcmp() would cause a segmentation fault. Would it be safer
to use the evsel__name(evsel) wrapper instead?

> +		return 0;
> +
>  	addr_location__init(&al);

[ ... ]

> diff --git a/tools/perf/util/arm-spe.c b/tools/perf/util/arm-spe.c
> index 552f063f126e6..e027e3b200bfb 100644
> --- a/tools/perf/util/arm-spe.c
> +++ b/tools/perf/util/arm-spe.c
> @@ -403,6 +403,7 @@ static void arm_spe_prep_sample(struct arm_spe *spe,
>  	event->sample.header.type = PERF_RECORD_SAMPLE;
>  	event->sample.header.misc = sample->cpumode;
>  	event->sample.header.size = sizeof(struct perf_event_header);
> +	event->auxtrace_info.type = PERF_AUXTRACE_ARM_SPE;

[Severity: Medium]
Writing to auxtrace_info.type modifies offset 8 of the event union, which
aliases the sample payload.

If trace injection is used, does perf_event__synthesize_sample() subsequently
overwrite this offset with the actual sample payload (such as sample->id),
erasing this magic value and breaking the deduplication entirely?

>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260701035355.752944-1-wutengda@huaweicloud.com?part=10