Re: [PATCH V5 4/6] powerpc/perf: Capture the HTM memory configuration as part of perf data

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] The fallback logic for draining memory configuration records uses an uninitialized stack variable `handle` when `perf_aux_output_begin()` fails, leading to a wild pointer dereference and a kernel crash.
- [High] The `HTM_MEM_BUF_SIZE` calculation assumes a worst-case fixed overhead of 92 bytes, failing to account for dynamic sample sizes when `PERF_SAMPLE_READ` is combined with `PERF_FORMAT_GROUP`. This allows the sample size to exceed 65535, overflowing `perf_event_header.size` and corrupting the ring buffer.
--

--- Patch [4]: [PATCH V5 4/6] powerpc/perf: Capture the HTM memory configuration as part of perf data ---
commit 46bf60d42ede8a36ee43812e6bfd3aeda1d041a9
Author: Athira Rajeev <[email protected]>

powerpc/perf: Capture the HTM memory configuration as part of perf data

This commit captures the HTM system memory configuration during AUX trace
dumps and emits it as raw perf sample data. It adds a staging buffer for
the hypervisor call and handles drain iteration alongside the AUX data path.

> diff --git a/arch/powerpc/perf/htm-perf.c b/arch/powerpc/perf/htm-perf.c
> index 90d26a721616b..7e8ae35269852 100644
> --- a/arch/powerpc/perf/htm-perf.c
> +++ b/arch/powerpc/perf/htm-perf.c

[ ... ]

> @@ -203,6 +206,168 @@ static ssize_t htm_return_check(int rc)

[ ... ]

> + * 65440 = 32 + 2043 * 32 is the largest multiple of 32 satisfying all
> + * constraints:
> + *   - total record: 65440 + 92 = 65532 < 65535 (3-byte u16 margin)
> + *
> + * HTM_MEM_MAX_ENTRIES is derived from HTM_MEM_BUF_SIZE — not the other
> + * way around — so the hcall is always given the true buffer size and
> + * the WARN_ON_ONCE(to_copy > HTM_MEM_BUF_SIZE) guard is a genuine
> + * impossibility check rather than a post-overflow assertion.
> + */
> +#define HTM_MEM_BUF_SIZE	65440U
> +#define HTM_MEM_MAX_ENTRIES	((HTM_MEM_BUF_SIZE - 32) / 32)	/* 2043 */

[Severity: High]
Does this buffer size properly account for dynamic sample sizes when 
PERF_SAMPLE_READ is combined with PERF_FORMAT_GROUP?

If a large group of siblings is configured, the read format payload can
scale up significantly. When htm_collect_memory_config() emits a memory
configuration sample, could the computed size (65440 bytes plus the read
format overhead) exceed 65535?

If it exceeds 65535, wouldn't this overflow the u16 perf_event_header.size,
causing perf_output_begin() to reserve much less space than
perf_output_sample() actually writes, resulting in ring buffer corruption?

[ ... ]

> @@ -594,10 +759,25 @@ static ssize_t htm_dump_sample_data(struct perf_event *event)
>  	 * NMI reentrancy from corrupting an outer transaction's handle.
>  	 */
>  	aux_buf = perf_aux_output_begin(&handle, event);
> -	if (!aux_buf)
> +	if (!aux_buf) {
> +		/*
> +		 * AUX ring buffer is full: perf_aux_output_begin() returned NULL.
> +		 * If the AUX trace dump is already complete but memory
> +		 * configuration collection is still in progress, we must not
> +		 * return 0 here — that would signal EOF to htm_event_read() and
> +		 * permanently abandon the mem config drain.  Memory config
> +		 * records go to the main ring buffer via perf_event_overflow(),
> +		 * which is entirely independent of the AUX ring.  Retrieve the
> +		 * aux_buf from the ring's aux_private and call directly.
> +		 */
> +		struct htm_pmu_buf *fb = perf_get_aux(&handle);

[Severity: Critical]
If perf_aux_output_begin() returns NULL (for instance, if the user didn't
mmap an AUX buffer), does this leave the stack variable handle
uninitialized?

If handle is uninitialized, wouldn't perf_get_aux(&handle) dereference
uninitialized stack memory when it reads handle.event and
handle.rb->aux_priv?

> +
> +		if (fb && !fb->collect_htm_trace && fb->collect_htm_mem)
> +			return htm_collect_memory_config(event, fb);
>  		return 0;
> +	}
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4