linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/3] fix emulation error on Windows bootup
@ 2019-08-27 13:07 Jan Dakinevich
  2019-08-27 13:07 ` [PATCH 1/3] KVM: x86: fix wrong return code Jan Dakinevich
                   ` (3 more replies)
  0 siblings, 4 replies; 14+ messages in thread
From: Jan Dakinevich @ 2019-08-27 13:07 UTC (permalink / raw)
  To: linux-kernel
  Cc: Jan Dakinevich, Denis Lunev, Roman Kagan, Denis Plotnikov,
	Paolo Bonzini, Radim Krčmář,
	Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
	Joerg Roedel, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	H. Peter Anvin, x86, kvm

This series intended to fix (again) a bug that was a subject of the 
following change:

  6ea6e84 ("KVM: x86: inject exceptions produced by x86_decode_insn")

Suddenly, that fix had a couple mistakes. First, ctxt->have_exception was 
not set if fault happened during instruction decoding. Second, returning 
value of inject_emulated_instruction was used to make the decision to 
reenter guest, but this could happen iff on nested page fault, that is not 
the scope where this bug could occur.

However, I have still deep doubts about 3rd commit in the series. Could
you please, make me an advise if it is the correct handling of guest page 
fault?

Jan Dakinevich (3):
  KVM: x86: fix wrong return code
  KVM: x86: set ctxt->have_exception in x86_decode_insn()
  KVM: x86: always stop emulation on page fault

 arch/x86/kvm/emulate.c | 4 +++-
 arch/x86/kvm/x86.c     | 4 +++-
 2 files changed, 6 insertions(+), 2 deletions(-)

-- 
2.1.4


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 1/3] KVM: x86: fix wrong return code
  2019-08-27 13:07 [PATCH 0/3] fix emulation error on Windows bootup Jan Dakinevich
@ 2019-08-27 13:07 ` Jan Dakinevich
  2019-08-27 13:42   ` Sean Christopherson
  2019-08-27 13:07 ` [PATCH 2/3] KVM: x86: set ctxt->have_exception in x86_decode_insn() Jan Dakinevich
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 14+ messages in thread
From: Jan Dakinevich @ 2019-08-27 13:07 UTC (permalink / raw)
  To: linux-kernel
  Cc: Jan Dakinevich, Denis Lunev, Roman Kagan, Denis Plotnikov,
	Paolo Bonzini, Radim Krčmář,
	Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
	Joerg Roedel, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	H. Peter Anvin, x86, kvm

x86_emulate_instruction(), the caller of x86_decode_insn(), expects
that x86_decode_insn()'s returning value belongs to EMULATION_* name
space. However, this function may return value from X86EMUL_* name
space.

Although, the code behaves properly (because both X86EMUL_CONTINUE and
EMULATION_OK are equal to 0) this change makes code more consistent and
it is required for further fixes.

Cc: Denis Lunev <den@virtuozzo.com>
Cc: Roman Kagan <rkagan@virtuozzo.com>
Cc: Denis Plotnikov <dplotnikov@virtuozzo.com>
Signed-off-by: Jan Dakinevich <jan.dakinevich@virtuozzo.com>
---
 arch/x86/kvm/emulate.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 718f7d9..6170ddf 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -5144,7 +5144,7 @@ int x86_decode_insn(struct x86_emulate_ctxt *ctxt, void *insn, int insn_len)
 	else {
 		rc = __do_insn_fetch_bytes(ctxt, 1);
 		if (rc != X86EMUL_CONTINUE)
-			return rc;
+			goto done;
 	}
 
 	switch (mode) {
-- 
2.1.4


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 2/3] KVM: x86: set ctxt->have_exception in x86_decode_insn()
  2019-08-27 13:07 [PATCH 0/3] fix emulation error on Windows bootup Jan Dakinevich
  2019-08-27 13:07 ` [PATCH 1/3] KVM: x86: fix wrong return code Jan Dakinevich
@ 2019-08-27 13:07 ` Jan Dakinevich
  2019-08-27 14:53   ` Sean Christopherson
  2019-08-27 13:07 ` [PATCH 3/3] KVM: x86: always stop emulation on page fault Jan Dakinevich
  2019-09-11 15:51 ` [PATCH 0/3] fix emulation error on Windows bootup Paolo Bonzini
  3 siblings, 1 reply; 14+ messages in thread
From: Jan Dakinevich @ 2019-08-27 13:07 UTC (permalink / raw)
  To: linux-kernel
  Cc: Jan Dakinevich, Denis Lunev, Roman Kagan, Denis Plotnikov,
	Paolo Bonzini, Radim Krčmář,
	Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
	Joerg Roedel, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	H. Peter Anvin, x86, kvm

x86_emulate_instruction() takes into account ctxt->have_exception flag
during instruction decoding, but in practice this flag is never set in
x86_decode_insn().

