Re: [RFC PATCH 3/4] perf data-convert: Add perf.data to trace.dat conversion backend

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] Out-of-bounds heap write when tracepoint event data size exceeds the remaining space in a trace.dat page.
- [Medium] The conversion tool fails to register required callbacks for pipe mode (such as `.attr`, `.feature`, and `.tracing_data`), silently dropping events.
- [Medium] The output file is overwritten unconditionally, ignoring the `opts->force` flag.
- [Medium] A `realloc` failure in `trace_dat__collect_cpu_event` causes a memory leak and subsequent segmentation fault.
--

commit da74698d88efeca5fa33802144416838acb15d2a
Author: Tanushree Shah <tshah@linux.ibm.com>

perf data-convert: Add perf.data to trace.dat conversion backend

Add data-convert-trace.c implementing trace_convert__perf2dat() to
convert perf.data tracepoint events to trace.dat format.

process_sample_event() is invoked for each PERF_TYPE_TRACEPOINT sample
during perf_session__process_events(), storing raw event bytes per-cpu
via trace_dat__collect_cpu_event().

Once all samples are collected:
- trace_dat__write_options_section1() writes the OPTIONS section with
  CPUCOUNT, TRACECLOCK, HEADER_INFO, FTRACE_EVENTS, EVENT_FORMATS,
  KALLSYMS, CMDLINES and DONE options.
- trace_dat__write__options_section2() writes the OPTIONS section with
  BUFFER option holding per-cpu data offset placeholders and the DONE
  option.
- trace_dat__write_flyrecord_section() builds ring buffer pages
  per-cpu and patches BUFFER option with final offsets and sizes

Per-cpu buffers are sized to tep_get_page_size() from the session
tep handle and released on all exit paths.

> diff --git a/tools/perf/util/data-convert-trace.c b/tools/perf/util/data-convert-trace.c
> new file mode 100644
> index 0000000000000..e4f8b817be36d
> --- /dev/null
> +++ b/tools/perf/util/data-convert-trace.c
[ ... ]
> +/* Store raw tracepoint event data in per-cpu buffer for trace.dat flyrecord */
> +static int process_sample_event(const struct perf_tool *tool,
> +				union perf_event *event __maybe_unused,
> +				struct perf_sample *sample,
> +				struct evsel *evsel,
> +				struct machine *machine __maybe_unused)
> +{
> +	struct trace_convert *tc = container_of(tool, struct trace_convert, tool);
> +
> +	/* Collect raw tracepoint data per-cpu */
> +	if (trace_dat_fp && sample->raw_size > 0 &&
> +	    evsel->core.attr.type == PERF_TYPE_TRACEPOINT) {
> +		if (trace_dat__collect_cpu_event(sample->cpu, sample->time,
> +					sample->raw_data, sample->raw_size) < 0) {

[Severity: Medium]
Is it possible for a memory allocation failure here to cause a crash
during cleanup?

If trace_dat__collect_cpu_event() fails during a realloc(), it might return
-ENOMEM while leaving the events array pointer as NULL but keeping the count
intact.

When trace_dat__free_cpu_buffers() is called on the error path, it could
dereference that NULL pointer:

tools/perf/util/trace-dat.c:trace_dat__free_cpu_buffers() {
	...
	for (i = 0; i < trace_cpu_data[cpu].count; i++)
		free(trace_cpu_data[cpu].events[i].raw);
	...
}

> +			pr_err("Failed to collect CPU event\n");
> +			return -ENOMEM;
> +		}
> +		tc->events_count++;
> +	}
> +
> +	return 0;
> +}
[ ... ]
> +/* Convert perf.data tracepoint events to trace.dat format */
> +int trace_convert__perf2dat(const char *input, const char *to_trace,
> +			   struct perf_data_convert_opts *opts)
> +{
> +	struct perf_session *session;
> +	struct trace_convert tc = {
> +		.events_count = 0,
> +	};
> +	struct perf_data data = {
> +		.path = input,
> +		.mode = PERF_DATA_MODE_READ,
> +		.force = opts->force,
> +	};
> +	int ret = -EINVAL;
> +	bool cpu_buffers_initialized = false;
> +
> +	/* Initialize tool with all required callbacks */
> +	perf_tool__init(&tc.tool, /*ordered_events=*/true);
> +	tc.tool.sample = process_sample_event;

[Severity: Medium]
Does this initialization omit the required callbacks for pipe mode?

By only overriding .sample, the .attr, .feature, and .tracing_data
callbacks remain as default stubs from perf_tool__init().

In pipe mode, without these callbacks, the tool might silently fail to
populate trace metadata and generate an empty trace.dat file.

> +
> +	/* Open output trace.dat file */
> +	trace_dat_fp = fopen(to_trace, "wb");
> +	if (!trace_dat_fp) {

[Severity: Medium]
Can this unconditionally overwrite an existing file?

The fopen() call uses the "wb" mode which truncates the file, but it
doesn't appear to check the opts->force flag.

This regression could lead to accidental data loss.

> +		pr_err("Failed to open output file: %s\n", to_trace);
> +		return -EINVAL;
> +	}
[ ... ]
> +	/* Write trace.dat options and flyrecord sections */
> +	if (trace_dat__write_options_section1() < 0) {
> +		pr_err("Failed to write options section1\n");
> +		ret = -EIO;
> +		goto out_delete;
> +	}
> +	if (trace_dat__write_options_section2() < 0) {
> +		pr_err("Failed to write options section2\n");
> +		ret = -EIO;
> +		goto out_delete;
> +	}
> +	if (trace_dat__write_flyrecord_section() < 0) {

[Severity: Critical]
Could an unusually large tracepoint event cause an out-of-bounds memory
access when writing the flyrecord section?

When trace_dat__write_cpu_dat() processes records, it queues them to a new
page without checking if the rec_size fits within trace_dat_page_size.

If an event size exceeds the page size, trace_dat__write_page() allocates a
fixed-size buffer and copies the data into it:

tools/perf/util/trace-dat.c:trace_dat__write_page() {
	page = calloc(1, trace_dat_page_size);
	...
	for (i = 0; i < nr_recs; i++) {
		memcpy(page + offset, records[i], rec_sizes[i]);
	...
}

This regression could result in a heap buffer overflow.

> +		pr_err("Failed to write flyrecord section\n");
> +		ret = -EIO;
> +		goto out_delete;
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260608125951.90425-2-tshah@linux.ibm.com?part=3