Re: [PATCH 2/3] KVM: x86: Fix a semi theoretical bug in kvm_arch_async_page_present_queued

[Sean Christopherson <seanjc@google.com>]

On Tue, Sep 23, 2025, Sean Christopherson wrote:
> On Tue, Sep 23, 2025, Paolo Bonzini wrote:
> > On 9/23/25 20:55, Sean Christopherson wrote:
> > > On Tue, Sep 23, 2025, Paolo Bonzini wrote:
> > > > On 8/13/25 21:23, Maxim Levitsky wrote:
> > > > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > > > > index 9018d56b4b0a..3d45a4cd08a4 100644
> > > > > --- a/arch/x86/kvm/x86.c
> > > > > +++ b/arch/x86/kvm/x86.c
> > > > > @@ -13459,9 +13459,14 @@ void kvm_arch_async_page_present(struct kvm_vcpu *vcpu,
> > > > >    void kvm_arch_async_page_present_queued(struct kvm_vcpu *vcpu)
> > > > >    {
> > > > > -	kvm_make_request(KVM_REQ_APF_READY, vcpu);
> > > > > -	if (!vcpu->arch.apf.pageready_pending)
> > > > > +	/* Pairs with smp_store_release in vcpu_enter_guest. */
> > > > > +	bool in_guest_mode = (smp_load_acquire(&vcpu->mode) == IN_GUEST_MODE);
> > > > > +	bool page_ready_pending = READ_ONCE(vcpu->arch.apf.pageready_pending);
> > > > > +
> > > > > +	if (!in_guest_mode || !page_ready_pending) {
> > > > > +		kvm_make_request(KVM_REQ_APF_READY, vcpu);
> > > > >    		kvm_vcpu_kick(vcpu);
> > > > > +	}
> > > > 
> > > > Unlike Sean, I think the race exists in abstract and is not benign
> > > 
> > > How is it not benign?  I never said the race doesn't exist, I said that consuming
> > > a stale vcpu->arch.apf.pageready_pending in kvm_arch_async_page_present_queued()
> > > is benign.
> > 
> > In principle there is a possibility that a KVM_REQ_APF_READY is missed.
> 
> I think you mean a kick (wakeup or IPI), is missed, not that the APF_READY itself
> is missed.  I.e. KVM_REQ_APF_READY will never be lost, KVM just might enter the
> guest or schedule out the vCPU with the flag set.
> 
> All in all, I think we're in violent agreement.  I agree that kvm_vcpu_kick()
> could be missed (theoretically), but I'm saying that missing the kick would be
> benign due to a myriad of other barriers and checks, i.e. that the vCPU is
> guaranteed to see KVM_REQ_APF_READY anyways.
> 
> E.g. my suggestion earlier regarding OUTSIDE_GUEST_MODE was to rely on the
> smp_mb__after_srcu_read_{,un}lock() barriers in vcpu_enter_guest() to ensure
> KVM_REQ_APF_READY would be observed before trying VM-Enter, and that if KVM might
> be in the process of emulating HLT (blocking), that either KVM_REQ_APF_READY is
> visible to the vCPU or that kvm_arch_async_page_present() wakes the vCPU.  Oh,
> hilarious, async_pf_execute() also does an unconditional __kvm_vcpu_wake_up().
> 
> Huh.  But isn't that a real bug?  KVM doesn't consider KVM_REQ_APF_READY to be a
> wake event, so isn't this an actual race?
> 
>   vCPU                                  async #PF
>   kvm_check_async_pf_completion()
>   pageready_pending = false
>   VM-Enter
>   HLT
>   VM-Exit
>                                         kvm_make_request(KVM_REQ_APF_READY, vcpu)
>                                         kvm_vcpu_kick(vcpu)  // nop as the vCPU isn't blocking, yet
>                                         __kvm_vcpu_wake_up() // nop for the same reason
>   vcpu_block()
>   <hang>
> 
> On x86, the "page ready" IRQ is only injected from vCPU context, so AFAICT nothing
> is guarnateed wake the vCPU in the above sequence.

Gah, KVM checks async_pf.done instead of the request.  So I don't think there's
a bug, just weird code.

  bool kvm_vcpu_has_events(struct kvm_vcpu *vcpu)
  {
	if (!list_empty_careful(&vcpu->async_pf.done))  <===
		return true;