Skip to contentSkip to conversation

perf record: fix multi-record Zstd compression

PATCH Applied Dmitry Ilvokhin 3 participants 5 messages Jul 8 – Jul 14

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

v3 cover perf record: fix multi-record Zstd compression

Fix a 'perf record -z' regression that aborts recording and writes a perf.data that cannot be decompressed (see full analysis in patch 2).

v2 -> v3:

- Dropped "perf record: Avoid overrunning the zstd output buffer" and "perf
record: Don't store raw data on zstd compression failure": both are now fixed in perf-tools-next.
- Added "perf record: Return the written size from process_comp_header()", a no-functional-change prep patch, so the fix's per-record padding can't overrun 'dst'
- Removed blank line from record+zstd_comp_decomp_multi_record.sh between test description and license (Namhyung Kim).
- Added missing <string.h> include (Sashiko).
- Rebased onto perf-tools-next (Namhyung Kim).

v1 -> v2:

- Patch 2 ("perf record: Avoid overrunning the zstd output buffer"):
rework the overly strict output buffer check to pass the remaining dst size to process_comp_header() and bail only when a record won't fit (Sashiko).

v1: https://lore.kernel.org/all/[email protected]/
v2: https://lore.kernel.org/all/[email protected]/

Dmitry Ilvokhin (2):
perf record: Return the written size from process_comp_header() perf record: Fix multiple PERF_RECORD_COMPRESSED2 records per push

 tools/perf/builtin-record.c                   | 49 +++++++++------
 .../record+zstd_comp_decomp_multi_record.sh   | 63 +++++++++++++++++++
 tools/perf/util/compress.h                    |  6 +-
 tools/perf/util/zstd.c                        | 25 ++++----
 4 files changed, 111 insertions(+), 32 deletions(-)
 create mode 100755 tools/perf/tests/shell/record+zstd_comp_decomp_multi_record.sh
Signature · 1 line
2.53.0-Meta
#1 lore raw

PATCH v3 1/2 perf record: Return the written size from process_comp_header()

process_comp_header() is called from zstd_compress_stream_to_records() twice per record: once with data_size == 0 to write the record header, and once with the payload size to finalize it. It returns the increment it was passed, and the loop separately decides whether a record still fits by comparing the remaining 'dst_size' against the header size.

With the fit check split from the code that writes the record, process_comp_header() cannot reject a record on its own, so any bytes it writes into 'dst' have to be bounds-checked by the caller instead of where they are produced.

Pass the space left in 'dst' to process_comp_header(), let it return the number of bytes written or -1 when the header does not fit, and account the compressed payload in the loop.

No functional change intended.

---
 tools/perf/builtin-record.c | 17 +++++++++++++----
 tools/perf/util/compress.h  |  6 ++++--
 tools/perf/util/zstd.c      | 25 ++++++++++++++-----------
 3 files changed, 31 insertions(+), 17 deletions(-)
tools/perf/builtin-record.c +13 -4
index ebd3ed0c9b3e..757bf4486898 100644
@@ -1592,16 +1592,25 @@ static void record__adjust_affinity(struct record *rec, struct mmap *map)
 	}
 }
 
-static size_t process_comp_header(void *record, size_t increment)
+/*
+ * Called once with data_size == 0 to start a record, then once with
+ * data_size == compressed payload size to finalize.
+ * Returns the bytes written, or -1 if it won't fit.
+ */
+static ssize_t process_comp_header(void *record, size_t dst_size,
+				   size_t data_size)
 {
 	struct perf_record_compressed2 *event = record;
 	size_t size = sizeof(*event);
 
-	if (increment) {
-		event->header.size += increment;
-		return increment;
+	if (data_size) {
+		event->header.size += data_size;
+		return 0;
 	}
 
+	if (size > dst_size)
+		return -1;
+
 	event->header.type = PERF_RECORD_COMPRESSED2;
 	event->header.size = size;
 
tools/perf/util/compress.h +4 -2
index 6cfecfca16f2..ec6c38129e24 100644
@@ -54,7 +54,8 @@ int zstd_fini(struct zstd_data *data);
 
 ssize_t zstd_compress_stream_to_records(struct zstd_data *data, void *dst, size_t dst_size,
 				       void *src, size_t src_size, size_t max_record_size,
-				       size_t process_header(void *record, size_t increment));
+				       ssize_t process_header(void *record, size_t dst_size,
+							      size_t data_size));
 
 size_t zstd_decompress_stream(struct zstd_data *data, void *src, size_t src_size,
 			      void *dst, size_t dst_size);
@@ -75,7 +76,8 @@ ssize_t zstd_compress_stream_to_records(struct zstd_data *data __maybe_unused,
 				       void *dst __maybe_unused, size_t dst_size __maybe_unused,
 				       void *src __maybe_unused, size_t src_size __maybe_unused,
 				       size_t max_record_size __maybe_unused,
-				       size_t process_header(void *record, size_t increment) __maybe_unused)
+				       ssize_t process_header(void *record, size_t dst_size,
+							      size_t data_size) __maybe_unused)
 {
 	return 0;
 }
tools/perf/util/zstd.c +14 -11
index 21a0eb58597c..d98014902f01 100644
@@ -31,9 +31,11 @@ int zstd_fini(struct zstd_data *data)
 
 ssize_t zstd_compress_stream_to_records(struct zstd_data *data, void *dst, size_t dst_size,
 				       void *src, size_t src_size, size_t max_record_size,
-				       size_t process_header(void *record, size_t increment))
+				       ssize_t process_header(void *record, size_t dst_size,
+							      size_t data_size))
 {
-	size_t ret, size, compressed = 0;
+	size_t ret, compressed = 0;
+	ssize_t size;
 	ZSTD_inBuffer input = { src, src_size, 0 };
 	ZSTD_outBuffer output;
 	void *record;
@@ -55,12 +57,9 @@ ssize_t zstd_compress_stream_to_records(struct zstd_data *data, void *dst, size_
 
 	while (input.pos < input.size) {
 		record = dst;
-		/* process_header writes the event header into record */
-		if (dst_size < sizeof(struct perf_event_header))
-			goto reset;
-		size = process_header(record, 0);
+		size = process_header(record, dst_size, 0);
 		/* Output buffer full — cannot fit even the record header */
-		if (size > dst_size)
+		if (size < 0)
 			goto reset;
 		compressed += size;
 		dst += size;
@@ -74,17 +73,21 @@ ssize_t zstd_compress_stream_to_records(struct zstd_data *data, void *dst, size_
 				(long)src_size, ZSTD_getErrorName(ret));
 			goto reset;
 		}
-		size = output.pos;
+		compressed += output.pos;
+		dst += output.pos;
+		dst_size -= output.pos;
 		/*
 		 * No progress: ZSTD couldn't emit any bytes into the
 		 * remaining output buffer.  Calling process_header
-		 * with size=0 would re-trigger header initialization,
+		 * with output.pos=0 would re-trigger header initialization,
 		 * double-subtracting the header size from dst_size and
 		 * underflowing the unsigned counter.
 		 */
-		if (size == 0)
+		if (output.pos == 0)
+			goto reset;
+		size = process_header(record, dst_size, output.pos);
+		if (size < 0)
 			goto reset;
-		size = process_header(record, size);
 		compressed += size;
 		dst += size;
 		dst_size -= size;
Signature · 1 line
2.53.0-Meta
#2 lore raw

Re: [PATCH v3 1/2] perf record: Return the written size from process_comp_header()

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] When a large buffer is split into multiple compressed records, record__pushfn overwrites the first record's header size with the total compressed size, corrupting the stream and causing integer truncation.
- [Medium] The process_comp_header function fails to initialize event->data_size and does not pad PERF_RECORD_COMPRESSED2 records to an 8-byte boundary, leading to an uninitialized heap memory disclosure. --

--- Patch [1]: [PATCH v3 1/2] perf record: Return the written size from process_comp_header() --- commit a7fcd622c85922789abb8c299306610b9ac81440
Author: Dmitry Ilvokhin <[email protected]>

perf record: Return the written size from process_comp_header()

This commit modifies process_comp_header() to take the remaining destination space and return the bytes written or -1 if the header won't fit. The bounds checking is now handled directly by the header processing function rather than the loop in zstd_compress_stream_to_records().

Quoted 3 lines
tools/perf/builtin-record.c +0 -0

[ ... ]

Quoted 19 lines
tools/perf/builtin-record.c +7 -4
@@ -1591,16 +1591,25 @@ static void record__adjust_affinity(struct record *rec, struct mmap *map)
 	}
 }
 
