Re: [PATCH 1/2] KVM: register asynchronous page fault waits with interrupts disabled

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] KVM: x86: race on host_apf_node.wq in apf_task_wake_one() after vCPU migration
--

commit a7551b9e239e704cac35f0fb0f96e22e31414870
Author: Paolo Bonzini <pbonzini@redhat.com>

KVM: register asynchronous page fault waits with interrupts disabled

This commit splits asynchronous page fault waiter registration from waiting so
that KVM can queue a per-vCPU sleep node while interrupts are still disabled
on #PF VM-exit, avoiding dummy sleep node allocations when a vCPU thread is
preempted before reaching kvm_handle_page_fault().

Link: https://lore.kernel.org/kvm/6adf2939.67317f00.38c4bc.00ae.GAE@google.com/

> diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
> index 9f0efbe947934..ca5dcdc7ffde8 100644
> --- a/arch/x86/kernel/kvm.c
> +++ b/arch/x86/kernel/kvm.c
> @@ -108,12 +100,14 @@ static struct kvm_task_sleep_node *_find_apf_task(struct kvm_task_sleep_head *b,
>  	return NULL;
>  }
>  
> -static bool kvm_async_pf_queue_task(u32 token, struct kvm_task_sleep_node *n)
> +bool kvm_async_pf_queue_task(u32 token, struct kvm_task_sleep_node *n)
>  {
>  	u32 key = hash_32(token, KVM_TASK_SLEEP_HASHBITS);
>  	struct kvm_task_sleep_head *b = &async_pf_sleepers[key];
>  	struct kvm_task_sleep_node *e;
>  
> +	lockdep_assert_irqs_disabled();
> +
>  	raw_spin_lock(&b->lock);
>  	e = _find_apf_task(b, token);
>  	if (e) {
> @@ -142,34 +136,31 @@ static bool kvm_async_pf_queue_task(u32 token, struct kvm_task_sleep_node *n)
>  	raw_spin_unlock(&b->lock);
>  	return true;
>  }
> +EXPORT_SYMBOL_FOR_KVM(kvm_async_pf_queue_task);
>  
>  /*
>   * kvm_async_pf_task_wait_schedule - Wait for pagefault to be handled
> - * @token:	Token to identify the sleep node entry
> + * @n:	Previously queued sleep node.
>   *
>   * Invoked from the async pagefault handling code or from the VM exit page
>   * fault handler. In both cases RCU is watching.
>   */
> -void kvm_async_pf_task_wait_schedule(u32 token)
> +void kvm_async_pf_task_wait_schedule(struct kvm_task_sleep_node *n)
>  {
> -	struct kvm_task_sleep_node n;
>  	DECLARE_SWAITQUEUE(wait);
>  
>  	lockdep_assert_irqs_disabled();
>  
> -	if (!kvm_async_pf_queue_task(token, &n))
> -		return;
> -
>  	for (;;) {
> -		prepare_to_swait_exclusive(&n.wq, &wait, TASK_UNINTERRUPTIBLE);
> -		if (hlist_unhashed(&n.link))
> +		prepare_to_swait_exclusive(&n->wq, &wait, TASK_UNINTERRUPTIBLE);
> +		if (hlist_unhashed(&n->link))
>  			break;
>  
>  		local_irq_enable();
>  		schedule();
>  		local_irq_disable();
>  	}
> -	finish_swait(&n.wq, &wait);
> +	finish_swait(&n->wq, &wait);
>  }
>  EXPORT_SYMBOL_FOR_KVM(kvm_async_pf_task_wait_schedule);

[ ... ]

> diff --git a/arch/x86/kvm/mmu.h b/arch/x86/kvm/mmu.h
> index b2a295a9f084e..3cb2616d88aca 100644
> --- a/arch/x86/kvm/mmu.h
> +++ b/arch/x86/kvm/mmu.h
> @@ -162,6 +162,20 @@ void kvm_configure_mmu(bool enable_tdp, int tdp_forced_root_level,
>  bool kvm_can_do_async_pf(struct kvm_vcpu *vcpu);
>  int kvm_handle_page_fault(struct kvm_vcpu *vcpu, u64 error_code,
>  				u64 fault_address, char *insn, int insn_len);
> +
> +static inline void kvm_handle_host_apf_irqoff(struct kvm_vcpu *vcpu, u32 token)
> +{
> +	vcpu->arch.apf.host_apf_flags = kvm_read_and_reset_apf_flags();
> +	if ((vcpu->arch.apf.host_apf_flags & KVM_PV_REASON_PAGE_NOT_PRESENT) &&
> +	    !kvm_async_pf_queue_task(token, &vcpu->arch.apf.host_apf_node)) {

[Severity: High]
Can splitting kvm_async_pf_queue_task() from kvm_async_pf_task_wait_schedule()
cause a race on vcpu->arch.apf.host_apf_node.wq if the vCPU task is preempted
and migrated between VM-exit and kvm_handle_page_fault()?

When svm_vcpu_run() or handle_exception_irqoff() calls
kvm_handle_host_apf_irqoff(), host_apf_node is linked into async_pf_sleepers
on CPU A while interrupts are disabled. Afterward, vcpu_enter_guest() enables
interrupts and preemption before calling into handle_exit() ->
kvm_handle_page_fault().

If the vCPU task is preempted in that window and migrated to CPU B, both CPUs
can access host_apf_node concurrently when the page-ready interrupt arrives on
CPU A:

CPU B (vcpu task)                      CPU A (page-ready IRQ)
-----------------                      ----------------------
kvm_handle_page_fault()
  kvm_async_pf_task_wait_schedule()
    prepare_to_swait_exclusive(&n->wq, &wait, ...)
      /* adds wait to n->wq.task_list
         and drops n->wq.lock */
                                       kvm_async_pf_task_wake()
                                         raw_spin_lock(&b->lock);
                                         apf_task_wake_one(n)
                                           hlist_del_init(&n->link);
                                           if (swq_has_sleeper(&n->wq))
    if (hlist_unhashed(&n->link))
      break;
    finish_swait(&n->wq, &wait);
                                             swake_up_one(&n->wq);

Because apf_task_wake_one() unhashes n->link before calling swake_up_one(),
and kvm_async_pf_task_wait_schedule() does not synchronize with b->lock before
returning, CPU B can observe hlist_unhashed(&n->link), remove wait via
finish_swait(), and return from kvm_async_pf_task_wait_schedule() while CPU A
is still about to execute swake_up_one(&n->wq).

Could CPU B then re-enter the guest and call kvm_async_pf_queue_task() ->
init_swait_queue_head(&n->wq) for a subsequent async #PF (or exit to userspace
and free the vCPU) while swake_up_one(&n->wq) is still running on CPU A?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103124930.2278671-1-pbonzini@redhat.com?part=1