linux-kernel - Re: [PATCH 17/18] KVM: x86: Push acquisition of SRCU in fastpath into kvm_pmu_trigger

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aJSqlMViOHAHHyCq@google.com>
Date: Thu, 7 Aug 2025 06:31:00 -0700
From: Sean Christopherson <seanjc@...gle.com>
To: Dapeng Mi <dapeng1.mi@...ux.intel.com>
Cc: Paolo Bonzini <pbonzini@...hat.com>, kvm@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Xin Li <xin@...or.com>, Sandipan Das <sandipan.das@....com>
Subject: Re: [PATCH 17/18] KVM: x86: Push acquisition of SRCU in fastpath into kvm_pmu_trigger_event()

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@...gle.com>
> >>> ---
> > ...
> >
> >>> @@ -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()