Re: [PATCH V3 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] HTM_MEM_BUF_SIZE (65440 bytes) leaves insufficient headroom for dynamic perf sample fields, causing perf_event_header.size to overflow its u16 limit and corrupt the ring buffer.
- [High] Uninitialized kernel stack memory in struct pt_regs is leaked to userspace when the event is configured with PERF_SAMPLE_REGS_INTR.
- [High] Misinterpretation of perf_event_overflow() return value causes silent data loss when the ring buffer is full, and data duplication when the event is throttled.
- [High] htm_mem_buf is allocated with kmalloc_node instead of kzalloc_node, potentially leaking uninitialized kernel heap memory to userspace.
- [Medium] htm_collect_memory_config fails to accumulate the emitted entry counts across its loop iterations, resulting in metric under-reporting.
--

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

This commit captures HTM system memory configuration records by dumping them
into the AUX buffer and emitting them as raw perf samples. It introduces
buffers for the hypervisor dump, handles layout sizing, and iterates through
memory config batches using H_HTM_OP_DUMP_SYSMEM_CONF.

> diff --git a/arch/powerpc/perf/htm-perf.c b/arch/powerpc/perf/htm-perf.c
> index f880a5fc88330..af9a8412ff20f 100644
> --- a/arch/powerpc/perf/htm-perf.c
> +++ b/arch/powerpc/perf/htm-perf.c

[ ... ]

> +/*
> + * HTM_MEM_BUF_SIZE is the allocation size for the hcall staging buffer.
[ ... ]
> + * 65440 = 32 + 2043 * 32 is the largest multiple of 32 satisfying all
> + * three constraints:
> + *   - covers 64480 with 960 bytes headroom
> + *   - total record: 65440 + 92 = 65532 < 65535 (3-byte u16 margin)
> + *
> + * HTM_MEM_MAX_ENTRIES is derived from HTM_MEM_BUF_SIZE — not the other
> + * way around — so the hcall is always given the true buffer size and
> + * the WARN_ON_ONCE(to_copy > HTM_MEM_BUF_SIZE) guard is a genuine
> + * impossibility check rather than a post-overflow assertion.
> + */
> +#define HTM_MEM_BUF_SIZE	65440U

[Severity: High]
Could the buffer size calculation cause an overflow if dynamic sample
attributes are used?

The commit message states the fixed overhead is 92 bytes, which leaves
(65535 - 92 = 65443) bytes for the payload. However, if the event is
configured with PERF_SAMPLE_REGS_INTR, the size of struct pt_regs (which is
over 300 bytes on PowerPC) is added to the sample size.

Does this cause perf_sample_data_size() to return a value greater than 65535,
which would wrap around when assigned to the __u16 header.size in
perf_prepare_header(), potentially leading to ring buffer corruption?

> +#define HTM_MEM_MAX_ENTRIES	((HTM_MEM_BUF_SIZE - 32) / 32)	/* 2043 */

[ ... ]

> +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;

[Severity: High]
Is it possible for this uninitialized stack variable to leak kernel memory?

The regs structure is partially initialized by perf_fetch_caller_regs(),
which on PowerPC only sets nip, gpr[1], result, and msr:

perf_arch_fetch_caller_regs(regs, CALLER_ADDR0) {
    ...
}

If the event uses PERF_SAMPLE_REGS_INTR, will perf_output_sample() write the
uninitialized portions of this pt_regs structure into the perf ring buffer,
exposing stale kernel stack contents to userspace? Should this be initialized
with an empty initialization?

> +	u8 *htm_mem_buf = aux_buf->htm_mem_buf;
> +	__be64 *num_entries;

[ ... ]

> +		perf_sample_data_init(&data, 0, event->hw.last_period);
> +		memset(&raw, 0, sizeof(raw));
> +		raw.frag.data = emit_buf;
> +		raw.frag.size = to_copy;
> +		perf_sample_save_raw_data(&data, event, &raw);
> +
> +		if (perf_event_overflow(event, &data, &regs)) {

[Severity: High]
Does this logic misinterpret the return value of perf_event_overflow()?

Looking at __perf_event_overflow(), it returns non-zero (1) only when an
event is throttled. If it returns 1, the current sample was already
successfully written before the throttle triggered. However, the driver
assumes failure here, skips advancing mem_start, and emits the exact same
block again on the next pass.

Conversely, if the ring buffer fills up, the core drops the sample but
__perf_event_overflow() still returns 0. When it returns 0, the driver
assumes success, advances mem_start, and silently loses configuration data.

Is this behavior intended, or should it be adjusted to avoid silent data
loss and duplication?

> +			/*
> +			 * Event throttled: the record was not written to the
> +			 * ring buffer.  Do NOT advance mem_start — the same
> +			 * block will be retried on the next htm_event_read()
> +			 * call once the event is unthrottled.  Leave
> +			 * collect_htm_mem set so the retry path is entered.
> +			 * Return -ENOSPC so htm_event_read() sets event->count=1,
> +			 * keeping the drain loop alive until the ring buffer
> +			 * consumer catches up.
> +			 */
> +			ret = -ENOSPC;
> +			break;
> +		}
> +
> +		/* Record written successfully: advance the iterator */
> +		aux_buf->mem_start = next_start;
> +
> +		/*
> +		 * Return the number of 32-byte memory configuration entries
> +		 * in this batch (to_copy / 32).  Dividing here keeps
> +		 * htm_event_read() free of format knowledge, consistent with
> +		 * the AUX trace path returning chunk_size / 128.
> +		 */
> +		ret = (ssize_t)(to_copy / 32);

[Severity: Medium]
Does this assignment under-report the emitted entry counts?

Since this is inside a while (true) loop, multiple chunks of memory
configuration data might be processed. By directly overwriting ret instead
of accumulating it (e.g., ret += to_copy / 32), the function seems to return
only the count from the last chunk processed.

Will this permanently drop the counts of all records emitted in prior loop
iterations during the same session?

> +
> +		if (!next_start) {
> +			aux_buf->collect_htm_mem = 0;
> +			break;
> +		}
> +	}
> +
> +	return ret;
> +}

[ ... ]

> +static void *htm_setup_aux(struct perf_event *event, void **pages,
> +		int nr_pages, bool snapshot)
> +{

[ ... ]

> +	/*
> +	 * htm_mem_buf is the staging area passed directly to the
> +	 * H_HTM_OP_DUMP_SYSMEM_CONF hcall.  The hypervisor is told the
> +	 * buffer length is HTM_MEM_BUF_SIZE (65440 bytes); allocate exactly
> +	 * that amount.  See the HTM_MEM_BUF_SIZE comment for the derivation.
> +	 */
> +	buf->htm_mem_buf = kmalloc_node(HTM_MEM_BUF_SIZE, GFP_KERNEL, cpu_to_node(cpu));

[Severity: High]
Could allocating this buffer with kmalloc_node() lead to an information leak?

This buffer is passed to the hypervisor and subsequently copied to userspace
via the perf ring buffer:

htm_collect_memory_config() {
    ...
    memcpy(emit_buf, aux_buf->htm_mem_buf, to_copy);
    ...
}

If the hypervisor successfully returns but writes fewer entries than it
claims, leaves reserved fields untouched, or inserts padding bytes, will the
uninitialized kernel heap memory be exposed to userspace? Should this use
kzalloc_node() instead to defensively zero-initialize the memory?

> +	if (!buf->htm_mem_buf) {
> +		kfree(buf);
> +		return NULL;
> +	}

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