-static size_t process_comp_header(void *record, size_t increment)
+/*
+ * Called once with data_size == 0 to start a record, then once with
+ * data_size == compressed payload size to finalize.
+ * Returns the bytes written, or -1 if it won't fit.
+ */
+static ssize_t process_comp_header(void *record, size_t dst_size,
+				   size_t data_size)
 {
 	struct perf_record_compressed2 *event = record;
 	size_t size = sizeof(*event);
 
-	if (increment) {
-		event->header.size += increment;
-		return increment;

tools/perf/builtin-record.c static void record__adjust_affinity(struct record *rec, struct mmap *map) · line 1614

Quoted 10 lines
tools/perf/builtin-record.c +6 -0
+	if (data_size) {
+		event->header.size += data_size;
+		return 0;
 	}
 
+	if (size > dst_size)
+		return -1;
+
 	event->header.type = PERF_RECORD_COMPRESSED2;
 	event->header.size = size;

[Severity: Medium]
This is a pre-existing issue, but does process_comp_header() leave event->data_size uninitialized here?

If called from AIO mode via record__aio_pushfn(), the map->aio.data memory could be allocated via malloc() and reused without zeroing.

Since event->data_size isn't set and the record isn't padded to an 8-byte boundary, this could write uninitialized heap memory to the perf.data file and break ZSTD decompression.

Quoted 3 lines
tools/perf/util/zstd.c +0 -0

[ ... ]

tools/perf/util/zstd.c ssize_t zstd_compress_stream_to_records(struct zstd_data *data, void *dst, size_ · line 62

Quoted 12 lines
tools/perf/util/zstd.c +2 -5
@@ -55,12 +57,9 @@ ssize_t zstd_compress_stream_to_records(struct zstd_data *data, void *dst, size_
 
 	while (input.pos < input.size) {
 		record = dst;
-		/* process_header writes the event header into record */
-		if (dst_size < sizeof(struct perf_event_header))
-			goto reset;
-		size = process_header(record, 0);
+		size = process_header(record, dst_size, 0);
 		/* Output buffer full — cannot fit even the record header */
-		if (size > dst_size)
+		if (size < 0)

[Severity: High]
This is a pre-existing issue, but when zstd_compress_stream_to_records() splits large outputs into multiple records, doesn't returning the total compressed size cause issues in the caller?

Looking at record__pushfn() in tools/perf/builtin-record.c, the caller seems to assume a single record was produced and overwrites the first record's header size:

	event->data_size = compressed - sizeof(struct perf_record_compressed2);
	event->header.size = PERF_ALIGN(compressed, sizeof(u64));
	padding = event->header.size - compressed;

Since header.size is a u16, if a chunk produces more than 65535 bytes, this would silently truncate the value.

This would cause the padding calculation to underflow into an enormous size_t value, and record__write() would attempt to write exabytes of zero padding to disk.

Even without truncation, overwriting the first header would cause the decompressor to treat subsequent intermediate headers as ZSTD payload.

Quoted 3 lines
tools/perf/util/zstd.c +0 -0
 			goto reset;
 		compressed += size;
 		dst += size;
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
#3 lore raw

PATCH v3 2/2 perf record: Fix multiple PERF_RECORD_COMPRESSED2 records per push

In reply to Dmitry Ilvokhin

With Zstd compression enabled ('perf record -z'), a single mmap push whose compressed output exceeds the maximum record size makes zstd_compress_stream_to_records() emit several PERF_RECORD_COMPRESSED2 records back to back. record__pushfn() however rewrote only the first record's header to describe the whole blob as one record:

  event->data_size   = compressed - sizeof(struct perf_record_compressed2);
  event->header.size = PERF_ALIGN(compressed, sizeof(u64));
  padding            = event->header.size - compressed;
  ...
  record__write(rec, map, &pad, padding);

perf_event_header::size is a __u16, so once the compressed blob no longer fits in it the header.size assignment truncates and 'padding' (size_t) underflows. write() is then handed that bogus length and fails with EFAULT, aborting the recording:

failed to write perf data, error: Bad address

The bytes that did reach the file are mis-framed, so reading it back cannot be decompressed.

This is easy to hit with a high event rate and a large buffer, e.g.:

  perf record -z -F max -m 32M --per-thread -- perf test -w thloop 5 1

The single-record fixup is wrong by construction: because header.size is 16 bits a compressed record cannot exceed 64KB, so the compressor must split a push into a chain of records, and the session reader already consumes them as such.

Frame each record where it is produced instead: make process_comp_header() set the per-record data_size, 8-byte-align header.size and zero the trailing padding, and let record__pushfn() write the resulting blob, as the AIO path already does. Reduce max_record_size by sizeof(u64) so the per-record alignment padding cannot push header.size past its u16 field. process_comp_header() returns -1 when that padding would not fit the space left in 'dst', so the compressor stops instead of overrunning the output buffer.

There is no on-disk format change; a perf.data written by the fixed tool is still read by existing perf.

Fixes 208c0e168344 ("perf record: Add 8-byte aligned event type PERF_RECORD_COMPRESSED2")
---
 tools/perf/builtin-record.c                   | 38 +++++------
 .../record+zstd_comp_decomp_multi_record.sh   | 63 +++++++++++++++++++
 2 files changed, 83 insertions(+), 18 deletions(-)
 create mode 100755 tools/perf/tests/shell/record+zstd_comp_decomp_multi_record.sh
tools/perf/builtin-record.c +20 -18
index 757bf4486898..3a4702eb88fa 100644
@@ -64,6 +64,7 @@
 #include <poll.h>
 #include <pthread.h>
 #include <unistd.h>
+#include <string.h>
 #ifndef HAVE_GETTID
 #include <syscall.h>
 #endif
@@ -653,27 +654,14 @@ static int record__pushfn(struct mmap *map, void *to, void *bf, size_t size)
 	struct record *rec = to;
 
 	if (record__comp_enabled(rec)) {
-		struct perf_record_compressed2 *event = map->data;
-		size_t padding = 0;
-		u8 pad[8] = {0};
 		ssize_t compressed = zstd_compress(rec->session, map, map->data,
 						   mmap__mmap_len(map), bf, size);
 
 		if (compressed < 0)
 			return (int)compressed;
 
-		bf = event;
 		thread->samples++;
-
-		/*
-		 * The record from `zstd_compress` is not 8 bytes aligned, which would cause asan
-		 * error. We make it aligned here.
-		 */
-		event->data_size = compressed - sizeof(struct perf_record_compressed2);
-		event->header.size = PERF_ALIGN(compressed, sizeof(u64));
-		padding = event->header.size - compressed;
-		return record__write(rec, map, bf, compressed) ||
-		       record__write(rec, map, &pad, padding);
+		return record__write(rec, map, map->data, compressed);
 	}
 
 	thread->samples++;
@@ -1594,7 +1582,8 @@ static void record__adjust_affinity(struct record *rec, struct mmap *map)
 
 /*
  * Called once with data_size == 0 to start a record, then once with
- * data_size == compressed payload size to finalize.
+ * data_size == compressed payload size to finalize and 8-byte-pad it
+ * (unaligned records trip ASan in the reader).
  * Returns the bytes written, or -1 if it won't fit.
  */
 static ssize_t process_comp_header(void *record, size_t dst_size,
@@ -1604,8 +1593,15 @@ static ssize_t process_comp_header(void *record, size_t dst_size,
 	size_t size = sizeof(*event);
 
 	if (data_size) {
-		event->header.size += data_size;
-		return 0;
+		size_t padding;
+
+		event->data_size = data_size;
+		event->header.size = PERF_ALIGN(size + data_size, sizeof(u64));
+		padding = event->header.size - size - data_size;
+		if (padding > dst_size)
+			return -1;
+		memset(record + size + data_size, 0, padding);
+		return padding;
 	}
 
 	if (size > dst_size)
@@ -1613,6 +1609,7 @@ static ssize_t process_comp_header(void *record, size_t dst_size,
 
 	event->header.type = PERF_RECORD_COMPRESSED2;
 	event->header.size = size;
+	event->data_size = 0;
 
 	return size;
 }
@@ -1621,7 +1618,12 @@ static ssize_t zstd_compress(struct perf_session *session, struct mmap *map,
 			    void *dst, size_t dst_size, void *src, size_t src_size)
 {
 	ssize_t compressed;
-	size_t max_record_size = PERF_SAMPLE_MAX_SIZE - sizeof(struct perf_record_compressed2) - 1;
+	/*
+	 * Reserve space so per-record PERF_ALIGN() padding keeps header.size
+	 * within u16.
+	 */
+	size_t max_record_size = PERF_SAMPLE_MAX_SIZE
+		- sizeof(struct perf_record_compressed2) - sizeof(u64);
 	struct zstd_data *zstd_data = &session->zstd_data;
 
 	if (map && map->file)
tools/perf/tests/shell/record+zstd_comp_decomp_multi_record.sh new file +63 -0

63 lines not on this page · raw message

Signature · 1 line
2.53.0-Meta
maintainer Applied
#4 lore raw
In reply to Dmitry Ilvokhin
Quoted 16 lines · Dmitry Ilvokhin

Fix a 'perf record -z' regression that aborts recording and writes a perf.data that cannot be decompressed (see full analysis in patch 2).

v2 -> v3:

- Dropped "perf record: Avoid overrunning the zstd output buffer" and "perf
record: Don't store raw data on zstd compression failure": both are now fixed in perf-tools-next.
- Added "perf record: Return the written size from process_comp_header()", a no-functional-change prep patch, so the fix's per-record padding can't overrun 'dst'
- Removed blank line from record+zstd_comp_decomp_multi_record.sh between test description and license (Namhyung Kim).
- Added missing <string.h> include (Sashiko).
- Rebased onto perf-tools-next (Namhyung Kim).

Quoted 1 line

[...]

Applied to perf-tools-next, thanks!

Best regards,
Namhyung