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

[Gautam Menghani <gautam@linux.ibm.com>]

On Thu, Aug 13, 2026 at 09:58:50AM +0000, sashiko-bot@kernel.org wrote:
> 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;
> 

A dangling pointer is stored, but will that cause a UAF? When tracing a
particular tid/pid, the perf stat command exits if the pid (guest) is destroyed.

> [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.

The vpa-pmu driver is only meant to report context switch times of individual
vCPUs. So reporting 0 when a vCPU is not running is expected behaviour.

> 
> [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.

Yes, valid point.

> 
> [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.

Valid find - in vpa_pmu_del() the counters should be disabled only if they
were enabled in vpa_pmu_add()

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

Valid find, will fix

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