All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 0/3] perf record: fix multi-record Zstd compression
@ 2026-06-30  7:16 Dmitry Ilvokhin
  2026-06-30  7:17 ` [PATCH v2 1/3] perf record: Fix multiple PERF_RECORD_COMPRESSED2 records per push Dmitry Ilvokhin
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Dmitry Ilvokhin @ 2026-06-30  7:16 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Namhyung Kim, Mark Rutland, Alexander Shishkin, Jiri Olsa,
	Ian Rogers, Adrian Hunter, James Clark, Nick Terrell,
	David Sterba
  Cc: linux-kernel, linux-perf-users, kernel-team, Farid Zakaria,
	Dmitry Ilvokhin

Patch 1 fixes a 'perf record -z' regression that aborts recording with
"Bad address" and produces an undecompressable perf.data.

While fixing it I found two more latent bugs in the same compressor,
zstd_compress_stream_to_records(): an output-buffer overrun and a broken
zstd-error fallback. Addressed in patches 2 and 3.

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/cover.1781797544.git.d@ilvokhin.com/

Dmitry Ilvokhin (3):
  perf record: Fix multiple PERF_RECORD_COMPRESSED2 records per push
  perf record: Avoid overrunning the zstd output buffer
  perf record: Don't store raw data on zstd compression failure

 tools/perf/builtin-record.c                   | 48 ++++++++------
 .../record+zstd_comp_decomp_multi_record.sh   | 64 +++++++++++++++++++
 tools/perf/util/compress.h                    |  6 +-
 tools/perf/util/zstd.c                        | 28 +++++---
 4 files changed, 116 insertions(+), 30 deletions(-)
 create mode 100755 tools/perf/tests/shell/record+zstd_comp_decomp_multi_record.sh

-- 
2.53.0-Meta


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH v2 1/3] perf record: Fix multiple PERF_RECORD_COMPRESSED2 records per push
  2026-06-30  7:16 [PATCH v2 0/3] perf record: fix multi-record Zstd compression Dmitry Ilvokhin
@ 2026-06-30  7:17 ` Dmitry Ilvokhin
  2026-06-30  7:32   ` sashiko-bot
  2026-07-04 17:08   ` Namhyung Kim
  2026-06-30  7:17 ` [PATCH v2 2/3] perf record: Avoid overrunning the zstd output buffer Dmitry Ilvokhin
  2026-06-30  7:17 ` [PATCH v2 3/3] perf record: Don't store raw data on zstd compression failure Dmitry Ilvokhin
  2 siblings, 2 replies; 7+ messages in thread
From: Dmitry Ilvokhin @ 2026-06-30  7:17 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Namhyung Kim, Mark Rutland, Alexander Shishkin, Jiri Olsa,
	Ian Rogers, Adrian Hunter, James Clark, Nick Terrell,
	David Sterba
  Cc: linux-kernel, linux-perf-users, kernel-team, Farid Zakaria,
	Dmitry Ilvokhin, stable

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.

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")
Reported-by: Farid Zakaria <fmzakari@meta.com>
Signed-off-by: Dmitry Ilvokhin <d@ilvokhin.com>
Cc: stable@vger.kernel.org
---
 tools/perf/builtin-record.c                   | 41 ++++++------
 .../record+zstd_comp_decomp_multi_record.sh   | 64 +++++++++++++++++++
 tools/perf/util/zstd.c                        |  2 +-
 3 files changed, 87 insertions(+), 20 deletions(-)
 create mode 100755 tools/perf/tests/shell/record+zstd_comp_decomp_multi_record.sh

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index 4a5eba498c02..2562c3177eae 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -652,27 +652,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++;
@@ -1590,18 +1577,29 @@ 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 and 8-byte-pad it
+ * (unaligned records trip ASan in the reader).
+ */
+static size_t process_comp_header(void *record, 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) {
+		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;
+		memset(record + size + data_size, 0, padding);
+		return data_size + padding;
 	}
 
 	event->header.type = PERF_RECORD_COMPRESSED2;
 	event->header.size = size;
+	event->data_size = 0;
 
 	return size;
 }
