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

[Sean Christopherson <seanjc@google.com>]

On Fri, Jul 28, 2023, Michal Luczaj wrote:
> Attempt to modify vcpu->run->s.regs _after_ the sanity checks performed by
> KVM_CAP_SYNC_REGS's arch/x86/kvm/x86.c:sync_regs(). This could lead to some
> nonsensical vCPU states accompanied by kernel splats.
> 
> Signed-off-by: Michal Luczaj <mhal@rbox.co>
> ---
>  .../selftests/kvm/x86_64/sync_regs_test.c     | 124 ++++++++++++++++++
>  1 file changed, 124 insertions(+)
> 
> diff --git a/tools/testing/selftests/kvm/x86_64/sync_regs_test.c b/tools/testing/selftests/kvm/x86_64/sync_regs_test.c
> index 2da89fdc2471..feebc7d44c17 100644
> --- a/tools/testing/selftests/kvm/x86_64/sync_regs_test.c
> +++ b/tools/testing/selftests/kvm/x86_64/sync_regs_test.c
> @@ -15,12 +15,14 @@
>  #include <stdlib.h>
>  #include <string.h>
>  #include <sys/ioctl.h>
> +#include <pthread.h>
>  
>  #include "test_util.h"
>  #include "kvm_util.h"
>  #include "processor.h"
>  
>  #define UCALL_PIO_PORT ((uint16_t)0x1000)
> +#define TIMEOUT	2	/* seconds, roughly */

I think it makes sense to make this a const in race_sync_regs(), that way its
usage is a bit more obvious.

>  struct ucall uc_none = {
>  	.cmd = UCALL_NONE,
> @@ -80,6 +82,124 @@ static void compare_vcpu_events(struct kvm_vcpu_events *left,
>  #define TEST_SYNC_FIELDS   (KVM_SYNC_X86_REGS|KVM_SYNC_X86_SREGS|KVM_SYNC_X86_EVENTS)
>  #define INVALID_SYNC_FIELD 0x80000000
>  
> +/*
> + * WARNING: CPU: 0 PID: 1115 at arch/x86/kvm/x86.c:10095 kvm_check_and_inject_events+0x220/0x500 [kvm]
> + *
> + * arch/x86/kvm/x86.c:kvm_check_and_inject_events():
> + * WARN_ON_ONCE(vcpu->arch.exception.injected &&
> + *		vcpu->arch.exception.pending);
> + */

For comments in selftests, describe what's happening without referencing KVM code,
things like this in particular will become stale sooner than later.  It's a-ok
(and encouraged) to put the WARNs and function references in changelogs though,
as those are explicitly tied to a specific time in history.

> +static void race_sync_regs(void *racer, bool poke_mmu)
> +{
> +	struct kvm_translation tr;
> +	struct kvm_vcpu *vcpu;
> +	struct kvm_run *run;
> +	struct kvm_vm *vm;
> +	pthread_t thread;
> +	time_t t;
> +
> +	vm = vm_create_with_one_vcpu(&vcpu, guest_code);
> +	run = vcpu->run;
> +
> +	run->kvm_valid_regs = KVM_SYNC_X86_SREGS;
> +	vcpu_run(vcpu);
> +	TEST_REQUIRE(run->s.regs.sregs.cr4 & X86_CR4_PAE);

This can be an assert, and should also check EFER.LME.  Jump-starting in long mode
is a property of selftests, i.e. not something that should ever randomly "fail".

> +	run->kvm_valid_regs = 0;
> +
> +	ASSERT_EQ(pthread_create(&thread, NULL, racer, (void *)run), 0);
> +
> +	for (t = time(NULL) + TIMEOUT; time(NULL) < t;) {
> +		__vcpu_run(vcpu);
> +
> +		if (poke_mmu) {

Rather than pass a boolean, I think it makes sense to do

		if (racer == race_sregs_cr4)

It's arguably just trading ugliness for subtlety, but IMO it's worth avoiding
the boolean.

> +			tr = (struct kvm_translation) { .linear_address = 0 };
> +			__vcpu_ioctl(vcpu, KVM_TRANSLATE, &tr);
> +		}
> +	}
> +
> +	ASSERT_EQ(pthread_cancel(thread), 0);
> +	ASSERT_EQ(pthread_join(thread, NULL), 0);
> +
> +	/*
> +	 * 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.

> +	if (!poke_mmu)
> +		kvm_vm_free(vm);
> +}
> +
>  int main(int argc, char *argv[])
>  {
>  	struct kvm_vcpu *vcpu;
> @@ -218,5 +338,9 @@ int main(int argc, char *argv[])
>  
>  	kvm_vm_free(vm);
>  
> +	race_sync_regs(race_sregs_cr4, true);
> +	race_sync_regs(race_events_exc, false);
> +	race_sync_regs(race_events_inj_pen, false);

I'll fix up all of the above when applying, and will also split this into three
patches, mostly so that each splat can be covered in a changelog, i.e. is tied
to its testcase.