* [PATCH master/3.5.y] KVM: VMX: Fix ds/es corruption on i386 with preemption
@ 2012-08-01 13:48 Avi Kivity
2012-08-01 14:36 ` Chris Clayton
0 siblings, 1 reply; 3+ messages in thread
From: Avi Kivity @ 2012-08-01 13:48 UTC (permalink / raw)
To: Marcelo Tosatti; +Cc: kvm, chris2553
Commit b2da15ac26a0c ("KVM: VMX: Optimize %ds, %es reload") broke i386
in the following scenario:
vcpu_load
...
vmx_save_host_state
vmx_vcpu_run
(ds.rpl, es.rpl cleared by hardware)
interrupt
push ds, es # pushes bad ds, es
schedule
vmx_vcpu_put
vmx_load_host_state
reload ds, es (with __USER_DS)
pop ds, es # of other thread's stack
iret
# other thread runs
interrupt
push ds, es
schedule # back in vcpu thread
pop ds, es # now with rpl=0
iret
...
vcpu_put
resume_userspace
iret # clears ds, es due to mismatched rpl
(instead of resume_userspace, we might return with SYSEXIT and then
take an exception; when the exception IRETs we end up with cleared
ds, es)
Fix by avoiding the optimization on i386 and reloading ds, es on the
lightweight exit path.
Reported-by: Chris Clayron <chris2553@googlemail.com>
Signed-off-by: Avi Kivity <avi@redhat.com>
---
arch/x86/kvm/vmx.c | 20 +++++++++++++-------
1 file changed, 13 insertions(+), 7 deletions(-)
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index c39b607..c00f03d 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -1488,13 +1488,6 @@ static void __vmx_load_host_state(struct vcpu_vmx *vmx)
loadsegment(ds, vmx->host_state.ds_sel);
loadsegment(es, vmx->host_state.es_sel);
}
-#else
- /*
- * The sysexit path does not restore ds/es, so we must set them to
- * a reasonable value ourselves.
- */
- loadsegment(ds, __USER_DS);
- loadsegment(es, __USER_DS);
#endif
reload_tss();
#ifdef CONFIG_X86_64
@@ -6370,6 +6363,19 @@ static void __noclone vmx_vcpu_run(struct kvm_vcpu *vcpu)
#endif
);
+#ifndef CONFIG_X86_64
+ /*
+ * The sysexit path does not restore ds/es, so we must set them to
+ * a reasonable value ourselves.
+ *
+ * We can't defer this to vmx_load_host_state() since that function
+ * may be executed in interrupt context, which saves and restore segments
+ * around it, nullifying its effect.
+ */
+ loadsegment(ds, __USER_DS);
+ loadsegment(es, __USER_DS);
+#endif
+
vcpu->arch.regs_avail = ~((1 << VCPU_REGS_RIP) | (1 << VCPU_REGS_RSP)
| (1 << VCPU_EXREG_RFLAGS)
| (1 << VCPU_EXREG_CPL)
--
1.7.11.3
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH master/3.5.y] KVM: VMX: Fix ds/es corruption on i386 with preemption
2012-08-01 13:48 [PATCH master/3.5.y] KVM: VMX: Fix ds/es corruption on i386 with preemption Avi Kivity
@ 2012-08-01 14:36 ` Chris Clayton
2012-08-07 7:09 ` Fwd: " Chris Clayton
0 siblings, 1 reply; 3+ messages in thread
From: Chris Clayton @ 2012-08-01 14:36 UTC (permalink / raw)
To: Avi Kivity; +Cc: Marcelo Tosatti, kvm
On 08/01/12 14:48, Avi Kivity wrote:
> Commit b2da15ac26a0c ("KVM: VMX: Optimize %ds, %es reload") broke i386
> in the following scenario:
>
> vcpu_load
> ...
> vmx_save_host_state
> vmx_vcpu_run
> (ds.rpl, es.rpl cleared by hardware)
>
> interrupt
> push ds, es # pushes bad ds, es
> schedule
> vmx_vcpu_put
> vmx_load_host_state
> reload ds, es (with __USER_DS)
> pop ds, es # of other thread's stack
> iret
> # other thread runs
> interrupt
> push ds, es
> schedule # back in vcpu thread
> pop ds, es # now with rpl=0
> iret
> ...
> vcpu_put
> resume_userspace
> iret # clears ds, es due to mismatched rpl
>
> (instead of resume_userspace, we might return with SYSEXIT and then
> take an exception; when the exception IRETs we end up with cleared
> ds, es)
>
> Fix by avoiding the optimization on i386 and reloading ds, es on the
> lightweight exit path.
>
> Reported-by: Chris Clayron <chris2553@googlemail.com>
> Signed-off-by: Avi Kivity <avi@redhat.com>
I've just had 15 successful runs of qemu-kvm-1.1.1 with kernel 3.5.0
plus this patch, so:
Tested-by: Chris Clayton <chris2553@googlemail.com>
I assume this will be forwarded to stable once it has been applied to
mainline.
> ---
> arch/x86/kvm/vmx.c | 20 +++++++++++++-------
> 1 file changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index c39b607..c00f03d 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -1488,13 +1488,6 @@ static void __vmx_load_host_state(struct vcpu_vmx *vmx)
> loadsegment(ds, vmx->host_state.ds_sel);
> loadsegment(es, vmx->host_state.es_sel);
> }
> -#else
> - /*
> - * The sysexit path does not restore ds/es, so we must set them to
> - * a reasonable value ourselves.
> - */
> - loadsegment(ds, __USER_DS);
> - loadsegment(es, __USER_DS);
> #endif
> reload_tss();
> #ifdef CONFIG_X86_64
> @@ -6370,6 +6363,19 @@ static void __noclone vmx_vcpu_run(struct kvm_vcpu *vcpu)
> #endif
> );
>
> +#ifndef CONFIG_X86_64
> + /*
> + * The sysexit path does not restore ds/es, so we must set them to
> + * a reasonable value ourselves.
> + *
> + * We can't defer this to vmx_load_host_state() since that function
> + * may be executed in interrupt context, which saves and restore segments
> + * around it, nullifying its effect.
> + */
> + loadsegment(ds, __USER_DS);
> + loadsegment(es, __USER_DS);
> +#endif
> +
> vcpu->arch.regs_avail = ~((1 << VCPU_REGS_RIP) | (1 << VCPU_REGS_RSP)
> | (1 << VCPU_EXREG_RFLAGS)
> | (1 << VCPU_EXREG_CPL)
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Fwd: Re: [PATCH master/3.5.y] KVM: VMX: Fix ds/es corruption on i386 with preemption
2012-08-01 14:36 ` Chris Clayton
@ 2012-08-07 7:09 ` Chris Clayton
0 siblings, 0 replies; 3+ messages in thread
From: Chris Clayton @ 2012-08-07 7:09 UTC (permalink / raw)
To: Avi Kivity, Marcelo Tosatti, kvm
HI,
I've just been looking through the patches Greg KH has stored for
3.5-stable and this doesn't seem to be amongst them.
I can't see any way in which it contravenes the stable rules and, given
there's a distinct possibility of a mix of i386, kvm-1.1.x and a 3.5
kernel resulting in a crash, it seems to me to be a good candidate. The
commit in mainline is aa67f6096c19bcdb1951ef88be3cf3d2118809dc.
Thanks.
-------- Original Message --------
Subject: Re: [PATCH master/3.5.y] KVM: VMX: Fix ds/es corruption on i386
with preemption
Date: Wed, 01 Aug 2012 15:36:30 +0100
From: Chris Clayton <chris2553@googlemail.com>
To: Avi Kivity <avi@redhat.com>
CC: Marcelo Tosatti <mtosatti@redhat.com>, kvm@vger.kernel.org
On 08/01/12 14:48, Avi Kivity wrote:
> Commit b2da15ac26a0c ("KVM: VMX: Optimize %ds, %es reload") broke i386
> in the following scenario:
>
> vcpu_load
> ...
> vmx_save_host_state
> vmx_vcpu_run
> (ds.rpl, es.rpl cleared by hardware)
>
> interrupt
> push ds, es # pushes bad ds, es
> schedule
> vmx_vcpu_put
> vmx_load_host_state
> reload ds, es (with __USER_DS)
> pop ds, es # of other thread's stack
> iret
> # other thread runs
> interrupt
> push ds, es
> schedule # back in vcpu thread
> pop ds, es # now with rpl=0
> iret
> ...
> vcpu_put
> resume_userspace
> iret # clears ds, es due to mismatched rpl
>
> (instead of resume_userspace, we might return with SYSEXIT and then
> take an exception; when the exception IRETs we end up with cleared
> ds, es)
>
> Fix by avoiding the optimization on i386 and reloading ds, es on the
> lightweight exit path.
>
> Reported-by: Chris Clayron <chris2553@googlemail.com>
> Signed-off-by: Avi Kivity <avi@redhat.com>
I've just had 15 successful runs of qemu-kvm-1.1.1 with kernel 3.5.0
plus this patch, so:
Tested-by: Chris Clayton <chris2553@googlemail.com>
I assume this will be forwarded to stable once it has been applied to
mainline.
> ---
> arch/x86/kvm/vmx.c | 20 +++++++++++++-------
> 1 file changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index c39b607..c00f03d 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -1488,13 +1488,6 @@ static void __vmx_load_host_state(struct vcpu_vmx *vmx)
> loadsegment(ds, vmx->host_state.ds_sel);
> loadsegment(es, vmx->host_state.es_sel);
> }
> -#else
> - /*
> - * The sysexit path does not restore ds/es, so we must set them to
> - * a reasonable value ourselves.
> - */
> - loadsegment(ds, __USER_DS);
> - loadsegment(es, __USER_DS);
> #endif
> reload_tss();
> #ifdef CONFIG_X86_64
> @@ -6370,6 +6363,19 @@ static void __noclone vmx_vcpu_run(struct kvm_vcpu *vcpu)
> #endif
> );
>
> +#ifndef CONFIG_X86_64
> + /*
> + * The sysexit path does not restore ds/es, so we must set them to
> + * a reasonable value ourselves.
> + *
> + * We can't defer this to vmx_load_host_state() since that function
> + * may be executed in interrupt context, which saves and restore segments
> + * around it, nullifying its effect.
> + */
> + loadsegment(ds, __USER_DS);
> + loadsegment(es, __USER_DS);
> +#endif
> +
> vcpu->arch.regs_avail = ~((1 << VCPU_REGS_RIP) | (1 << VCPU_REGS_RSP)
> | (1 << VCPU_EXREG_RFLAGS)
> | (1 << VCPU_EXREG_CPL)
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2012-08-07 7:09 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-08-01 13:48 [PATCH master/3.5.y] KVM: VMX: Fix ds/es corruption on i386 with preemption Avi Kivity
2012-08-01 14:36 ` Chris Clayton
2012-08-07 7:09 ` Fwd: " Chris Clayton
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).