Fixes: 6ea6e84 ("KVM: x86: inject exceptions produced by x86_decode_insn")
Cc: Denis Lunev <den@virtuozzo.com>
Cc: Roman Kagan <rkagan@virtuozzo.com>
Cc: Denis Plotnikov <dplotnikov@virtuozzo.com>
Signed-off-by: Jan Dakinevich <jan.dakinevich@virtuozzo.com>
---
 arch/x86/kvm/emulate.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 6170ddf..f93880f 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -5395,6 +5395,8 @@ int x86_decode_insn(struct x86_emulate_ctxt *ctxt, void *insn, int insn_len)
 					ctxt->memopp->addr.mem.ea + ctxt->_eip);
 
 done:
+	if (rc == X86EMUL_PROPAGATE_FAULT)
+		ctxt->have_exception = true;
 	return (rc != X86EMUL_CONTINUE) ? EMULATION_FAILED : EMULATION_OK;
 }
 
-- 
2.1.4


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 3/3] KVM: x86: always stop emulation on page fault
  2019-08-27 13:07 [PATCH 0/3] fix emulation error on Windows bootup Jan Dakinevich
  2019-08-27 13:07 ` [PATCH 1/3] KVM: x86: fix wrong return code Jan Dakinevich
  2019-08-27 13:07 ` [PATCH 2/3] KVM: x86: set ctxt->have_exception in x86_decode_insn() Jan Dakinevich
@ 2019-08-27 13:07 ` Jan Dakinevich
  2019-08-27 14:50   ` Sean Christopherson
  2019-09-11 15:51 ` [PATCH 0/3] fix emulation error on Windows bootup Paolo Bonzini
  3 siblings, 1 reply; 14+ messages in thread
From: Jan Dakinevich @ 2019-08-27 13:07 UTC (permalink / raw)
  To: linux-kernel
  Cc: Jan Dakinevich, Denis Lunev, Roman Kagan, Denis Plotnikov,
	Paolo Bonzini, Radim Krčmář,
	Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
	Joerg Roedel, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	H. Peter Anvin, x86, kvm

inject_emulated_exception() returns true if and only if nested page
fault happens. However, page fault can come from guest page tables
walk, either nested or not nested. In both cases we should stop an
attempt to read under RIP and give guest to step over its own page
fault handler.

Fixes: 6ea6e84 ("KVM: x86: inject exceptions produced by x86_decode_insn")
Cc: Denis Lunev <den@virtuozzo.com>
Cc: Roman Kagan <rkagan@virtuozzo.com>
Cc: Denis Plotnikov <dplotnikov@virtuozzo.com>
Signed-off-by: Jan Dakinevich <jan.dakinevich@virtuozzo.com>
---
 arch/x86/kvm/x86.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 93b0bd4..45caa69 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -6521,8 +6521,10 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu,
 			if (reexecute_instruction(vcpu, cr2, write_fault_to_spt,
 						emulation_type))
 				return EMULATE_DONE;
-			if (ctxt->have_exception && inject_emulated_exception(vcpu))
+			if (ctxt->have_exception) {
+				inject_emulated_exception(vcpu);
 				return EMULATE_DONE;
+			}
 			if (emulation_type & EMULTYPE_SKIP)
 				return EMULATE_FAIL;
 			return handle_emulation_failure(vcpu, emulation_type);
-- 
2.1.4


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/3] KVM: x86: fix wrong return code
  2019-08-27 13:07 ` [PATCH 1/3] KVM: x86: fix wrong return code Jan Dakinevich
@ 2019-08-27 13:42   ` Sean Christopherson
  0 siblings, 0 replies; 14+ messages in thread
From: Sean Christopherson @ 2019-08-27 13:42 UTC (permalink / raw)
  To: Jan Dakinevich
  Cc: linux-kernel, Denis Lunev, Roman Kagan, Denis Plotnikov,
	Paolo Bonzini, Radim Krčmář,
	Vitaly Kuznetsov, Wanpeng Li, Jim Mattson, Joerg Roedel,
	Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin,
	x86, kvm

On Tue, Aug 27, 2019 at 01:07:04PM +0000, Jan Dakinevich wrote:
> x86_emulate_instruction(), the caller of x86_decode_insn(), expects
> that x86_decode_insn()'s returning value belongs to EMULATION_* name
> space. However, this function may return value from X86EMUL_* name
> space.
> 
> Although, the code behaves properly (because both X86EMUL_CONTINUE and
> EMULATION_OK are equal to 0) this change makes code more consistent and
> it is required for further fixes.
> 
> Cc: Denis Lunev <den@virtuozzo.com>
> Cc: Roman Kagan <rkagan@virtuozzo.com>
> Cc: Denis Plotnikov <dplotnikov@virtuozzo.com>
> Signed-off-by: Jan Dakinevich <jan.dakinevich@virtuozzo.com>
> ---
>  arch/x86/kvm/emulate.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index 718f7d9..6170ddf 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -5144,7 +5144,7 @@ int x86_decode_insn(struct x86_emulate_ctxt *ctxt, void *insn, int insn_len)
>  	else {
>  		rc = __do_insn_fetch_bytes(ctxt, 1);
>  		if (rc != X86EMUL_CONTINUE)
> -			return rc;
> +			goto done;

Funny how things go unnoticed for years and then suddenly...

https://lkml.kernel.org/r/9bf79098-703c-e82b-7e7d-1c0a6a1023c2@redhat.com

>  	}
>  
>  	switch (mode) {
> -- 
> 2.1.4
> 

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 3/3] KVM: x86: always stop emulation on page fault
  2019-08-27 13:07 ` [PATCH 3/3] KVM: x86: always stop emulation on page fault Jan Dakinevich
@ 2019-08-27 14:50   ` Sean Christopherson
  2019-08-27 14:55     ` Sean Christopherson
  2019-08-28 10:19     ` Jan Dakinevich
  0 siblings, 2 replies; 14+ messages in thread
From: Sean Christopherson @ 2019-08-27 14:50 UTC (permalink / raw)
  To: Jan Dakinevich
  Cc: linux-kernel, Denis Lunev, Roman Kagan, Denis Plotnikov,
	Paolo Bonzini, Radim Krčmář,
	Vitaly Kuznetsov, Wanpeng Li, Jim Mattson, Joerg Roedel,
	Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin,
	x86, kvm

+Cc Peng Hao and Yi Wang

On Tue, Aug 27, 2019 at 01:07:09PM +0000, Jan Dakinevich wrote:
> inject_emulated_exception() returns true if and only if nested page
> fault happens. However, page fault can come from guest page tables
> walk, either nested or not nested. In both cases we should stop an
> attempt to read under RIP and give guest to step over its own page
> fault handler.
> 
> Fixes: 6ea6e84 ("KVM: x86: inject exceptions produced by x86_decode_insn")
> Cc: Denis Lunev <den@virtuozzo.com>
> Cc: Roman Kagan <rkagan@virtuozzo.com>
> Cc: Denis Plotnikov <dplotnikov@virtuozzo.com>
> Signed-off-by: Jan Dakinevich <jan.dakinevich@virtuozzo.com>
> ---
>  arch/x86/kvm/x86.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 93b0bd4..45caa69 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -6521,8 +6521,10 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu,
>  			if (reexecute_instruction(vcpu, cr2, write_fault_to_spt,
>  						emulation_type))
>  				return EMULATE_DONE;
> -			if (ctxt->have_exception && inject_emulated_exception(vcpu))
> +			if (ctxt->have_exception) {
> +				inject_emulated_exception(vcpu);
>  				return EMULATE_DONE;
> +			}


Yikes, this patch and the previous have quite the sordid history.


The non-void return from inject_emulated_exception() was added by commit

  ef54bcfeea6c ("KVM: x86: skip writeback on injection of nested exception")

for the purpose of skipping writeback.  At the time, the above blob in the
decode flow didn't exist.


Decode exception handling was added by commit

  6ea6e84309ca ("KVM: x86: inject exceptions produced by x86_decode_insn")

but it was dead code even then.  The patch discussion[1] even point out that
it was dead code, i.e. the change probably should have been reverted.


Peng Hao and Yi Wang later ran into what appears to be the same bug you're
hitting[2][3], and even had patches temporarily queued[4][5], but the
patches never made it to mainline as they broke kvm-unit-tests.  Fun side
note, Radim even pointed out[4] the bug fixed by patch 1/3.

So, the patches look correct, but there's the open question of why the
hypercall test was failing for Paolo.  I've tried to reproduce the #DF to
no avail.

[1] https://lore.kernel.org/patchwork/patch/850077/
[2] https://lkml.kernel.org/r/1537311828-4547-1-git-send-email-penghao122@sina.com.cn
[3] https://lkml.kernel.org/r/20190111133002.GA14852@flask
[4] https://lkml.kernel.org/r/20190111133002.GA14852@flask
[5] https://lkml.kernel.org/r/9835d255-dd9a-222b-f4a2-93611175b326@redhat.com

>  			if (emulation_type & EMULTYPE_SKIP)
>  				return EMULATE_FAIL;
>  			return handle_emulation_failure(vcpu, emulation_type);
> -- 
> 2.1.4
> 

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/3] KVM: x86: set ctxt->have_exception in x86_decode_insn()
  2019-08-27 13:07 ` [PATCH 2/3] KVM: x86: set ctxt->have_exception in x86_decode_insn() Jan Dakinevich
@ 2019-08-27 14:53   ` Sean Christopherson
  2019-08-28 10:19     ` Jan Dakinevich
  0 siblings, 1 reply; 14+ messages in thread
From: Sean Christopherson @ 2019-08-27 14:53 UTC (permalink / raw)
  To: Jan Dakinevich
  Cc: linux-kernel, Denis Lunev, Roman Kagan, Denis Plotnikov,
	Paolo Bonzini, Radim Krčmář,
	Vitaly Kuznetsov, Wanpeng Li, Jim Mattson, Joerg Roedel,
	Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin,
	x86, kvm

On Tue, Aug 27, 2019 at 01:07:08PM +0000, Jan Dakinevich wrote:
> x86_emulate_instruction() takes into account ctxt->have_exception flag
> during instruction decoding, but in practice this flag is never set in
> x86_decode_insn().
> 
> Fixes: 6ea6e84 ("KVM: x86: inject exceptions produced by x86_decode_insn")
> Cc: Denis Lunev <den@virtuozzo.com>
> Cc: Roman Kagan <rkagan@virtuozzo.com>
> Cc: Denis Plotnikov <dplotnikov@virtuozzo.com>
> Signed-off-by: Jan Dakinevich <jan.dakinevich@virtuozzo.com>
> ---
>  arch/x86/kvm/emulate.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index 6170ddf..f93880f 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -5395,6 +5395,8 @@ int x86_decode_insn(struct x86_emulate_ctxt *ctxt, void *insn, int insn_len)
>  					ctxt->memopp->addr.mem.ea + ctxt->_eip);
>  
>  done:
> +	if (rc == X86EMUL_PROPAGATE_FAULT)
> +		ctxt->have_exception = true;

We should add a sanity check or two on the vector since the emulator code
goes all over the place, e.g. #UD should not be injected/propagated, and
trap-like exceptions should not be handled/encountered during decode.
Note, exception_type() also warns on illegal vectors.

  WARN_ON_ONCE(ctxt->exception.vector == UD_VECTOR ||
	       exception_type(ctxt->exception.vector) == EXCPT_TRAP);

>  	return (rc != X86EMUL_CONTINUE) ? EMULATION_FAILED : EMULATION_OK;
>  }
>  
> -- 
> 2.1.4
> 

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 3/3] KVM: x86: always stop emulation on page fault
  2019-08-27 14:50   ` Sean Christopherson
@ 2019-08-27 14:55     ` Sean Christopherson
  2019-08-28 10:19     ` Jan Dakinevich
  1 sibling, 0 replies; 14+ messages in thread
From: Sean Christopherson @ 2019-08-27 14:55 UTC (permalink / raw)
  To: Jan Dakinevich
  Cc: linux-kernel, Denis Lunev, Roman Kagan, Denis Plotnikov,
	Paolo Bonzini, Radim Krčmář,
	Vitaly Kuznetsov, Wanpeng Li, Jim Mattson, Joerg Roedel,
	Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin,
	x86, kvm, Yi Wang, Peng Hao

Actually adding Peng Hao and Yi Wang...

On Tue, Aug 27, 2019 at 07:50:30AM -0700, Sean Christopherson wrote:
> +Cc Peng Hao and Yi Wang
> 
> On Tue, Aug 27, 2019 at 01:07:09PM +0000, Jan Dakinevich wrote:
> > inject_emulated_exception() returns true if and only if nested page
> > fault happens. However, page fault can come from guest page tables
> > walk, either nested or not nested. In both cases we should stop an
> > attempt to read under RIP and give guest to step over its own page
> > fault handler.
> > 
> > Fixes: 6ea6e84 ("KVM: x86: inject exceptions produced by x86_decode_insn")
> > Cc: Denis Lunev <den@virtuozzo.com>
> > Cc: Roman Kagan <rkagan@virtuozzo.com>
> > Cc: Denis Plotnikov <dplotnikov@virtuozzo.com>
> > Signed-off-by: Jan Dakinevich <jan.dakinevich@virtuozzo.com>
> > ---
> >  arch/x86/kvm/x86.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index 93b0bd4..45caa69 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -6521,8 +6521,10 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu,
> >  			if (reexecute_instruction(vcpu, cr2, write_fault_to_spt,
> >  						emulation_type))
> >  				return EMULATE_DONE;
> > -			if (ctxt->have_exception && inject_emulated_exception(vcpu))
> > +			if (ctxt->have_exception) {
> > +				inject_emulated_exception(vcpu);
> >  				return EMULATE_DONE;
> > +			}
> 
> 
> Yikes, this patch and the previous have quite the sordid history.
> 
> 
> The non-void return from inject_emulated_exception() was added by commit
> 
>   ef54bcfeea6c ("KVM: x86: skip writeback on injection of nested exception")
> 
> for the purpose of skipping writeback.  At the time, the above blob in the
> decode flow didn't exist.
> 
> 
> Decode exception handling was added by commit
> 
>   6ea6e84309ca ("KVM: x86: inject exceptions produced by x86_decode_insn")
> 
> but it was dead code even then.  The patch discussion[1] even point out that
> it was dead code, i.e. the change probably should have been reverted.
> 
> 
> Peng Hao and Yi Wang later ran into what appears to be the same bug you're
> hitting[2][3], and even had patches temporarily queued[4][5], but the
> patches never made it to mainline as they broke kvm-unit-tests.  Fun side
> note, Radim even pointed out[4] the bug fixed by patch 1/3.
> 
> So, the patches look correct, but there's the open question of why the
> hypercall test was failing for Paolo.  I've tried to reproduce the #DF to
> no avail.
> 
> [1] https://lore.kernel.org/patchwork/patch/850077/
> [2] https://lkml.kernel.org/r/1537311828-4547-1-git-send-email-penghao122@sina.com.cn
> [3] https://lkml.kernel.org/r/20190111133002.GA14852@flask
> [4] https://lkml.kernel.org/r/20190111133002.GA14852@flask
> [5] https://lkml.kernel.org/r/9835d255-dd9a-222b-f4a2-93611175b326@redhat.com
> 
> >  			if (emulation_type & EMULTYPE_SKIP)
> >  				return EMULATE_FAIL;
> >  			return handle_emulation_failure(vcpu, emulation_type);
> > -- 
> > 2.1.4
> > 

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/3] KVM: x86: set ctxt->have_exception in x86_decode_insn()
  2019-08-27 14:53   ` Sean Christopherson
@ 2019-08-28 10:19     ` Jan Dakinevich
  0 siblings, 0 replies; 14+ messages in thread
From: Jan Dakinevich @ 2019-08-28 10:19 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: linux-kernel, Denis Lunev, Roman Kagan, Denis Plotnikov,
	Paolo Bonzini, Radim Krčmář,
	Vitaly Kuznetsov, Wanpeng Li, Jim Mattson, Joerg Roedel,
	Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin,
	x86, kvm

On Tue, 27 Aug 2019 07:53:58 -0700
Sean Christopherson <sean.j.christopherson@intel.com> wrote:

> On Tue, Aug 27, 2019 at 01:07:08PM +0000, Jan Dakinevich wrote:
> > x86_emulate_instruction() takes into account ctxt->have_exception flag
> > during instruction decoding, but in practice this flag is never set in
> > x86_decode_insn().
> > 
> > Fixes: 6ea6e84 ("KVM: x86: inject exceptions produced by x86_decode_insn")
> > Cc: Denis Lunev <den@virtuozzo.com>
> > Cc: Roman Kagan <rkagan@virtuozzo.com>
> > Cc: Denis Plotnikov <dplotnikov@virtuozzo.com>
> > Signed-off-by: Jan Dakinevich <jan.dakinevich@virtuozzo.com>
> > ---
> >  arch/x86/kvm/emulate.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> > index 6170ddf..f93880f 100644
> > --- a/arch/x86/kvm/emulate.c
> > +++ b/arch/x86/kvm/emulate.c
> > @@ -5395,6 +5395,8 @@ int x86_decode_insn(struct x86_emulate_ctxt *ctxt, void *insn, int insn_len)
> >  					ctxt->memopp->addr.mem.ea + ctxt->_eip);
> >  
> >  done:
> > +	if (rc == X86EMUL_PROPAGATE_FAULT)
> > +		ctxt->have_exception = true;
> 
> We should add a sanity check or two on the vector since the emulator code
> goes all over the place, e.g. #UD should not be injected/propagated, and
> trap-like exceptions should not be handled/encountered during decode.
> Note, exception_type() also warns on illegal vectors.
> 
>   WARN_ON_ONCE(ctxt->exception.vector == UD_VECTOR ||
> 	       exception_type(ctxt->exception.vector) == EXCPT_TRAP);

Ok.

> 
> >  	return (rc != X86EMUL_CONTINUE) ? EMULATION_FAILED : EMULATION_OK;
> >  }
> >  
> > -- 
> > 2.1.4
> > 


-- 
Best regards
Jan Dakinevich

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 3/3] KVM: x86: always stop emulation on page fault
  2019-08-27 14:50   ` Sean Christopherson
  2019-08-27 14:55     ` Sean Christopherson
@ 2019-08-28 10:19     ` Jan Dakinevich
  2019-08-28 14:23       ` Sean Christopherson
  1 sibling, 1 reply; 14+ messages in thread
From: Jan Dakinevich @ 2019-08-28 10:19 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: linux-kernel, Denis Lunev, Roman Kagan, Denis Plotnikov,
	Paolo Bonzini, Radim Krčmář,
	Vitaly Kuznetsov, Wanpeng Li, Jim Mattson, Joerg Roedel,
	Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin,
	x86, kvm, Yi Wang, Peng Hao

On Tue, 27 Aug 2019 07:50:30 -0700
Sean Christopherson <sean.j.christopherson@intel.com> wrote:

> +Cc Peng Hao and Yi Wang
> 
> On Tue, Aug 27, 2019 at 01:07:09PM +0000, Jan Dakinevich wrote:
> > inject_emulated_exception() returns true if and only if nested page
> > fault happens. However, page fault can come from guest page tables
> > walk, either nested or not nested. In both cases we should stop an
> > attempt to read under RIP and give guest to step over its own page
> > fault handler.
> > 
> > Fixes: 6ea6e84 ("KVM: x86: inject exceptions produced by x86_decode_insn")
> > Cc: Denis Lunev <den@virtuozzo.com>
> > Cc: Roman Kagan <rkagan@virtuozzo.com>
> > Cc: Denis Plotnikov <dplotnikov@virtuozzo.com>
> > Signed-off-by: Jan Dakinevich <jan.dakinevich@virtuozzo.com>
> > ---
> >  arch/x86/kvm/x86.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index 93b0bd4..45caa69 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -6521,8 +6521,10 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu,
> >  			if (reexecute_instruction(vcpu, cr2, write_fault_to_spt,
> >  						emulation_type))
> >  				return EMULATE_DONE;
> > -			if (ctxt->have_exception && inject_emulated_exception(vcpu))
> > +			if (ctxt->have_exception) {
> > +				inject_emulated_exception(vcpu);
> >  				return EMULATE_DONE;
> > +			}
> 
> 
> Yikes, this patch and the previous have quite the sordid history.
> 
> 
> The non-void return from inject_emulated_exception() was added by commit
> 
>   ef54bcfeea6c ("KVM: x86: skip writeback on injection of nested exception")
> 
> for the purpose of skipping writeback.  At the time, the above blob in the
> decode flow didn't exist.
> 
> 
> Decode exception handling was added by commit
> 
>   6ea6e84309ca ("KVM: x86: inject exceptions produced by x86_decode_insn")
> 
> but it was dead code even then.  The patch discussion[1] even point out that
> it was dead code, i.e. the change probably should have been reverted.
> 
> 
> Peng Hao and Yi Wang later ran into what appears to be the same bug you're
> hitting[2][3], and even had patches temporarily queued[4][5], but the
> patches never made it to mainline as they broke kvm-unit-tests.  Fun side
> note, Radim even pointed out[4] the bug fixed by patch 1/3.
> 
> So, the patches look correct, but there's the open question of why the
> hypercall test was failing for Paolo.  

Sorry, I'm little confused. Could you please, point me which test or tests 
were broken? I've just run kvm-unit-test and I see same results with and 
without my changes.

> I've tried to reproduce the #DF to
> no avail.
> 
> [1] https://lore.kernel.org/patchwork/patch/850077/
> [2] https://lkml.kernel.org/r/1537311828-4547-1-git-send-email-penghao122@sina.com.cn
> [3] https://lkml.kernel.org/r/20190111133002.GA14852@flask
> [4] https://lkml.kernel.org/r/20190111133002.GA14852@flask
> [5] https://lkml.kernel.org/r/9835d255-dd9a-222b-f4a2-93611175b326@redhat.com
> 
> >  			if (emulation_type & EMULTYPE_SKIP)
> >  				return EMULATE_FAIL;
> >  			return handle_emulation_failure(vcpu, emulation_type);
> > -- 
> > 2.1.4
> > 


-- 
Best regards
Jan Dakinevich

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 3/3] KVM: x86: always stop emulation on page fault
  2019-08-28 10:19     ` Jan Dakinevich
@ 2019-08-28 14:23       ` Sean Christopherson
  0 siblings, 0 replies; 14+ messages in thread
From: Sean Christopherson @ 2019-08-28 14:23 UTC (permalink / raw)
  To: Jan Dakinevich
  Cc: linux-kernel, Denis Lunev, Roman Kagan, Denis Plotnikov,
	Paolo Bonzini, Radim Krčmář,
	Vitaly Kuznetsov, Wanpeng Li, Jim Mattson, Joerg Roedel,
	Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin,
	x86, kvm, Yi Wang, Peng Hao

On Wed, Aug 28, 2019 at 10:19:51AM +0000, Jan Dakinevich wrote:
> On Tue, 27 Aug 2019 07:50:30 -0700
> Sean Christopherson <sean.j.christopherson@intel.com> wrote:
> > Yikes, this patch and the previous have quite the sordid history.
> > 
> > 
> > The non-void return from inject_emulated_exception() was added by commit
> > 
> >   ef54bcfeea6c ("KVM: x86: skip writeback on injection of nested exception")
> > 
> > for the purpose of skipping writeback.  At the time, the above blob in the
> > decode flow didn't exist.
> > 
> > 
> > Decode exception handling was added by commit
> > 
> >   6ea6e84309ca ("KVM: x86: inject exceptions produced by x86_decode_insn")
> > 
> > but it was dead code even then.  The patch discussion[1] even point out that
> > it was dead code, i.e. the change probably should have been reverted.
> > 
> > 
> > Peng Hao and Yi Wang later ran into what appears to be the same bug you're
> > hitting[2][3], and even had patches temporarily queued[4][5], but the
> > patches never made it to mainline as they broke kvm-unit-tests.  Fun side
> > note, Radim even pointed out[4] the bug fixed by patch 1/3.
> > 
> > So, the patches look correct, but there's the open question of why the
> > hypercall test was failing for Paolo.  
> 
> Sorry, I'm little confused. Could you please, point me which test or tests 
> were broken? I've just run kvm-unit-test and I see same results with and 
> without my changes.
> 
> > I've tried to reproduce the #DF to
> > no avail.

Aha!  The #DF occurs if patch 2/3, but not patch 3/3, is applied, and the
VMware backdoor is enabled.  The backdoor is off by default, which is why
only Paolo was seeing the #DF.

To handle the VMware backdoor, KVM intercepts #GP faults, which includes
the non-canonical #GP from the hypercall unit test.  With only patch 2/3
applied, x86_emulate_instruction() injects a #GP for the non-canonical RIP
but returns EMULATE_FAIL instead of EMULATE_DONE.   EMULATE_FAIL causes
handle_exception_nmi() (or gp_interception() for SVM) to re-inject the
original #GP because it thinks emulation failed due to a non-VMware opcode.

Applying patch 3/3 resolves the issue as x86_emulate_instruction() returns
EMULATE_DONE after injecting the #GP.


TL;DR:

Swap the order of patches and everything should be hunky dory.  Please
rebase to the latest kvm/queue, which has an equivalent to patch 1/3.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 0/3] fix emulation error on Windows bootup
  2019-08-27 13:07 [PATCH 0/3] fix emulation error on Windows bootup Jan Dakinevich
                   ` (2 preceding siblings ...)
  2019-08-27 13:07 ` [PATCH 3/3] KVM: x86: always stop emulation on page fault Jan Dakinevich
@ 2019-09-11 15:51 ` Paolo Bonzini
  2019-09-11 19:53   ` Sean Christopherson
  3 siblings, 1 reply; 14+ messages in thread
From: Paolo Bonzini @ 2019-09-11 15:51 UTC (permalink / raw)
  To: Jan Dakinevich, linux-kernel
  Cc: Denis Lunev, Roman Kagan, Denis Plotnikov,
	Radim Krčmář,
	Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
	Joerg Roedel, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	H. Peter Anvin, x86, kvm

On 27/08/19 15:07, Jan Dakinevich wrote:
> This series intended to fix (again) a bug that was a subject of the 
> following change:
> 
>   6ea6e84 ("KVM: x86: inject exceptions produced by x86_decode_insn")
> 
> Suddenly, that fix had a couple mistakes. First, ctxt->have_exception was 
> not set if fault happened during instruction decoding. Second, returning 
> value of inject_emulated_instruction was used to make the decision to 
> reenter guest, but this could happen iff on nested page fault, that is not 
> the scope where this bug could occur.
> 
> However, I have still deep doubts about 3rd commit in the series. Could
> you please, make me an advise if it is the correct handling of guest page 
> fault?
> 
> Jan Dakinevich (3):
>   KVM: x86: fix wrong return code
>   KVM: x86: set ctxt->have_exception in x86_decode_insn()
>   KVM: x86: always stop emulation on page fault
> 
>  arch/x86/kvm/emulate.c | 4 +++-
>  arch/x86/kvm/x86.c     | 4 +++-
>  2 files changed, 6 insertions(+), 2 deletions(-)
> 

Queued, thanks.  I added the WARN_ON_ONCE that Sean suggested.

Paolo

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 0/3] fix emulation error on Windows bootup
  2019-09-11 15:51 ` [PATCH 0/3] fix emulation error on Windows bootup Paolo Bonzini
