On Thu, Aug 13, 2026 at 09:58:50AM +0000, [email protected] 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 <[email protected]> > 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/[email protected]?part=2
