Message ID | 20220713122507.29236-4-likexu@tencent.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | KVM: x86/pmu: Fix some corner cases including Intel PEBS | expand |
On Wed, Jul 13, 2022, Like Xu wrote: > From: Like Xu <likexu@tencent.com> > > In the extreme case of host counters multiplexing and contention, the > perf_event requested by the guest's pebs counter is not allocated to any > actual physical counter, in which case hw.idx is bookkept as -1, > resulting in an out-of-bounds access to host_cross_mapped_mask. > > Fixes: 854250329c02 ("KVM: x86/pmu: Disable guest PEBS temporarily in two rare situations") > Signed-off-by: Like Xu <likexu@tencent.com> > --- > arch/x86/kvm/vmx/pmu_intel.c | 11 +++++------ > 1 file changed, 5 insertions(+), 6 deletions(-) > > diff --git a/arch/x86/kvm/vmx/pmu_intel.c b/arch/x86/kvm/vmx/pmu_intel.c > index 53ccba896e77..1588627974fa 100644 > --- a/arch/x86/kvm/vmx/pmu_intel.c > +++ b/arch/x86/kvm/vmx/pmu_intel.c > @@ -783,20 +783,19 @@ static void intel_pmu_cleanup(struct kvm_vcpu *vcpu) > void intel_pmu_cross_mapped_check(struct kvm_pmu *pmu) > { > struct kvm_pmc *pmc = NULL; > - int bit; > + int bit, hw_idx; > > for_each_set_bit(bit, (unsigned long *)&pmu->global_ctrl, > X86_PMC_IDX_MAX) { > pmc = intel_pmc_idx_to_pmc(pmu, bit); > > if (!pmc || !pmc_speculative_in_use(pmc) || > - !intel_pmc_is_enabled(pmc)) > + !intel_pmc_is_enabled(pmc) || !pmc->perf_event) > continue; > > - if (pmc->perf_event && pmc->idx != pmc->perf_event->hw.idx) { > - pmu->host_cross_mapped_mask |= > - BIT_ULL(pmc->perf_event->hw.idx); > - } > + hw_idx = pmc->perf_event->hw.idx; > + if (hw_idx != pmc->idx && hw_idx != -1) How about "hw_idx > 0" so that KVM is a little less dependent on perf's exact behavior? A comment here would be nice too. > + pmu->host_cross_mapped_mask |= BIT_ULL(hw_idx); > } > } > > -- > 2.37.0 >
On 21/7/2022 8:45 am, Sean Christopherson wrote: > On Wed, Jul 13, 2022, Like Xu wrote: >> From: Like Xu <likexu@tencent.com> >> >> In the extreme case of host counters multiplexing and contention, the >> perf_event requested by the guest's pebs counter is not allocated to any >> actual physical counter, in which case hw.idx is bookkept as -1, >> resulting in an out-of-bounds access to host_cross_mapped_mask. >> >> Fixes: 854250329c02 ("KVM: x86/pmu: Disable guest PEBS temporarily in two rare situations") >> Signed-off-by: Like Xu <likexu@tencent.com> >> --- >> arch/x86/kvm/vmx/pmu_intel.c | 11 +++++------ >> 1 file changed, 5 insertions(+), 6 deletions(-) >> >> diff --git a/arch/x86/kvm/vmx/pmu_intel.c b/arch/x86/kvm/vmx/pmu_intel.c >> index 53ccba896e77..1588627974fa 100644 >> --- a/arch/x86/kvm/vmx/pmu_intel.c >> +++ b/arch/x86/kvm/vmx/pmu_intel.c >> @@ -783,20 +783,19 @@ static void intel_pmu_cleanup(struct kvm_vcpu *vcpu) >> void intel_pmu_cross_mapped_check(struct kvm_pmu *pmu) >> { >> struct kvm_pmc *pmc = NULL; >> - int bit; >> + int bit, hw_idx; >> >> for_each_set_bit(bit, (unsigned long *)&pmu->global_ctrl, >> X86_PMC_IDX_MAX) { >> pmc = intel_pmc_idx_to_pmc(pmu, bit); >> >> if (!pmc || !pmc_speculative_in_use(pmc) || >> - !intel_pmc_is_enabled(pmc)) >> + !intel_pmc_is_enabled(pmc) || !pmc->perf_event) >> continue; >> >> - if (pmc->perf_event && pmc->idx != pmc->perf_event->hw.idx) { >> - pmu->host_cross_mapped_mask |= >> - BIT_ULL(pmc->perf_event->hw.idx); >> - } >> + hw_idx = pmc->perf_event->hw.idx; >> + if (hw_idx != pmc->idx && hw_idx != -1) > > How about "hw_idx > 0" so that KVM is a little less dependent on perf's exact > behavior? A comment here would be nice too. The "hw->idx = 0" means that it occupies counter 0, so this part will look like this: hw_idx = pmc->perf_event->hw.idx; /* make it a little less dependent on perf's exact behavior */ if (hw_idx != pmc->idx && hw_idx > -1) pmu->host_cross_mapped_mask |= BIT_ULL(hw_idx); , what do you think ? > >> + pmu->host_cross_mapped_mask |= BIT_ULL(hw_idx); >> } >> } >> >> -- >> 2.37.0 >>
On Thu, Jul 21, 2022, Like Xu wrote: > On 21/7/2022 8:45 am, Sean Christopherson wrote: > > On Wed, Jul 13, 2022, Like Xu wrote: > > > From: Like Xu <likexu@tencent.com> > > > > > > In the extreme case of host counters multiplexing and contention, the > > > perf_event requested by the guest's pebs counter is not allocated to any > > > actual physical counter, in which case hw.idx is bookkept as -1, > > > resulting in an out-of-bounds access to host_cross_mapped_mask. > > > > > > Fixes: 854250329c02 ("KVM: x86/pmu: Disable guest PEBS temporarily in two rare situations") > > > Signed-off-by: Like Xu <likexu@tencent.com> > > > --- > > > arch/x86/kvm/vmx/pmu_intel.c | 11 +++++------ > > > 1 file changed, 5 insertions(+), 6 deletions(-) > > > > > > diff --git a/arch/x86/kvm/vmx/pmu_intel.c b/arch/x86/kvm/vmx/pmu_intel.c > > > index 53ccba896e77..1588627974fa 100644 > > > --- a/arch/x86/kvm/vmx/pmu_intel.c > > > +++ b/arch/x86/kvm/vmx/pmu_intel.c > > > @@ -783,20 +783,19 @@ static void intel_pmu_cleanup(struct kvm_vcpu *vcpu) > > > void intel_pmu_cross_mapped_check(struct kvm_pmu *pmu) > > > { > > > struct kvm_pmc *pmc = NULL; > > > - int bit; > > > + int bit, hw_idx; > > > for_each_set_bit(bit, (unsigned long *)&pmu->global_ctrl, > > > X86_PMC_IDX_MAX) { > > > pmc = intel_pmc_idx_to_pmc(pmu, bit); > > > if (!pmc || !pmc_speculative_in_use(pmc) || > > > - !intel_pmc_is_enabled(pmc)) > > > + !intel_pmc_is_enabled(pmc) || !pmc->perf_event) > > > continue; > > > - if (pmc->perf_event && pmc->idx != pmc->perf_event->hw.idx) { > > > - pmu->host_cross_mapped_mask |= > > > - BIT_ULL(pmc->perf_event->hw.idx); > > > - } > > > + hw_idx = pmc->perf_event->hw.idx; > > > + if (hw_idx != pmc->idx && hw_idx != -1) > > > > How about "hw_idx > 0" so that KVM is a little less dependent on perf's exact > > behavior? A comment here would be nice too. > > The "hw->idx = 0" means that it occupies counter 0, so this part will look > like this: Doh, typo on my part, meant "hw_idx >= 0". "> -1" is ok, though it's definitely less idiomatic: $ git grep ">= 0" | wc -l 5679 $ git grep "> -1" | wc -l 66
diff --git a/arch/x86/kvm/vmx/pmu_intel.c b/arch/x86/kvm/vmx/pmu_intel.c index 53ccba896e77..1588627974fa 100644 --- a/arch/x86/kvm/vmx/pmu_intel.c +++ b/arch/x86/kvm/vmx/pmu_intel.c @@ -783,20 +783,19 @@ static void intel_pmu_cleanup(struct kvm_vcpu *vcpu) void intel_pmu_cross_mapped_check(struct kvm_pmu *pmu) { struct kvm_pmc *pmc = NULL; - int bit; + int bit, hw_idx; for_each_set_bit(bit, (unsigned long *)&pmu->global_ctrl, X86_PMC_IDX_MAX) { pmc = intel_pmc_idx_to_pmc(pmu, bit); if (!pmc || !pmc_speculative_in_use(pmc) || - !intel_pmc_is_enabled(pmc)) + !intel_pmc_is_enabled(pmc) || !pmc->perf_event) continue; - if (pmc->perf_event && pmc->idx != pmc->perf_event->hw.idx) { - pmu->host_cross_mapped_mask |= - BIT_ULL(pmc->perf_event->hw.idx); - } + hw_idx = pmc->perf_event->hw.idx; + if (hw_idx != pmc->idx && hw_idx != -1) + pmu->host_cross_mapped_mask |= BIT_ULL(hw_idx); } }