Patchwork KVM: selftests: assert on exit reason in CR4/cpuid sync test

login
register
mail settings
Submitter Christopherson, Sean J
Date March 13, 2019, 8:19 p.m.
Message ID <20190313201926.7868-1-sean.j.christopherson@intel.com>
Download mbox | patch
Permalink /patch/748391/
State New
Headers show

Comments

Christopherson, Sean J - March 13, 2019, 8:19 p.m.
...so that the test doesn't end up in an infinite loop if it fails for
whatever reason, e.g. SHUTDOWN due to gcc inserting stack canary code
into ucall() and attempting to derefence a null segment.

Fixes: ca359066889f7 ("kvm: selftests: add cr4_cpuid_sync_test")
Cc: Wei Huang <wei@redhat.com>
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 .../kvm/x86_64/cr4_cpuid_sync_test.c          | 35 ++++++++++---------
 1 file changed, 19 insertions(+), 16 deletions(-)
Wei Huang - March 21, 2019, 4:50 a.m.
On 3/13/19 3:19 PM, Sean Christopherson wrote:
> ...so that the test doesn't end up in an infinite loop if it fails for
> whatever reason, e.g. SHUTDOWN due to gcc inserting stack canary code
> into ucall() and attempting to derefence a null segment.

You fix passed on my box. Before I ack it, could you share the failure
case (i.e. the steps to reproduce infinite loop) mentioned above?

> 
> Fixes: ca359066889f7 ("kvm: selftests: add cr4_cpuid_sync_test")
> Cc: Wei Huang <wei@redhat.com>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
>  .../kvm/x86_64/cr4_cpuid_sync_test.c          | 35 ++++++++++---------
>  1 file changed, 19 insertions(+), 16 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/x86_64/cr4_cpuid_sync_test.c b/tools/testing/selftests/kvm/x86_64/cr4_cpuid_sync_test.c
> index d503a51fad30..7c2c4d4055a8 100644
> --- a/tools/testing/selftests/kvm/x86_64/cr4_cpuid_sync_test.c
> +++ b/tools/testing/selftests/kvm/x86_64/cr4_cpuid_sync_test.c
> @@ -87,22 +87,25 @@ int main(int argc, char *argv[])
>  	while (1) {
>  		rc = _vcpu_run(vm, VCPU_ID);
>  
> -		if (run->exit_reason == KVM_EXIT_IO) {
> -			switch (get_ucall(vm, VCPU_ID, &uc)) {
> -			case UCALL_SYNC:
> -				/* emulate hypervisor clearing CR4.OSXSAVE */
> -				vcpu_sregs_get(vm, VCPU_ID, &sregs);
> -				sregs.cr4 &= ~X86_CR4_OSXSAVE;
> -				vcpu_sregs_set(vm, VCPU_ID, &sregs);
> -				break;
> -			case UCALL_ABORT:
> -				TEST_ASSERT(false, "Guest CR4 bit (OSXSAVE) unsynchronized with CPUID bit.");
> -				break;
> -			case UCALL_DONE:
> -				goto done;
> -			default:
> -				TEST_ASSERT(false, "Unknown ucall 0x%x.", uc.cmd);
> -			}
> +		TEST_ASSERT(run->exit_reason == KVM_EXIT_IO,
> +			    "Unexpected exit reason: %u (%s),\n",
> +			    run->exit_reason,
> +			    exit_reason_str(run->exit_reason));
> +
> +		switch (get_ucall(vm, VCPU_ID, &uc)) {
> +		case UCALL_SYNC:
> +			/* emulate hypervisor clearing CR4.OSXSAVE */
> +			vcpu_sregs_get(vm, VCPU_ID, &sregs);
> +			sregs.cr4 &= ~X86_CR4_OSXSAVE;
> +			vcpu_sregs_set(vm, VCPU_ID, &sregs);
> +			break;
> +		case UCALL_ABORT:
> +			TEST_ASSERT(false, "Guest CR4 bit (OSXSAVE) unsynchronized with CPUID bit.");
> +			break;
> +		case UCALL_DONE:
> +			goto done;
> +		default:
> +			TEST_ASSERT(false, "Unknown ucall 0x%x.", uc.cmd);
>  		}
>  	}
>  
>
Christopherson, Sean J - March 27, 2019, 5:29 p.m.
On Wed, Mar 20, 2019 at 11:50:26PM -0500, Wei Huang wrote:
> 
> 
> On 3/13/19 3:19 PM, Sean Christopherson wrote:
> > ...so that the test doesn't end up in an infinite loop if it fails for
> > whatever reason, e.g. SHUTDOWN due to gcc inserting stack canary code
> > into ucall() and attempting to derefence a null segment.
> 
> You fix passed on my box. Before I ack it, could you share the failure
> case (i.e. the steps to reproduce infinite loop) mentioned above?

Hit a triple fault shutdown due to derefencing an invalid fs: selector[1]
or because the addresses for the guest are borked[2].

[1] https://patchwork.kernel.org/patch/10851713/
[2] https://patchwork.kernel.org/patch/10851913/
Wei Huang - April 2, 2019, 8:30 p.m.
On 3/13/19 3:19 PM, Sean Christopherson wrote:
> ...so that the test doesn't end up in an infinite loop if it fails for
> whatever reason, e.g. SHUTDOWN due to gcc inserting stack canary code
> into ucall() and attempting to derefence a null segment.
> 
> Fixes: ca359066889f7 ("kvm: selftests: add cr4_cpuid_sync_test")
> Cc: Wei Huang <wei@redhat.com>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
>  .../kvm/x86_64/cr4_cpuid_sync_test.c          | 35 ++++++++++---------
>  1 file changed, 19 insertions(+), 16 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/x86_64/cr4_cpuid_sync_test.c b/tools/testing/selftests/kvm/x86_64/cr4_cpuid_sync_test.c
> index d503a51fad30..7c2c4d4055a8 100644
> --- a/tools/testing/selftests/kvm/x86_64/cr4_cpuid_sync_test.c
> +++ b/tools/testing/selftests/kvm/x86_64/cr4_cpuid_sync_test.c
> @@ -87,22 +87,25 @@ int main(int argc, char *argv[])
>  	while (1) {
>  		rc = _vcpu_run(vm, VCPU_ID);
>  
> -		if (run->exit_reason == KVM_EXIT_IO) {
> -			switch (get_ucall(vm, VCPU_ID, &uc)) {
> -			case UCALL_SYNC:
> -				/* emulate hypervisor clearing CR4.OSXSAVE */
> -				vcpu_sregs_get(vm, VCPU_ID, &sregs);
> -				sregs.cr4 &= ~X86_CR4_OSXSAVE;
> -				vcpu_sregs_set(vm, VCPU_ID, &sregs);
> -				break;
> -			case UCALL_ABORT:
> -				TEST_ASSERT(false, "Guest CR4 bit (OSXSAVE) unsynchronized with CPUID bit.");
> -				break;
> -			case UCALL_DONE:
> -				goto done;
> -			default:
> -				TEST_ASSERT(false, "Unknown ucall 0x%x.", uc.cmd);
> -			}
> +		TEST_ASSERT(run->exit_reason == KVM_EXIT_IO,
> +			    "Unexpected exit reason: %u (%s),\n",
> +			    run->exit_reason,
> +			    exit_reason_str(run->exit_reason));
> +
> +		switch (get_ucall(vm, VCPU_ID, &uc)) {
> +		case UCALL_SYNC:
> +			/* emulate hypervisor clearing CR4.OSXSAVE */
> +			vcpu_sregs_get(vm, VCPU_ID, &sregs);
> +			sregs.cr4 &= ~X86_CR4_OSXSAVE;
> +			vcpu_sregs_set(vm, VCPU_ID, &sregs);
> +			break;
> +		case UCALL_ABORT:
> +			TEST_ASSERT(false, "Guest CR4 bit (OSXSAVE) unsynchronized with CPUID bit.");
> +			break;
> +		case UCALL_DONE:
> +			goto done;
> +		default:
> +			TEST_ASSERT(false, "Unknown ucall 0x%x.", uc.cmd);
>  		}
>  	}
>  
> 

Acked-by: Wei Huang <wei@redhat.com>

Patch

diff --git a/tools/testing/selftests/kvm/x86_64/cr4_cpuid_sync_test.c b/tools/testing/selftests/kvm/x86_64/cr4_cpuid_sync_test.c
index d503a51fad30..7c2c4d4055a8 100644
--- a/tools/testing/selftests/kvm/x86_64/cr4_cpuid_sync_test.c
+++ b/tools/testing/selftests/kvm/x86_64/cr4_cpuid_sync_test.c
@@ -87,22 +87,25 @@  int main(int argc, char *argv[])
 	while (1) {
 		rc = _vcpu_run(vm, VCPU_ID);
 
-		if (run->exit_reason == KVM_EXIT_IO) {
-			switch (get_ucall(vm, VCPU_ID, &uc)) {
-			case UCALL_SYNC:
-				/* emulate hypervisor clearing CR4.OSXSAVE */
-				vcpu_sregs_get(vm, VCPU_ID, &sregs);
-				sregs.cr4 &= ~X86_CR4_OSXSAVE;
-				vcpu_sregs_set(vm, VCPU_ID, &sregs);
-				break;
-			case UCALL_ABORT:
-				TEST_ASSERT(false, "Guest CR4 bit (OSXSAVE) unsynchronized with CPUID bit.");
-				break;
-			case UCALL_DONE:
-				goto done;
-			default:
-				TEST_ASSERT(false, "Unknown ucall 0x%x.", uc.cmd);
-			}
+		TEST_ASSERT(run->exit_reason == KVM_EXIT_IO,
+			    "Unexpected exit reason: %u (%s),\n",
+			    run->exit_reason,
+			    exit_reason_str(run->exit_reason));
+
+		switch (get_ucall(vm, VCPU_ID, &uc)) {
+		case UCALL_SYNC:
+			/* emulate hypervisor clearing CR4.OSXSAVE */
+			vcpu_sregs_get(vm, VCPU_ID, &sregs);
+			sregs.cr4 &= ~X86_CR4_OSXSAVE;
+			vcpu_sregs_set(vm, VCPU_ID, &sregs);
+			break;
+		case UCALL_ABORT:
+			TEST_ASSERT(false, "Guest CR4 bit (OSXSAVE) unsynchronized with CPUID bit.");
+			break;
+		case UCALL_DONE:
+			goto done;
+		default:
+			TEST_ASSERT(false, "Unknown ucall 0x%x.", uc.cmd);
 		}
 	}