@@ -1610,7 +1608,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)
diff --git a/tools/perf/tests/shell/record+zstd_comp_decomp_multi_record.sh b/tools/perf/tests/shell/record+zstd_comp_decomp_multi_record.sh
new file mode 100755
index 000000000000..42efe7260def
--- /dev/null
+++ b/tools/perf/tests/shell/record+zstd_comp_decomp_multi_record.sh
@@ -0,0 +1,64 @@
+#!/bin/bash
+# Zstd perf.data compression/decompression of multi-record data
+
+# SPDX-License-Identifier: GPL-2.0
+
+perfdata=$(mktemp /tmp/__perf_test.perf.data.XXXXX)
+recout=$(mktemp /tmp/__perf_test.zstd.rec.XXXXX)
+injout=$(mktemp /tmp/__perf_test.zstd.inj.XXXXX)
+perf_tool=perf
+
+cleanup() {
+	rm -f "${perfdata}" "${perfdata}".old "${perfdata}".decomp "${recout}" "${injout}"
+}
+trap cleanup EXIT TERM INT
+
+skip_if_no_z_record() {
+	$perf_tool record -h 2>&1 | grep -q -- '-z, --compression-level'
+}
+
+collect_z_record() {
+	echo "Collecting compressed record file:"
+	[ "$(uname -m)" != s390x ] && gflag='-g'
+	$perf_tool record -o "${perfdata}" $gflag -z -F max -m 32M --per-thread -- \
+		$perf_tool test -w thloop 5 1 \
+		>/dev/null 2>"${recout}"
+}
+
+check_record() {
+	echo "Checking record did not fail to write data:"
+	if grep -q "failed to write perf data" "${recout}"; then
+		cat "${recout}"
+		return 1
+	fi
+}
+
+check_decompress() {
+	echo "Checking compressed file decompresses cleanly:"
+	if ! $perf_tool inject -i "${perfdata}" -o "${perfdata}".decomp 2>"${injout}"; then
+		cat "${injout}"
+		return 1
+	fi
+	if grep -Eqi "decompress|corrupt|failed to process type" "${injout}"; then
+		cat "${injout}"
+		return 1
+	fi
+}
+
+skip_if_no_z_record || exit 2
+collect_z_record
+check_record || exit 1
+
+# Need >1 record, else the multi-record path wasn't exercised.
+# Skip rather than pass/fail spuriously.
+nr=$($perf_tool report -i "${perfdata}" --stats 2>/dev/null |
+	awk '/COMPRESSED2 events:/ { print $3 }')
+if [ -z "${nr}" ] || [ "${nr}" -lt 2 ]; then
+	echo "less than two compressed records (${nr:-0}), skipping"
+	exit 2
+fi
+echo "Produced ${nr} compressed records"
+
+check_decompress
+err=$?
+exit $err
diff --git a/tools/perf/util/zstd.c b/tools/perf/util/zstd.c
index 57027e0ac7b6..1955fa2431d1 100644
--- a/tools/perf/util/zstd.c
+++ b/tools/perf/util/zstd.c
@@ -30,7 +30,7 @@ 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))
+				       size_t process_header(void *record, size_t data_size))
 {
 	size_t ret, size, compressed = 0;
 	ZSTD_inBuffer input = { src, src_size, 0 };
-- 
2.53.0-Meta


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH v2 2/3] perf record: Avoid overrunning the zstd output buffer
  2026-06-30  7:16 [PATCH v2 0/3] perf record: fix multi-record Zstd compression Dmitry Ilvokhin
  2026-06-30  7:17 ` [PATCH v2 1/3] perf record: Fix multiple PERF_RECORD_COMPRESSED2 records per push Dmitry Ilvokhin
@ 2026-06-30  7:17 ` Dmitry Ilvokhin
  2026-06-30  7:17 ` [PATCH v2 3/3] perf record: Don't store raw data on zstd compression failure Dmitry Ilvokhin
  2 siblings, 0 replies; 7+ messages in thread
From: Dmitry Ilvokhin @ 2026-06-30  7:17 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Namhyung Kim, Mark Rutland, Alexander Shishkin, Jiri Olsa,
	Ian Rogers, Adrian Hunter, James Clark, Nick Terrell,
	David Sterba
  Cc: linux-kernel, linux-perf-users, kernel-team, Farid Zakaria,
	Dmitry Ilvokhin

zstd_compress_stream_to_records() compresses one mmap chunk into 'dst'
as a chain of records, decrementing 'dst_size' as each record's header
and compressed payload are written, but it never checks that the space
left can still hold a record before writing one.

When the compressed output does not fit in 'dst', 'dst_size -= size'
underflows ('dst_size' is a size_t), the output size is then taken as
max_record_size again, and ZSTD_compressStream() writes past the end
of 'dst'.

Pass the space left in 'dst' to process_comp_header() and let it return
-1 when the record header or its 8-byte alignment padding would not fit.
Stop compressing on -1 instead of writing past 'dst'.

Fixes: f24c1d7523e6 ("perf tools: Introduce Zstd streaming based compression API")
Signed-off-by: Dmitry Ilvokhin <d@ilvokhin.com>
---
 tools/perf/builtin-record.c | 11 +++++++++--
 tools/perf/util/compress.h  |  6 ++++--
 tools/perf/util/zstd.c      | 23 ++++++++++++++++++-----
 3 files changed, 31 insertions(+), 9 deletions(-)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index 2562c3177eae..fc89ccc78d4d 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -1581,8 +1581,10 @@ 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 and 8-byte-pad it
  * (unaligned records trip ASan in the reader).
+ * Returns the bytes written, or -1 if it won't fit.
  */
-static size_t process_comp_header(void *record, size_t data_size)
+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);
@@ -1593,10 +1595,15 @@ static size_t process_comp_header(void *record, size_t data_size)
 		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 data_size + padding;
+		return padding;
 	}
 
+	if (size > dst_size)
+		return -1;
+
 	event->header.type = PERF_RECORD_COMPRESSED2;
 	event->header.size = size;
 	event->data_size = 0;
diff --git a/tools/perf/util/compress.h b/tools/perf/util/compress.h
index 6cfecfca16f2..ec6c38129e24 100644
--- a/tools/perf/util/compress.h
+++ b/tools/perf/util/compress.h
@@ -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;
 }
diff --git a/tools/perf/util/zstd.c b/tools/perf/util/zstd.c
index 1955fa2431d1..04910dfed84f 100644
--- a/tools/perf/util/zstd.c
+++ b/tools/perf/util/zstd.c
@@ -30,9 +30,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 data_size))
+				       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;
@@ -54,7 +56,11 @@ ssize_t zstd_compress_stream_to_records(struct zstd_data *data, void *dst, size_
 
 	while (input.pos < input.size) {
 		record = dst;
-		size = process_header(record, 0);
+		size = process_header(record, dst_size, 0);
+		if (size < 0) {
+			pr_err("failed to compress: output buffer too small\n");
+			return -1;
+		}
 		compressed += size;
 		dst += size;
 		dst_size -= size;
@@ -68,8 +74,15 @@ ssize_t zstd_compress_stream_to_records(struct zstd_data *data, void *dst, size_
 			memcpy(dst, src, src_size);
 			return src_size;
 		}
-		size = output.pos;
-		size = process_header(record, size);
+		compressed += output.pos;
+		dst += output.pos;
+		dst_size -= output.pos;
+
+		size = process_header(record, dst_size, output.pos);
+		if (size < 0) {
+			pr_err("failed to compress: output buffer too small for padding\n");
+			return -1;
+		}
 		compressed += size;
 		dst += size;
 		dst_size -= size;
-- 
2.53.0-Meta


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH v2 3/3] perf record: Don't store raw data on zstd compression failure
  2026-06-30  7:16 [PATCH v2 0/3] perf record: fix multi-record Zstd compression Dmitry Ilvokhin
  2026-06-30  7:17 ` [PATCH v2 1/3] perf record: Fix multiple PERF_RECORD_COMPRESSED2 records per push Dmitry Ilvokhin
  2026-06-30  7:17 ` [PATCH v2 2/3] perf record: Avoid overrunning the zstd output buffer Dmitry Ilvokhin
@ 2026-06-30  7:17 ` Dmitry Ilvokhin
  2 siblings, 0 replies; 7+ messages in thread
