[PATCH 0/2] Xen/mem_event: Do not rely on the toolstack being bug-free

[PATCH 0/2] Xen/mem_event: Do not rely on the toolstack being bug-free

[Andrew Cooper]

Xen performs insufficient validation of the contents of mem_event responses
from the toolstack.  As a result, a buggy toolstack could cause Xen to walk
off the end of a domain's vcpu list, and get out of sync with vcpu pause
reference counts.

These two fixes are compile tested only, as I have no way to plausibly test
the mem-event functionality itself.

Andrew Cooper (2):
  Xen/mem_event: Validate the response vcpu_id before acting on it.
  Xen/mem_event: Prevent underflow of vcpu pause counts

 xen/arch/x86/hvm/hvm.c          |    2 +-
 xen/arch/x86/mm/mem_event.c     |   14 ++++++++++++++
 xen/arch/x86/mm/mem_sharing.c   |   13 +++++++++++--
 xen/arch/x86/mm/p2m.c           |   26 ++++++++++++++++++++++----
 xen/include/asm-x86/mem_event.h |    3 +++
 xen/include/xen/sched.h         |    2 ++
 6 files changed, 53 insertions(+), 7 deletions(-)

[PATCH 1/2] Xen/mem_event: Validate the response vcpu_id before acting on it.

[Andrew Cooper]

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>
---
 xen/arch/x86/mm/mem_sharing.c |   11 ++++++++++-
 xen/arch/x86/mm/p2m.c         |   22 ++++++++++++++++++++--
 2 files changed, 30 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
index 7293f31..ec99266 100644
--- a/xen/arch/x86/mm/mem_sharing.c
+++ b/xen/arch/x86/mm/mem_sharing.c
@@ -596,11 +596,20 @@ int mem_sharing_sharing_resume(struct domain *d)
     /* Get all requests off the ring */
     while ( mem_event_get_response(d, &d->mem_event->share, &rsp) )
     {
+        struct vcpu *v;
+
         if ( rsp.flags & MEM_EVENT_FLAG_DUMMY )
             continue;
+
+        /* Validate the vcpu_id in the response. */
+        if ( (rsp.vcpu_id >= d->max_vcpus) || !d->vcpu[rsp.vcpu_id] )
+            continue;
+
+        v = d->vcpu[rsp.vcpu_id];
+
         /* Unpause domain/vcpu */
         if ( rsp.flags & MEM_EVENT_FLAG_VCPU_PAUSED )
-            vcpu_unpause(d->vcpu[rsp.vcpu_id]);
+            vcpu_unpause(v);
     }
 
     return 0;
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 642ec28..f213a39 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1290,8 +1290,17 @@ void p2m_mem_paging_resume(struct domain *d)
     /* Pull all responses off the ring */
     while( mem_event_get_response(d, &d->mem_event->paging, &rsp) )
     {
+        struct vcpu *v;
+
         if ( rsp.flags & MEM_EVENT_FLAG_DUMMY )
             continue;
+
+        /* Validate the vcpu_id in the response. */
+        if ( (rsp.vcpu_id >= d->max_vcpus) || !d->vcpu[rsp.vcpu_id] )
+            continue;
+
+        v = d->vcpu[rsp.vcpu_id];
+
         /* Fix p2m entry if the page was not dropped */
         if ( !(rsp.flags & MEM_EVENT_FLAG_DROP_PAGE) )
         {
@@ -1310,7 +1319,7 @@ void p2m_mem_paging_resume(struct domain *d)
         }
         /* Unpause domain */
         if ( rsp.flags & MEM_EVENT_FLAG_VCPU_PAUSED )
-            vcpu_unpause(d->vcpu[rsp.vcpu_id]);
+            vcpu_unpause(v);
     }
 }
 
@@ -1418,11 +1427,20 @@ void p2m_mem_access_resume(struct domain *d)
     /* Pull all responses off the ring */
     while( mem_event_get_response(d, &d->mem_event->access, &rsp) )
     {
+        struct vcpu *v;
+
         if ( rsp.flags & MEM_EVENT_FLAG_DUMMY )
             continue;
+
+        /* Validate the vcpu_id in the response. */
+        if ( (rsp.vcpu_id >= d->max_vcpus) || !d->vcpu[rsp.vcpu_id] )
+            continue;
+
+        v = d->vcpu[rsp.vcpu_id];
+
         /* Unpause domain */
         if ( rsp.flags & MEM_EVENT_FLAG_VCPU_PAUSED )
-            vcpu_unpause(d->vcpu[rsp.vcpu_id]);
+            vcpu_unpause(v);
     }
 }
 
