George Dunlap
2010-Aug-27 11:23 UTC
[Xen-devel] [PATCH] ept: Put locks around ept_get_entry
There''s a subtle race in ept_get_entry, such that if tries to read an
entry that ept_set_entry is modifying, it gets neither the old entry
nor the new entry, but empty. In the case of multi-cpu populate-on-demand
guests, this manifests as a guest crash when one vcpu tries to read a
page which another page is trying to populate, and ept_get_entry returns
p2m_mmio_dm.
This bug can also be fixed by making both ept_set_entry and ept_next_level
access-once (i.e., ept_next_level reads full ept_entry and then works with
local value; ept_set_entry construct the entry locally and then sets it
in one write). But there doesn''t seem to be any major performance
implications
of just making ept_get_entry use locks; so the simpler, the better.
Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
diff -r 3c4c3d48a835 -r e17c8f37a2c2 xen/arch/x86/mm/hap/p2m-ept.c
--- a/xen/arch/x86/mm/hap/p2m-ept.c Thu Aug 26 11:16:56 2010 +0100
+++ b/xen/arch/x86/mm/hap/p2m-ept.c Fri Aug 27 12:23:27 2010 +0100
@@ -431,6 +431,10 @@
int i;
int ret = 0;
mfn_t mfn = _mfn(INVALID_MFN);
+ int do_locking = !p2m_locked_by_me(d->arch.p2m);
+
+ if ( do_locking )
+ p2m_lock(d->arch.p2m);
*t = p2m_mmio_dm;
@@ -507,6 +511,8 @@
}
out:
+ if ( do_locking )
+ p2m_unlock(d->arch.p2m);
unmap_domain_page(table);
return mfn;
}
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Christoph Egger
2010-Aug-27 11:38 UTC
Re: [Xen-devel] [PATCH] ept: Put locks around ept_get_entry
On Friday 27 August 2010 13:23:57 George Dunlap wrote:> There''s a subtle race in ept_get_entry, such that if tries to read an > entry that ept_set_entry is modifying, it gets neither the old entry > nor the new entry, but empty. In the case of multi-cpu populate-on-demand > guests, this manifests as a guest crash when one vcpu tries to read a > page which another page is trying to populate, and ept_get_entry returns > p2m_mmio_dm. > > This bug can also be fixed by making both ept_set_entry and ept_next_level > access-once (i.e., ept_next_level reads full ept_entry and then works with > local value; ept_set_entry construct the entry locally and then sets it > in one write). But there doesn''t seem to be any major performance > implications of just making ept_get_entry use locks; so the simpler, the > better. > > Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>Please use p2m_get_hostp2m(d) instead of d->arch.p2m. Christoph> diff -r 3c4c3d48a835 -r e17c8f37a2c2 xen/arch/x86/mm/hap/p2m-ept.c > --- a/xen/arch/x86/mm/hap/p2m-ept.c Thu Aug 26 11:16:56 2010 +0100 > +++ b/xen/arch/x86/mm/hap/p2m-ept.c Fri Aug 27 12:23:27 2010 +0100 > @@ -431,6 +431,10 @@ > int i; > int ret = 0; > mfn_t mfn = _mfn(INVALID_MFN); > + int do_locking = !p2m_locked_by_me(d->arch.p2m); > + > + if ( do_locking ) > + p2m_lock(d->arch.p2m); > > *t = p2m_mmio_dm; > > @@ -507,6 +511,8 @@ > } > > out: > + if ( do_locking ) > + p2m_unlock(d->arch.p2m); > unmap_domain_page(table); > return mfn; > } > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel-- ---to satisfy European Law for business letters: Advanced Micro Devices GmbH Einsteinring 24, 85609 Dornach b. Muenchen Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen Registergericht Muenchen, HRB Nr. 43632 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
George Dunlap
2010-Aug-27 11:58 UTC
Re: [Xen-devel] [PATCH] ept: Put locks around ept_get_entry
On Fri, Aug 27, 2010 at 12:38 PM, Christoph Egger <Christoph.Egger@amd.com> wrote:> Please use p2m_get_hostp2m(d) instead of d->arch.p2m....or, just use the p2m variable which was passed as a parameter to the function. :-) Sorry, I wrote this patch for XenServer (using 3.4), and didn''t take a close look. New patch on the way... -George> > Christoph > > >> diff -r 3c4c3d48a835 -r e17c8f37a2c2 xen/arch/x86/mm/hap/p2m-ept.c >> --- a/xen/arch/x86/mm/hap/p2m-ept.c Thu Aug 26 11:16:56 2010 +0100 >> +++ b/xen/arch/x86/mm/hap/p2m-ept.c Fri Aug 27 12:23:27 2010 +0100 >> @@ -431,6 +431,10 @@ >> int i; >> int ret = 0; >> mfn_t mfn = _mfn(INVALID_MFN); >> + int do_locking = !p2m_locked_by_me(d->arch.p2m); >> + >> + if ( do_locking ) >> + p2m_lock(d->arch.p2m); >> >> *t = p2m_mmio_dm; >> >> @@ -507,6 +511,8 @@ >> } >> >> out: >> + if ( do_locking ) >> + p2m_unlock(d->arch.p2m); >> unmap_domain_page(table); >> return mfn; >> } >> >> _______________________________________________ >> Xen-devel mailing list >> Xen-devel@lists.xensource.com >> http://lists.xensource.com/xen-devel > > > > -- > ---to satisfy European Law for business letters: > Advanced Micro Devices GmbH > Einsteinring 24, 85609 Dornach b. Muenchen > Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd > Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen > Registergericht Muenchen, HRB Nr. 43632 > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Christoph Egger
2010-Aug-27 12:42 UTC
Re: [Xen-devel] [PATCH] ept: Put locks around ept_get_entry
On Friday 27 August 2010 13:58:44 George Dunlap wrote:> On Fri, Aug 27, 2010 at 12:38 PM, Christoph Egger > > <Christoph.Egger@amd.com> wrote: > > Please use p2m_get_hostp2m(d) instead of d->arch.p2m. > > ...or, just use the p2m variable which was passed as a parameter to > the function. :-)That''s even better. The function defintion isn''t visible in the hunk. Christoph> Sorry, I wrote this patch for XenServer (using 3.4), and didn''t take a > close look. New patch on the way... > -George > > > Christoph > > > >> diff -r 3c4c3d48a835 -r e17c8f37a2c2 xen/arch/x86/mm/hap/p2m-ept.c > >> --- a/xen/arch/x86/mm/hap/p2m-ept.c Thu Aug 26 11:16:56 2010 +0100 > >> +++ b/xen/arch/x86/mm/hap/p2m-ept.c Fri Aug 27 12:23:27 2010 +0100 > >> @@ -431,6 +431,10 @@ > >> int i; > >> int ret = 0; > >> mfn_t mfn = _mfn(INVALID_MFN); > >> + int do_locking = !p2m_locked_by_me(d->arch.p2m); > >> + > >> + if ( do_locking ) > >> + p2m_lock(d->arch.p2m); > >> > >> *t = p2m_mmio_dm; > >> > >> @@ -507,6 +511,8 @@ > >> } > >> > >> out: > >> + if ( do_locking ) > >> + p2m_unlock(d->arch.p2m); > >> unmap_domain_page(table); > >> return mfn; > >> }-- ---to satisfy European Law for business letters: Advanced Micro Devices GmbH Einsteinring 24, 85609 Dornach b. Muenchen Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen Registergericht Muenchen, HRB Nr. 43632 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel