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

[Athira Rajeev <atrajeev@linux.ibm.com>]

> On 20 Jul 2026, at 4:39 PM, sashiko-bot@kernel.org wrote:
> 
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [Critical] The staging buffer `htm_mem_buf` is allocated as `PAGE_SIZE` but the hypervisor is instructed to write up to `HTM_MEM_BUF_SIZE` (64448 bytes) into it, causing a physical memory overflow on 4K page configurations.
> - [High] Uninitialized `struct pt_regs regs` allocated on the stack is passed directly to `perf_event_overflow()`, leading to a kernel stack memory leak.
> - [Medium] The memory configuration iterator `mem_start` is not advanced when `perf_event_overflow` returns a non-zero (throttled) value, causing data duplication.
> --
> 
> --- Patch [4]: [PATCH V2 4/6] powerpc/perf: Capture the HTM memory configuration as part of perf data ---
> commit 5929e601ef9335650c92ad97d9999f8482e8f648
> Author: Athira Rajeev <atrajeev@linux.ibm.com>
> 
> powerpc/perf: Capture the HTM memory configuration as part of perf data
> 
> This commit captures HTM system memory configuration during tracing by
> retrieving it via a hypercall. The records are emitted as raw perf sample
> data after the AUX trace dump completes.
> 
>> diff --git a/arch/powerpc/perf/htm-perf.c b/arch/powerpc/perf/htm-perf.c
>> index b4020a6e8f30b..0fda0f552b4e1 100644
>> --- a/arch/powerpc/perf/htm-perf.c
>> +++ b/arch/powerpc/perf/htm-perf.c
> [ ... ]
>> @@ -173,6 +177,99 @@ static ssize_t htm_return_check(int rc)
>> #define HTM_TRACING_ACTIVE 1
>> #define HTM_TRACING_INACTIVE 0
>> 
>> +#define HTM_MEM_MAX_ENTRIES 2013U
>> +#define HTM_MEM_BUF_SIZE (32 + HTM_MEM_MAX_ENTRIES * 32)
>> +
>> +static int 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]
> Does this leak kernel stack memory to userspace?
> 
> The regs struct is allocated on the stack but never zeroed or initialized.
> If userspace configures the perf event with PERF_SAMPLE_REGS_INTR or
> PERF_SAMPLE_CALLCHAIN, the core perf framework will read these
> uninitialized bytes in perf_event_overflow() and write them to the ring
> buffer.
> 
> Should regs be initialized, for example via perf_fetch_caller_regs(&regs)
> or memset(&regs, 0, sizeof(regs))?

Thanks for catching this. Will send a V3 addressing this

> 
>> + u8 *htm_mem_buf = aux_buf->htm_mem_buf;
>> + __be64 *num_entries;
>> + u64 next_start;
>> + u64 to_copy;
> [ ... ]
>> + memcpy(emit_buf, aux_buf->htm_mem_buf, to_copy);
>> +
>> + 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)) {
>> + ret = 0;
>> + break;
>> + }
>                ^^^^^
> 
> [Severity: Medium]
> If perf_event_overflow() throttles the event and returns a non-zero value,
> we break out of the loop before advancing the iterator.
> 
> When tracing resumes and htm_collect_memory_config() is called again,
> it will start with the stale aux_buf->mem_start value and repeatedly ask
> the hypervisor for the same configuration block. Will this emit duplicate
> records into the perf stream?

Clarified the perf_event_overflow() throttle path: when the event is
  throttled, mem_start is intentionally not advanced (the same block will
  be retried on the next drain pass once unthrottled).  Added a comment in V3
  making this explicit and distinguishing it from the error/EOF paths
  that clear collect_htm_mem.


> 
>> +
>> + /* Record written: advance next read iterator */
>> + aux_buf->mem_start = next_start;
>> + emitted = 1;
>> +
>> + if (!next_start) {
>> + aux_buf->collect_htm_mem = 0;
>> + break;
>> + }
>> + }
> [ ... ]
>> @@ -568,7 +672,23 @@ static void *htm_setup_aux(struct perf_event *event, void **pages,
>> return NULL;
>> }
>> 
>> + buf->htm_mem_buf = kmalloc_node(PAGE_SIZE, GFP_KERNEL, cpu_to_node(cpu));
>        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> [Severity: Critical]
> Can this corrupt physical memory on systems with 4K pages?
> 
> buf->htm_mem_buf is allocated using PAGE_SIZE, which is 4096 bytes if
> CONFIG_PPC_4K_PAGES is enabled.
> 
> However, in htm_collect_memory_config(), the hypervisor is instructed that
> the buffer length is HTM_MEM_BUF_SIZE:
> 
> arch/powerpc/perf/htm-perf.c:htm_collect_memory_config() {
> ...
> rc = htm_hcall_wrapper(htmflags, 0, 0, 0,
>       0, H_HTM_OP_DUMP_SYSMEM_CONF,
>       virt_to_phys(aux_buf->htm_mem_buf),
>       HTM_MEM_BUF_SIZE, aux_buf->mem_start);
> ...
> }
> 
> Since HTM_MEM_BUF_SIZE is defined as 32 + 2013 * 32 = 64448, the hypervisor
> will assume it can write 64448 bytes, silently overwriting up to ~60KB of
> contiguous physical kernel memory past the 4KB allocation.
> 
> Should this allocate HTM_MEM_BUF_SIZE bytes instead of PAGE_SIZE?

Sure, will fix this using HTM_MEM_BUF_SIZE in V3
> 
> 
>> + if (!buf->htm_mem_buf) {
>> + kfree(buf);
>> + return NULL;
>> + }
>> +
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260720104447.11843-1-atrajeev@linux.ibm.com?part=4