Re: [RFC PATCH v3 1/5] perf/trace-dat: Add trace.dat export infrastructure

Thank you for your contribution! Sashiko AI review found 5 potential issue(=
s) to consider:
- [High] The `TIME_EXTEND` upper bits are calculated using an incorrect bit=
wise right shift of 5 instead of 27, resulting in corrupted extended timest=
amps.
- [High] The ftrace ring buffer event length encoding for large events inco=
rrectly writes the unaligned payload length, omitting necessary alignment p=
adding.
- [High] Using 32-bit `long` variables and `ftell()`/`fseek()` for file off=
sets truncates trace files larger than 2GB on 32-bit systems.
- [High] Trace data export file headers and ftrace metadata are written in =
host endianness instead of the target trace's endianness, breaking cross-pl=
atform analysis.
- [Medium] Lack of validation for `trace_dat_page_size` can result in a fat=
al integer division by zero exception when calculating page padding.
--

--- Patch [1]: [RFC PATCH v3 1/5] perf/trace-dat: Add trace.dat export infr=
astructure ---
commit 34a2391aa9ca8393dfbdda4a477f39710a409324
Author: Tanushree Shah <[email protected]>

perf/trace-dat: Add trace.dat export infrastructure

This commit adds new utility files to implement infrastructure for
exporting perf.data tracepoints to the trace.dat format. It provides
functions for managing per-cpu event buffers and constructing ftrace
ring buffer pages.

