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

[Namhyung Kim <namhyung@kernel.org>]

On Wed, Jul 01, 2026 at 04:06:05AM +0000, sashiko-bot@kernel.org wrote:
> 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?

Right, you should not access event->auxtrace in process_sample_event().
You need to find other way to detect if it's processing an ARM SPE
synthesized sample.

> 
> > +	    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?

Agreed.

> 
> > +		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?

Right, please don't do this.

Thanks,
Namhyung