From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH v3 2/2] Xen/mem_event: Prevent underflow of vcpu pause counts Date: Fri, 18 Jul 2014 17:44:40 +0100 Message-ID: <53C94EF8.3010504@citrix.com> References: <1405680093-31763-1-git-send-email-andrew.cooper3@citrix.com> <1405691588-1186-1-git-send-email-andrew.cooper3@citrix.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1757339753075279287==" 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: Aravindh Puthiyaparambil , Tim Deegan , Jan Beulich , Xen-devel List-Id: xen-devel@lists.xenproject.org --===============1757339753075279287== Content-Type: multipart/alternative; boundary="------------090809010904020603070602" --------------090809010904020603070602 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit On 18/07/14 17:37, Andres Lagar Cavilla wrote: > On Fri, Jul 18, 2014 at 9:53 AM, Andrew Cooper > > wrote: > > Signed-off-by: Andrew Cooper > > Tested-by: Razvan Cojocaru > > CC: Jan Beulich > > CC: Tim Deegan > > CC: Andres Lagar-Cavilla > > CC: Aravindh Puthiyaparambil > > > Reviewed-by: Andres Lagar-Cavilla > > > Couple nits below ... > > > -- > v3: > * Newline on warning > v2: > * Allow for multiple pause refcounts > --- > xen/arch/x86/hvm/hvm.c | 2 +- > xen/arch/x86/mm/mem_event.c | 31 > +++++++++++++++++++++++++++++++ > 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 | 3 +++ > 6 files changed, 44 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..ba7e71e 100644 > --- a/xen/arch/x86/mm/mem_event.c > +++ b/xen/arch/x86/mm/mem_event.c > @@ -663,6 +663,37 @@ 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); > + > + atomic_inc(&v->mem_event_pause_count); > > Nit #1: A buggy toolstack going into an infinite loop could overflow > this. No it can't (or rather, I am fairly certain it can't). This is unconditionally in the context of a running vcpu, given the assertion above. This cannot increment the refcount more than number of mem_events triggered from a single exit into Xen. > + vcpu_pause_nosync(v); > +} > + > +void mem_event_vcpu_unpause(struct vcpu *v) > +{ > + int old, new, prev = v->mem_event_pause_count.counter; > > Nit #2: not fresh in my mind whether atomic is supposed to be signed > or unsigned. The flawless comparison below is to take these all as > unsigned and check for new > old. > > Thanks > Andres The inards of an atomic_t is signed. Ideally I would make use of atomic_dec_bounded() from an early version of my pausedomain patch, but Jan requested that I drop that part for easier backport, given only a single consumer. Given new consumers, that decision might be up for reconsideration. ~Andrew --------------090809010904020603070602 Content-Type: text/html; charset="UTF-8" Content-Length: 7272 Content-Transfer-Encoding: quoted-printable
On 18/07/14 17:37, Andres Lagar Cavilla wrote:
On Fri, Jul 18, 2014 at 9:53 AM, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Tested-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
CC: Jan Beulich <JBeulich@suse.com>
CC: Tim Deegan <tim@xen.org>
CC: Andres Lagar-Cavilla <andres@lagarcavilla.org>
CC: Aravindh Puthiyaparambil <aravindp@cisco.com>
Reviewed-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>

Couple nits below ...

--
v3:
=C2=A0* Newline on warning
v2:
=C2=A0* Allow for multiple pause refcounts
---
=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 31 +++++++++++++++++++++++++++++++
=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=A03 +++
=C2=A06 files changed, 44 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..ba7e71e 100644
--- a/xen/arch/x86/mm/mem_event.c
+++ b/xen/arch/x86/mm/mem_event.c
@@ -663,6 +663,37 @@ 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=A0atomic_inc(&v->mem_event_pause_count);
Nit #1: A buggy toolstack going into an infinite loop could overflow this.

No it can't (or rather, I am fairly certain it can't).=C2=A0 This is unconditionally in the context of a running vcpu, given the assertion above.=C2=A0 This cannot increment the refcount more than number of mem_events triggered from a single exit into Xen.

+ =C2=A0 =C2=A0vcpu_pause_nosync(v);
+}
+
+void mem_event_vcpu_unpause(struct vcpu *v)
+{
+ =C2=A0 =C2=A0int old, new, prev =3D v->mem_event_pause_count.counter;
Nit #2: not fresh in my mind whether atomic is supposed to be signed or unsigned. The flawless comparison below is to take these all as unsigned and check for new > old.

Thanks
Andres

The inards of an atomic_t is signed.

Ideally I would make use of atomic_dec_bounded() from an early version of my pausedomain patch, but Jan requested that I drop that part for easier backport, given only a single consumer.

Given new consumers, that decision might be up for reconsideration.

~Andrew
--------------090809010904020603070602-- --===============1757339753075279287== 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 --===============1757339753075279287==--