From: Dmitry Ilvokhin @ 2026-06-30  7:17 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Namhyung Kim, Mark Rutland, Alexander Shishkin, Jiri Olsa,
	Ian Rogers, Adrian Hunter, James Clark, Nick Terrell,
	David Sterba
  Cc: linux-kernel, linux-perf-users, kernel-team, Farid Zakaria,
	Dmitry Ilvokhin

On a ZSTD_compressStream() error, zstd_compress_stream_to_records()
falls back to copying the uncompressed input into the output buffer and
returning its size. That cannot work:

- a COMPRESSED2 payload is always fed to zstd_decompress_stream(), so
  raw bytes placed there can never be read back.

- the record header is never finalized: process_header() is not called
  to write the real header.size and data_size, so they keep their
  initial values and do not match the bytes written.

- the copy is unbounded and can write past the output buffer.

Propagate the error instead.

Fixes: f24c1d7523e6 ("perf tools: Introduce Zstd streaming based compression API")
Signed-off-by: Dmitry Ilvokhin <d@ilvokhin.com>
---
 tools/perf/util/zstd.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/tools/perf/util/zstd.c b/tools/perf/util/zstd.c
index 04910dfed84f..cbb150b0f807 100644
--- a/tools/perf/util/zstd.c
+++ b/tools/perf/util/zstd.c
@@ -1,7 +1,5 @@
 // SPDX-License-Identifier: GPL-2.0
 
-#include <string.h>
-
 #include "util/compress.h"
 #include "util/debug.h"
 
@@ -71,8 +69,7 @@ ssize_t zstd_compress_stream_to_records(struct zstd_data *data, void *dst, size_
 		if (ZSTD_isError(ret)) {
 			pr_err("failed to compress %ld bytes: %s\n",
 				(long)src_size, ZSTD_getErrorName(ret));
-			memcpy(dst, src, src_size);
-			return src_size;
+			return -1;
 		}
 		compressed += output.pos;
 		dst += output.pos;
-- 
2.53.0-Meta


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH v2 1/3] perf record: Fix multiple PERF_RECORD_COMPRESSED2 records per push
  2026-06-30  7:17 ` [PATCH v2 1/3] perf record: Fix multiple PERF_RECORD_COMPRESSED2 records per push Dmitry Ilvokhin
@ 2026-06-30  7:32   ` sashiko-bot
  2026-07-04 17:08   ` Namhyung Kim
  1 sibling, 0 replies; 7+ messages in thread
From: sashiko-bot @ 2026-06-30  7:32 UTC (permalink / raw)
  To: Dmitry Ilvokhin; +Cc: linux-perf-users

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] Missing explicit `#include <string.h>` for `memset` in `builtin-record.c`.
--

commit cb8ac7d313a36a42601f85ffc5fb9a9061002bc0
Author: Dmitry Ilvokhin <d@ilvokhin.com>

perf record: Fix multiple PERF_RECORD_COMPRESSED2 records per push

This commit frames compressed records individually when produced rather
than rewriting a single header for an entire blob. This prevents truncation
of the header.size field when a compressed blob exceeds 64KB.

> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index 4a5eba498c025..2562c3177eae1 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c

[ ... ]

> @@ -1590,18 +1577,29 @@ 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 and 8-byte-pad it
> + * (unaligned records trip ASan in the reader).
> + */
> +static size_t process_comp_header(void *record, 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) {
> +		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;
> +		memset(record + size + data_size, 0, padding);

[Severity: Low]
Does this file need an explicit include for <string.h>?

Relying on implicit header inclusion for POSIX libc functions like memset can
cause build failures on systems using musl libc, where implicit header graphs
differ from glibc.

> +		return data_size + padding;
>  	}
>  
>  	event->header.type = PERF_RECORD_COMPRESSED2;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1782743187.git.d@ilvokhin.com?part=1

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2 1/3] perf record: Fix multiple PERF_RECORD_COMPRESSED2 records per push
  2026-06-30  7:17 ` [PATCH v2 1/3] perf record: Fix multiple PERF_RECORD_COMPRESSED2 records per push Dmitry Ilvokhin
  2026-06-30  7:32   ` sashiko-bot