-- 
1.7.10.4

[PATCH 2/2] Xen/mem_event: Prevent underflow of vcpu pause counts

[Andrew Cooper]

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>
---
 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);
+    vcpu_pause_nosync(v);
+}
+
+void mem_event_vcpu_unpause(struct vcpu *v)
+{
+    if ( test_and_clear_bool(v->paused_for_mem_event) )
+        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

Re: [PATCH 0/2] Xen/mem_event: Do not rely on the toolstack being bug-free

[Tim Deegan]

At 14:10 +0100 on 17 Jul (1405602635), Andrew Cooper wrote:
> Xen performs insufficient validation of the contents of mem_event responses
> from the toolstack.  As a result, a buggy toolstack could cause Xen to walk
> off the end of a domain's vcpu list, and get out of sync with vcpu pause
> reference counts.
> 
> These two fixes are compile tested only, as I have no way to plausibly test
> the mem-event functionality itself.

Reviewed-by: Tim Deegan <tim@xen.org>

Re: [PATCH 0/2] Xen/mem_event: Do not rely on the toolstack being bug-free

[Razvan Cojocaru]

On 07/17/2014 04:10 PM, Andrew Cooper wrote:
> Xen performs insufficient validation of the contents of mem_event responses
> from the toolstack.  As a result, a buggy toolstack could cause Xen to walk
> off the end of a domain's vcpu list, and get out of sync with vcpu pause
> reference counts.
> 
> These two fixes are compile tested only, as I have no way to plausibly test
> the mem-event functionality itself.

I've ran a few tests for our application (heavily based on mem_event),
and everything seems to work fine.


Hope this helps,
Razvan Cojocaru

Re: [PATCH 0/2] Xen/mem_event: Do not rely on the toolstack being bug-free

[Andrew Cooper]

On 17/07/14 15:40, Razvan Cojocaru wrote:
> On 07/17/2014 04:10 PM, Andrew Cooper wrote:
>> Xen performs insufficient validation of the contents of mem_event responses
>> from the toolstack.  As a result, a buggy toolstack could cause Xen to walk
>> off the end of a domain's vcpu list, and get out of sync with vcpu pause
>> reference counts.
>>
>> These two fixes are compile tested only, as I have no way to plausibly test
>> the mem-event functionality itself.
> I've ran a few tests for our application (heavily based on mem_event),
> and everything seems to work fine.
>
>
> Hope this helps,
> Razvan Cojocaru

Fantastic - thanks!

Does that mean you are happy for a "Tested-by: Razvan Cojocaru
<rcojocaru@bitdefender.com>" tag to accompany the series?

Re: [PATCH 0/2] Xen/mem_event: Do not rely on the toolstack being bug-free

[Razvan Cojocaru]

On 07/17/2014 05:46 PM, Andrew Cooper wrote:
> On 17/07/14 15:40, Razvan Cojocaru wrote:
>> On 07/17/2014 04:10 PM, Andrew Cooper wrote:
>>> Xen performs insufficient validation of the contents of mem_event responses
>>> from the toolstack.  As a result, a buggy toolstack could cause Xen to walk
>>> off the end of a domain's vcpu list, and get out of sync with vcpu pause
>>> reference counts.
>>>
>>> These two fixes are compile tested only, as I have no way to plausibly test
>>> the mem-event functionality itself.
>> I've ran a few tests for our application (heavily based on mem_event),
>> and everything seems to work fine.
>>
>>
>> Hope this helps,
>> Razvan Cojocaru
> 
> Fantastic - thanks!
> 
> Does that mean you are happy for a "Tested-by: Razvan Cojocaru
> <rcojocaru@bitdefender.com>" tag to accompany the series?

Sure, glad to be of service.Tested-by: Razvan Cojocaru
<rcojocaru@bitdefender.com>

Re: [PATCH 1/2] Xen/mem_event: Validate the response vcpu_id before acting on it.

[Andres Lagar Cavilla]

[-- Attachment #1.1: Type: text/plain, Size: 3168 bytes --]

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>
>
Reviewed-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>

> ---
>  xen/arch/x86/mm/mem_sharing.c |   11 ++++++++++-
>  xen/arch/x86/mm/p2m.c         |   22 ++++++++++++++++++++--
>  2 files changed, 30 insertions(+), 3 deletions(-)
>
> diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
> index 7293f31..ec99266 100644
> --- a/xen/arch/x86/mm/mem_sharing.c
> +++ b/xen/arch/x86/mm/mem_sharing.c
> @@ -596,11 +596,20 @@ int mem_sharing_sharing_resume(struct domain *d)
>      /* Get all requests off the ring */
>      while ( mem_event_get_response(d, &d->mem_event->share, &rsp) )
>      {
> +        struct vcpu *v;
> +
>          if ( rsp.flags & MEM_EVENT_FLAG_DUMMY )
>              continue;
> +
> +        /* Validate the vcpu_id in the response. */
> +        if ( (rsp.vcpu_id >= d->max_vcpus) || !d->vcpu[rsp.vcpu_id] )
> +            continue;
> +
> +        v = d->vcpu[rsp.vcpu_id];
> +
>          /* Unpause domain/vcpu */
>          if ( rsp.flags & MEM_EVENT_FLAG_VCPU_PAUSED )
> -            vcpu_unpause(d->vcpu[rsp.vcpu_id]);
> +            vcpu_unpause(v);
>      }
>
>      return 0;
> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> index 642ec28..f213a39 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -1290,8 +1290,17 @@ void p2m_mem_paging_resume(struct domain *d)
>      /* Pull all responses off the ring */
>      while( mem_event_get_response(d, &d->mem_event->paging, &rsp) )
>      {
> +        struct vcpu *v;
> +
>          if ( rsp.flags & MEM_EVENT_FLAG_DUMMY )
>              continue;
> +
> +        /* Validate the vcpu_id in the response. */
> +        if ( (rsp.vcpu_id >= d->max_vcpus) || !d->vcpu[rsp.vcpu_id] )
> +            continue;
> +
> +        v = d->vcpu[rsp.vcpu_id];
> +
>          /* Fix p2m entry if the page was not dropped */
>          if ( !(rsp.flags & MEM_EVENT_FLAG_DROP_PAGE) )
>          {
> @@ -1310,7 +1319,7 @@ void p2m_mem_paging_resume(struct domain *d)
>          }
>          /* Unpause domain */
>          if ( rsp.flags & MEM_EVENT_FLAG_VCPU_PAUSED )
> -            vcpu_unpause(d->vcpu[rsp.vcpu_id]);
> +            vcpu_unpause(v);
>      }
>  }
>
> @@ -1418,11 +1427,20 @@ void p2m_mem_access_resume(struct domain *d)
>      /* Pull all responses off the ring */
>      while( mem_event_get_response(d, &d->mem_event->access, &rsp) )
>      {
> +        struct vcpu *v;
> +
>          if ( rsp.flags & MEM_EVENT_FLAG_DUMMY )
>              continue;
> +
> +        /* Validate the vcpu_id in the response. */
> +        if ( (rsp.vcpu_id >= d->max_vcpus) || !d->vcpu[rsp.vcpu_id] )
> +            continue;
> +
> +        v = d->vcpu[rsp.vcpu_id];
> +
>          /* Unpause domain */
>          if ( rsp.flags & MEM_EVENT_FLAG_VCPU_PAUSED )
> -            vcpu_unpause(d->vcpu[rsp.vcpu_id]);
> +            vcpu_unpause(v);
>      }
>  }
>
> --
> 1.7.10.4
>
>