> diff --git a/tools/perf/util/trace-dat.c b/tools/perf/util/trace-dat.c
> new file mode 100644
> index 0000000000000..55a7bd982c6f8
> --- /dev/null
> +++ b/tools/perf/util/trace-dat.c
[ ... ]
> +static int trace_dat__write_page(FILE *fp, unsigned long long base_ts,
> +			char **records, int *rec_sizes, int nr_recs)
> +{
> +	unsigned long long commit =3D 0;
> +	int offset =3D TRACE_DAT_RECORD_HEADER_SIZE;
> +	int i;
> +	char *page;
> +
> +	page =3D calloc(1, trace_dat_page_size);
> +	if (!page)
> +		return -ENOMEM;
> +
> +	for (i =3D 0; i < nr_recs; i++) {
> +		memcpy(page + offset, records[i], rec_sizes[i]);
> +		offset +=3D rec_sizes[i];
> +		commit +=3D rec_sizes[i];
> +	}
> +
> +	memcpy(page, &base_ts, sizeof(base_ts));
> +	memcpy(page + sizeof(base_ts), &commit, sizeof(commit));

[Severity: High]
Does this write the ftrace page header using native host endianness?

If a trace was recorded on a different architecture, copying the integers
directly to the output stream might mix host-endian metadata with
target-endian payloads.

Should this metadata be byte-swapped to match the target architecture
to ensure cross-platform trace analysis works correctly?

> +
> +	if (!fwrite(page, 1, trace_dat_page_size, fp)) {
> +		free(page);
> +		return -EIO;
> +	}
> +	free(page);
> +
> +	return 0;
> +}
[ ... ]
> +static int trace_dat__write_cpu_dat(FILE *fp, int cpu, unsigned long lon=
g *file_offset_out)
> +{
[ ... ]
> +		/* Emit TIME_EXTEND when delta does not fit in 27 bits */
> +		if (time_delta > TRACE_DAT_RECORD_TIME_MASK) {
> +			unsigned int extend_hdr;
> +			unsigned int delta_upper;
> +
> +			extend_size =3D TRACE_DAT_RECORD_TIME_EXTEND_SIZE;
> +			extend =3D calloc(1, extend_size);
> +			if (!extend)
> +				return -ENOMEM;
> +
> +			extend_hdr =3D
> +				((time_delta & TRACE_DAT_RECORD_TIME_MASK) <<
> +				 TRACE_DAT_RECORD_TIME_SHIFT) |
> +				TRACE_DAT_RECORD_TYPE_TIME_EXTEND;
> +			delta_upper =3D time_delta >> TRACE_DAT_RECORD_TIME_SHIFT;

[Severity: High]
Does this shift the upper bits of the time delta by the correct amount?

The code uses TRACE_DAT_RECORD_TIME_SHIFT, which is 5.
This calculates the upper bits as time_delta >> 5 rather than >> 27.
Would this copy the middle 22 bits instead of the upper bits, causing
timestamp corruption for any events that follow a time extend?

> +
> +			memcpy(extend, &extend_hdr, TRACE_DAT_WORD_SIZE);
> +			memcpy(extend + TRACE_DAT_WORD_SIZE, &delta_upper,
> +				TRACE_DAT_WORD_SIZE);
> +
> +			time_delta =3D 0;
> +		}
[ ... ]
> +		hdr_word =3D (time_delta << TRACE_DAT_RECORD_TIME_SHIFT) | type_len;
> +
> +		data_rec =3D calloc(1, data_rec_size);
> +		if (!data_rec) {
> +			free(extend);
> +			ret =3D -ENOMEM;
> +			goto out_free;
> +		}
> +
> +		memcpy(data_rec, &hdr_word, TRACE_DAT_WORD_SIZE);
> +
> +		/* Large events: write actual byte length after header */
> +		if (type_len =3D=3D 0)
> +			memcpy(data_rec + TRACE_DAT_WORD_SIZE, &data_len, TRACE_DAT_WORD_SIZE=
);

[Severity: High]
For large events, is this writing the raw, unaligned data_len into the
payload length field?

The internal offset data_rec_size advances by the padded size, but the
unaligned data_len is written here. If data_len is not a multiple of 4,
a parser reading this file might compute an incorrect total length and
fail to skip the alignment padding.

Could this cause the reader to misread the next event boundary?

> +
> +		memcpy(data_rec + payload_offset, event->raw, data_len);
> +
[ ... ]
> +int trace_dat__write_flyrecord_section(void)
> +{
[ ... ]
> +	section_size_pos =3D ftell(trace_dat_fp);
> +	if (!fwrite(&section_size, sizeof(unsigned long long), 1, trace_dat_fp)=
) {
> +		ret =3D -EIO;
> +		goto cleanup;
> +	}
> +
> +	/* Align to page boundary */
> +	after_header   =3D ftell(trace_dat_fp);
> +	padding_needed =3D (trace_dat_page_size -
> +			 (after_header % trace_dat_page_size)) % trace_dat_page_size;

[Severity: Medium]
Is trace_dat_page_size guaranteed to be non-zero here?

If the caller fails to initialize this global variable or parses a
configuration that sets it to zero, this modulo operation will trigger a
fatal integer division by zero exception.

> +
> +	if (padding_needed > 0) {
> +		pad =3D calloc(1, padding_needed);
> +		if (!pad) {
> +			ret =3D -ENOMEM;
> +			goto cleanup;
> +		}
> +
> +		if (!fwrite(pad, 1, padding_needed, trace_dat_fp)) {
> +			free(pad);
> +			ret =3D -EIO;
> +			goto cleanup;
> +		}
> +		free(pad);
> +	}
> +
> +	/* write per-cpu trace data */
[ ... ]
> diff --git a/tools/perf/util/trace-dat.h b/tools/perf/util/trace-dat.h
> new file mode 100644
> index 0000000000000..9aec37b708d41
> --- /dev/null
> +++ b/tools/perf/util/trace-dat.h
> @@ -0,0 +1,83 @@
[ ... ]
> +extern FILE *trace_dat_fp;
> +extern int trace_dat_page_size;
> +extern int trace_dat_nr_cpus;
> +extern long trace_dat_options_offset;
> +extern long trace_dat_header_info_offset;
> +extern long trace_dat_events_format_offset;
> +extern long trace_dat_ftrace_format_offset;
> +extern long trace_dat_kallsyms_offset;
> +extern long trace_dat_cmdline_offset;
> +extern long trace_dat_next_options_offset;

[Severity: High]
Are these offsets safe for trace files larger than 2GB on 32-bit systems?

Using long for offsets and relying on ftell() limits the maximum supported
file size to 2GB on 32-bit architectures. Since perf.data files can easily
exceed this size, could this result in integer overflow and corrupted offse=
ts?

Should this use standard 64-bit interfaces like u64 and ftello()/fseeko()
instead?

> +
> +/* collect and manage per-cpu tracepoint event buffers */
> +#define TRACE_DAT_RECORD_TYPE_TIME_EXTEND 30
> +#define TRACE_DAT_RECORD_TIME_EXTEND_SIZE 8
> +#define TRACE_DAT_RECORD_TYPE_LEN_MAX 28
[ ... ]

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260803145958.2999=
[email protected]?part=3D1