[PATCH] xen/common: Properly reference count DOMCTL_{, un}pausedomain hypercalls

[PATCH] xen/common: Properly reference count DOMCTL_{, un}pausedomain hypercalls

[Andrew Cooper]

For safety reasons, c/s 6ae2df93c27 "mem_access: Add helper API to setup
ring and enable mem_access" has to pause the domain while it performs a set of
operations.

However without properly reference counted hypercalls, xc_mem_event_enable()
now unconditionally unpauses a previously paused domain.

To prevent toolstack software running wild, there is an arbitrary limit of 255
on the toolstack pause count.  This is high enough for several components of
the toolstack to safely use, but prevents over/underflow of d->pause_count.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Ian Campbell <ian.campbell@citrix.com>
CC: Stefano Stabellini <stefano.stabellini@citrix.com>
CC: Tim Deegan <tim@xen.org>

---

This has been functionally tested on x86, and compile tested on both flavours
of ARM.

Personally I dislike the addition of the spinlock, but I can't find a safe way
of preventing over/underflow of d->pause_count with an atomic_t
toolstack_pause_count alone.  At least they are fine-grained locks and
unlikely to be contended in practice.
---
 xen/arch/x86/domctl.c   |    6 ++---
 xen/common/domain.c     |   58 ++++++++++++++++++++++++++++++++++-------------
 xen/common/domctl.c     |    8 +++----
 xen/include/xen/sched.h |   19 ++++++++++++----
 4 files changed, 63 insertions(+), 28 deletions(-)

diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index a4effc3..e4b11c8 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -1027,7 +1027,7 @@ long arch_do_domctl(
         struct vcpu *v;
 
         ret = -EBUSY;
-        if ( !d->is_paused_by_controller )
+        if ( d->toolstack_pause_count > 0 )
             break;
         ret = -EINVAL;
         if ( domctl->u.gdbsx_pauseunp_vcpu.vcpu >= MAX_VIRT_CPUS ||
@@ -1043,7 +1043,7 @@ long arch_do_domctl(
         struct vcpu *v;
 
         ret = -EBUSY;
-        if ( !d->is_paused_by_controller )
+        if ( d->toolstack_pause_count > 0 )
             break;
         ret = -EINVAL;
         if ( domctl->u.gdbsx_pauseunp_vcpu.vcpu >= MAX_VIRT_CPUS ||
@@ -1061,7 +1061,7 @@ long arch_do_domctl(
         struct vcpu *v;
 
         domctl->u.gdbsx_domstatus.vcpu_id = -1;
-        domctl->u.gdbsx_domstatus.paused = d->is_paused_by_controller;
+        domctl->u.gdbsx_domstatus.paused = !!d->toolstack_pause_count;
         if ( domctl->u.gdbsx_domstatus.paused )
         {
             for_each_vcpu ( d, v )
diff --git a/xen/common/domain.c b/xen/common/domain.c
index c3a576e..386b5cf 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -279,6 +279,7 @@ struct domain *domain_create(
     d->shutdown_code = -1;
 
     spin_lock_init(&d->pbuf_lock);
+    spin_lock_init(&d->toolstack_pause_lock);
 
     err = -ENOMEM;
     if ( !zalloc_cpumask_var(&d->domain_dirty_cpumask) )
@@ -317,7 +318,7 @@ struct domain *domain_create(
         if ( (err = xsm_domain_create(XSM_HOOK, d, ssidref)) != 0 )
             goto fail;
 
-        d->is_paused_by_controller = 1;
+        d->toolstack_pause_count = 1;
         atomic_inc(&d->pause_count);
 
         if ( !is_hardware_domain(d) )
@@ -755,18 +756,13 @@ void vcpu_end_shutdown_deferral(struct vcpu *v)
 #ifdef HAS_GDBSX
 void domain_pause_for_debugger(void)
 {
-    struct domain *d = current->domain;
-    struct vcpu *v;
-
-    atomic_inc(&d->pause_count);
-    if ( test_and_set_bool(d->is_paused_by_controller) )
-        domain_unpause(d); /* race-free atomic_dec(&d->pause_count) */
+    struct vcpu *v = current;
+    struct domain *d = v->domain;
 
-    for_each_vcpu ( d, v )
-        vcpu_sleep_nosync(v);
+    domain_pause_by_systemcontroller_nosync(d);
 
     /* if gdbsx active, we just need to pause the domain */
-    if (current->arch.gdbsx_vcpu_event == 0)
+    if (v->arch.gdbsx_vcpu_event == 0)
         send_global_virq(VIRQ_DEBUGGER);
 }
 #endif
@@ -914,17 +910,47 @@ void domain_unpause(struct domain *d)
             vcpu_wake(v);
 }
 
-void domain_pause_by_systemcontroller(struct domain *d)
+int __domain_pause_by_systemcontroller(struct domain *d,
+                                       void (*pause_fn)(struct domain *d))
 {
-    domain_pause(d);
-    if ( test_and_set_bool(d->is_paused_by_controller) )
-        domain_unpause(d);
+    int ret = -E2BIG;
+
+    spin_lock(&d->toolstack_pause_lock);
+
+    /*
+     * Limit the toolstack pause count to an arbitrary 255 to prevent the
+     * toolstack overflowing d->pause_count with many repeated hypercalls.
+     */
+    if ( d->toolstack_pause_count < 255 )
+    {
+        d->toolstack_pause_count++;
+        ret = 0;
+
+        pause_fn(d);
+    }
+
+    spin_unlock(&d->toolstack_pause_lock);
+
+    return ret;
 }
 
-void domain_unpause_by_systemcontroller(struct domain *d)
+int domain_unpause_by_systemcontroller(struct domain *d)
 {
-    if ( test_and_clear_bool(d->is_paused_by_controller) )
+    int ret = -EINVAL;
+
+    spin_lock(&d->toolstack_pause_lock);
+
+    if ( d->toolstack_pause_count > 0 )
+    {
+        d->toolstack_pause_count--;
+        ret = 0;
+
         domain_unpause(d);
+    }
+
+    spin_unlock(&d->toolstack_pause_lock);
+
+    return ret;
 }
 
 int vcpu_reset(struct vcpu *v)
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index 000993f..3d0cb4e 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -181,7 +181,7 @@ void getdomaininfo(struct domain *d, struct xen_domctl_getdomaininfo *info)
     info->flags = (info->nr_online_vcpus ? flags : 0) |
         ((d->is_dying == DOMDYING_dead) ? XEN_DOMINF_dying    : 0) |
         (d->is_shut_down                ? XEN_DOMINF_shutdown : 0) |
-        (d->is_paused_by_controller     ? XEN_DOMINF_paused   : 0) |
+        (!!d->toolstack_pause_count     ? XEN_DOMINF_paused   : 0) |
         (d->debugger_attached           ? XEN_DOMINF_debugged : 0) |
         d->shutdown_code << XEN_DOMINF_shutdownshift;
 
@@ -398,16 +398,14 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
         ret = -EINVAL;
         if ( d != current->domain )
         {
-            domain_pause_by_systemcontroller(d);
-            ret = 0;
+            ret = domain_pause_by_systemcontroller(d);
         }
     }
     break;
 
     case XEN_DOMCTL_unpausedomain:
     {
-        domain_unpause_by_systemcontroller(d);
-        ret = 0;
+        ret = domain_unpause_by_systemcontroller(d);
     }
     break;
 
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index f920e1a..0f8d504 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -365,8 +365,6 @@ struct domain
     bool_t           debugger_attached;
     /* Is this guest dying (i.e., a zombie)? */
     enum { DOMDYING_alive, DOMDYING_dying, DOMDYING_dead } is_dying;
-    /* Domain is paused by controller software? */
-    bool_t           is_paused_by_controller;
     /* Domain's VCPUs are pinned 1:1 to physical CPUs? */
     bool_t           is_pinned;
 
@@ -389,6 +387,10 @@ struct domain
 
     atomic_t         pause_count;
 
+    /* Domain is paused by controller software? */
+    spinlock_t       toolstack_pause_lock;
+    unsigned int     toolstack_pause_count;
+
     unsigned long    vm_assist;
 
     atomic_t         refcnt;
@@ -769,8 +771,17 @@ void domain_pause(struct domain *d);
 void domain_pause_nosync(struct domain *d);
 void vcpu_unpause(struct vcpu *v);
 void domain_unpause(struct domain *d);
-void domain_pause_by_systemcontroller(struct domain *d);
-void domain_unpause_by_systemcontroller(struct domain *d);
+int domain_unpause_by_systemcontroller(struct domain *d);
+int __domain_pause_by_systemcontroller(struct domain *d,
+                                       void (*pause_fn)(struct domain *d));
+static inline int domain_pause_by_systemcontroller(struct domain *d)
+{
+    return __domain_pause_by_systemcontroller(d, domain_pause);
+}
+static inline int domain_pause_by_systemcontroller_nosync(struct domain *d)
+{
+    return __domain_pause_by_systemcontroller(d, domain_pause_nosync);
+}
 void cpu_init(void);
 
 struct scheduler;
-- 
1.7.10.4

Re: [PATCH] xen/common: Properly reference count DOMCTL_{, un}pausedomain hypercalls

[Slutz, Donald Christopher]

On 07/01/14 15:57, Andrew Cooper wrote:
> For safety reasons, c/s 6ae2df93c27 "mem_access: Add helper API to setup
> ring and enable mem_access" has to pause the domain while it performs a set of
> operations.
>
> However without properly reference counted hypercalls, xc_mem_event_enable()
> now unconditionally unpauses a previously paused domain.
>
> To prevent toolstack software running wild, there is an arbitrary limit of 255
> on the toolstack pause count.  This is high enough for several components of
> the toolstack to safely use, but prevents over/underflow of d->pause_count.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Keir Fraser <keir@xen.org>
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Ian Campbell <ian.campbell@citrix.com>
> CC: Stefano Stabellini <stefano.stabellini@citrix.com>
> CC: Tim Deegan <tim@xen.org>
>
> ---
>
> This has been functionally tested on x86, and compile tested on both flavours
> of ARM.
>
> Personally I dislike the addition of the spinlock, but I can't find a safe way
> of preventing over/underflow of d->pause_count with an atomic_t
> toolstack_pause_count alone.  At least they are fine-grained locks and
> unlikely to be contended in practice.

atomic_cmpxchg() can be used to safely preventing over/underflow.

For example:


int safe_atomic_inc(int * intp)
{
     int prev, old, new;

     prev = atomic_read(*intp);
     do {
         old = prev;
         new = old + 1;
         if ( new > 255 )
             return 1;
         prev = atomic_cmpxchg(intp, old, new);
     } while ( prev != old );
     return 0;
}

int safe_atomic_dec(int * intp)
{
     int prev, old, new;

     prev = atomic_read(*intp);
     do {
         old = prev;
         new = old - 1;
         if ( new < 0 )
             return 1;
         prev = atomic_cmpxchg(intp, old, new);
     } while ( prev != old );
     return 0;
}


does what you are asking about (adjusted for correct types).  Here is
a quick c prog using them:

#include <stdio.h>

#define atomic_read(x) x
#define atomic_cmpxchg __sync_val_compare_and_swap

int safe_atomic_inc(int * intp)
{
     int prev, old, new;

     prev = atomic_read(*intp);
     do {
         old = prev;
         new = old + 1;
         if ( new > 255 )
             return 1;
         prev = atomic_cmpxchg(intp, old, new);
     } while ( prev != old );
     return 0;
}

int safe_atomic_dec(int * intp)
{
     int prev, old, new;

     prev = atomic_read(*intp);
     do {
         old = prev;
         new = old - 1;
         if ( new < 0 )
             return 1;
         prev = atomic_cmpxchg(intp, old, new);
     } while ( prev != old );
     return 0;
}

int
main(void)
{
     int foo = 0;
     int rc;

     printf("foo=%d\n", foo);
     rc = safe_atomic_dec(&foo);
     printf("rc=%d foo--=%d\n", rc, foo);
     rc = safe_atomic_inc(&foo);
     printf("rc=%d foo++=%d\n", rc, foo);
     foo = 253;
     printf("foo=%d\n", foo);
     rc = safe_atomic_dec(&foo);
     printf("rc=%d foo--=%d\n", rc, foo);
     rc = safe_atomic_inc(&foo);
     printf("rc=%d foo++=%d\n", rc, foo);
     rc = safe_atomic_inc(&foo);
     printf("rc=%d foo++=%d\n", rc, foo);
     rc = safe_atomic_inc(&foo);
     printf("rc=%d foo++=%d\n", rc, foo);
     rc = safe_atomic_inc(&foo);
     printf("rc=%d foo++=%d\n", rc, foo);

     return rc;
}

     -Don Slutz

Re: [PATCH] xen/common: Properly reference count DOMCTL_{, un}pausedomain hypercalls

[Jan Beulich]

>>> On 01.07.14 at 21:57, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -365,8 +365,6 @@ struct domain
>      bool_t           debugger_attached;
>      /* Is this guest dying (i.e., a zombie)? */
>      enum { DOMDYING_alive, DOMDYING_dying, DOMDYING_dead } is_dying;
> -    /* Domain is paused by controller software? */
> -    bool_t           is_paused_by_controller;
>      /* Domain's VCPUs are pinned 1:1 to physical CPUs? */
>      bool_t           is_pinned;
>  
> @@ -389,6 +387,10 @@ struct domain
>  
>      atomic_t         pause_count;
>  
> +    /* Domain is paused by controller software? */
> +    spinlock_t       toolstack_pause_lock;
> +    unsigned int     toolstack_pause_count;

I think I like "controller" better than "toolstack" here, and I even
wonder whether the old name, just stripped of the "is_", couldn't
remain.

And I also like Don's cmpxchg suggestion, avoiding the need for a
lock - we're doing this kind of lock avoidance in various other places
already anyway.

Jan