@ 2019-09-11 19:53   ` Sean Christopherson
  2019-09-13  9:51     ` Paolo Bonzini
  0 siblings, 1 reply; 14+ messages in thread
From: Sean Christopherson @ 2019-09-11 19:53 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: Jan Dakinevich, linux-kernel, Denis Lunev, Roman Kagan,
	Denis Plotnikov, Radim Krčmář,
	Vitaly Kuznetsov, Wanpeng Li, Jim Mattson, Joerg Roedel,
	Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin,
	x86, kvm

On Wed, Sep 11, 2019 at 05:51:05PM +0200, Paolo Bonzini wrote:
> On 27/08/19 15:07, Jan Dakinevich wrote:
> > This series intended to fix (again) a bug that was a subject of the 
> > following change:
> > 
> >   6ea6e84 ("KVM: x86: inject exceptions produced by x86_decode_insn")
> > 
> > Suddenly, that fix had a couple mistakes. First, ctxt->have_exception was 
> > not set if fault happened during instruction decoding. Second, returning 
> > value of inject_emulated_instruction was used to make the decision to 
> > reenter guest, but this could happen iff on nested page fault, that is not 
> > the scope where this bug could occur.
> > 
> > However, I have still deep doubts about 3rd commit in the series. Could
> > you please, make me an advise if it is the correct handling of guest page 
> > fault?
> > 
> > Jan Dakinevich (3):
> >   KVM: x86: fix wrong return code
> >   KVM: x86: set ctxt->have_exception in x86_decode_insn()
> >   KVM: x86: always stop emulation on page fault
> > 
> >  arch/x86/kvm/emulate.c | 4 +++-
> >  arch/x86/kvm/x86.c     | 4 +++-
> >  2 files changed, 6 insertions(+), 2 deletions(-)
> > 
> 
> Queued, thanks.  I added the WARN_ON_ONCE that Sean suggested.

