Re: [PATCH V2 3/6] powerpc/perf: Add AUX buffer management to capture HTM trace data

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Reentrancy corruption of the per-CPU perf_output_handle leads to Use-After-Free.
- [High] Transient conditions permanently disable trace collection and falsely signal EOF.
--

--- Patch [3]: [PATCH V2 3/6] powerpc/perf: Add AUX buffer management to capture HTM trace data ---
commit edd04ff363591ec700b1e3d07e1003b1787ccb4a
Author: Athira Rajeev <atrajeev@linux.ibm.com>

powerpc/perf: Add AUX buffer management to capture HTM trace data
    
This commit adds support for auxiliary (AUX) ring buffers in the HTM PMU
driver. It implements the core lifecycle hooks and dynamically checks for
physical continuity before dumping data to prevent memory corruption.

> diff --git a/arch/powerpc/perf/htm-perf.c b/arch/powerpc/perf/htm-perf.c
> index a188a40287044..b4020a6e8f30b 100644
> --- a/arch/powerpc/perf/htm-perf.c
> +++ b/arch/powerpc/perf/htm-perf.c
[ ... ]
> +struct htm_pmu_ctx {
> +	struct perf_output_handle handle;
> +};
> +
> +static DEFINE_PER_CPU(struct htm_pmu_ctx, htm_pmu_ctx);
> +
[ ... ]
> +static int htm_dump_sample_data(struct perf_event *event)
> +{
> +	struct htm_pmu_ctx *htm_ctx = this_cpu_ptr(&htm_pmu_ctx);
[ ... ]
> +	/* Start AUX transaction session framework */
> +	aux_buf = perf_aux_output_begin(&htm_ctx->handle, event);

[Severity: High]
What happens to the perf_output_handle if this is re-entered from an NMI
context?

htm_event_read() initiates a synchronous AUX output transaction using a
per-CPU struct perf_output_handle. Since this can be called from process
context (for instance, userspace reading the perf event fd), an NMI
interrupting this could trigger a BPF program that calls
bpf_perf_event_read() on the same CPU.

Would this overwrite the per-CPU handle, causing the interrupted outer
call to use a corrupted handle when it resumes, leading to underflowing
reference counts and a use-after-free of the AUX pages? Could the
handle be allocated on the stack instead to prevent this?

> +	if (!aux_buf)
> +		return 0;
[ ... ]
> +	/*
> +	 * Assess constraints regarding space remaining across the mapping
> +	 * context boundary
> +	 */
> +	chunk_size = htm_ctx->handle.size;
> +	chunk_size &= PAGE_MASK;
> +
> +	if (chunk_size > (aux_buf->size - dump_offset))
> +		chunk_size = aux_buf->size - dump_offset;
[ ... ]
> +	/* Bound transfer length tightly within the validated contiguous window */
> +	if (chunk_size > max_contiguous_bytes)
> +		chunk_size = max_contiguous_bytes;
> +
> +	if (!chunk_size) {
> +		aux_buf->collect_htm_trace = 0;

[Severity: High]
Does this permanently disable trace collection during a transient buffer full
condition?

If the remaining space in the user-space perf ring buffer is temporarily
less than PAGE_SIZE, chunk_size will evaluate to 0. This sets
collect_htm_trace to 0, which prevents any future trace collection for the
lifetime of the buffer.

> +		perf_aux_output_end(&htm_ctx->handle, 0);
> +		return 0;
> +	}
> +
[ ... ]
> +	do {
> +		/*
> +		 * Invoke H_HTM call with:
> +		 * - operation as htm dump (H_HTM_OP_DUMP_DATA)
> +		 * - last three values are address, size and offset
> +		 */
> +		rc = htm_hcall_wrapper(htmflags, cfg.nodeindex, cfg.nodalchipindex,
> +				cfg.coreindexonchip, cfg.htmtype, H_HTM_OP_DUMP_DATA,
> +				hypervisor_target_phys, chunk_size, aux_buf->head);
> +		ret = htm_return_check(rc);
> +	} while (ret == -EBUSY && ++retries < MAX_RETRIES);
> +
> +	if (ret > 0) {
> +		aux_buf->head += chunk_size;
> +		aux_buf->trace_records++;
> +		perf_aux_output_end(&htm_ctx->handle, chunk_size);
> +		return ret;
> +	}
> +
> +	aux_buf->collect_htm_trace = 0;

[Severity: High]
If the hypervisor returns a transient long-busy status (like
H_LONG_BUSY_ORDER_1_MSEC), htm_return_check() returns -EAGAIN.

Will the loop exit and fall through to this block, permanently disabling
tracing for this buffer session?

> +	perf_aux_output_end(&htm_ctx->handle, 0);
> +	return ret;
> +}
> +
> +static void htm_event_read(struct perf_event *event)
> +{
> +	int ret;
> +
> +	/*
> +	 * Update event->count as a binary indicator:
> +	 * 1 if data was dumped into the
> +	 * AUX buffer, 0 otherwise. Actual trace record
> +	 * decoding is left to userspace.
> +	 */
> +	ret = htm_dump_sample_data(event);
> +	if (ret <= 0)
> +		local64_set(&event->count, 0);

[Severity: High]
Since the transient buffer full condition or hypervisor busy state causes
htm_dump_sample_data() to return 0 or -EAGAIN, does this falsely signal
an EOF to userspace by setting event->count to 0?

> +	else
> +		local64_set(&event->count, 1);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260720104447.11843-1-atrajeev@linux.ibm.com?part=3