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

> On 25 Jul 2026, at 1:15 PM, [email protected] wrote:
> 
> 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 <[email protected]>
> 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?

No, because, The reentrancy concern is handled by perf_aux_output_begin() itself 

 nest = READ_ONCE(rb->aux_nest);
        /*
         * Nesting is not supported for AUX area, make sure nested
         * writers are caught early
         */
        if (WARN_ON_ONCE(nest))
                goto err_put;


rb->aux_nest is a per-ring-buffer nesting counter maintained by the perf core. Before writing anything to the per-CPU handle, perf_aux_output_begin() checks this counter. If a nested caller (NMI, BPF, or any interrupt) attempts to begin an AUX transaction on the same ring buffer while one is already in progress, it hits the WARN_ON_ONCE and returns NULL. 

>> + 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?
Sure, good catch, will address this in V4
> 
>> + 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?
Sure
I will have this change in V4

Thanks
Athira
> 
>> + buf->size = (u64)nr_pages << PAGE_SHIFT;
>> + buf->pages = pages;
>> +
>> + buf->base = pages[0];
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3