[-- Attachment #1.2: Type: text/html, Size: 4519 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 2/2] Xen/mem_event: Prevent underflow of vcpu pause counts

[Andres Lagar Cavilla]

[-- Attachment #1.1: Type: text/plain, Size: 5614 bytes --]

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.

> ---
>  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.

> +    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.

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
>
>

[-- Attachment #1.2: Type: text/html, Size: 7338 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 2/2] Xen/mem_event: Prevent underflow of vcpu pause counts

[Aravindh Puthiyaparambil (aravindp)]

>> +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.

Very true. I have seen this event pile up occur in practice in our product.

Thanks,
Aravindh

Re: [PATCH 2/2] Xen/mem_event: Prevent underflow of vcpu pause counts

[Andres Lagar Cavilla]

[-- Attachment #1.1: Type: text/plain, Size: 823 bytes --]

On Thu, Jul 17, 2014 at 2:51 PM, Aravindh Puthiyaparambil (aravindp) <
aravindp@cisco.com> wrote:

> >> +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.
>
> Very true. I have seen this event pile up occur in practice in our product.
>
The problem becomes how to tell apart real event responses that should dec
the pause count from spurious crap from the toolstack. IOW, how to not
unpause the vcpu when count reaches zero due to bad responses. I think the
answer is: you can't, if the toolstack is evil, behavior undefined and
bigger fish to fry.

Andres

>
> Thanks,
> Aravindh
>
>

[-- Attachment #1.2: Type: text/html, Size: 1353 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 2/2] Xen/mem_event: Prevent underflow of vcpu pause counts

[Andrew Cooper]

[-- Attachment #1.1: Type: text/plain, Size: 6785 bytes --]

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 <mailto:andrew.cooper3@citrix.com>> wrote:
>
>     Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com
>     <mailto:andrew.cooper3@citrix.com>>
>     CC: Jan Beulich <JBeulich@suse.com <mailto:JBeulich@suse.com>>
>     CC: Tim Deegan <tim@xen.org <mailto:tim@xen.org>>
>     CC: Andres Lagar-Cavilla <andres@lagarcavilla.org
>     <mailto:andres@lagarcavilla.org>>
>
> 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
>
>


[-- Attachment #1.2: Type: text/html, Size: 11880 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 2/2] Xen/mem_event: Prevent underflow of vcpu pause counts

[Aravindh Puthiyaparambil (aravindp)]

[-- Attachment #1.1: Type: text/plain, Size: 790 bytes --]

>> +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.
Very true. I have seen this event pile up occur in practice in our product.
The problem becomes how to tell apart real event responses that should dec the pause count from spurious crap from the toolstack. IOW, how to not unpause the vcpu when count reaches zero due to bad responses. I think the answer is: you can't, if the toolstack is evil, behavior undefined and bigger fish to fry.

Would that be a problem? AFAIK, you can have only one mem_event listener for a domain at a time.

Aravindh

[-- Attachment #1.2: Type: text/html, Size: 3306 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 0/2] Xen/mem_event: Do not rely on the toolstack being bug-free

[Aravindh Puthiyaparambil (aravindp)]

>Xen performs insufficient validation of the contents of mem_event responses
>from the toolstack.  As a result, a buggy toolstack could cause Xen to walk off
>the end of a domain's vcpu list, and get out of sync with vcpu pause reference
>counts.
>
>These two fixes are compile tested only, as I have no way to plausibly test the
>mem-event functionality itself.

One easy way of testing is to use the tools/tests/xen-access test program which exercises mem_access and thereby mem_event. It is fairly easy to run. Bring up a domain and execute " xen-access <domain_id> write|exec". But I understand if you are under time constraints and cannot do it. If you Cc me on these patches, I will gladly test them for you.

Thanks,
Aravindh

Re: [PATCH 2/2] Xen/mem_event: Prevent underflow of vcpu pause counts

[Andrew Cooper]

[-- Attachment #1.1: Type: text/plain, Size: 1593 bytes --]

On 17/07/14 19:54, Andres Lagar Cavilla wrote:
> On Thu, Jul 17, 2014 at 2:51 PM, Aravindh Puthiyaparambil (aravindp)
> <aravindp@cisco.com <mailto:aravindp@cisco.com>> wrote:
>
>     >> +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.
>
>     Very true. I have seen this event pile up occur in practice in our
>     product.
>
> The problem becomes how to tell apart real event responses that should
> dec the pause count from spurious crap from the toolstack. IOW, how to
> not unpause the vcpu when count reaches zero due to bad responses. I
> think the answer is: you can't, if the toolstack is evil, behavior
> undefined and bigger fish to fry.
>
> Andres

You really can't, but the important bit is to ensure that Xen is
sufficiently insulated from buggy toolstack components that it doesn't
fall over.

>From my experimenting with the pausedomain refcoutnging, weird stuff
happens when the domain pause count turns negative.  I ended up with a
domain which would never be scheduled again (even after returning the
count to positive and back to 0), and a domain which couldn't be killed
using `xl destroy`.  Rebooting was the only option.

So long as Xen doesn't fall into these problems, a buggy toolstack
(especially with mem_events) already has many ways to screw over a
domain, so one more is not a problem.

~Andrew

[-- Attachment #1.2: Type: text/html, Size: 2946 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 2/2] Xen/mem_event: Prevent underflow of vcpu pause counts

[Aravindh Puthiyaparambil (aravindp)]

[-- Attachment #1.1: Type: text/plain, Size: 1584 bytes --]

On Thu, Jul 17, 2014 at 2:51 PM, Aravindh Puthiyaparambil (aravindp) <aravindp@cisco.com<mailto:aravindp@cisco.com>> wrote:
>> +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.
Very true. I have seen this event pile up occur in practice in our product.
The problem becomes how to tell apart real event responses that should dec the pause count from spurious crap from the toolstack. IOW, how to not unpause the vcpu when count reaches zero due to bad responses. I think the answer is: you can't, if the toolstack is evil, behavior undefined and bigger fish to fry.

Andres

You really can't, but the important bit is to ensure that Xen is sufficiently insulated from buggy toolstack components that it doesn't fall over.

From my experimenting with the pausedomain refcoutnging, weird stuff happens when the domain pause count turns negative.  I ended up with a domain which would never be scheduled again (even after returning the count to positive and back to 0), and a domain which couldn't be killed using `xl destroy`.  Rebooting was the only option.

So long as Xen doesn't fall into these problems, a buggy toolstack (especially with mem_events) already has many ways to screw over a domain, so one more is not a problem.

I misunderstood Andre’s point. Your response made it clear what the concern was.

Thanks,
Aravindh


[-- Attachment #1.2: Type: text/html, Size: 4378 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 0/2] Xen/mem_event: Do not rely on the toolstack being bug-free

[Razvan Cojocaru]

On 07/17/2014 10:01 PM, Aravindh Puthiyaparambil (aravindp) wrote:
>> Xen performs insufficient validation of the contents of mem_event responses
>>from the toolstack.  As a result, a buggy toolstack could cause Xen to walk off
>> the end of a domain's vcpu list, and get out of sync with vcpu pause reference
>> counts.
>>
>> These two fixes are compile tested only, as I have no way to plausibly test the
>> mem-event functionality itself.
> 
> One easy way of testing is to use the tools/tests/xen-access test program which exercises mem_access and thereby mem_event. It is fairly easy to run. Bring up a domain and execute " xen-access <domain_id> write|exec". But I understand if you are under time constraints and cannot do it. If you Cc me on these patches, I will gladly test them for you.

Indeed, our application is very xen-access-like (except quite a bit more
involved), and I've tested the original patches with 5 different domains
3 times over - but it's a well-behaved citizen of the Xen ecosystem and
there were no gimmicks involved. No mem_events piled up, and there was
always just one mem_event handler per domain.

Everything went without a hitch, but I did not try to pause the domain
while it was running or try to trick the hypervisor in any way.


Thanks,
Razvan Cojocaru

Re: [PATCH 0/2] Xen/mem_event: Do not rely on the toolstack being bug-free

[Tamas Lengyel]

[-- Attachment #1.1: Type: text/plain, Size: 2242 bytes --]

I've also tested the patch with LibVMI and everything works fine. The
pause/unpause reference count now does take effect, so the previous issue I
reported (a paused domain getting unpaused by mem_event_enable) is fixed by
this patch.

One question I have, what if the toolstack wants to unconditionally (force)
unpause a domain? Right now with this patch if someone runs 'xl pause
domain' a couple times he has no other recourse then to issue 'xl unpause
domain' at least the same number of times, or to restart the entire domain.
Might be user-friendlier if there was an override provided in case a domain
got paused a million times by accident.

Cheers,
Tamas


On Thu, Jul 17, 2014 at 10:26 PM, Razvan Cojocaru <rcojocaru@bitdefender.com
> wrote:

> On 07/17/2014 10:01 PM, Aravindh Puthiyaparambil (aravindp) wrote:
> >> Xen performs insufficient validation of the contents of mem_event
> responses
> >>from the toolstack.  As a result, a buggy toolstack could cause Xen to
> walk off
> >> the end of a domain's vcpu list, and get out of sync with vcpu pause
> reference
> >> counts.
> >>
> >> These two fixes are compile tested only, as I have no way to plausibly
> test the
> >> mem-event functionality itself.
> >
> > One easy way of testing is to use the tools/tests/xen-access test
> program which exercises mem_access and thereby mem_event. It is fairly easy
> to run. Bring up a domain and execute " xen-access <domain_id> write|exec".
> But I understand if you are under time constraints and cannot do it. If you
> Cc me on these patches, I will gladly test them for you.
>
> Indeed, our application is very xen-access-like (except quite a bit more
> involved), and I've tested the original patches with 5 different domains
> 3 times over - but it's a well-behaved citizen of the Xen ecosystem and
> there were no gimmicks involved. No mem_events piled up, and there was
> always just one mem_event handler per domain.
>
> Everything went without a hitch, but I did not try to pause the domain
> while it was running or try to trick the hypervisor in any way.
>
>
> Thanks,
> Razvan Cojocaru
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>

[-- Attachment #1.2: Type: text/html, Size: 2934 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 0/2] Xen/mem_event: Do not rely on the toolstack being bug-free

[Andrew Cooper]

[-- Attachment #1.1: Type: text/plain, Size: 1605 bytes --]

On 17/07/2014 23:17, Tamas Lengyel wrote:
> I've also tested the patch with LibVMI and everything works fine. The
> pause/unpause reference count now does take effect, so the previous
> issue I reported (a paused domain getting unpaused by
> mem_event_enable) is fixed by this patch.
>
> One question I have, what if the toolstack wants to unconditionally
> (force) unpause a domain? Right now with this patch if someone runs
> 'xl pause domain' a couple times he has no other recourse then to
> issue 'xl unpause domain' at least the same number of times, or to
> restart the entire domain. Might be user-friendlier if there was an
> override provided in case a domain got paused a million times by accident.
>
> Cheers,
> Tamas

I don't think that would be a good idea.  The entire point of the proper
refcounting is so bits of toolstack subsystems can guarentee that the
domain stays paused during a critical set of operations.  Providing a
"DOMCTL_unpausedomain --force" would undermine the whole purpose of this.

As already expressed, there are plenty of ways a buggy/dumb toolstack
can shoot itself in the foot with regards to a domain.  I include in
this users with dom0 root access and `xl`.

The two key points are that:

1) a buggy toolstack can't cause Xen perform an unintentional action
(e.g. walking off the end of an array, as demonstrated in patch 1 of
this series) and
2) several non-buggy parts of a toolstack can operate safely together
with respect to a Xen resource.

Any attempt to work around a buggy bit of a toolstack in Xen is effort
better spent fixing the toolstack.

~Andrew

[-- Attachment #1.2: Type: text/html, Size: 2455 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 2/2] Xen/mem_event: Prevent underflow of vcpu pause counts

[Ian Campbell]

On Thu, 2014-07-17 at 14:38 -0400, Andres Lagar Cavilla wrote:
>         +    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.

Since some x86 instructions can have both its input and output in memory
it's not hard to imagine both being MMIO, at least if you were trying to
break things...

Ian.

[PATCH v2 2/2] Xen/mem_event: Prevent underflow of vcpu pause counts

[Andrew Cooper]

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>
CC: Razvan Cojocaru <rcojocaru@bitdefender.com>
CC: Aravindh Puthiyaparambil <aravindp@cisco.com>

--
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..bdde426 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);
+    vcpu_pause_nosync(v);
+}
+
+void mem_event_vcpu_unpause(struct vcpu *v)
+{
+    int old, new, prev = v->mem_event_pause_count.counter;
+
+    /* All unpause requests as a result of toolstack responses.  Prevent
+     * underflow of the vcpu pause count. */
+    do
+    {
+        old = prev;
+        new = old - 1;
+
+        if ( new < 0 )
+        {
+            printk(XENLOG_G_WARNING
+                   "%pv mem_event: Too many unpause attempts", v);
+            return;
+        }
+
+        prev = cmpxchg(&v->mem_event_pause_count.counter, old, new);
+    } while ( prev != old );
+
+    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..62a4785 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -214,6 +214,9 @@ struct vcpu
     unsigned long    pause_flags;
     atomic_t         pause_count;
 
+    /* VCPU paused for mem_event replies. */
+    atomic_t         mem_event_pause_count;
+
     /* IRQ-safe virq_lock protects against delivering VIRQ to stale evtchn. */
     evtchn_port_t    virq_to_evtchn[NR_VIRQS];
     spinlock_t       virq_lock;
-- 
1.7.10.4

Re: [PATCH v2 2/2] Xen/mem_event: Prevent underflow of vcpu pause counts

[Razvan Cojocaru]

On 07/18/2014 01:41 PM, Andrew Cooper 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>
> CC: Razvan Cojocaru <rcojocaru@bitdefender.com>
> CC: Aravindh Puthiyaparambil <aravindp@cisco.com>
> 
> --
> 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(-)

Tested-by: Razvan Cojocaru <rcojocaru@bitdefender.com>


Thanks,
Razvan Cojocaru

[PATCH v3 2/2] Xen/mem_event: Prevent underflow of vcpu pause counts

[Andrew Cooper]

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>

--
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);
+    vcpu_pause_nosync(v);
+}
+
+void mem_event_vcpu_unpause(struct vcpu *v)
+{
+    int old, new, prev = v->mem_event_pause_count.counter;
+
+    /* All unpause requests as a result of toolstack responses.  Prevent
+     * underflow of the vcpu pause count. */
+    do
+    {
+        old = prev;
+        new = old - 1;
+
+        if ( new < 0 )
+        {
+            printk(XENLOG_G_WARNING
+                   "%pv mem_event: Too many unpause attempts\n", v);
+            return;
+        }
+
+        prev = cmpxchg(&v->mem_event_pause_count.counter, old, new);
+    } while ( prev != old );
+
+    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..62a4785 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -214,6 +214,9 @@ struct vcpu
     unsigned long    pause_flags;
     atomic_t         pause_count;
 
+    /* VCPU paused for mem_event replies. */
+    atomic_t         mem_event_pause_count;
+
     /* IRQ-safe virq_lock protects against delivering VIRQ to stale evtchn. */
     evtchn_port_t    virq_to_evtchn[NR_VIRQS];
     spinlock_t       virq_lock;
-- 
1.7.10.4

Re: [PATCH v3 2/2] Xen/mem_event: Prevent underflow of vcpu pause counts

[Andres Lagar Cavilla]

[-- Attachment #1.1: Type: text/plain, Size: 6100 bytes --]

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:
>  * 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.

> +    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

> +
> +    /* All unpause requests as a result of toolstack responses.  Prevent
> +     * underflow of the vcpu pause count. */
> +    do
> +    {
> +        old = prev;
> +        new = old - 1;
> +
> +        if ( new < 0 )
> +        {
> +            printk(XENLOG_G_WARNING
> +                   "%pv mem_event: Too many unpause attempts\n", v);
> +            return;
> +        }
> +
> +        prev = cmpxchg(&v->mem_event_pause_count.counter, old, new);
> +    } while ( prev != old );
> +
> +    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..62a4785 100644
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -214,6 +214,9 @@ struct vcpu
>      unsigned long    pause_flags;
>      atomic_t         pause_count;
>
> +    /* VCPU paused for mem_event replies. */
> +    atomic_t         mem_event_pause_count;
> +
>      /* IRQ-safe virq_lock protects against delivering VIRQ to stale
> evtchn. */
>      evtchn_port_t    virq_to_evtchn[NR_VIRQS];
>      spinlock_t       virq_lock;
> --
> 1.7.10.4
>
>

[-- Attachment #1.2: Type: text/html, Size: 8268 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v3 2/2] Xen/mem_event: Prevent underflow of vcpu pause counts

[Andrew Cooper]

[-- Attachment #1.1: Type: text/plain, Size: 3328 bytes --]

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 <mailto:andrew.cooper3@citrix.com>> wrote:
>
>     Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com
>     <mailto:andrew.cooper3@citrix.com>>
>     Tested-by: Razvan Cojocaru <rcojocaru@bitdefender.com
>     <mailto:rcojocaru@bitdefender.com>>
>     CC: Jan Beulich <JBeulich@suse.com <mailto:JBeulich@suse.com>>
>     CC: Tim Deegan <tim@xen.org <mailto:tim@xen.org>>
>     CC: Andres Lagar-Cavilla <andres@lagarcavilla.org
>     <mailto:andres@lagarcavilla.org>>
>     CC: Aravindh Puthiyaparambil <aravindp@cisco.com
>     <mailto:aravindp@cisco.com>>
>
> Reviewed-by: Andres Lagar-Cavilla <andres@lagarcavilla.org
> <mailto:andres@lagarcavilla.org>>
>
> 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

[-- Attachment #1.2: Type: text/html, Size: 6846 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v3 2/2] Xen/mem_event: Prevent underflow of vcpu pause counts

[Aravindh Puthiyaparambil (aravindp)]

>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>
>
>--
>v3:
> * Newline on warning

Tested-by: Aravindh Puthiyaparambil <aravindp@cisco.com>