Hi Sean, Oliver, On 11/7/24 21:12, Oliver Upton wrote: > On Thu, Nov 07, 2024 at 11:56:32AM -0800, Sean Christopherson wrote: >> On Thu, Nov 07, 2024, Oliver Upton wrote: >>> On Thu, Nov 07, 2024 at 09:55:52AM -0800, Sean Christopherson wrote: >>>> On Thu, Nov 07, 2024, Eric Auger wrote: >>>>> In case a KVM_REQ_VM_DEAD request was sent to a VM, subsequent >>>>> KVM ioctls will fail and cause test failure. This now happens >>>>> with an aarch64 vgic test where the kvm_vm_free() fails. Let's >>>>> add a new kvm_vm_dead_free() helper that does all the deallocation >>>>> besides the KVM_SET_USER_MEMORY_REGION2 ioctl. >>>> Please no. I don't want to bleed the kvm->vm_dead behavior all over selftests. >>>> The hack in __TEST_ASSERT_VM_VCPU_IOCTL() is there purely to provide users with >>>> a more helpful error message, it is most definitely not intended to be an "official" >>>> way to detect and react to the VM being dead. >>>> >>>> IMO, tests that intentionally result in a dead VM should assert that subsequent >>>> VM/vCPU ioctls return -EIO, and that's all. Attempting to gracefully free >>>> resources adds complexity and pollutes the core selftests APIs, with very little >>>> benefit. >>> Encouraging tests to explicitly leak resources to fudge around assertions >>> in the selftests library seems off to me. >> I don't disagree, but I really, really don't want to add vm_dead(). > It'd still be valuable to test that the VM is properly dead and > subsequent ioctls also return EIO, but I understand the hesitation. > >>> IMO, the better approach would be to provide a helper that gives the >>> impression of freeing the VM but implicitly leaks it, paired with some >>> reasoning for it. >> Actually, duh. There's no need to manually delete KVM memslots for *any* VM, >> dead or alive. Just skip that unconditionally when freeing the VM, and then the >> vGIC test just needs to assert on -EIO instead -ENXIO/-EBUSY. > Yeah, that'd tighten up the assertions a bit more to the exact ioctl > where we expect the VM to go sideways. > >> --- >> From: Sean Christopherson <seanjc@xxxxxxxxxx> >> Date: Thu, 7 Nov 2024 11:39:59 -0800 >> Subject: [PATCH] KVM: selftests: Don't bother deleting memslots in KVM when >> freeing VMs >> >> When freeing a VM, don't call into KVM to manually remove each memslot, >> simply cleanup and free any userspace assets associated with the memory >> region. KVM is ultimately responsible for ensuring kernel resources are >> freed when the VM is destroyed, deleting memslots one-by-one is >> unnecessarily slow, and unless a test is already leaking the VM fd, the >> VM will be destroyed when kvm_vm_release() is called. >> >> Not deleting KVM's memslot also allows cleaning up dead VMs without having >> to care whether or not the to-be-freed VM is dead or alive. > Can you add a comment to kvm_vm_free() about why we want to avoid ioctls > in that helper? It'd help discourage this situation from happening again > in the future in the unlikely case someone wants to park an ioctl there. > >> Reported-by: Eric Auger <eric.auger@xxxxxxxxxx> >> Reported-by: Mark Brown <broonie@xxxxxxxxxx> >> Signed-off-by: Sean Christopherson <seanjc@xxxxxxxxxx> > I'm assuming you want to take this, happy to grab it otherwise. > > Reviewed-by: Oliver Upton <oliver.upton@xxxxxxxxx> Reviewed-by: Eric Auger <eric.auger@xxxxxxxxxx> Tested-by: Eric Auger <eric.auger@xxxxxxxxxx> Thanks Eric > >>>> Marking a VM dead should be a _very_ rare event; it's not something that I think >>>> we should encourage, i.e. we shouldn't make it easier to deal with. Ideally, >>>> use of kvm_vm_dead() should be limited to things like sev_vm_move_enc_context_from(), >>>> where KVM needs to prever accessing the source VM to protect the host. IMO, the >>>> vGIC case and x86's enter_smm() are hacks. E.g. I don't see any reason why the >>>> enter_smm() case can't synthesize a triple fault. >>> The VGIC case is at least better than the alternative of slapping >>> bandaids all over the shop to cope with a half-baked VM and ensure we >>> tear it down correctly. Userspace is far up shit creek at the point the >>> VM is marked as dead, so I don't see any value in hobbling along >>> afterwards. >> Again, I don't disagree, but I don't want to normalize shooting the VM on errors. > Definitely not. It is very much a break-glass situation where this is > even somewhat OK. >