@ 2026-07-04 17:08   ` Namhyung Kim
  2026-07-06 16:42     ` Dmitry Ilvokhin
  1 sibling, 1 reply; 7+ messages in thread
From: Namhyung Kim @ 2026-07-04 17:08 UTC (permalink / raw)
  To: Dmitry Ilvokhin
  Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Ian Rogers,
	Adrian Hunter, James Clark, Nick Terrell, David Sterba,
	linux-kernel, linux-perf-users, kernel-team, Farid Zakaria,
	stable

Hello,

On Tue, Jun 30, 2026 at 07:17:00AM +0000, Dmitry Ilvokhin wrote:
> 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.
> 
> There is no on-disk format change; a perf.data written by the fixed tool
> is still read by existing perf.

Thanks for working on this!

> 
> Fixes: 208c0e168344 ("perf record: Add 8-byte aligned event type PERF_RECORD_COMPRESSED2")
> Reported-by: Farid Zakaria <fmzakari@meta.com>
> Signed-off-by: Dmitry Ilvokhin <d@ilvokhin.com>
> Cc: stable@vger.kernel.org
> ---
>  tools/perf/builtin-record.c                   | 41 ++++++------
>  .../record+zstd_comp_decomp_multi_record.sh   | 64 +++++++++++++++++++
>  tools/perf/util/zstd.c                        |  2 +-
>  3 files changed, 87 insertions(+), 20 deletions(-)
>  create mode 100755 tools/perf/tests/shell/record+zstd_comp_decomp_multi_record.sh
> 
> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index 4a5eba498c02..2562c3177eae 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -652,27 +652,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++;
> @@ -1590,18 +1577,29 @@ 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 and 8-byte-pad it
> + * (unaligned records trip ASan in the reader).
> + */
> +static size_t process_comp_header(void *record, 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) {
> +		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;
> +		memset(record + size + data_size, 0, padding);
> +		return data_size + padding;
>  	}
>  
>  	event->header.type = PERF_RECORD_COMPRESSED2;
>  	event->header.size = size;
> +	event->data_size = 0;
>  
>  	return size;
>  }
> @@ -1610,7 +1608,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)
> diff --git a/tools/perf/tests/shell/record+zstd_comp_decomp_multi_record.sh b/tools/perf/tests/shell/record+zstd_comp_decomp_multi_record.sh
> new file mode 100755
> index 000000000000..42efe7260def
> --- /dev/null
> +++ b/tools/perf/tests/shell/record+zstd_comp_decomp_multi_record.sh
> @@ -0,0 +1,64 @@
> +#!/bin/bash
> +# Zstd perf.data compression/decompression of multi-record data
> +
> +# SPDX-License-Identifier: GPL-2.0

Can you please remove the blank line here?

Also, this series cannot apply anymore.  Please rebase onto the latest
perf-tools-next.

Thanks,
Namhyung

