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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] The HTM_MEM_BUF_SIZE (65440 bytes) combined with dynamic sample types causes perf_event_header.size to overflow its 16-bit limit, leading to severe ring buffer corruption.
- [High] BPF programs running in NMI context can concurrently invoke the driver's .read callback, causing unprotected state mutations and hardware desynchronization.
- [High] Uninitialized `struct pt_regs regs` in `htm_collect_memory_config()` leaks kernel stack memory to userspace via `PERF_SAMPLE_CALLCHAIN` and BPF attachments.
- [Medium] The memory configuration dump is prematurely aborted if the AUX ring buffer is full.
- [Medium] Silent loss of HTM memory configuration data on main ring buffer pressure due to incorrectly advancing the hypervisor iterator.
--

--- Patch [4]: [PATCH V4 4/6] powerpc/perf: Capture the HTM memory configuration as part of perf data ---
commit 2c90eb9bba9472cb2bbddbb6b37c3d4c8154e12b
Author: Athira Rajeev <atrajeev@linux.ibm.com>

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

This commit introduces functionality to capture the hypervisor's memory
configuration during an HTM trace. It handles emitting the configuration as
a raw perf sample data record after the AUX trace dump completes. A
dedicated buffer is tracked per AUX session to stage the hcall payload.

> diff --git a/arch/powerpc/perf/htm-perf.c b/arch/powerpc/perf/htm-perf.c
> index 0b954a7ed4dd9..093d54d803ce9 100644
> --- a/arch/powerpc/perf/htm-perf.c
> +++ b/arch/powerpc/perf/htm-perf.c
[ ... ]
> +static ssize_t htm_collect_memory_config(struct perf_event *event,
> +					 struct htm_pmu_buf *aux_buf)
> +{
> +	struct perf_sample_data data;
> +	struct perf_raw_record raw;
> +	struct pt_regs regs;
[ ... ]
> +	/*
> +	 * Initialise regs to the current caller context.  perf_event_overflow()
> +	 * may sample register state into the ring buffer. Use
> +	 * perf_fetch_caller_regs() to capture a safe, deterministic snapshot
> +	 * of the current CPU state — the same pattern used by tracepoints and
> +	 * BPF perf-event helpers for synthetic sample emission.
> +	 */
> +	perf_fetch_caller_regs(&regs);

[Severity: High]
Does this leak uninitialized stack memory?

perf_fetch_caller_regs() on PowerPC only initializes a few fields (result,
nip, gpr[1], msr) in the pt_regs structure. The rest of the stack-allocated
regs structure remains uninitialized.

If the event is opened with PERF_SAMPLE_CALLCHAIN, perf_callchain_kernel()
can read from regs->link and write the uninitialized stack bytes to the ring
buffer. Is it necessary to zero the structure before fetching the regs?

> +
> +	/* Issue one hcall with the current iterator position */
[ ... ]
> +	perf_sample_data_init(&data, 0, event->hw.last_period);
> +	memset(&raw, 0, sizeof(raw));
> +	raw.frag.data = htm_mem_buf;
> +	raw.frag.size = to_copy;
> +	perf_sample_save_raw_data(&data, event, &raw);

[Severity: High]
Can the calculated size overflow perf_event_header.size?

The HTM_MEM_BUF_SIZE is 65440 bytes. With 92 bytes of fixed overhead, this
leaves very little room before overflowing the 65535-byte limit of the u16
header size.

If dynamic sample types like PERF_SAMPLE_CALLCHAIN or PERF_SAMPLE_STACK_USER
are configured (since they are not rejected in htm_event_init()), dyn_size
will increase and the total size will exceed 65535, truncating the header
size and causing ring buffer corruption. Should dynamic sample sizes be
restricted for this event?

> +
> +	if (perf_event_overflow(event, &data, &regs)) {
> +		/*
> +		 * Event throttled: overflow_handler ran unconditionally before
> +		 * returning, so the sample WAS written to the ring buffer.
> +		 * Advance mem_start so the same block is not emitted again.
> +		 * Return -ENOSPC so htm_event_read() sets event->count=1,
> +		 * keeping the drain loop alive to emit the next block once
> +		 * the event is unthrottled.
> +		 */
> +		aux_buf->mem_start = next_start;
> +		if (!next_start)
> +			aux_buf->collect_htm_mem = 0;
> +		return -ENOSPC;
> +	}
> +
> +	/* Record emitted successfully: advance the iterator */
> +	aux_buf->mem_start = next_start;

[Severity: Medium]
Does this unconditionally advance the hypervisor iterator even if the sample
is dropped?

If the main ring buffer is full, perf_output_begin() fails and the sample is
dropped. However, perf_event_overflow() still returns 0 (it only returns
non-zero if the event is throttled).

Since the return value is 0, the code falls through and advances
aux_buf->mem_start = next_start. Could this cause silent data loss of the
memory configuration records?

> +	if (!next_start)
> +		aux_buf->collect_htm_mem = 0;
> +
> +	/*
> +	 * Return the number of 32-byte entries emitted.  Dividing here keeps
> +	 * htm_event_read() free of format knowledge, consistent with the AUX
> +	 * trace path returning chunk_size / 128.
> +	 */
> +	return (ssize_t)(to_copy / 32);
> +}
[ ... ]
> @@ -523,7 +674,7 @@ static ssize_t htm_dump_sample_data(struct perf_event *event)
[ ... ]
>  	/* Start AUX transaction session framework */
>  	aux_buf = perf_aux_output_begin(&htm_ctx->handle, event);
>  	if (!aux_buf)
>  		return 0;

[Severity: Medium]
Does returning 0 here prematurely abort the memory configuration dump?

If the AUX ring buffer is full, perf_aux_output_begin() returns NULL.
Returning 0 signals EOF to userspace, preventing htm_collect_memory_config()
from being called. Does this prevent userspace from receiving the required
configuration data to decode the trace?

>  
> -	if (!aux_buf->collect_htm_trace) {
> +	if (!aux_buf->collect_htm_trace && !aux_buf->collect_htm_mem) {
>  		/*
>  		 * collect_htm_trace is cleared on hcall error and reset to 1
>  		 * in htm_event_start() when tracing restarts.  If it is still
[ ... ]
>  	if (target->tracing_active == HTM_TRACING_ACTIVE) {
>  		htm_event_stop(event, 0);

[Severity: High]
Can this lockless evaluation race with a BPF program running in NMI context?

If a BPF program attached to an NMI event calls bpf_perf_event_read_value()
on the HTM event, it invokes htm_event_read() -> htm_dump_sample_data()
locklessly.

If this interrupts htm_event_start() between a successful start hcall and
setting target->tracing_active = HTM_TRACING_ACTIVE, this read will see
HTM_TRACING_INACTIVE and incorrectly issue H_HTM_OP_DUMP_DATA while the
hardware is actively running, leading to hardware desynchronization.

>  		if (target->tracing_active == HTM_TRACING_ACTIVE) {
>  			/*
>  			 * H_HTM_OP_STOP failed.

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