From mboxrd@z Thu Jan 1 00:00:00 1970 From: Chris Clayton Subject: Fwd: Re: [PATCH master/3.5.y] KVM: VMX: Fix ds/es corruption on i386 with preemption Date: Tue, 07 Aug 2012 08:09:20 +0100 Message-ID: <5020BF20.5020203@googlemail.com> References: <50193EEE.2010000@googlemail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit To: Avi Kivity , Marcelo Tosatti , kvm@vger.kernel.org Return-path: Received: from mail-ey0-f174.google.com ([209.85.215.174]:57888 "EHLO mail-ey0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750901Ab2HGHJQ (ORCPT ); Tue, 7 Aug 2012 03:09:16 -0400 Received: by eaac11 with SMTP id c11so1055558eaa.19 for ; Tue, 07 Aug 2012 00:09:14 -0700 (PDT) In-Reply-To: <50193EEE.2010000@googlemail.com> Sender: kvm-owner@vger.kernel.org List-ID: 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 To: Avi Kivity CC: Marcelo Tosatti , 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 > Signed-off-by: Avi Kivity 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 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) >