Re: [PATCH 04/29] perf zstd: Fix compression error path in zstd_compress_stream_to_records()

[sashiko-bot@kernel.org]

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

New issues:
- [High] The patch skips the final record__mmap_read_all() flush on error in the main thread to avoid retrying a failed read, but misses the identical retry path in parallel worker threads.

Pre-existing issues:
- [High] ZSTD compression silently corrupts the perf.data stream if the output requires multiple records, because record__pushfn overwrites the multi-record headers and incorrectly wraps them into a single record.
- [High] The `zstd_compress_stream_to_records` loop advances the `dst` buffer by unaligned sizes, which can trigger unaligned memory access exceptions on strict architectures.
--

commit 62a2343957107fded20fb827b93d6a1cadb0934a
Author: Arnaldo Carvalho de Melo <acme@redhat.com>

perf zstd: Fix compression error path in zstd_compress_stream_to_records()

This commit fixes bugs in the error fallback path of
zstd_compress_stream_to_records(). It returns -1 on error instead of doing
an unsafe memcpy, skips the final flush on mmap-read failure, and adds
boundary checks to ensure the compression context is properly reset.

> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c

[ ... ]

> @@ -2890,6 +2890,10 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
>  out_child:
>  	record__stop_threads(rec);
>  	record__mmap_read_all(rec, true);
> +	goto out_free_threads;
> +out_child_no_flush:
> +	/* mmap read already failed — retrying would just fail again */
> +	record__stop_threads(rec);

[Severity: High]
The patch avoids retrying the failed read in the main thread by skipping
record__mmap_read_all() here, but what happens in parallel worker threads?

If a parallel worker thread running record__thread() encounters a read
error, it breaks its loop and unconditionally calls
record__mmap_read_all(thread->rec, true). 

Does this circumvent the error handling logic and cause the worker thread
to immediately retry the failed read?

>  out_free_threads:
>  	record__free_thread_data(rec);
>  	evlist__finalize_ctlfd(rec->evlist);

> diff --git a/tools/perf/util/zstd.c b/tools/perf/util/zstd.c
> --- a/tools/perf/util/zstd.c
> +++ b/tools/perf/util/zstd.c

[ ... ]

> @@ -54,7 +55,13 @@ 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);

[Severity: High]
This is a pre-existing issue, but does this function corrupt the perf.data
stream if the output requires multiple records?

If the compressed output exceeds max_record_size, this loop splits it into
multiple chunks with a new perf_event_header for each. However,
record__pushfn() assumes a single event is returned and blindly overwrites
the first event's header.size and data_size with the total size of all
chunks.

Since header.size is a u16, will it overflow and truncate if the total size
exceeds 65535 bytes, while also swallowing subsequent headers into the
payload?

> +		/* Output buffer full — cannot fit even the record header */
> +		if (size > dst_size)
> +			goto reset;
>  		compressed += size;
>  		dst += size;
>  		dst_size -= size;

[ ... ]

> @@ -65,10 +72,18 @@ 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;
> +			goto reset;
>  		}
>  		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,
> +		 * double-subtracting the header size from dst_size and
> +		 * underflowing the unsigned counter.
> +		 */
> +		if (size == 0)
> +			goto reset;
>  		size = process_header(record, size);
>  		compressed += size;
>  		dst += size;

[Severity: High]
This is a pre-existing issue, but does advancing dst by an unaligned
size cause a crash on strict architectures?

Since output.pos is not guaranteed to be 8-byte or 4-byte aligned, dst
can become unaligned here. 

On the next iteration, process_header() casts this unaligned address to
struct perf_record_compressed2 * and writes a 32-bit value to
event->header.type. Will this raise a SIGBUS exception on architectures
like SPARC?

>  	}
>  
>  	return compressed;

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260524032709.1080771-1-acme@kernel.org?part=4