Which version did you queue?  It sounds like you queued v1, which breaks
VMware backdoor emulation due to incorrect patch ordering.  v3[*] fixes
the ordering issue and adds the WARN_ON_ONCE.

[*] https://patchwork.kernel.org/cover/11120627/

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 0/3] fix emulation error on Windows bootup
  2019-09-11 19:53   ` Sean Christopherson
@ 2019-09-13  9:51     ` Paolo Bonzini
  0 siblings, 0 replies; 14+ messages in thread
From: Paolo Bonzini @ 2019-09-13  9:51 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: Jan Dakinevich, linux-kernel, Denis Lunev, Roman Kagan,
	Denis Plotnikov, Radim Krčmář,
	Vitaly Kuznetsov, Wanpeng Li, Jim Mattson, Joerg Roedel,
	Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin,
	x86, kvm

On 11/09/19 21:53, Sean Christopherson wrote:
> On Wed, Sep 11, 2019 at 05:51:05PM +0200, Paolo Bonzini wrote:
>> On 27/08/19 15:07, Jan Dakinevich wrote:
>>> This series intended to fix (again) a bug that was a subject of the 
>>> following change:
>>>
>>>   6ea6e84 ("KVM: x86: inject exceptions produced by x86_decode_insn")
>>>
>>> Suddenly, that fix had a couple mistakes. First, ctxt->have_exception was 
>>> not set if fault happened during instruction decoding. Second, returning 
>>> value of inject_emulated_instruction was used to make the decision to 
>>> reenter guest, but this could happen iff on nested page fault, that is not 
>>> the scope where this bug could occur.
>>>
>>> However, I have still deep doubts about 3rd commit in the series. Could
>>> you please, make me an advise if it is the correct handling of guest page 
>>> fault?
>>>
>>> Jan Dakinevich (3):
>>>   KVM: x86: fix wrong return code
>>>   KVM: x86: set ctxt->have_exception in x86_decode_insn()
>>>   KVM: x86: always stop emulation on page fault
>>>
>>>  arch/x86/kvm/emulate.c | 4 +++-
>>>  arch/x86/kvm/x86.c     | 4 +++-
>>>  2 files changed, 6 insertions(+), 2 deletions(-)
>>>
>>
>> Queued, thanks.  I added the WARN_ON_ONCE that Sean suggested.
> 
> Which version did you queue?  It sounds like you queued v1, which breaks
> VMware backdoor emulation due to incorrect patch ordering.  v3[*] fixes
> the ordering issue and adds the WARN_ON_ONCE.

I applied v1 with all the fixes, then found out v3 existed and replaced
with it (but still added a comment).

Paolo


^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2019-09-13  9:51 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-27 13:07 [PATCH 0/3] fix emulation error on Windows bootup Jan Dakinevich
2019-08-27 13:07 ` [PATCH 1/3] KVM: x86: fix wrong return code Jan Dakinevich
2019-08-27 13:42   ` Sean Christopherson
2019-08-27 13:07 ` [PATCH 2/3] KVM: x86: set ctxt->have_exception in x86_decode_insn() Jan Dakinevich
2019-08-27 14:53   ` Sean Christopherson
2019-08-28 10:19     ` Jan Dakinevich
2019-08-27 13:07 ` [PATCH 3/3] KVM: x86: always stop emulation on page fault Jan Dakinevich
2019-08-27 14:50   ` Sean Christopherson
2019-08-27 14:55     ` Sean Christopherson
2019-08-28 10:19     ` Jan Dakinevich
2019-08-28 14:23       ` Sean Christopherson
2019-09-11 15:51 ` [PATCH 0/3] fix emulation error on Windows bootup Paolo Bonzini
2019-09-11 19:53   ` Sean Christopherson
2019-09-13  9:51     ` Paolo Bonzini

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).