| Message ID | 20260813094535.10083-4-gautam@linux.ibm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | powerpc/perf: Fix vpa-pmu driver to report correct numbers | expand |
On Thu, Aug 13, 2026 at 10:01:02AM +0000, sashiko-bot@kernel.org wrote: > Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: > > New issues: > - [Medium] Missing atomic cmpxchg loop in vpa_pmu_read() when updating prev_count. > > Pre-existing issues: > - [High] The VPA PMU `add` callback stores a raw pointer to `struct kvm_vcpu` in `event->pmu_private` without reference counting, leading to a Use-After-Free. > -- > > --- Patch [3]: [PATCH v2 3/3] powerpc/perf: Update prev_count of event to get accurate values --- > commit 7c392d724ba481a5282a90aed95397fac4533493 > Author: Gautam Menghani <gautam@linux.ibm.com> > > powerpc/perf: Update prev_count of event to get accurate values > > This patch updates the VPA PMU read function to keep event->hw.prev_count > current. This prevents stale values from being returned on consecutive reads > while a process is running on a CPU. > > > diff --git a/arch/powerpc/perf/vpa-pmu.c b/arch/powerpc/perf/vpa-pmu.c > > index e79d98447c743..efc39f71553bf 100644 > > --- a/arch/powerpc/perf/vpa-pmu.c > > +++ b/arch/powerpc/perf/vpa-pmu.c > > @@ -156,6 +156,7 @@ static void vpa_pmu_read(struct perf_event *event) > > final_data = new_data - prev_data; > > > > local64_add(final_data, &event->count); > > + local64_set(&event->hw.prev_count, new_data); > > [Severity: Medium] > Is it safe to use local64_set() here without a local64_cmpxchg() loop? > > If vpa_pmu_read() is interrupted by an NMI (such as a hardware PMI) and a BPF > program attached to the NMI reads the same event via bpf_perf_event_read(), > the nested read will calculate its delta and update prev_count. > > When the outer vpa_pmu_read() resumes, it could calculate its delta using the > old prev_data (resulting in double-counting the inner delta) and then > non-atomically overwrite prev_count. > > Should this use a local64_cmpxchg() loop to safely update prev_count? Yes, valid point. > > > } > > > > static void vpa_pmu_del(struct perf_event *event, int flags) > > [Severity: High] > This is a pre-existing issue, but does this code leave a stale vcpu pointer > in event->pmu_private that can lead to a use-after-free? > > Looking at vpa_pmu_add(), a raw pointer to struct kvm_vcpu is stored without > taking a reference: > > arch/powerpc/perf/vpa-pmu.c:vpa_pmu_add() { > ... > vcpu = local_paca->kvm_hstate.kvm_vcpu; > if (!vcpu) > goto out; > ... > event->pmu_private = vcpu; > } > > When the thread context-switches out, vpa_pmu_del() stops the event but > does not clear event->pmu_private to NULL. When the thread is scheduled > back in outside of KVM, vpa_pmu_add() sees the vcpu is NULL and returns > early, leaving the event active with the stale vcpu pointer. > > If the VM is then destroyed and the vcpu freed, would a subsequent thread > exit or user read() calling vpa_pmu_read() trigger a use-after-free when > dereferencing vcpu->arch.l1_to_l2_cs? > Valid find, will fix in v3. > -- > Sashiko AI review ยท https://sashiko.dev/#/patchset/20260813094535.10083-1-gautam@linux.ibm.com?part=3
diff --git a/arch/powerpc/perf/vpa-pmu.c b/arch/powerpc/perf/vpa-pmu.c index e79d98447c74..efc39f71553b 100644 --- a/arch/powerpc/perf/vpa-pmu.c +++ b/arch/powerpc/perf/vpa-pmu.c @@ -156,6 +156,7 @@ static void vpa_pmu_read(struct perf_event *event) final_data = new_data - prev_data; local64_add(final_data, &event->count); + local64_set(&event->hw.prev_count, new_data); } static void vpa_pmu_del(struct perf_event *event, int flags)
When vpa_pmu_read() is called twice while a process is running on a cpu, the "event->hw.prev_count" variable returns a stale value on the 2nd read which results in a larger value getting added to event->count. Fix this by updating the event->hw.prev_count every time vpa_pmu_read() is called. Fixes: 176cda0619b6 ("powerpc/perf: Add perf interface to expose vpa counters") Cc: <stable@vger.kernel.org> Signed-off-by: Gautam Menghani <gautam@linux.ibm.com> --- v1 -> v2: 1. Add stable/fixes tags. arch/powerpc/perf/vpa-pmu.c | 1 + 1 file changed, 1 insertion(+)