Message ID | 20231018204624.1905300-3-seanjc@google.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a05:612c:2908:b0:403:3b70:6f57 with SMTP id ib8csp5057814vqb; Wed, 18 Oct 2023 13:47:06 -0700 (PDT) X-Google-Smtp-Source: AGHT+IFK5UQqp8qm592LfSpQVGecmezslpT5STLURti1lqh4l2RsyWHI1dOdlgX5e65ANeYUT1JU X-Received: by 2002:a05:6830:718d:b0:6b9:ed64:1423 with SMTP id el13-20020a056830718d00b006b9ed641423mr467681otb.2.1697662026010; Wed, 18 Oct 2023 13:47:06 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1697662025; cv=none; d=google.com; s=arc-20160816; b=h189IUNlDrD6bTtNDoddH2nvWyY61u95C7n9x8mF/qWFSeN2/11rcFabzl0Nk4eKsL 6hcLrxU1xpoe7Phhb2xQYfNhRCgiZQ73Qn3mAOEi88ahqwuWjof91jW5kFKqYWHC1EX4 VJeADLbC/9nTYu1Ua8kDdHi/OdtKhZf7caB5SQzxxnE5f986r+sWLq4J59Obu7YngeVT 4pnqUyn4DzvHWokeGZBpOp37S2rwYthN23kkBoOZPhBo86fc7XovQmPsVlpLrbFk1w7P 9z02gLdiL7XB0QIp4amLMtzUcURK1D5DaH4dHFJGt0Ivy8te5KdV4nlXagYWZgtbKPGJ 2X+g== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:cc:to:from:subject:message-id:references :mime-version:in-reply-to:date:reply-to:dkim-signature; bh=w+Tq8ZbDqZnNACP1ZGd4eGNbYVTAqWG4yMn2szlYYbk=; fh=S++QvovuovGo8dWglP3zfPiw3tBjpuKi+q/HqjmYx30=; b=e6CfV5znMAyBlVr8vXxf2UxmdJwBaBOikkwinF3q5C/3HC/Zuep5fQfxwamwgtQ6T+ 6K6c8tXA+7a9U6728rAJxp7JHcYl4pbSqjJlUdiqnxQBq4gzeJ+IG9NBh66AJy8Ffzit +P08T+PvdJsg3o0Sm+SofuTJmBvZsmgLsu+/Cl8ZbDMwqIFCi5SeUvxF5JpBVCcTajhg uJyCbW3xdkDVFV2Op7183Tkz0OhwwR0z/jsONJF1j9tgYmlOe6vVxlRcd5Q3qya6+hwB J/XQByLzzL2fMCR9UOeAUF4Z52+RuAw4yKgpFmexFtMxpFylreAAwd2vLVdrFAlmcrJQ 3dVw== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@google.com header.s=20230601 header.b=T98kk+jN; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:1 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=google.com Received: from morse.vger.email (morse.vger.email. [2620:137:e000::3:1]) by mx.google.com with ESMTPS id bk13-20020a056a02028d00b005a9f776c59csi3005698pgb.468.2023.10.18.13.47.05 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 18 Oct 2023 13:47:05 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:1 as permitted sender) client-ip=2620:137:e000::3:1; Authentication-Results: mx.google.com; dkim=pass header.i=@google.com header.s=20230601 header.b=T98kk+jN; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:1 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=google.com Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by morse.vger.email (Postfix) with ESMTP id EC17A823F5F9; Wed, 18 Oct 2023 13:47:01 -0700 (PDT) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.10 at morse.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232387AbjJRUqn (ORCPT <rfc822;zwp10758@gmail.com> + 24 others); Wed, 18 Oct 2023 16:46:43 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:51042 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232149AbjJRUqh (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Wed, 18 Oct 2023 16:46:37 -0400 Received: from mail-yw1-x114a.google.com (mail-yw1-x114a.google.com [IPv6:2607:f8b0:4864:20::114a]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id C2467A4 for <linux-kernel@vger.kernel.org>; Wed, 18 Oct 2023 13:46:35 -0700 (PDT) Received: by mail-yw1-x114a.google.com with SMTP id 00721157ae682-5a7af53bde4so117957207b3.0 for <linux-kernel@vger.kernel.org>; Wed, 18 Oct 2023 13:46:35 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1697661995; x=1698266795; darn=vger.kernel.org; h=cc:to:from:subject:message-id:references:mime-version:in-reply-to :date:reply-to:from:to:cc:subject:date:message-id:reply-to; bh=w+Tq8ZbDqZnNACP1ZGd4eGNbYVTAqWG4yMn2szlYYbk=; b=T98kk+jNDwt/ZiuSK4thE7SNRmSmHKpNUjbVFTUOUn1zp7roA0T/GxYtAivo2NfBRp iSok0N2azlFKcVN4CBiS6e/ST5N5Qfbc2K6a9737JC+ra4WSrvNb74LU20wMm4A8emdm uPM8dmBoUr9ZFKz4C9oiTP9DIR01oYLTL6QGFfMMIwG3CFyGBUYGU3CGT0y6L05aA1ay 0OikiUefL/VdGER5iJKu+15WKaK6Xu6B6Nm+qkr/7lJETspWuQDyLSsoXwdkBcqSkpWA 5jQ52I8t7dt9ka/SHKX7tCKAOfhYpCtpkPL1qrqY/B10qsAAQIAp0wSbn4jKeWl/80nC rcvg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1697661995; x=1698266795; h=cc:to:from:subject:message-id:references:mime-version:in-reply-to :date:reply-to:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=w+Tq8ZbDqZnNACP1ZGd4eGNbYVTAqWG4yMn2szlYYbk=; b=AdbN3jfaN1un/qGJl2BUb9+YdsMmhxQ69mz3Z7Vk1894uxDhUmXydyo82hczVOgp5f BCznRIaCDno+GIIPNQrZps0/zAqzv+FLbl2Cn7uKLZKIVC/BDyv/V/84SrI4//y1zSmR UlHRMHfaH4jXU+O31M2p2DL2zTGO9OC2Y/aY+AWlZhFEJqmykOxaMdJhTkE1tGjVAVhR YWwsPl4w1PJtiIqwPsT1FsQ/d2E+OQdPy9Lax59cBYoixJszF9GIbUiF+0RzlH/n1/wg e9+uu4J/AAudvyJUu5lfL4oSTqk0gh7lzw96KOgGQRiiqsgzv8+R5YuBL1S1r26LT1c/ oO+Q== X-Gm-Message-State: AOJu0YwAJC2/4qBmvY+uo/caJ6w+JVjKp7xO8KKvP9v6snNP27X8RWp4 Y4JF/BqNZ9oqS4pYwSNlTGO+NuACurA= X-Received: from zagreus.c.googlers.com ([fda3:e722:ac3:cc00:7f:e700:c0a8:5c37]) (user=seanjc job=sendgmr) by 2002:a0d:cb89:0:b0:5a7:7683:995d with SMTP id n131-20020a0dcb89000000b005a77683995dmr11570ywd.5.1697661995065; Wed, 18 Oct 2023 13:46:35 -0700 (PDT) Reply-To: Sean Christopherson <seanjc@google.com> Date: Wed, 18 Oct 2023 13:46:23 -0700 In-Reply-To: <20231018204624.1905300-1-seanjc@google.com> Mime-Version: 1.0 References: <20231018204624.1905300-1-seanjc@google.com> X-Mailer: git-send-email 2.42.0.655.g421f12c284-goog Message-ID: <20231018204624.1905300-3-seanjc@google.com> Subject: [PATCH 2/3] KVM: Always flush async #PF workqueue when vCPU is being destroyed From: Sean Christopherson <seanjc@google.com> To: Sean Christopherson <seanjc@google.com>, Paolo Bonzini <pbonzini@redhat.com> Cc: kvm@vger.kernel.org, linux-kernel@vger.kernel.org, Al Viro <viro@zeniv.linux.org.uk>, David Matlack <dmatlack@google.com> Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-8.4 required=5.0 tests=DKIMWL_WL_MED,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,USER_IN_DEF_DKIM_WL autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on morse.vger.email Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (morse.vger.email [0.0.0.0]); Wed, 18 Oct 2023 13:47:02 -0700 (PDT) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1780127656306434455 X-GMAIL-MSGID: 1780127656306434455 |
Series |
KVM: Fix KVM-owned file refcounting of KVM module(s)
|
|
Commit Message
Sean Christopherson
Oct. 18, 2023, 8:46 p.m. UTC
Always flush the per-vCPU async #PF workqueue when a vCPU is clearing its
completion queue, i.e. when a VM and all its vCPUs is being destroyed.
KVM must ensure that none of its workqueue callbacks is running when the
last reference to the KVM _module_ is put. Gifting a reference to the
associated VM prevents the workqueue callback from dereferencing freed
vCPU/VM memory, but does not prevent the KVM module from being unloaded
before the callback completes.
Drop the misguided VM refcount gifting, as calling kvm_put_kvm() from
async_pf_execute() if kvm_put_kvm() flushes the async #PF workqueue will
result in deadlock. async_pf_execute() can't return until kvm_put_kvm()
finishes, and kvm_put_kvm() can't return until async_pf_execute() finishes:
WARNING: CPU: 8 PID: 251 at virt/kvm/kvm_main.c:1435 kvm_put_kvm+0x2d/0x320 [kvm]
Modules linked in: vhost_net vhost vhost_iotlb tap kvm_intel kvm irqbypass
CPU: 8 PID: 251 Comm: kworker/8:1 Tainted: G W 6.6.0-rc1-e7af8d17224a-x86/gmem-vm #119
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 0.0.0 02/06/2015
Workqueue: events async_pf_execute [kvm]
RIP: 0010:kvm_put_kvm+0x2d/0x320 [kvm]
Call Trace:
<TASK>
async_pf_execute+0x198/0x260 [kvm]
process_one_work+0x145/0x2d0
worker_thread+0x27e/0x3a0
kthread+0xba/0xe0
ret_from_fork+0x2d/0x50
ret_from_fork_asm+0x11/0x20
</TASK>
---[ end trace 0000000000000000 ]---
INFO: task kworker/8:1:251 blocked for more than 120 seconds.
Tainted: G W 6.6.0-rc1-e7af8d17224a-x86/gmem-vm #119
"echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
task:kworker/8:1 state:D stack:0 pid:251 ppid:2 flags:0x00004000
Workqueue: events async_pf_execute [kvm]
Call Trace:
<TASK>
__schedule+0x33f/0xa40
schedule+0x53/0xc0
schedule_timeout+0x12a/0x140
__wait_for_common+0x8d/0x1d0
__flush_work.isra.0+0x19f/0x2c0
kvm_clear_async_pf_completion_queue+0x129/0x190 [kvm]
kvm_arch_destroy_vm+0x78/0x1b0 [kvm]
kvm_put_kvm+0x1c1/0x320 [kvm]
async_pf_execute+0x198/0x260 [kvm]
process_one_work+0x145/0x2d0
worker_thread+0x27e/0x3a0
kthread+0xba/0xe0
ret_from_fork+0x2d/0x50
ret_from_fork_asm+0x11/0x20
</TASK>
If kvm_clear_async_pf_completion_queue() actually flushes the workqueue,
then there's no need to gift async_pf_execute() a reference because all
invocations of async_pf_execute() will be forced to complete before the
vCPU and its VM are destroyed/freed. And that in turn fixes the module
unloading bug as __fput() won't do module_put() on the last vCPU reference
until the vCPU has been freed, e.g. if closing the vCPU file also puts the
last reference to the KVM module.
Note, commit 5f6de5cbebee ("KVM: Prevent module exit until all VMs are
freed") *tried* to fix the module refcounting issue by having VMs grab a
reference to the module, but that only made the bug slightly harder to hit
as it gave async_pf_execute() a bit more time to complete before the KVM
module could be unloaded.
Fixes: af585b921e5d ("KVM: Halt vcpu if page it tries to access is swapped out")
Cc: stable@vger.kernel.org
Cc: David Matlack <dmatlack@google.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
virt/kvm/async_pf.c | 15 ++++++++++++---
1 file changed, 12 insertions(+), 3 deletions(-)
Comments
On 2023-10-18 01:46 PM, Sean Christopherson wrote: > Always flush the per-vCPU async #PF workqueue when a vCPU is clearing its > completion queue, i.e. when a VM and all its vCPUs is being destroyed. nit: ... or when the guest toggles CR0.PG or async_pf support. > KVM must ensure that none of its workqueue callbacks is running when the > last reference to the KVM _module_ is put. Gifting a reference to the > associated VM prevents the workqueue callback from dereferencing freed > vCPU/VM memory, but does not prevent the KVM module from being unloaded > before the callback completes. > > Drop the misguided VM refcount gifting, as calling kvm_put_kvm() from > async_pf_execute() if kvm_put_kvm() flushes the async #PF workqueue will > result in deadlock. async_pf_execute() can't return until kvm_put_kvm() > finishes, and kvm_put_kvm() can't return until async_pf_execute() finishes: > [...] > > Note, commit 5f6de5cbebee ("KVM: Prevent module exit until all VMs are > freed") *tried* to fix the module refcounting issue by having VMs grab a > reference to the module, but that only made the bug slightly harder to hit > as it gave async_pf_execute() a bit more time to complete before the KVM > module could be unloaded. Blegh! Thanks for the fix. > > Fixes: af585b921e5d ("KVM: Halt vcpu if page it tries to access is swapped out") > Cc: stable@vger.kernel.org > Cc: David Matlack <dmatlack@google.com> > Signed-off-by: Sean Christopherson <seanjc@google.com> Reviewed-by: David Matlack <dmatlack@google.com>
On Wed, Oct 18, 2023 at 01:46:23PM -0700, Sean Christopherson wrote: > Always flush the per-vCPU async #PF workqueue when a vCPU is clearing its > completion queue, i.e. when a VM and all its vCPUs is being destroyed. > KVM must ensure that none of its workqueue callbacks is running when the > last reference to the KVM _module_ is put. Gifting a reference to the > associated VM prevents the workqueue callback from dereferencing freed > vCPU/VM memory, but does not prevent the KVM module from being unloaded > before the callback completes. > > Drop the misguided VM refcount gifting, as calling kvm_put_kvm() from > async_pf_execute() if kvm_put_kvm() flushes the async #PF workqueue will > result in deadlock. async_pf_execute() can't return until kvm_put_kvm() > finishes, and kvm_put_kvm() can't return until async_pf_execute() finishes: > > WARNING: CPU: 8 PID: 251 at virt/kvm/kvm_main.c:1435 kvm_put_kvm+0x2d/0x320 [kvm] > Modules linked in: vhost_net vhost vhost_iotlb tap kvm_intel kvm irqbypass > CPU: 8 PID: 251 Comm: kworker/8:1 Tainted: G W 6.6.0-rc1-e7af8d17224a-x86/gmem-vm #119 > Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 0.0.0 02/06/2015 > Workqueue: events async_pf_execute [kvm] > RIP: 0010:kvm_put_kvm+0x2d/0x320 [kvm] > Call Trace: > <TASK> > async_pf_execute+0x198/0x260 [kvm] > process_one_work+0x145/0x2d0 > worker_thread+0x27e/0x3a0 > kthread+0xba/0xe0 > ret_from_fork+0x2d/0x50 > ret_from_fork_asm+0x11/0x20 > </TASK> > ---[ end trace 0000000000000000 ]--- > INFO: task kworker/8:1:251 blocked for more than 120 seconds. > Tainted: G W 6.6.0-rc1-e7af8d17224a-x86/gmem-vm #119 > "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message. > task:kworker/8:1 state:D stack:0 pid:251 ppid:2 flags:0x00004000 > Workqueue: events async_pf_execute [kvm] > Call Trace: > <TASK> > __schedule+0x33f/0xa40 > schedule+0x53/0xc0 > schedule_timeout+0x12a/0x140 > __wait_for_common+0x8d/0x1d0 > __flush_work.isra.0+0x19f/0x2c0 > kvm_clear_async_pf_completion_queue+0x129/0x190 [kvm] > kvm_arch_destroy_vm+0x78/0x1b0 [kvm] > kvm_put_kvm+0x1c1/0x320 [kvm] > async_pf_execute+0x198/0x260 [kvm] > process_one_work+0x145/0x2d0 > worker_thread+0x27e/0x3a0 > kthread+0xba/0xe0 > ret_from_fork+0x2d/0x50 > ret_from_fork_asm+0x11/0x20 > </TASK> > > If kvm_clear_async_pf_completion_queue() actually flushes the workqueue, > then there's no need to gift async_pf_execute() a reference because all > invocations of async_pf_execute() will be forced to complete before the > vCPU and its VM are destroyed/freed. And that in turn fixes the module > unloading bug as __fput() won't do module_put() on the last vCPU reference > until the vCPU has been freed, e.g. if closing the vCPU file also puts the > last reference to the KVM module. > > Note, commit 5f6de5cbebee ("KVM: Prevent module exit until all VMs are > freed") *tried* to fix the module refcounting issue by having VMs grab a > reference to the module, but that only made the bug slightly harder to hit > as it gave async_pf_execute() a bit more time to complete before the KVM > module could be unloaded. > > Fixes: af585b921e5d ("KVM: Halt vcpu if page it tries to access is swapped out") > Cc: stable@vger.kernel.org > Cc: David Matlack <dmatlack@google.com> > Signed-off-by: Sean Christopherson <seanjc@google.com> > --- > virt/kvm/async_pf.c | 15 ++++++++++++--- > 1 file changed, 12 insertions(+), 3 deletions(-) > > diff --git a/virt/kvm/async_pf.c b/virt/kvm/async_pf.c > index e033c79d528e..7aeb9d1f43b1 100644 > --- a/virt/kvm/async_pf.c > +++ b/virt/kvm/async_pf.c > @@ -87,7 +87,6 @@ static void async_pf_execute(struct work_struct *work) > __kvm_vcpu_wake_up(vcpu); > > mmput(mm); > - kvm_put_kvm(vcpu->kvm); > } > > void kvm_clear_async_pf_completion_queue(struct kvm_vcpu *vcpu) > @@ -114,7 +113,6 @@ void kvm_clear_async_pf_completion_queue(struct kvm_vcpu *vcpu) > #else > if (cancel_work_sync(&work->work)) { > mmput(work->mm); > - kvm_put_kvm(vcpu->kvm); /* == work->vcpu->kvm */ > kmem_cache_free(async_pf_cache, work); > } > #endif > @@ -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... Thanks, Yilun > kmem_cache_free(async_pf_cache, work); > + spin_lock(&vcpu->async_pf.lock); > } > spin_unlock(&vcpu->async_pf.lock); > > @@ -186,7 +196,6 @@ bool kvm_setup_async_pf(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa, > work->arch = *arch; > work->mm = current->mm; > mmget(work->mm); > - kvm_get_kvm(work->vcpu->kvm); > > INIT_WORK(&work->work, async_pf_execute); > > -- > 2.42.0.655.g421f12c284-goog >
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. 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.
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.
diff --git a/virt/kvm/async_pf.c b/virt/kvm/async_pf.c index e033c79d528e..7aeb9d1f43b1 100644 --- a/virt/kvm/async_pf.c +++ b/virt/kvm/async_pf.c @@ -87,7 +87,6 @@ static void async_pf_execute(struct work_struct *work) __kvm_vcpu_wake_up(vcpu); mmput(mm); - kvm_put_kvm(vcpu->kvm); } void kvm_clear_async_pf_completion_queue(struct kvm_vcpu *vcpu) @@ -114,7 +113,6 @@ void kvm_clear_async_pf_completion_queue(struct kvm_vcpu *vcpu) #else if (cancel_work_sync(&work->work)) { mmput(work->mm); - kvm_put_kvm(vcpu->kvm); /* == work->vcpu->kvm */ kmem_cache_free(async_pf_cache, work); } #endif @@ -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); kmem_cache_free(async_pf_cache, work); + spin_lock(&vcpu->async_pf.lock); } spin_unlock(&vcpu->async_pf.lock); @@ -186,7 +196,6 @@ bool kvm_setup_async_pf(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa, work->arch = *arch; work->mm = current->mm; mmget(work->mm); - kvm_get_kvm(work->vcpu->kvm); INIT_WORK(&work->work, async_pf_execute);