Re: [PATCH v2 2/3] powerpc/perf: Use the aggregate context switch values from vcpu struct

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

New issues:
- [High] Task-bound PMU events cache a stale vCPU pointer, resulting in tracking the wrong vCPU.
- [High] CPU-wide and system-wide PMU events are broken and silently report 0 if added when no guest is running.
- [High] Uninitialized event state on task migration leaks stale vCPU pointers.
- [Medium] Unbalanced disablement of the global hardware PMU counters.

Pre-existing issues:
- [Critical] Dereferencing `kvm_hstate.kvm_vcpu` leads to a Use-After-Free if the previously running VM has been destroyed.
--

--- Patch [2]: [PATCH v2 2/3] powerpc/perf: Use the aggregate context switch values from vcpu struct ---
commit e328e1f6c8baf0140410c1983f559e87d1d0efdc
Author: Gautam Menghani <gautam@linux.ibm.com>
powerpc/perf: Use the aggregate context switch values from vcpu struct

This commit aims to fix inaccurate vpa-pmu counter reporting during vCPU
migrations and when vCPUs are not running by fetching aggregate values
from the vcpu struct instead of relying on the host cpu's PACA.

> diff --git a/arch/powerpc/perf/vpa-pmu.c b/arch/powerpc/perf/vpa-pmu.c
> --- a/arch/powerpc/perf/vpa-pmu.c
> +++ b/arch/powerpc/perf/vpa-pmu.c
[ ... ]
> @@ -99,19 +99,19 @@ static unsigned long get_counter_data(struct perf_event *event)
>  	switch (config) {
>  	case L1_TO_L2_CS_LAT:
>  		if (event->attach_state & PERF_ATTACH_TASK)
> -			data = kvmhv_get_l1_to_l2_cs_time_vcpu();
> +			data = vcpu->arch.l1_to_l2_cs;

[Severity: Critical]
This is a pre-existing issue, but does dereferencing the vcpu pointer here
lead to a use-after-free if the previously running VM has been destroyed?

When KVM exits a guest, it does not clear local_paca->kvm_hstate.kvm_vcpu,
leaving a dangling pointer in the host's PACA. 

The PMU driver reads this dangling pointer in vpa_pmu_add() and permanently
caches it in event->pmu_private. Subsequent calls to vpa_pmu_read() pass
this cached pointer to get_counter_data(), which dereferences the freed
memory here.

>  		else
>  			data = kvmhv_get_l1_to_l2_cs_time();
>  		break;
[ ... ]
> @@ -126,21 +126,33 @@ static int vpa_pmu_add(struct perf_event *event, int flags)
>  {
>  	u64 data;
> +	struct kvm_vcpu *vcpu;
>  
> +	vcpu = local_paca->kvm_hstate.kvm_vcpu;
> +	if (!vcpu)
> +		goto out;

[Severity: High]
Does this early exit break CPU-wide and system-wide profiling?

If a user initiates a CPU-wide profiling session (e.g. perf stat -a) and no
guest is actively running on that specific CPU at that moment, vcpu is NULL.
The function exits early returning 0 without initializing event->pmu_private
or event->hw.prev_count. 

Subsequent calls to vpa_pmu_read() check if (!vcpu) and immediately return,
silently leaving the event counter permanently at 0.

[Severity: High]
Could this early exit also leak state from a previous CPU when a task
migrates?

When a task with a vpa_pmu event migrates to a new CPU, perf core calls
pmu->del on the old CPU and pmu->add on the new CPU. If vcpu is NULL on the
new CPU, it returns 0 (success) but jumps to out without updating or
clearing event->pmu_private. 

The event silently retains the old vcpu pointer and prev_count from the
previous CPU, resulting in corrupted profiling data calculated against an
unrelated CPU's context.

[Severity: Medium]
Are the global hardware PMU counters skipped here on early bailout, leading
to an unbalanced disablement later?

If a user creates a PMU event on a CPU where no vCPU is active, this early
exit skips calling kvmhv_set_l2_counters_status(cpu, true). 

However, when the event is deleted or scheduled out, vpa_pmu_del()
unconditionally calls kvmhv_set_l2_counters_status(cpu, false). This
unbalanced behavior forcefully disables the hardware counters even when they
were never enabled, which could break legitimate concurrent KVM tracing
sessions.

> +
> +	event->pmu_private = vcpu;

[Severity: High]
Is it possible caching the vcpu pointer here results in tracking the wrong
vCPU for task-bound events?

When a user creates a task-bound vpa_pmu perf event, the core perf
subsystem calls vpa_pmu_add() during perf_event_context_sched_in(). At this
point, the task has been scheduled but has not yet invoked the KVM_RUN
ioctl.

Because of this, local_paca->kvm_hstate.kvm_vcpu does not point to the
incoming task's vCPU; it contains a stale pointer to the last vCPU that
executed on this physical host CPU. Caching this pointer permanently means
the event tracks unrelated data.

>  	kvmhv_set_l2_counters_status(smp_processor_id(), true);
>  
> -	data = get_counter_data(event);
> +	data = get_counter_data(vcpu, event);
>  	local64_set(&event->hw.prev_count, data);
>  
> +out:
>  	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260813094535.10083-1-gautam@linux.ibm.com?part=2