> +
> +perfdata=$(mktemp /tmp/__perf_test.perf.data.XXXXX)
> +recout=$(mktemp /tmp/__perf_test.zstd.rec.XXXXX)
> +injout=$(mktemp /tmp/__perf_test.zstd.inj.XXXXX)
> +perf_tool=perf
> +
> +cleanup() {
> +	rm -f "${perfdata}" "${perfdata}".old "${perfdata}".decomp "${recout}" "${injout}"
> +}
> +trap cleanup EXIT TERM INT
> +
> +skip_if_no_z_record() {
> +	$perf_tool record -h 2>&1 | grep -q -- '-z, --compression-level'
> +}
> +
> +collect_z_record() {
> +	echo "Collecting compressed record file:"
> +	[ "$(uname -m)" != s390x ] && gflag='-g'
> +	$perf_tool record -o "${perfdata}" $gflag -z -F max -m 32M --per-thread -- \
> +		$perf_tool test -w thloop 5 1 \
> +		>/dev/null 2>"${recout}"
> +}
> +
> +check_record() {
> +	echo "Checking record did not fail to write data:"
> +	if grep -q "failed to write perf data" "${recout}"; then
> +		cat "${recout}"
> +		return 1
> +	fi
> +}
> +
> +check_decompress() {
> +	echo "Checking compressed file decompresses cleanly:"
> +	if ! $perf_tool inject -i "${perfdata}" -o "${perfdata}".decomp 2>"${injout}"; then
> +		cat "${injout}"
> +		return 1
> +	fi
> +	if grep -Eqi "decompress|corrupt|failed to process type" "${injout}"; then
> +		cat "${injout}"
> +		return 1
> +	fi
> +}
> +
> +skip_if_no_z_record || exit 2
> +collect_z_record
> +check_record || exit 1
> +
> +# Need >1 record, else the multi-record path wasn't exercised.
> +# Skip rather than pass/fail spuriously.
> +nr=$($perf_tool report -i "${perfdata}" --stats 2>/dev/null |
> +	awk '/COMPRESSED2 events:/ { print $3 }')
> +if [ -z "${nr}" ] || [ "${nr}" -lt 2 ]; then
> +	echo "less than two compressed records (${nr:-0}), skipping"
> +	exit 2
> +fi
> +echo "Produced ${nr} compressed records"
> +
> +check_decompress
> +err=$?
> +exit $err
> diff --git a/tools/perf/util/zstd.c b/tools/perf/util/zstd.c
> index 57027e0ac7b6..1955fa2431d1 100644
> --- a/tools/perf/util/zstd.c
> +++ b/tools/perf/util/zstd.c
> @@ -30,7 +30,7 @@ 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))
> +				       size_t process_header(void *record, size_t data_size))
>  {
>  	size_t ret, size, compressed = 0;
>  	ZSTD_inBuffer input = { src, src_size, 0 };
> -- 
> 2.53.0-Meta
> 

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2 1/3] perf record: Fix multiple PERF_RECORD_COMPRESSED2 records per push
  2026-07-04 17:08   ` Namhyung Kim
@ 2026-07-06 16:42     ` Dmitry Ilvokhin
  0 siblings, 0 replies; 7+ messages in thread
From: Dmitry Ilvokhin @ 2026-07-06 16:42 UTC (permalink / raw)
  To: Namhyung Kim
  Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Ian Rogers,
	Adrian Hunter, James Clark, Nick Terrell, David Sterba,
	linux-kernel, linux-perf-users, kernel-team, Farid Zakaria,
	stable

On Sat, Jul 04, 2026 at 10:08:03AM -0700, Namhyung Kim wrote:
> > diff --git a/tools/perf/tests/shell/record+zstd_comp_decomp_multi_record.sh b/tools/perf/tests/shell/record+zstd_comp_decomp_multi_record.sh
> > new file mode 100755
> > index 000000000000..42efe7260def
> > --- /dev/null
> > +++ b/tools/perf/tests/shell/record+zstd_comp_decomp_multi_record.sh
> > @@ -0,0 +1,64 @@
> > +#!/bin/bash
> > +# Zstd perf.data compression/decompression of multi-record data
> > +
> > +# SPDX-License-Identifier: GPL-2.0
> 
> Can you please remove the blank line here?
> 
> Also, this series cannot apply anymore.  Please rebase onto the latest
> perf-tools-next.
> 

Thanks for taking a look, Namhyung.

Absolutely. I'll fix it up, rebase on latest perf-tools-next and respin
the series.

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2026-07-06 16:42 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-06-30  7:16 [PATCH v2 0/3] perf record: fix multi-record Zstd compression Dmitry Ilvokhin
2026-06-30  7:17 ` [PATCH v2 1/3] perf record: Fix multiple PERF_RECORD_COMPRESSED2 records per push Dmitry Ilvokhin
2026-06-30  7:32   ` sashiko-bot
2026-07-04 17:08   ` Namhyung Kim
2026-07-06 16:42     ` Dmitry Ilvokhin
2026-06-30  7:17 ` [PATCH v2 2/3] perf record: Avoid overrunning the zstd output buffer Dmitry Ilvokhin
2026-06-30  7:17 ` [PATCH v2 3/3] perf record: Don't store raw data on zstd compression failure Dmitry Ilvokhin

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.