From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH 2/2] Xen/mem_event: Prevent underflow of vcpu pause counts Date: Thu, 17 Jul 2014 19:55:05 +0100 Message-ID: <53C81C09.6070802@citrix.com> References: <1405602637-8321-1-git-send-email-andrew.cooper3@citrix.com> <1405602637-8321-3-git-send-email-andrew.cooper3@citrix.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============8712368068408197500==" Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Andres Lagar Cavilla Cc: Tim Deegan , Jan Beulich , Xen-devel List-Id: xen-devel@lists.xenproject.org --===============8712368068408197500== Content-Type: multipart/alternative; boundary="------------040607010306020806090406" --------------040607010306020806090406 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit On 17/07/14 19:38, Andres Lagar Cavilla wrote: > On Thu, Jul 17, 2014 at 9:10 AM, Andrew Cooper > > wrote: > > Signed-off-by: Andrew Cooper > > CC: Jan Beulich > > CC: Tim Deegan > > CC: Andres Lagar-Cavilla > > > Not so sure about this one, see below. > > --- > xen/arch/x86/hvm/hvm.c | 2 +- > xen/arch/x86/mm/mem_event.c | 14 ++++++++++++++ > xen/arch/x86/mm/mem_sharing.c | 4 ++-- > xen/arch/x86/mm/p2m.c | 8 ++++---- > xen/include/asm-x86/mem_event.h | 3 +++ > xen/include/xen/sched.h | 2 ++ > 6 files changed, 26 insertions(+), 7 deletions(-) > > diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c > index ef2411c..efd79b8 100644 > --- a/xen/arch/x86/hvm/hvm.c > +++ b/xen/arch/x86/hvm/hvm.c > @@ -6113,7 +6113,7 @@ static int hvm_memory_event_traps(long p, > uint32_t reason, > if ( (p & HVMPME_MODE_MASK) == HVMPME_mode_sync ) > { > req.flags |= MEM_EVENT_FLAG_VCPU_PAUSED; > - vcpu_pause_nosync(v); > + mem_event_vcpu_pause(v); > } > > req.gfn = value; > diff --git a/xen/arch/x86/mm/mem_event.c b/xen/arch/x86/mm/mem_event.c > index 40ae841..ef5197c 100644 > --- a/xen/arch/x86/mm/mem_event.c > +++ b/xen/arch/x86/mm/mem_event.c > @@ -663,6 +663,20 @@ int mem_event_domctl(struct domain *d, > xen_domctl_mem_event_op_t *mec, > return rc; > } > > +void mem_event_vcpu_pause(struct vcpu *v) > +{ > + ASSERT(v == current); > + > + /* Ensure we don't try to repeatedly pause the same vcpu. */ > + BUG_ON(test_and_set_bool(v->paused_for_mem_event) != 0); > > This is a problem. It relies on a vcpu being able to cause a single > mem event during a vmexit. I don't think that can be guaranteed. While > I can't pinpoint the exact conversation from years ago, it is not hard > to imagine scenarios in which an mmio emulation can touch multiple > pages. So I don't think we can BUG_ON at all. Hmm - Would the emulator not fail entirely after first fail only? Still - it is probably better to accept multiple pause requests. > + vcpu_pause_nosync(v); > +} > + > +void mem_event_vcpu_unpause(struct vcpu *v) > +{ > + if ( test_and_clear_bool(v->paused_for_mem_event) ) > > And now that we consider more than one mem event piling up to pause a > vcpu, this has to become an atomic counter, which unpauses on zero, > and takes care of underflow. I shall spin a v2 with this problem addressed. I suspect it will look curiously similar to the DOMCTL_{,un}pausedomain refcounting fix which suffered from exactly the same problem wrt underflow/overflow of the pause count. ~Andrew > > This is great and thanks for catching these things. > Andres > > + vcpu_unpause(v); > +} > > /* > * Local variables: > diff --git a/xen/arch/x86/mm/mem_sharing.c > b/xen/arch/x86/mm/mem_sharing.c > index ec99266..79188b9 100644 > --- a/xen/arch/x86/mm/mem_sharing.c > +++ b/xen/arch/x86/mm/mem_sharing.c > @@ -568,7 +568,7 @@ int mem_sharing_notify_enomem(struct domain > *d, unsigned long gfn, > if ( v->domain == d ) > { > req.flags = MEM_EVENT_FLAG_VCPU_PAUSED; > - vcpu_pause_nosync(v); > + mem_event_vcpu_pause(v); > } > > req.p2mt = p2m_ram_shared; > @@ -609,7 +609,7 @@ int mem_sharing_sharing_resume(struct domain *d) > > /* Unpause domain/vcpu */ > if ( rsp.flags & MEM_EVENT_FLAG_VCPU_PAUSED ) > - vcpu_unpause(v); > + mem_event_vcpu_unpause(v); > } > > return 0; > diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c > index f213a39..2c7bc0f 100644 > --- a/xen/arch/x86/mm/p2m.c > +++ b/xen/arch/x86/mm/p2m.c > @@ -1158,7 +1158,7 @@ void p2m_mem_paging_populate(struct domain > *d, unsigned long gfn) > /* Pause domain if request came from guest and gfn has paging > type */ > if ( p2m_is_paging(p2mt) && v->domain == d ) > { > - vcpu_pause_nosync(v); > + mem_event_vcpu_pause(v); > req.flags |= MEM_EVENT_FLAG_VCPU_PAUSED; > } > /* No need to inform pager if the gfn is not in the page-out > path */ > @@ -1319,7 +1319,7 @@ void p2m_mem_paging_resume(struct domain *d) > } > /* Unpause domain */ > if ( rsp.flags & MEM_EVENT_FLAG_VCPU_PAUSED ) > - vcpu_unpause(v); > + mem_event_vcpu_unpause(v); > } > } > > @@ -1414,7 +1414,7 @@ bool_t p2m_mem_access_check(paddr_t gpa, > bool_t gla_valid, unsigned long gla, > > /* Pause the current VCPU */ > if ( p2ma != p2m_access_n2rwx ) > - vcpu_pause_nosync(v); > + mem_event_vcpu_pause(v); > > /* VCPU may be paused, return whether we promoted > automatically */ > return (p2ma == p2m_access_n2rwx); > @@ -1440,7 +1440,7 @@ void p2m_mem_access_resume(struct domain *d) > > /* Unpause domain */ > if ( rsp.flags & MEM_EVENT_FLAG_VCPU_PAUSED ) > - vcpu_unpause(v); > + mem_event_vcpu_unpause(v); > } > } > > diff --git a/xen/include/asm-x86/mem_event.h > b/xen/include/asm-x86/mem_event.h > index 045ef9b..ed4481a 100644 > --- a/xen/include/asm-x86/mem_event.h > +++ b/xen/include/asm-x86/mem_event.h > @@ -66,6 +66,9 @@ int do_mem_event_op(int op, uint32_t domain, > void *arg); > int mem_event_domctl(struct domain *d, xen_domctl_mem_event_op_t > *mec, > XEN_GUEST_HANDLE_PARAM(void) u_domctl); > > +void mem_event_vcpu_pause(struct vcpu *v); > +void mem_event_vcpu_unpause(struct vcpu *v); > + > #endif /* __MEM_EVENT_H__ */ > > > diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h > index 2f876f5..ea33b7d 100644 > --- a/xen/include/xen/sched.h > +++ b/xen/include/xen/sched.h > @@ -199,6 +199,8 @@ struct vcpu > bool_t paused_for_shutdown; > /* VCPU need affinity restored */ > bool_t affinity_broken; > + /* VCPU paused for mem event reply. */ > + bool_t paused_for_mem_event; > > > /* > -- > 1.7.10.4 > > --------------040607010306020806090406 Content-Type: text/html; charset="UTF-8" Content-Length: 13156 Content-Transfer-Encoding: quoted-printable
On 17/07/14 19:38, Andres Lagar Cavilla wrote:
On Thu, Jul 17, 2014 at 9:10 AM, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Jan Beulich <JBeulich@suse.com>
CC: Tim Deegan <tim@xen.org>
CC: Andres Lagar-Cavilla <andres@lagarcavilla.org>
Not so sure about this one, see below.=C2=A0
---
=C2=A0xen/arch/x86/hvm/hvm.c =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0| =C2=A0 =C2=A02 +-
=C2=A0xen/arch/x86/mm/mem_event.c =C2=A0 =C2=A0 | =C2=A0 14 ++++++++++++++
=C2=A0xen/arch/x86/mm/mem_sharing.c =C2=A0 | =C2=A0 =C2=A04 ++--
=C2=A0xen/arch/x86/mm/p2m.c =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 | =C2=A0 =C2=A08 ++++----
=C2=A0xen/include/asm-x86/mem_event.h | =C2=A0 =C2=A03 +++
=C2=A0xen/include/xen/sched.h =C2=A0 =C2=A0 =C2=A0 =C2=A0 | =C2=A0 =C2=A02 ++
=C2=A06 files changed, 26 insertions(+), 7 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index ef2411c..efd79b8 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -6113,7 +6113,7 @@ static int hvm_memory_event_traps(long p, uint32_t reason,
=C2=A0 =C2=A0 =C2=A0if ( (p & HVMPME_MODE_MASK) =3D=3D HVMPME_mode_sync )
=C2=A0 =C2=A0 =C2=A0{
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0req.flags |=3D MEM_EVENT_FLAG_VCPU_PAUSED;
- =C2=A0 =C2=A0 =C2=A0 =C2=A0vcpu_pause_nosync(v);
+ =C2=A0 =C2=A0 =C2=A0 =C2=A0mem_event_vcpu_pause(v);
=C2=A0 =C2=A0 =C2=A0}

=C2=A0 =C2=A0 =C2=A0req.gfn =3D value;
diff --git a/xen/arch/x86/mm/mem_event.c b/xen/arch/x86/mm/mem_event.c
index 40ae841..ef5197c 100644
--- a/xen/arch/x86/mm/mem_event.c
+++ b/xen/arch/x86/mm/mem_event.c
@@ -663,6 +663,20 @@ int mem_event_domctl(struct domain *d, xen_domctl_mem_event_op_t *mec,
=C2=A0 =C2=A0 =C2=A0return rc;
=C2=A0}

+void mem_event_vcpu_pause(struct vcpu *v)
+{
+ =C2=A0 =C2=A0ASSERT(v =3D=3D current);
+
+ =C2=A0 =C2=A0/* Ensure we don't try to repeatedly pause the same vcpu. */
+ =C2=A0 =C2=A0BUG_ON(test_and_set_bool(v->paused_for_mem_event) !=3D 0);
This is a problem. It relies on a vcpu being able to cause a single mem event during a vmexit. I don't think that can be guaranteed. While I can't pinpoint the exact conversation from years ago, it is not hard to imagine scenarios in which an mmio emulation can touch multiple pages. So I don't think we can BUG_ON at all.

Hmm - Would the emulator not fail entirely after first fail only=3F

Still - it is probably better to accept multiple pause requests.

+ =C2=A0 =C2=A0vcpu_pause_nosync(v);
+}
+
+void mem_event_vcpu_unpause(struct vcpu *v)
+{
+ =C2=A0 =C2=A0if ( test_and_clear_bool(v->paused_for_mem_event) )
And now that we consider more than one mem event piling up to pause a vcpu, this has to become an atomic counter, which unpauses on zero, and takes care of underflow.

I shall spin a v2 with this problem addressed.=C2=A0 I suspect it will look curiously similar to the DOMCTL_{,un}pausedomain refcounting fix which suffered from exactly the same problem wrt underflow/overflow of the pause count.

~Andrew


This is great and thanks for catching these things.
Andres=C2=A0
+ =C2=A0 =C2=A0 =C2=A0 =C2=A0vcpu_unpause(v);
+}

=C2=A0/*
=C2=A0 * Local variables:
diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
index ec99266..79188b9 100644
--- a/xen/arch/x86/mm/mem_sharing.c
+++ b/xen/arch/x86/mm/mem_sharing.c
@@ -568,7 +568,7 @@ int mem_sharing_notify_enomem(struct domain *d, unsigned long gfn,
=C2=A0 =C2=A0 =C2=A0if ( v->domain =3D=3D d )
=C2=A0 =C2=A0 =C2=A0{
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0req.flags =3D MEM_EVENT_FLAG_VCPU_PAUSED;
- =C2=A0 =C2=A0 =C2=A0 =C2=A0vcpu_pause_nosync(v);
+ =C2=A0 =C2=A0 =C2=A0 =C2=A0mem_event_vcpu_pause(v);
=C2=A0 =C2=A0 =C2=A0}

=C2=A0 =C2=A0 =C2=A0req.p2mt =3D p2m_ram_shared;
@@ -609,7 +609,7 @@ int mem_sharing_sharing_resume(struct domain *d)

=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0/* Unpause domain/vcpu */
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if ( rsp.flags & MEM_EVENT_FLAG_VCPU_PAUSED )
- =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0vcpu_unpause(v);
+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0mem_event_vcpu_unpause(v);
=C2=A0 =C2=A0 =C2=A0}

