| Message ID | 20250224235542.2562848-4-seanjc@google.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | KVM: x86: nVMX IRQ fix and VM teardown cleanups | expand |
| Context | Check | Description |
|---|---|---|
| bjorn/pre-ci_am | success | Success |
| bjorn/build-rv32-defconfig | success | build-rv32-defconfig |
| bjorn/build-rv64-clang-allmodconfig | success | build-rv64-clang-allmodconfig |
| bjorn/build-rv64-gcc-allmodconfig | success | build-rv64-gcc-allmodconfig |
| bjorn/build-rv64-nommu-k210-defconfig | success | build-rv64-nommu-k210-defconfig |
| bjorn/build-rv64-nommu-k210-virt | success | build-rv64-nommu-k210-virt |
| bjorn/checkpatch | success | checkpatch |
| bjorn/dtb-warn-rv64 | success | dtb-warn-rv64 |
| bjorn/header-inline | success | header-inline |
| bjorn/kdoc | success | kdoc |
| bjorn/module-param | success | module-param |
| bjorn/verify-fixes | success | verify-fixes |
| bjorn/verify-signedoff | success | verify-signedoff |
On Mon, Feb 24, 2025 at 03:55:38PM -0800, Sean Christopherson wrote: > After freeing a vCPU, assert that it is no longer reachable, and that > kvm_get_vcpu() doesn't return garbage or a pointer to some other vCPU. > While KVM obviously shouldn't be attempting to access a freed vCPU, it's > all too easy for KVM to make a VM-wide request, e.g. via KVM_BUG_ON() or > kvm_flush_remote_tlbs(). > > Alternatively, KVM could short-circuit problematic paths if the VM's > refcount has gone to zero, e.g. in kvm_make_all_cpus_request(), or KVM > could try disallow making global requests during teardown. But given that > deleting the vCPU from the array Just Works, adding logic to the requests > path is unnecessary, and trying to make requests illegal during teardown > would be a fool's errand. > > Signed-off-by: Sean Christopherson <seanjc@google.com> > --- > virt/kvm/kvm_main.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index 201c14ff476f..991e8111e88b 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -489,6 +489,14 @@ void kvm_destroy_vcpus(struct kvm *kvm) > kvm_for_each_vcpu(i, vcpu, kvm) { > kvm_vcpu_destroy(vcpu); > xa_erase(&kvm->vcpu_array, i); > + > + /* > + * Assert that the vCPU isn't visible in any way, to ensure KVM > + * doesn't trigger a use-after-free if destroying vCPUs results > + * in VM-wide request, e.g. to flush remote TLBs when tearing > + * down MMUs, or to mark the VM dead if a KVM_BUG_ON() fires. > + */ > + WARN_ON_ONCE(xa_load(&kvm->vcpu_array, i) || kvm_get_vcpu(kvm, i)); As xa_erase() says "After this function returns, loading from @index will return %NULL", is this checking of xa_load() necessary? > } > > atomic_set(&kvm->online_vcpus, 0); > -- > 2.48.1.658.g4767266eb4-goog >
On Tue, Feb 25, 2025, Yan Zhao wrote: > On Mon, Feb 24, 2025 at 03:55:38PM -0800, Sean Christopherson wrote: > > After freeing a vCPU, assert that it is no longer reachable, and that > > kvm_get_vcpu() doesn't return garbage or a pointer to some other vCPU. > > While KVM obviously shouldn't be attempting to access a freed vCPU, it's > > all too easy for KVM to make a VM-wide request, e.g. via KVM_BUG_ON() or > > kvm_flush_remote_tlbs(). > > > > Alternatively, KVM could short-circuit problematic paths if the VM's > > refcount has gone to zero, e.g. in kvm_make_all_cpus_request(), or KVM > > could try disallow making global requests during teardown. But given that > > deleting the vCPU from the array Just Works, adding logic to the requests > > path is unnecessary, and trying to make requests illegal during teardown > > would be a fool's errand. > > > > Signed-off-by: Sean Christopherson <seanjc@google.com> > > --- > > virt/kvm/kvm_main.c | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > > index 201c14ff476f..991e8111e88b 100644 > > --- a/virt/kvm/kvm_main.c > > +++ b/virt/kvm/kvm_main.c > > @@ -489,6 +489,14 @@ void kvm_destroy_vcpus(struct kvm *kvm) > > kvm_for_each_vcpu(i, vcpu, kvm) { > > kvm_vcpu_destroy(vcpu); > > xa_erase(&kvm->vcpu_array, i); > > + > > + /* > > + * Assert that the vCPU isn't visible in any way, to ensure KVM > > + * doesn't trigger a use-after-free if destroying vCPUs results > > + * in VM-wide request, e.g. to flush remote TLBs when tearing > > + * down MMUs, or to mark the VM dead if a KVM_BUG_ON() fires. > > + */ > > + WARN_ON_ONCE(xa_load(&kvm->vcpu_array, i) || kvm_get_vcpu(kvm, i)); > As xa_erase() says "After this function returns, loading from @index will return > %NULL", is this checking of xa_load() necessary? None of this is "necessary". My goal with the assert is to (a) document that KVM relies the vCPU to be NULL/unreachable and (b) to help ensure that doesn't change in the future. Checking xa_load() is mostly about (a). That said, I agree checking xa_load() is more than a bit gratuitous. I have no objection to checking only kvm_get_vcpu().
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 201c14ff476f..991e8111e88b 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -489,6 +489,14 @@ void kvm_destroy_vcpus(struct kvm *kvm) kvm_for_each_vcpu(i, vcpu, kvm) { kvm_vcpu_destroy(vcpu); xa_erase(&kvm->vcpu_array, i); + + /* + * Assert that the vCPU isn't visible in any way, to ensure KVM + * doesn't trigger a use-after-free if destroying vCPUs results + * in VM-wide request, e.g. to flush remote TLBs when tearing + * down MMUs, or to mark the VM dead if a KVM_BUG_ON() fires. + */ + WARN_ON_ONCE(xa_load(&kvm->vcpu_array, i) || kvm_get_vcpu(kvm, i)); } atomic_set(&kvm->online_vcpus, 0);
After freeing a vCPU, assert that it is no longer reachable, and that kvm_get_vcpu() doesn't return garbage or a pointer to some other vCPU. While KVM obviously shouldn't be attempting to access a freed vCPU, it's all too easy for KVM to make a VM-wide request, e.g. via KVM_BUG_ON() or kvm_flush_remote_tlbs(). Alternatively, KVM could short-circuit problematic paths if the VM's refcount has gone to zero, e.g. in kvm_make_all_cpus_request(), or KVM could try disallow making global requests during teardown. But given that deleting the vCPU from the array Just Works, adding logic to the requests path is unnecessary, and trying to make requests illegal during teardown would be a fool's errand. Signed-off-by: Sean Christopherson <seanjc@google.com> --- virt/kvm/kvm_main.c | 8 ++++++++ 1 file changed, 8 insertions(+)