| Message ID | 20241009150455.1057573-1-seanjc@google.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | KVM: Fix bugs in vCPUs xarray usage | expand |
On Wed, Oct 09, 2024 at 08:04:49AM -0700, Sean Christopherson wrote: > This series stems from Will's observation[*] that kvm_vm_ioctl_create_vcpu()'s > handling of xa_store() failure when inserting into vcpu_array is technically > broken, although in practice it's impossible for xa_store() to fail. > > After much back and forth and staring, I realized that commit afb2acb2e3a3 > ("KVM: Fix vcpu_array[0] races") papered over underlying bugs in > kvm_get_vcpu() and kvm_for_each_vcpu(). The core problem is that KVM > allowed other tasks to see vCPU0 while online_vcpus==0, and thus trying > to gracefully error out of vCPU creation led to use-after-free failures. > > So, rather than trying to solve the unsolvable problem for an error path > that should be impossible to hit, fix the underlying issue and ensure that > vcpu_array[0] is accessed if and only if online_vcpus is non-zero. > > Patch 3 fixes a race Michal identified when we were trying to figure out > how to handle the xa_store() mess. > > Patch 4 reverts afb2acb2e3a3. > > Patches 5 and 6 are tangentially related cleanups. Thanks, Sean. For the series: Acked-by: Will Deacon <will@kernel.org> I sympathise a little with Paolo on patch 4, but at the end of the day it's a revert and I think that the code is better for it, even if the whole scenario is messy. Will
On Wed, 09 Oct 2024 08:04:49 -0700, Sean Christopherson wrote: > This series stems from Will's observation[*] that kvm_vm_ioctl_create_vcpu()'s > handling of xa_store() failure when inserting into vcpu_array is technically > broken, although in practice it's impossible for xa_store() to fail. > > After much back and forth and staring, I realized that commit afb2acb2e3a3 > ("KVM: Fix vcpu_array[0] races") papered over underlying bugs in > kvm_get_vcpu() and kvm_for_each_vcpu(). The core problem is that KVM > allowed other tasks to see vCPU0 while online_vcpus==0, and thus trying > to gracefully error out of vCPU creation led to use-after-free failures. > > [...] Applied to kvm-x86 vcpu_array to get coverage in -next, and to force Paolo's hand :-). Paolo, I put this in a dedicated branch so that it's easy to toss if you want to go a different direction for the xarray insertion mess. [1/6] KVM: Explicitly verify target vCPU is online in kvm_get_vcpu() https://github.com/kvm-x86/linux/commit/1e7381f3617d [2/6] KVM: Verify there's at least one online vCPU when iterating over all vCPUs https://github.com/kvm-x86/linux/commit/0664dc74e9d0 [3/6] KVM: Grab vcpu->mutex across installing the vCPU's fd and bumping online_vcpus https://github.com/kvm-x86/linux/commit/6e2b2358b3ef [4/6] Revert "KVM: Fix vcpu_array[0] races" https://github.com/kvm-x86/linux/commit/d0831edcd87e [5/6] KVM: Don't BUG() the kernel if xa_insert() fails with -EBUSY https://github.com/kvm-x86/linux/commit/e53dc37f5a06 [6/6] KVM: Drop hack that "manually" informs lockdep of kvm->lock vs. vcpu->mutex https://github.com/kvm-x86/linux/commit/01528db67f28 -- https://github.com/kvm-x86/linux/tree/next
On Thu, Dec 19, 2024 at 3:44 AM Sean Christopherson <seanjc@google.com> wrote: > > On Wed, 09 Oct 2024 08:04:49 -0700, Sean Christopherson wrote: > > This series stems from Will's observation[*] that kvm_vm_ioctl_create_vcpu()'s > > handling of xa_store() failure when inserting into vcpu_array is technically > > broken, although in practice it's impossible for xa_store() to fail. > > > > After much back and forth and staring, I realized that commit afb2acb2e3a3 > > ("KVM: Fix vcpu_array[0] races") papered over underlying bugs in > > kvm_get_vcpu() and kvm_for_each_vcpu(). The core problem is that KVM > > allowed other tasks to see vCPU0 while online_vcpus==0, and thus trying > > to gracefully error out of vCPU creation led to use-after-free failures. > > > > [...] > > Applied to kvm-x86 vcpu_array to get coverage in -next, and to force Paolo's > hand :-). > > Paolo, I put this in a dedicated branch so that it's easy to toss if you want to > go a different direction for the xarray insertion mess. Go ahead; we already do more or less the same in kvm_vm_set_mem_attributes(), so I guess that's just an unavoidable weirdness of xa_reserve(). Paolo > > [1/6] KVM: Explicitly verify target vCPU is online in kvm_get_vcpu() > https://github.com/kvm-x86/linux/commit/1e7381f3617d > [2/6] KVM: Verify there's at least one online vCPU when iterating over all vCPUs > https://github.com/kvm-x86/linux/commit/0664dc74e9d0 > [3/6] KVM: Grab vcpu->mutex across installing the vCPU's fd and bumping online_vcpus > https://github.com/kvm-x86/linux/commit/6e2b2358b3ef > [4/6] Revert "KVM: Fix vcpu_array[0] races" > https://github.com/kvm-x86/linux/commit/d0831edcd87e > [5/6] KVM: Don't BUG() the kernel if xa_insert() fails with -EBUSY > https://github.com/kvm-x86/linux/commit/e53dc37f5a06 > [6/6] KVM: Drop hack that "manually" informs lockdep of kvm->lock vs. vcpu->mutex > https://github.com/kvm-x86/linux/commit/01528db67f28 > > -- > https://github.com/kvm-x86/linux/tree/next >
This series stems from Will's observation[*] that kvm_vm_ioctl_create_vcpu()'s handling of xa_store() failure when inserting into vcpu_array is technically broken, although in practice it's impossible for xa_store() to fail. After much back and forth and staring, I realized that commit afb2acb2e3a3 ("KVM: Fix vcpu_array[0] races") papered over underlying bugs in kvm_get_vcpu() and kvm_for_each_vcpu(). The core problem is that KVM allowed other tasks to see vCPU0 while online_vcpus==0, and thus trying to gracefully error out of vCPU creation led to use-after-free failures. So, rather than trying to solve the unsolvable problem for an error path that should be impossible to hit, fix the underlying issue and ensure that vcpu_array[0] is accessed if and only if online_vcpus is non-zero. Patch 3 fixes a race Michal identified when we were trying to figure out how to handle the xa_store() mess. Patch 4 reverts afb2acb2e3a3. Patches 5 and 6 are tangentially related cleanups. [*] https://lkml.kernel.org/r/20240730155646.1687-1-will%40kernel.org Sean Christopherson (6): KVM: Explicitly verify target vCPU is online in kvm_get_vcpu() KVM: Verify there's at least one online vCPU when iterating over all vCPUs KVM: Grab vcpu->mutex across installing the vCPU's fd and bumping online_vcpus Revert "KVM: Fix vcpu_array[0] races" KVM: Don't BUG() the kernel if xa_insert() fails with -EBUSY KVM: Drop hack that "manually" informs lockdep of kvm->lock vs. vcpu->mutex include/linux/kvm_host.h | 16 ++++++++-- virt/kvm/kvm_main.c | 68 ++++++++++++++++++++++++++++++---------- 2 files changed, 65 insertions(+), 19 deletions(-) base-commit: 8cf0b93919e13d1e8d4466eb4080a4c4d9d66d7b