Re: [PATCH V5 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:
- [High] Applying PAGE_MASK to handle.size incorrectly truncates the available AUX buffer size, causing an infinite loop for 1-page buffers and capacity loss.
- [High] Treating H_PARTIAL as a full success for H_HTM_OP_DUMP_DATA without adjusting the dump length causes trace data corruption.
- [Medium] The driver reads an untrusted power-of-two exponent from the hypervisor and uses it in an unbounded bitwise shift.
--

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

powerpc/perf: Add AUX buffer management to capture HTM trace data

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

> diff --git a/arch/powerpc/perf/htm-perf.c b/arch/powerpc/perf/htm-perf.c
> index c1ce12605014b..90d26a721616b 100644
> --- a/arch/powerpc/perf/htm-perf.c
> +++ b/arch/powerpc/perf/htm-perf.c
> @@ -277,6 +314,32 @@ static int htm_event_init(struct perf_event *event)
>  	list_add_tail(&target->list, &htm_active_targets_list);
>  	mutex_unlock(&htm_targets_lock);
>  
> +	/*
> +	 * Query the hardware-allocated HTM buffer size via H_HTM_OP_STATUS.
> +	 * The status output buffer header byte 0x01 holds
> +	 * CurrentNestHtmBufferSizeInPowerOf2 (for HTM_NEST) or
> +	 * CurrentCoreHtmBufferSizeInPowerOf2 (for HTM_CORE); both types
> +	 * use the same offset (0x01) and length (1 byte).
> +	 * hw_buf_size is used in htm_dump_sample_data() to bound dump
> +	 * offsets, preventing H_HTM_OP_DUMP_DATA calls past the end of the
> +	 * hardware buffer.  On failure hw_buf_size stays 0 and the boundary
> +	 * check is skipped gracefully — the contiguous-window clamp still
> +	 * applies.
> +	 */
> +	status_buf = kzalloc(PAGE_SIZE, GFP_KERNEL);
> +	if (status_buf) {
> +		do {
> +			src = htm_hcall_wrapper(htmflags, cfg.nodeindex,
> +					cfg.nodalchipindex, cfg.coreindexonchip,
> +					cfg.htmtype, H_HTM_OP_STATUS,
> +					virt_to_phys(status_buf), PAGE_SIZE, 0);
> +			sret = htm_return_check(src);
> +		} while (sret == -EBUSY && ++sretries < MAX_RETRIES);
> +		if (sret > 0)
> +			target->hw_buf_size = 1ULL << status_buf[0x01];

[Severity: Medium]
Can this shift result in undefined behavior? If the hypervisor returns a value
greater than or equal to 64 for status_buf[0x01], the left shift will invoke
undefined behavior. Should there be a bounds check on the exponent before
performing the shift?

> +		kfree(status_buf);
> +	}
> +
>  	event->pmu_private = target;
>  	event->destroy = reset_htm_active;
>  	return 0;

[ ... ]

> @@ -510,8 +576,308 @@ 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 perf_output_handle handle;
> +	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;

[ ... ]

> +	/*
> +	 * Assess constraints regarding space remaining across the mapping
> +	 * context boundary.
> +	 * handle.size is always page-aligned: perf_aux_output_begin() computes
> +	 * it as the distance from the write pointer to the wakeup boundary,
> +	 * rounded to PAGE_SIZE.  Masking with PAGE_MASK is therefore a no-op
> +	 * but is kept to make the page-granularity contract explicit.
> +	 */
> +	chunk_size = handle.size;
> +	chunk_size &= PAGE_MASK;
> +
> +	if (chunk_size > (aux_buf->size - dump_offset))
> +		chunk_size = aux_buf->size - dump_offset;

[Severity: High]
Does masking with PAGE_MASK artificially truncate the available AUX buffer size?

When the perf core allocator prepares a 1-page AUX buffer,
perf_aux_output_begin() computes handle.size as CIRC_SPACE(), which returns at
most size - 1 (e.g., 4095 for a 4096-byte page). 

The bitwise AND with PAGE_MASK (~4095) evaluates to 0. For a 1-page buffer,
the usable capacity becomes 0, causing the driver to indefinitely return
-ENOSPC and potentially trapping userspace in an infinite loop.

[ ... ]

> +	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;
> +		perf_aux_output_end(&handle, chunk_size);

[Severity: High]
If the hypervisor partially completes the dump, does this code advance the
offsets incorrectly?

htm_return_check() maps H_PARTIAL to a success value (1):

arch/powerpc/perf/htm-perf.c:htm_return_check() {
...
	switch (rc) {
	case H_SUCCESS:
	case H_PARTIAL:
		return 1;
...
}

Because htm_return_check() discards the actual bytes written, and this
condition assumes the entire chunk_size was written on any success code,
could uninitialized or stale memory be exposed to userspace as valid trace
records, and subsequent chunks be desynchronized?

> +		/*
> +		 * Return the number of 128-byte HTM trace records written.
> +		 * Dividing here keeps htm_event_read() free of format
> +		 * knowledge: it can simply use the returned count directly,
> +		 * regardless of which data path (AUX trace or memory config)
> +		 * produced it.
> +		 */
> +		return (ssize_t)(chunk_size / 128);
> +	}

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