Re: [Xen-devel] [PATCH for-4.5 RFC v2] x86/HVM: Unconditionally crash guests on repeated vmentry failures

[Tim Deegan <tim@xxxxxxx>]

At 11:16 +0000 on 27 Nov (1417083414), Jan Beulich wrote:
> >>> On 27.11.14 at 11:26, <tim@xxxxxxx> wrote:
> > At 08:42 +0000 on 27 Nov (1417074133), Jan Beulich wrote:
> >> >>> On 26.11.14 at 18:43, <andrew.cooper3@xxxxxxxxxx> wrote:
> >> > My v1 patch only fixes the VMX side of things.  SVM doesn't explicitly
> >> > identify a failed vmentry and lets it fall into the default case of the
> >> > vmexit handler.  As such, with v1, the infinite loop still affects AMD
> >> > hardware.
> >> 
> >> Right; I should have said "something along the lines of v1". An SVM
> >> part is needed, but that should probably extend beyond what you
> >> proposed in v2: There are a number of "goto exit_and_crash"
> >> statements ahead of where you place your addition. I think they
> >> all need to be treated similarly.
> >> 
> >> I therefore think we should revert the VMX part of 28b4baacd5
> >> and make SVM behavior consistent with what results for VMX:
> >> Crash the guest unconditionally on VMEXIT_INVALID, without
> >> looking for recurring VM entry failures. See below/attached.
> >> 
> >> Jan
> >> 
> >> x86/HVM: prevent infinite VM entry retries
> >> 
> >> This reverts the VMX side of commit 28b4baac ("x86/HVM: don't crash
> >> guest upon problems occurring in user mode") and gets SVM in line with
> >> the resulting VMX behavior. This is because Andrew validly says
> >> 
> >> "A failed vmentry is overwhelmingly likely to be caused by corrupt
> >>  VMC[SB] state.  As a result, injecting a fault and retrying the the
> >>  vmentry is likely to fail in the same way."
> >> 
> >> Reported-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> >> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> > 
> > Looking at the SVM side, AFAICT you're trying to filter out
> > VMEXIT_INVALID exits that couldn't be handled by nested SVM, but I
> > think it's fine just to always crash on nested-SVM failures: we know
> > the guest wasn't in user mode because it successfully executed VMRUN.
> > And looking at it, the other users of that label are for unexpected
> > debugging exits, which can't be caused by the guest userspace either.
> > 
> > So how about this for the SVM side, reverting to crashing for
> > everything except new, unsupported exit types?
> 
> Generally a good idea, but there are two paths to exit_and_crash
> (for VMEXIT_EXCEPTION_DB and VMEXIT_EXCEPTION_BP) which I
> think would better crash only conditionally.

Those are catching Xen bugs -- we don't (or at least shouldn't) enable
those exit types when the debugger is not attached.  I think that,
like with the p2m ENOMEM case, turning them into #UD is not really
helpful.  But if you prefer, those could be made into 'goto default'
to preserve the current behaviour, which would also retain the
debugging output.

> And finally, if doing it that way we might better remove the
> exit_and_crash label altogether and call domain_crash() directly
> in the places we mean it to be called.

Indeed.  How's this, then?

diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 8aca6e6..213fd6e 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -2362,7 +2362,8 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
                 goto out;
             case NESTEDHVM_VMEXIT_FATALERROR:
                 gdprintk(XENLOG_ERR, "unexpected nestedsvm_vmexit() error\n");
-                goto exit_and_crash;
+                domain_crash(v->domain);
+                goto out;
 
             default:
                 BUG();
@@ -2376,18 +2377,21 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
         case NESTEDHVM_VMEXIT_FATALERROR:
             gdprintk(XENLOG_ERR,
                 "unexpected nestedsvm_check_intercepts() error\n");
-            goto exit_and_crash;
+            domain_crash(v->domain);
+            goto out;
         default:
             gdprintk(XENLOG_INFO, "nestedsvm_check_intercepts() returned %i\n",
                 nsret);
-            goto exit_and_crash;
+            domain_crash(v->domain);
+            goto out;
         }
     }
 
     if ( unlikely(exit_reason == VMEXIT_INVALID) )
     {
         svm_vmcb_dump(__func__, vmcb);
-        goto exit_and_crash;
+        domain_crash(v->domain);
+        goto out;
     }
 
     perfc_incra(svmexits, exit_reason);
@@ -2422,13 +2426,13 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
 
     case VMEXIT_EXCEPTION_DB:
         if ( !v->domain->debugger_attached )
-            goto exit_and_crash;
+            goto unexpected_exit_type;
         domain_pause_for_debugger();
         break;
 
     case VMEXIT_EXCEPTION_BP:
         if ( !v->domain->debugger_attached )
-            goto exit_and_crash;
+            goto unexpected_exit_type;
         /* AMD Vol2, 15.11: INT3, INTO, BOUND intercepts do not update RIP. */
         if ( (inst_len = __get_instruction_length(v, INSTR_INT3)) == 0 )
             break;
@@ -2675,7 +2679,7 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
         break;
 
     default:
-    exit_and_crash:
+    unexpected_exit_type:
         gdprintk(XENLOG_ERR, "unexpected VMEXIT: exit reason = %#"PRIx64", "
                  "exitinfo1 = %#"PRIx64", exitinfo2 = %#"PRIx64"\n",
                  exit_reason, 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel