Linux kernel -stable discussions
 help / color / mirror / Atom feed
* [PATCH v2 1/3] perf record: Fix multiple PERF_RECORD_COMPRESSED2 records per push
       [not found] <cover.1782743187.git.d@ilvokhin.com>
@ 2026-06-30  7:17 ` Dmitry Ilvokhin
  2026-07-04 17:08   ` Namhyung Kim
  0 siblings, 1 reply; 3+ 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] 3+ 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-07-04 17:08   ` Namhyung Kim
  2026-07-06 16:42     ` Dmitry Ilvokhin
  0 siblings, 1 reply; 3+ 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] 3+ 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; 3+ 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] 3+ messages in thread

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

Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
     [not found] <cover.1782743187.git.d@ilvokhin.com>
2026-06-30  7:17 ` [PATCH v2 1/3] perf record: Fix multiple PERF_RECORD_COMPRESSED2 records per push Dmitry Ilvokhin
2026-07-04 17:08   ` Namhyung Kim
2026-07-06 16:42     ` Dmitry Ilvokhin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox