Re: [PATCH 2/2] KVM: selftests: Extend x86's sync_regs_test to check for races

[Sean Christopherson <seanjc@google.com>]

On Thu, Aug 03, 2023, Michal Luczaj wrote:
> On 8/2/23 23:07, Sean Christopherson wrote:
> > On Fri, Jul 28, 2023, Michal Luczaj wrote:
> >> +	/*
> >> +	 * If kvm->bugged then we won't survive TEST_ASSERT(). Leak.
> >> +	 *
> >> +	 * kvm_vm_free()
> >> +	 *   __vm_mem_region_delete()
> >> +	 *     vm_ioctl(vm, KVM_SET_USER_MEMORY_REGION, &region->region)
> >> +	 *       _vm_ioctl(vm, cmd, #cmd, arg)
> >> +	 *         TEST_ASSERT(!ret, __KVM_IOCTL_ERROR(name, ret))
> >> +	 */
> > 
> > We want the assert, it makes failures explicit.  The signature is a bit unfortunate,
> > but the WARN in the kernel log should provide a big clue.
> 
> Sure, I get it. And not that there is a way to check if VM is bugged/dead?

KVM doesn't expost the bugged/dead information, though I suppose userspace could
probe that information by doing an ioctl() that is guaranteed to succeeed and
looking for -EIO, e.g. KVM_CHECK_EXTENSION on the VM.

I was going to say that it's not worth trying to detect a bugged/dead VM in
selftests, because it requires having the pointer to the VM, and that's not
typically available when an assert fails, but the obviously solution is to tap
into the VM and vCPU ioctl() helpers.  That's also good motivation to add helpers
and consolidate asserts for ioctls() that return fds, i.e. for which a positive
return is considered success.

With the below (partial conversion), the failing testcase yields this.  Using a
heuristic isn't ideal, but practically speaking I can't see a way for the -EIO
check to go awry, and anything to make debugging errors easier is definitely worth
doing IMO.

==== Test Assertion Failure ====
  lib/kvm_util.c:689: false
  pid=80347 tid=80347 errno=5 - Input/output error
     1	0x00000000004039ab: __vm_mem_region_delete at kvm_util.c:689 (discriminator 5)
     2	0x0000000000404660: kvm_vm_free at kvm_util.c:724 (discriminator 12)
     3	0x0000000000402ac9: race_sync_regs at sync_regs_test.c:193
     4	0x0000000000401cb7: main at sync_regs_test.c:334 (discriminator 6)
     5	0x0000000000418263: __libc_start_call_main at libc-start.o:?
     6	0x00000000004198af: __libc_start_main_impl at ??:?
     7	0x0000000000401d90: _start at ??:?
  KVM killed/bugged the VM, check kernel log for clues


diff --git a/tools/testing/selftests/kvm/include/kvm_util_base.h b/tools/testing/selftests/kvm/include/kvm_util_base.h
index 07732a157ccd..e48ac57be13a 100644
--- a/tools/testing/selftests/kvm/include/kvm_util_base.h
+++ b/tools/testing/selftests/kvm/include/kvm_util_base.h
@@ -258,17 +258,42 @@ static __always_inline void static_assert_is_vm(struct kvm_vm *vm) { }
        kvm_do_ioctl((vm)->fd, cmd, arg);                       \
 })
 
+/*
+ * Assert that a VM or vCPU ioctl() succeeded (obviously), with extra magic to
+ * detect if the ioctl() failed because KVM killed/bugged the VM.  To detect a
+ * dead VM, probe KVM_CAP_USER_MEMORY, which (a) has been supported by KVM
+ * since before selftests existed and (b) should never outright fail, i.e. is
+ * supposed to return 0 or 1.  If KVM kills a VM, KVM returns -EIO for all
+ * ioctl()s for the VM and its vCPUs, including KVM_CHECK_EXTENSION.
+ */
+#define TEST_ASSERT_VM_VCPU_IOCTL_SUCCESS(name, ret, vm)                               \
+do {                                                                                   \
+       int __errno = errno;                                                            \
+                                                                                       \
+       static_assert_is_vm(vm);                                                        \
+                                                                                       \
+       if (!ret)                                                                       \
+               break;                                                                  \
+                                                                                       \
+       if (errno == EIO &&                                                             \
+           __vm_ioctl(vm, KVM_CHECK_EXTENSION, (void *)KVM_CAP_USER_MEMORY) < 0) {     \
+               TEST_ASSERT(errno == EIO, "KVM killed the VM, should return -EIO");     \
+               TEST_FAIL("KVM killed/bugged the VM, check kernel log for clues");      \
+       }                                                                               \
+       errno = __errno;                                                                \
+       TEST_FAIL(__KVM_IOCTL_ERROR(name, ret));                                        \
+} while (0)
+
 #define _vm_ioctl(vm, cmd, name, arg)                          \
 ({                                                             \
        int ret = __vm_ioctl(vm, cmd, arg);                     \
                                                                \
-       TEST_ASSERT(!ret, __KVM_IOCTL_ERROR(name, ret));        \
+       TEST_ASSERT_VM_VCPU_IOCTL_SUCCESS(name, ret, vm);       \
 })
 
 #define vm_ioctl(vm, cmd, arg)                                 \
        _vm_ioctl(vm, cmd, #cmd, arg)
 
-
 static __always_inline void static_assert_is_vcpu(struct kvm_vcpu *vcpu) { }
 
 #define __vcpu_ioctl(vcpu, cmd, arg)                           \
@@ -281,7 +306,7 @@ static __always_inline void static_assert_is_vcpu(struct kvm_vcpu *vcpu) { }
 ({                                                             \
        int ret = __vcpu_ioctl(vcpu, cmd, arg);                 \
                                                                \
-       TEST_ASSERT(!ret, __KVM_IOCTL_ERROR(name, ret));        \
+       TEST_ASSERT_VM_VCPU_IOCTL_SUCCESS(name, ret, vcpu->vm); \
 })
 
 #define vcpu_ioctl(vcpu, cmd, arg)                             \