perf record: fix multi-record Zstd compression
Subsystems (2)
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
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
@@ -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
@@ -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
@@ -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
[email protected]
AI reviewRe: [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
-0goto reset; compressed += size; dst += size;
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
PATCH v3 2/2 perf record: Fix multiple PERF_RECORD_COMPRESSED2 records per push
In reply to Dmitry IlvokhinWith 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.
--- 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
@@ -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
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 fromprocess_comp_header()", a no-functional-change prep patch, so the fix's per-record padding can't overrun 'dst'
- Removed blank line fromrecord+zstd_comp_decomp_multi_record.shbetween 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