Re: [PATCH 2/3] KVM: Always flush async #PF workqueue when vCPU is being destroyed

[Xu Yilun]

On Tue, Oct 24, 2023 at 08:49:24AM -0700, Sean Christopherson wrote:
> On Tue, Oct 24, 2023, Xu Yilun wrote:
> > On Wed, Oct 18, 2023 at 01:46:23PM -0700, Sean Christopherson wrote:
> > > @@ -126,7 +124,19 @@ void kvm_clear_async_pf_completion_queue(struct kvm_vcpu *vcpu)
> > >  			list_first_entry(&vcpu->async_pf.done,
> > >  					 typeof(*work), link);
> > >  		list_del(&work->link);
> > > +
> > > +		spin_unlock(&vcpu->async_pf.lock);
> > > +
> > > +		/*
> > > +		 * The async #PF is "done", but KVM must wait for the work item
> > > +		 * itself, i.e. async_pf_execute(), to run to completion.  If
> > > +		 * KVM is a module, KVM must ensure *no* code owned by the KVM
> > > +		 * (the module) can be run after the last call to module_put(),
> > > +		 * i.e. after the last reference to the last vCPU's file is put.
> > > +		 */
> > > +		flush_work(&work->work);
> > 
> > I see the flush_work() is inside the check:
> > 
> >   while (!list_empty(&vcpu->async_pf.done))
> > 
> > Is it possible all async_pf are already completed but the work item,
> > i.e. async_pf_execute, is not completed before this check? That the
> > work is scheduled out after kvm_arch_async_page_present_queued() and
> > all APF_READY requests have been handled. In this case the work
> > synchronization will be skipped...
> 
> Good gravy.  Yes, I assumed KVM wouldn't be so crazy to delete the work before it
> completed, but I obviously didn't see this comment in async_pf_execute():
> 
> 	/*
> 	 * apf may be freed by kvm_check_async_pf_completion() after
> 	 * this point
> 	 */
> 
> The most straightforward fix I see is to also flush the work in
> kvm_check_async_pf_completion(), and then delete the comment.  The downside is
> that there's a small chance a vCPU could be delayed waiting for the work to
> complete, but that's a very, very small chance, and likely a very small delay.
> kvm_arch_async_page_present_queued() unconditionaly makes a new request, i.e. will
> effectively delay entering the guest, so the remaining work is really just:
> 
>  	trace_kvm_async_pf_completed(addr, cr2_or_gpa);
> 
> 	__kvm_vcpu_wake_up(vcpu);
> 
> 	mmput(mm);
> 
> Since mmput() can't drop the last reference to the page tables if the vCPU is
> still alive.

OK, seems the impact is minor. I'm good to it.

Thanks,
Yilun

> 
> I think I'll spin off the async #PF fix to a separate series.  There's are other
> tangetially related cleanups that can be done, e.g. there's no reason to pin the
> page tables while work is queued, async_pf_execute() can do mmget_not_zero() and
> then bail if the process is dying.  Then there's no need to do mmput() when
> canceling work.