On 8/7/2025 9:31 PM, Sean Christopherson wrote: > On Thu, Aug 07, 2025, Dapeng Mi wrote: >> On 8/7/2025 1:33 AM, Sean Christopherson wrote: >>> On Wed, Aug 06, 2025, Dapeng Mi wrote: >>>> On 8/6/2025 3:05 AM, Sean Christopherson wrote: >>>>> Acquire SRCU in the VM-Exit fastpath if and only if KVM needs to check the >>>>> PMU event filter, to further trim the amount of code that is executed with >>>>> SRCU protection in the fastpath. Counter-intuitively, holding SRCU can do >>>>> more harm than good due to masking potential bugs, and introducing a new >>>>> SRCU-protected asset to code reachable via kvm_skip_emulated_instruction() >>>>> would be quite notable, i.e. definitely worth auditing. >>>>> >>>>> E.g. the primary user of kvm->srcu is KVM's memslots, accessing memslots >>>>> all but guarantees guest memory may be accessed, accessing guest memory >>>>> can fault, and page faults might sleep, which isn't allowed while IRQs are >>>>> disabled. Not acquiring SRCU means the (hypothetical) illegal sleep would >>>>> be flagged when running with PROVE_RCU=y, even if DEBUG_ATOMIC_SLEEP=n. >>>>> >>>>> Note, performance is NOT a motivating factor, as SRCU lock/unlock only >>>>> adds ~15 cycles of latency to fastpath VM-Exits. I.e. overhead isn't a >>>>> concern _if_ SRCU protection needs to be extended beyond PMU events, e.g. >>>>> to honor userspace MSR filters. >>>>> >>>>> Signed-off-by: Sean Christopherson <seanjc@xxxxxxxxxx> >>>>> --- >>> ... >>> >>>>> @@ -968,12 +968,14 @@ static void kvm_pmu_trigger_event(struct kvm_vcpu *vcpu, >>>>> (unsigned long *)&pmu->global_ctrl, X86_PMC_IDX_MAX)) >>>>> return; >>>>> >>>>> + idx = srcu_read_lock(&vcpu->kvm->srcu); >>>> It looks the asset what "kvm->srcu" protects here is >>>> kvm->arch.pmu_event_filter which is only read by pmc_is_event_allowed(). >>>> Besides here, pmc_is_event_allowed() is called by reprogram_counter() but >>>> without srcu_read_lock()/srcu_read_unlock() protection. >>> No, reprogram_counter() is only called called in the context of KVM_RUN, i.e. with >>> the vCPU loaded and thus with kvm->srcu already head for read (acquired by >>> kvm_arch_vcpu_ioctl_run()). >> Not sure if I understand correctly, but KVM_SET_PMU_EVENT_FILTER ioctl is a >> VM-level ioctl and it can be set when vCPUs are running. So assume >> KVM_SET_PMU_EVENT_FILTER ioctl is called at vCPU0 and vCPU1 is running >> reprogram_counter(). Is it safe without srcu_read_lock()/srcu_read_unlock() >> protection? > No, but reprogram_counter() can be reached if and only if the CPU holds SRCU. > > kvm_arch_vcpu_ioctl_run() => kvm_vcpu_srcu_read_lock(vcpu); > | > -> vcpu_run() > | > -> vcpu_enter_guest() > | > -> kvm_pmu_handle_event() > | > -> reprogram_counter() oh, yes. I missed it. Thanks for explaining. Reviewed-by: Dapeng Mi <dapeng1.mi@xxxxxxxxxxxxxxx> >
