Re: [PATCH v2 01/58] perf inject: Fix itrace branch stack synthesis

[sashiko-bot@kernel.org]

Sashiko AI review found 3 potential issue(s):
- [High] Hardware trace decoders omit `hw_idx` from synthetic events, causing payload corruption
- [High] Pipe mode emits mismatched ATTR headers and SAMPLE payloads, breaking stream parsing
- [High] Using `--itrace=l` bypasses AUX data removal, leaking raw payloads and breaking the ATTR schema
--

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

perf inject: Fix itrace branch stack synthesis

When using "perf inject --itrace=L" to synthesize branch stacks from AUX
data, several issues caused failures. This commit fixes them by adjusting
how sample_type and branch_sample_type are handled in synthesis, record
layout, and early injection processing.

> diff --git a/tools/perf/builtin-inject.c b/tools/perf/builtin-inject.c
> index f174bc69cec45..88c0ef4f5ff19 100644
> --- a/tools/perf/builtin-inject.c
> +++ b/tools/perf/builtin-inject.c
> @@ -375,7 +375,59 @@ static int perf_event__repipe_sample(const struct perf_tool *tool,
[ ... ]
> -	if (inject->itrace_synth_opts.set && sample->aux_sample.size) {
> +	if (inject->itrace_synth_opts.set &&
> +	    (inject->itrace_synth_opts.last_branch ||
> +	     inject->itrace_synth_opts.add_last_branch)) {
> +		union perf_event *event_copy = (void *)inject->event_copy;
> +		struct branch_stack dummy_bs = { .nr = 0 };
> +		int err;
> +		size_t sz;
> +		u64 orig_type = evsel->core.attr.sample_type;
> +		u64 orig_branch_type = evsel->core.attr.branch_sample_type;
> +
> +		if (event_copy == NULL) {
> +			inject->event_copy = malloc(PERF_SAMPLE_MAX_SIZE);
> +			if (!inject->event_copy)
> +				return -ENOMEM;
> +
> +			event_copy = (void *)inject->event_copy;
> +		}
> +
> +		if (!sample->branch_stack)
> +			sample->branch_stack = &dummy_bs;
> +
> +		if (inject->itrace_synth_opts.add_last_branch) {
> +			/* Temporarily add in type bits for synthesis. */
> +			evsel->core.attr.sample_type |= PERF_SAMPLE_BRANCH_STACK;
> +			evsel->core.attr.branch_sample_type |= PERF_SAMPLE_BRANCH_HW_INDEX;
> +			evsel->core.attr.sample_type &= ~PERF_SAMPLE_AUX;
> +		}

If --itrace=l is passed, last_branch is true but add_last_branch is false.

Will the code enter the first if branch but skip the add_last_branch block,
meaning evsel->core.attr.sample_type is not modified and still contains
PERF_SAMPLE_AUX?

Since it took the first if branch, it bypasses the else if fallback block
that calls perf_inject__cut_auxtrace_sample(). Does this mean it fails to
drop the AUX data?

Later, __cmd_inject() unconditionally strips PERF_SAMPLE_AUX from the file
header. Will this result in the output file falsely claiming there is no
AUX data in its header, while retaining the raw AUX payloads in the records?

[ ... ]
> @@ -2434,12 +2488,25 @@ static int __cmd_inject(struct perf_inject *inject)
>  		 * synthesized hardware events, so clear the feature flag.
>  		 */
>  		if (inject->itrace_synth_opts.set) {
> +			struct evsel *evsel;
> +
>  			perf_header__clear_feat(&session->header,
>  						HEADER_AUXTRACE);
> -			if (inject->itrace_synth_opts.last_branch ||
> -			    inject->itrace_synth_opts.add_last_branch)
> +
> +			evlist__for_each_entry(session->evlist, evsel) {
> +				evsel->core.attr.sample_type &= ~PERF_SAMPLE_AUX;
> +			}
> +
> +			if (inject->itrace_synth_opts.add_last_branch) {
>  				perf_header__set_feat(&session->header,
>  						      HEADER_BRANCH_STACK);
> +
> +				evlist__for_each_entry(session->evlist, evsel) {
> +					evsel->core.attr.sample_type |= PERF_SAMPLE_BRANCH_STACK;
> +					evsel->core.attr.branch_sample_type |=
> +						PERF_SAMPLE_BRANCH_HW_INDEX;
> +				}
> +			}
>  		}

These permanent evsel->core.attr modifications appear to be located in a
block that only runs when not in pipe mode (!inject->output.is_pipe).

In pipe mode (perf inject -o -), input ATTR events are repiped early and
unchanged. Later, perf_event__repipe_sample() temporarily mutates
evsel->core.attr.sample_type to inject the branch stack and removes AUX
data, repiping the SAMPLE payload using this modified layout.

Does this cause the downstream pipe consumer to receive ATTRs that specify
no branch stack and include AUX data, while the SAMPLE payloads are
mutated? Could this mismatch cause the parser to misinterpret the branch
stack bytes as subsequent fields, leading to out-of-bounds reads?

> diff --git a/tools/perf/util/intel-pt.c b/tools/perf/util/intel-pt.c
> index fc9eec8b54b82..2dce6106c038d 100644
> --- a/tools/perf/util/intel-pt.c
> +++ b/tools/perf/util/intel-pt.c
> @@ -1731,8 +1731,12 @@ static void intel_pt_prep_b_sample(struct intel_pt *pt,
>  static int intel_pt_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);
>  }

Hardware trace injection functions like intel_pt_inject_event pass 0 for
the branch_sample_type parameter here. Consequently, the hw_idx field is
not written to the synthetic sample payloads.

However, the event attributes generated by these trace synthesizers declare
that hw_idx is present (PERF_SAMPLE_BRANCH_HW_INDEX).

When perf report parses these events, will it expect the hw_idx field based
on the attribute header, misalign the payload, and consume the first branch
entry's from address as hw_idx?

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