=C2=A0 =C2=A0 =C2=A0return 0;
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index f213a39..2c7bc0f 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1158,7 +1158,7 @@ void p2m_mem_paging_populate(struct domain *d, unsigned long gfn)
=C2=A0 =C2=A0 =C2=A0/* Pause domain if request came from guest and gfn has paging type */
=C2=A0 =C2=A0 =C2=A0if ( p2m_is_paging(p2mt) && v->domain =3D=3D d )
=C2=A0 =C2=A0 =C2=A0{
- =C2=A0 =C2=A0 =C2=A0 =C2=A0vcpu_pause_nosync(v);
+ =C2=A0 =C2=A0 =C2=A0 =C2=A0mem_event_vcpu_pause(v);
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0req.flags |=3D MEM_EVENT_FLAG_VCPU_PAUSED;
=C2=A0 =C2=A0 =C2=A0}
=C2=A0 =C2=A0 =C2=A0/* No need to inform pager if the gfn is not in the page-out path */
@@ -1319,7 +1319,7 @@ void p2m_mem_paging_resume(struct domain *d)
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0}
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0/* Unpause domain */
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if ( rsp.flags & MEM_EVENT_FLAG_VCPU_PAUSED )
- =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0vcpu_unpause(v);
+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0mem_event_vcpu_unpause(v);
=C2=A0 =C2=A0 =C2=A0}
=C2=A0}

@@ -1414,7 +1414,7 @@ bool_t p2m_mem_access_check(paddr_t gpa, bool_t gla_valid, unsigned long gla,

=C2=A0 =C2=A0 =C2=A0/* Pause the current VCPU */
=C2=A0 =C2=A0 =C2=A0if ( p2ma !=3D p2m_access_n2rwx )
- =C2=A0 =C2=A0 =C2=A0 =C2=A0vcpu_pause_nosync(v);
+ =C2=A0 =C2=A0 =C2=A0 =C2=A0mem_event_vcpu_pause(v);

=C2=A0 =C2=A0 =C2=A0/* VCPU may be paused, return whether we promoted automatically */
=C2=A0 =C2=A0 =C2=A0return (p2ma =3D=3D p2m_access_n2rwx);
@@ -1440,7 +1440,7 @@ void p2m_mem_access_resume(struct domain *d)

=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0/* Unpause domain */
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if ( rsp.flags & MEM_EVENT_FLAG_VCPU_PAUSED )
- =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0vcpu_unpause(v);
+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0mem_event_vcpu_unpause(v);
=C2=A0 =C2=A0 =C2=A0}
=C2=A0}

diff --git a/xen/include/asm-x86/mem_event.h b/xen/include/asm-x86/mem_event.h
index 045ef9b..ed4481a 100644
--- a/xen/include/asm-x86/mem_event.h
+++ b/xen/include/asm-x86/mem_event.h
@@ -66,6 +66,9 @@ int do_mem_event_op(int op, uint32_t domain, void *arg);
=C2=A0int mem_event_domctl(struct domain *d, xen_domctl_mem_event_op_t *mec,
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 XEN_GUEST_HANDLE_PARAM(void) u_domctl);

+void mem_event_vcpu_pause(struct vcpu *v);
+void mem_event_vcpu_unpause(struct vcpu *v);
+
=C2=A0#endif /* __MEM_EVENT_H__ */


diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 2f876f5..ea33b7d 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -199,6 +199,8 @@ struct vcpu
=C2=A0 =C2=A0 =C2=A0bool_t =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 paused_for_shutdown;
=C2=A0 =C2=A0 =C2=A0/* VCPU need affinity restored */
=C2=A0 =C2=A0 =C2=A0bool_t =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 affinity_broken;
+ =C2=A0 =C2=A0/* VCPU paused for mem event reply. */
+ =C2=A0 =C2=A0bool_t =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 paused_for_mem_event;


=C2=A0 =C2=A0 =C2=A0/*
--
1.7.10.4



--------------040607010306020806090406-- --===============8712368068408197500== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel --===============8712368068408197500==--