From: David Woodhouse <[email protected]> kvm_create_plane() publishes the new plane in kvm->planes[] and takes the APICV_INHIBIT_REASON_PLANES inhibit before kvm_vm_ioctl_create_plane() can still fail at anon_inode_getfile(), and the error path freed the plane without unpublishing it: a VMM retrying plane setup after such a failure wrote overlays through the freed plane via KVM_SET_PLANE_MEMORY_OVERLAY, and VM teardown destroyed the plane a second time. The VM also kept APICv inhibited with no nonzero plane left to justify it.
Clear the kvm->planes[] slot before freeing, and drop the APICv inhibit in kvm_arch_plane_destroy() when the last nonzero plane of a live VM goes away (at VM teardown plane 0 is already gone, which skips the update). Make the overlay ioctl read kvm->planes[] under kvm->lock, which the create path holds across publish and unpublish, so a plane pointer it observes stays valid until VM teardown; publish with smp_store_release() for the remaining lock-free readers. Found by Sashiko review. Signed-off-by: David Woodhouse <[email protected]> Assisted-by: LLM --- arch/x86/kvm/x86.c | 32 ++++++++++++++++++++++++++++++++ virt/kvm/kvm_main.c | 25 ++++++++++++++++++++++--- 2 files changed, 54 insertions(+), 3 deletions(-) diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index c5122e6f42bd..afd668c6db3f 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -14003,7 +14003,39 @@ int kvm_arch_plane_init(struct kvm *kvm, struct kvm_plane *plane, void kvm_arch_plane_destroy(struct kvm_plane *plane) { + struct kvm *kvm = plane->kvm; + int i; + kvfree(rcu_dereference_check(plane->arch.apic_map, 1)); + + if (plane->level == 0) + return; + + /* + * Drop the APICv inhibit taken in kvm_arch_plane_init() when the + * last nonzero plane goes away. Only the KVM_CREATE_PLANE error + * path destroys a nonzero plane while the VM is live; at VM + * teardown plane 0 is destroyed first and kvm->planes[0] is + * already NULL, which skips the update (the inhibit machinery + * needs plane 0 and SRCU, both gone by then). + */ + if (!kvm->planes[0]) + return; + + /* + * Reaching here means a nonzero plane died while the VM is live, + * which only the KVM_CREATE_PLANE error path does - under + * kvm->lock. A future kvm_destroy_planes() reordered to tear + * nonzero planes down before plane 0 would get here locklessly; + * catch that instead of silently poking freed APICv state. + */ + lockdep_assert_held(&kvm->lock); + + for (i = 1; i < KVM_MAX_PLANES; i++) + if (kvm->planes[i] && kvm->planes[i] != plane) + return; + + kvm_clear_apicv_inhibit(kvm, APICV_INHIBIT_REASON_PLANES); } int kvm_arch_init_vm(struct kvm *kvm, unsigned long type) diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 3e021683c8a8..ae998cef3c36 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -1259,7 +1259,12 @@ static struct kvm_plane *kvm_create_plane(struct kvm *kvm, unsigned plane_level) if (kvm_arch_plane_init(kvm, plane, plane_level)) goto out_free_plane; - kvm->planes[plane_level] = plane; + /* + * Pairs with the lock-free readers of kvm->planes[] (MSI delivery, + * SEV VMPL targeting): the plane's fields must be visible before + * the pointer is. + */ + smp_store_release(&kvm->planes[plane_level], plane); return plane; @@ -5915,8 +5920,15 @@ static int kvm_vm_ioctl_create_plane(struct kvm *kvm, unsigned id) return fd; put_kvm: - kvm_put_kvm(kvm); + /* + * kvm_create_plane() published the plane; unpublish it before + * freeing (kvm->lock is held throughout, so readers who take it + * never observe the dead pointer) and so that VM teardown does + * not destroy it a second time. + */ + kvm->planes[id] = NULL; kvm_destroy_one_plane(plane); + kvm_put_kvm(kvm); put_fd: put_unused_fd(fd); return r; @@ -6037,7 +6049,14 @@ static int kvm_vm_ioctl_set_plane_overlay(struct kvm *kvm, return -EINVAL; if (ov->uaddr & ~PAGE_MASK) return -EINVAL; - plane = kvm->planes[ov->plane]; + /* + * Read under kvm->lock: KVM_CREATE_PLANE publishes and, on its + * error path, unpublishes-and-frees the plane under that lock, so + * a pointer observed here is either NULL or a plane that lives + * until VM teardown. + */ + scoped_guard(mutex, &kvm->lock) + plane = kvm->planes[ov->plane]; if (!plane) return -EINVAL; -- 2.50.1
