Re: [PATCH v2 1/2] perf inject: Support piped data with --convert-callchain

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issues to consider:
- [Medium] perf inject: unstripped pipe attrs in cmd_inject() for
  file-to-pipe mode
- [Medium] perf inject: attr/sample mismatch for non-DWARF events in
  piped mode
- [Medium] perf inject: unchecked attr.size write in
  perf_event__repipe_attr()
- [Medium] perf inject: missing ordered_events flush in
  perf_event__repipe_oe_synth()
--

commit c6a36707bb63847953234da6d70009330a9ffc22
Author: Serhei Makarov <serhei@serhei.io>

perf inject: Support piped data with --convert-callchain

This commit enables piped input and output with --convert-callchain by
stripping DWARF user register and stack sample attributes in
perf_event__repipe_attr() and deferring the DWARF callchain check to
perf_event__convert_sample_callchain().

> diff --git a/tools/perf/builtin-inject.c b/tools/perf/builtin-inject.c
> index ce7562fc788c2..9fd02a6f5849f 100644
> --- a/tools/perf/builtin-inject.c
> +++ b/tools/perf/builtin-inject.c
> @@ -229,6 +229,7 @@ static int perf_event__repipe_attr(const struct perf_tool *tool,
>  				   union perf_event *event,
>  				   struct evlist **pevlist)
>  {
> +	union perf_event *event0;
>  	struct perf_inject *inject = container_of(tool, struct perf_inject,
>  						  tool);
>  	struct perf_event_attr attr;
> @@ -258,6 +259,25 @@ static int perf_event__repipe_attr(const struct perf_tool *tool,
>  		goto out;
>  	}
>  
> +	if (inject->convert_callchain) {
> +		/* Copy event to repipe with corrected final attributes,
> +		   without confusing downstream users of pevlist: */
> +		event0 = event;
> +		event = (void *)inject->event_copy;
> +		if (event == NULL) {
> +			inject->event_copy = malloc(PERF_SAMPLE_MAX_SIZE);
> +			if (!inject->event_copy)
> +				return -ENOMEM;
> +			event = (void *)inject->event_copy;
> +		}
> +		memcpy(event, event0, event0->header.size);
> +
> +		event->attr.attr.sample_type &= ~(PERF_SAMPLE_REGS_USER | PERF_SAMPLE_STACK_USER);
> +		event->attr.attr.sample_regs_user = 0;
> +		event->attr.attr.sample_stack_user = 0;
> +		event->attr.attr.exclude_callchain_user = 0;
> +	}

[Severity: Medium]
Can clearing sample_regs_user and sample_stack_user here clobber the
trailing event ID array when event->attr.attr.size is smaller than
PERF_ATTR_SIZE_VER3?

A PERF_RECORD_HEADER_ATTR record places its u64 ID array immediately after
attr_size bytes (event->attr.attr.size ?: PERF_ATTR_SIZE_VER0), so writing
to sample_regs_user and sample_stack_user without checking attr_size
overwrites the event IDs in the repiped record for VER0, VER1, or VER2
attribute headers.

[ ... ]

> @@ -526,7 +558,7 @@ static int perf_event__convert_sample_callchain(const struct perf_tool *tool,
>  {
>  	struct perf_inject *inject = container_of(tool, struct perf_inject, tool);
>  	struct evsel *evsel = sample->evsel;
> -	struct callchain_cursor *cursor = get_tls_callchain_cursor();
> +	struct callchain_cursor *cursor;
>  	union perf_event *event_copy = (void *)inject->event_copy;
>  	struct callchain_cursor_node *node;
>  	struct thread *thread;
> @@ -535,6 +567,10 @@ static int perf_event__convert_sample_callchain(const struct perf_tool *tool,
>  	u64 i, k;
>  	int ret;
>  
> +	if (!evsel__has_dwarf_callchain(evsel))
> +		return perf_event__repipe(tool, event, sample, machine);

[Severity: Medium]
Does repiping non-DWARF samples unchanged leave their payload out of sync
with the stripped perf_event_attr headers in piped mode?

For piped input, cmd_inject() skips the upfront evsel__has_dwarf_callchain()
validation, while both perf_event__repipe_attr() and __cmd_inject()
unconditionally clear PERF_SAMPLE_REGS_USER and PERF_SAMPLE_STACK_USER on
every event's attributes.  If a piped event has PERF_SAMPLE_REGS_USER or
PERF_SAMPLE_STACK_USER without a DWARF callchain,
perf_event__convert_sample_callchain() writes the original sample payload
containing user registers or stack data while the output attributes claim
those fields are absent, causing downstream sample parsing to fail.

[ ... ]

> @@ -2956,15 +2980,15 @@ int cmd_inject(int argc, const char **argv)
>  	if (inject.convert_callchain) {
>  		struct evsel *evsel;
>  
> -		if (inject.output.is_pipe || inject.session->data->is_pipe) {
> -			pr_err("--convert-callchain cannot work with pipe\n");
> -			goto out_delete;
> -		}

[Severity: Medium]
When reading from an on-disk perf.data file and writing to a pipe (such as
perf inject -i perf.data --convert-callchain -o -), does cmd_inject() emit
unstripped PERF_RECORD_HEADER_ATTR records into the output pipe?

For non-pipe input (!data.is_pipe), perf_event__repipe_attr() is never
called; instead, cmd_inject() synthesizes the pipe's attribute headers
directly from inject.session->evlist:

tools/perf/builtin-inject.c:cmd_inject() {
    ...
		if (!data.is_pipe) {
			if (inject.aslr)
				aslr_tool__strip_evlist(tool, inject.session->evlist);

			ret = perf_event__synthesize_for_pipe(&inject.tool,
							      inject.session,
							      &inject.output,
							      perf_event__repipe);
    ...
}

Because inject.session->evlist still has PERF_SAMPLE_REGS_USER and
PERF_SAMPLE_STACK_USER set at that point while
perf_event__convert_sample_callchain() strips those fields from every
sample, the output pipe's attribute headers do not match its sample records
and downstream consumers fail with -EFAULT.

> -
> -		evlist__for_each_entry(inject.session->evlist, evsel) {
> -			if (!evsel__has_dwarf_callchain(evsel) && !evsel__is_dummy_event(evsel)) {
> -				pr_err("--convert-callchain requires DWARF call graph.\n");
> -				goto out_delete;
> +		/* For on-disk data, check evlist up-front.
> +		   For piped data, evlist is not available yet;
> +		   check in perf_event__convert_sample_callchain. */
> +		if (!inject.session->data->is_pipe) {
> +			evlist__for_each_entry(inject.session->evlist, evsel) {
> +				if (!evsel__has_dwarf_callchain(evsel) && !evsel__is_dummy_event(evsel)) {
> +					pr_err("--convert-callchain requires DWARF call graph.\n");
> +					goto out_delete;
> +				}
>  			}
>  		}

[Severity: Medium]
With piped input enabled for --convert-callchain, does leaving
inject.tool.finished_round set to perf_event__repipe_oe_synth() keep every
raw DWARF sample buffered in memory until EOF?

In __cmd_inject(), inject->convert_callchain enables
inject->tool.ordered_events, and __perf_session__process_pipe_events() turns
on copy_on_queue so every queued sample is duplicated onto the heap.  Because
perf_event__repipe_oe_synth() writes PERF_RECORD_FINISHED_ROUND directly to
the output without calling perf_event__process_finished_round() to flush
session->ordered_events, raw DWARF stack samples accumulate on the heap for
the entire session and all PERF_RECORD_FINISHED_ROUND events are emitted
ahead of the timestamped samples.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112201833.78630-1-serhei@serhei.io?part=1