Re: [PATCH V3 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 3 potential issue(s) to consider:
- [Critical] Using a shared per-CPU `perf_output_handle` causes a NULL pointer dereference on reentrancy between process and NMI contexts.
- [High] Stale AUX buffer state (`collect_htm_trace`) permanently prevents dumping trace data for subsequent tracing sessions.
- [Medium] The PMU driver fails to reject snapshot (overwrite) mode, leading to an infinite `-ENOSPC` drain loop in userspace.
--

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

This commit implements support for auxiliary (AUX) ring buffers in the HTM PMU
driver. It enables high-volume trace data to be streamed directly into a perf
AUX buffer for deferred post-processing.

> diff --git a/arch/powerpc/perf/htm-perf.c b/arch/powerpc/perf/htm-perf.c
> index 84a5601ee7f73..f880a5fc88330 100644
> --- a/arch/powerpc/perf/htm-perf.c
> +++ b/arch/powerpc/perf/htm-perf.c
[ ... ]
> @@ -415,8 +431,237 @@ static void htm_event_del(struct perf_event *event, int flags)
>  	/* pmu_private freed by event->destroy = reset_htm_active */
>  }
>  
> +static ssize_t htm_dump_sample_data(struct perf_event *event)
> +{
> +	struct htm_pmu_ctx *htm_ctx = this_cpu_ptr(&htm_pmu_ctx);
> +	struct htm_target_id *target = event->pmu_private;
> +	struct htm_pmu_buf *aux_buf;
> +	struct htm_config cfg = target->cfg;
> +	u64 chunk_size, dump_offset, page_index, page_offset;
> +	u64 max_contiguous_bytes, expected_phys, scan_index, actual_phys;
> +	u64 hypervisor_target_phys;
> +	void *target_page_virt;
> +	ssize_t ret = 0;
> +	int retries = 0;
> +	long rc;
> +
> +	/* Start AUX transaction session framework */
> +	aux_buf = perf_aux_output_begin(&htm_ctx->handle, event);

[Severity: Critical]
Does using a shared per-CPU perf_output_handle here risk a NULL pointer
dereference on reentrancy?

If a process context read is interrupted by an NMI on the same CPU, and that
NMI also calls perf_pmu_read() on the HTM event, perf_aux_output_begin() will
detect the nesting, clear handle->event to NULL, and exit.

When the NMI returns to the process context, could the outer
perf_aux_output_end() call subsequently dereference the NULL handle->event?

> +	if (!aux_buf)
> +		return 0;
> +
> +	if (!aux_buf->collect_htm_trace) {

[Severity: High]
Can stale AUX buffer state prevent dumping trace data for subsequent sessions?

If a trace is stopped and drained, aux_buf->collect_htm_trace is set to 0. 
If the event is restarted, htm_event_start() doesn't appear to reset this 
flag.

Would this early bailout then cause trace data for all subsequent sessions to
be permanently dropped?

> +		perf_aux_output_end(&htm_ctx->handle, 0);
> +		return 0;
> +	}
[ ... ]
> +/*
> + * Set up pmu-private data structures for an AUX area
> + * **pages contains the aux buffer allocated for this event
> + * for the corresponding cpu. rb_alloc_aux uses "alloc_pages_node"
> + * and returns pointer to each page address.
> + * PMU capabilities: PERF_PMU_CAP_AUX_NO_SG | PERF_PMU_CAP_AUX_PREFER_LARGE
> + * to try get closest possible physically contiguous page blocks.
> + *
> + * The aux private data structure ie, "struct htm_pmu_buf" mainly
> + * saves
> + * - buf->base: aux buffer base address
> + * - buf->head: offset from base address where data will be written to.
> + * - buf->size: Size of allocated memory
> + */
> +static void *htm_setup_aux(struct perf_event *event, void **pages,
> +		int nr_pages, bool snapshot)
> +{
> +	int cpu = event->cpu;
> +	struct htm_pmu_buf *buf;
> +
> +	if (!nr_pages)
> +		return NULL;
> +
> +	if (cpu == -1)
> +		cpu = raw_smp_processor_id();
> +
> +	buf = kzalloc_node(sizeof(*buf), GFP_KERNEL, cpu_to_node(cpu));
> +	if (!buf)
> +		return NULL;
> +
> +	buf->nr_pages = nr_pages;
> +	buf->snapshot = snapshot;

[Severity: Medium]
Does the driver need to validate and reject snapshot (overwrite) mode?

When in snapshot mode, perf_aux_output_begin() leaves handle.size = 0.
This would cause htm_dump_sample_data() to interpret chunk_size as 0, fallback
to returning -ENOSPC, and cause htm_event_read() to set event->count = 1.

Could this result in the userspace perf tool spinning in an infinite retry
loop waiting for space that will never become available?

> +	buf->size = (u64)nr_pages << PAGE_SHIFT;
> +	buf->pages = pages;
> +
> +	buf->base = pages[0];

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