Re: [PATCH v2 1/2] perf event: Fix size of synthesized sample with branch stacks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Synthesized branch stack samples in hardware tracers (arm-spe, cs-etm, intel-pt) omit the hw_idx field despite the event attribute claiming its presence, causing parsers to read misaligned data.
- [High] In perf_event__convert_sample_callchain(), passing the unmasked evsel->core.attr.sample_type causes the generated payload to serialize stack and register data out of sync with the reduced header.size, leading to data stream corruption.
--

commit eb27565513c2df3df4959ab5f50b38abd090fa86
Author: Ian Rogers <irogers@google.com>

perf event: Fix size of synthesized sample with branch stacks

This commit updates sample event synthesis to correctly size and write the
nr and hw_idx values for branch stacks by explicitly checking the
PERF_SAMPLE_BRANCH_HW_INDEX flag.

> 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
[ ... ]
> @@ -463,8 +463,13 @@ static int perf_event__convert_sample_callchain(const struct perf_tool *tool,
>  	/* remove sample_type {STACK,REGS}_USER for synthesize */
>  	sample_type &= ~(PERF_SAMPLE_STACK_USER | PERF_SAMPLE_REGS_USER);
>  
> -	perf_event__synthesize_sample(event_copy, sample_type,
> -				      evsel->core.attr.read_format, sample);
> +	ret = perf_event__synthesize_sample(event_copy, evsel->core.attr.sample_type,
> +					    evsel->core.attr.read_format,
> +					    evsel->core.attr.branch_sample_type, sample);

I noticed this is fixed later in the patch series by commit "perf inject:
Fix itrace branch stack synthesis".

Does passing the unmasked evsel->core.attr.sample_type here instead of the
local masked sample_type variable cause the generated payload to serialize
stack and register data out of sync with the reduced header size?

[ ... ]
> diff --git a/tools/perf/util/arm-spe.c b/tools/perf/util/arm-spe.c
> --- a/tools/perf/util/arm-spe.c
> +++ b/tools/perf/util/arm-spe.c
[ ... ]
>  static int arm_spe__inject_event(union perf_event *event, struct perf_sample *sample, u64 type)
>  {
> -	event->header.size = perf_event__sample_event_size(sample, type, 0);
> -	return perf_event__synthesize_sample(event, type, 0, sample);
> +	event->header.type = PERF_RECORD_SAMPLE;
> +	event->header.size = perf_event__sample_event_size(sample, type, /*read_format=*/0,
> +							   /*branch_sample_type=*/0);
> +	return perf_event__synthesize_sample(event, type, /*read_format=*/0,
> +					     /*branch_sample_type=*/0, sample);
>  }

I see this is fixed for intel-pt.c later in the series by commit
"perf inject: Fix itrace branch stack synthesis", but it appears it remains
unfixed for arm-spe.c and cs-etm.c.

If branch_sample_type is hardcoded to 0 here, will it omit the hw_idx field
even if the synthesized event attribute claims it is present?

If so, will parsers reading the payload misinterpret the first 8 bytes of
the branch entries as the hw_idx field, shifting subsequent sample fields?

[ ... ]
> diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c
> --- a/tools/perf/util/cs-etm.c
> +++ b/tools/perf/util/cs-etm.c
[ ... ]
>  static int cs_etm__inject_event(union perf_event *event,
>  			       struct perf_sample *sample, u64 type)
>  {
> -	event->header.size = perf_event__sample_event_size(sample, type, 0);
> -	return perf_event__synthesize_sample(event, type, 0, sample);
> +	event->header.size = perf_event__sample_event_size(sample, type, /*read_format=*/0,
> +							   /*branch_sample_type=*/0);
> +	return perf_event__synthesize_sample(event, type, /*read_format=*/0,
> +					     /*branch_sample_type=*/0, sample);
>  }

Does this function suffer from the same issue, where hardcoding
branch_sample_type to 0 omits the hw_idx field and causes misaligned reads
during parsing?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260429181136.2712655-1-irogers@google.com?part=1