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

[PATCH v3] 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.

The previous domain_{,un}pause_by_systemcontroller() functions are updated to
return an error code.  domain_pause_by_systemcontroller() is modified to have
a common stub and take a pause_fn pointer, allowing for both sync and nosync
domain pauses.  domain_pause_for_debugger() has a hand-rolled nosync pause
replaced with the new domain_pause_by_systemcontroller_nosync(), and has its
variables shuffled slightly to avoid rereading current multiple times.

Suggested-by: Don Slutz <dslutz@verizon.com>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Tim Deegan <tim@xen.org>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>

---

v3:
 * Squash into single patch with hand-rolled cmpxchg() loops for easier backport.

v2:
 * Use atomic_{inc,dec}_bounded() instead of having a spinlock.
---
 xen/arch/x86/domctl.c   |    7 +++---
 xen/common/domain.c     |   61 ++++++++++++++++++++++++++++++++++-------------
 xen/common/domctl.c     |    9 ++++---
 xen/include/xen/sched.h |   15 +++++++++---
 4 files changed, 64 insertions(+), 28 deletions(-)

diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index a4effc3..efa8746 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 ( atomic_read(&d->controller_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 ( atomic_read(&d->controller_pause_count) > 0 )
             break;
         ret = -EINVAL;
         if ( domctl->u.gdbsx_pauseunp_vcpu.vcpu >= MAX_VIRT_CPUS ||
@@ -1061,7 +1061,8 @@ 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 =
+            atomic_read(&d->controller_pause_count) > 0;
         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..efa1789 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -317,7 +317,7 @@ struct domain *domain_create(
         if ( (err = xsm_domain_create(XSM_HOOK, d, ssidref)) != 0 )
             goto fail;
 
-        d->is_paused_by_controller = 1;
+        atomic_set(&d->controller_pause_count, 1);
         atomic_inc(&d->pause_count);
 
         if ( !is_hardware_domain(d) )
@@ -755,18 +755,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 +909,49 @@ 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 old, new, prev = atomic_read(&d->controller_pause_count);
+
+    do
+    {
+        old = prev;
+        new = old + 1;
+
+        /*
+         * Limit the toolstack pause count to an arbitrary 255 to prevent the
+         * toolstack overflowing d->pause_count with many repeated hypercalls.
+         */
+        if ( new > 255 )
+            return -EUSERS;
+
+        prev = cmpxchg(&d->controller_pause_count.counter, old, new);
+    } while ( prev != old );
+
+    pause_fn(d);
+
+    return 0;
 }
 
-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) )
-        domain_unpause(d);
+    int old, new, prev = atomic_read(&d->controller_pause_count);
+
+    do
+    {
+        old = prev;
+        new = old - 1;
+
+        if ( new < 0 )
+            return -EINVAL;
+
+        prev = cmpxchg(&d->controller_pause_count.counter, old, new);
+    } while ( prev != old );
+
+    domain_unpause(d);
+
+    return 0;
 }
 
 int vcpu_reset(struct vcpu *v)
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index 000993f..7af1605 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -181,7 +181,8 @@ 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) |
+        (atomic_read(&d->controller_pause_count) > 0
+                                        ? XEN_DOMINF_paused   : 0) |
         (d->debugger_attached           ? XEN_DOMINF_debugged : 0) |
         d->shutdown_code << XEN_DOMINF_shutdownshift;
 
@@ -398,16 +399,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..ddfcfc4 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -366,7 +366,7 @@ struct domain
     /* 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;
+    atomic_t         controller_pause_count;
     /* Domain's VCPUs are pinned 1:1 to physical CPUs? */
     bool_t           is_pinned;
 
@@ -769,8 +769,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 v3] xen/common: Properly reference count DOMCTL_{, un}pausedomain hypercalls

[Jan Beulich]

>>> On 02.07.14 at 17:03, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -366,7 +366,7 @@ struct domain
>      /* 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;
> +    atomic_t         controller_pause_count;
>      /* Domain's VCPUs are pinned 1:1 to physical CPUs? */
>      bool_t           is_pinned;
>  

Now why did you leave this be an atomic_t, sitting between two
bool_t-s?

Jan

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

[Andrew Cooper]

On 02/07/14 16:29, Jan Beulich wrote:
>>>> On 02.07.14 at 17:03, <andrew.cooper3@citrix.com> wrote:
>> --- a/xen/include/xen/sched.h
>> +++ b/xen/include/xen/sched.h
>> @@ -366,7 +366,7 @@ struct domain
>>      /* 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;
>> +    atomic_t         controller_pause_count;
>>      /* Domain's VCPUs are pinned 1:1 to physical CPUs? */
>>      bool_t           is_pinned;
>>  
> Now why did you leave this be an atomic_t, sitting between two
> bool_t-s?
>
> Jan
>

It is not sitting between two bool_t's.  It is sitting next to an enum
which has the same natural width and alignment.

Given the other holes in this area, moving it elsewhere wouldn't affect
the structure size.

~Andrew

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

[Jan Beulich]

>>> On 02.07.14 at 17:41, <andrew.cooper3@citrix.com> wrote:
> On 02/07/14 16:29, Jan Beulich wrote:
>>>>> On 02.07.14 at 17:03, <andrew.cooper3@citrix.com> wrote:
>>> --- a/xen/include/xen/sched.h
>>> +++ b/xen/include/xen/sched.h
>>> @@ -366,7 +366,7 @@ struct domain
>>>      /* 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;
>>> +    atomic_t         controller_pause_count;
>>>      /* Domain's VCPUs are pinned 1:1 to physical CPUs? */
>>>      bool_t           is_pinned;
>>>  
>> Now why did you leave this be an atomic_t, sitting between two
>> bool_t-s?
> 
> It is not sitting between two bool_t's.  It is sitting next to an enum
> which has the same natural width and alignment.

Oh, right, the first one is being replaced.

But you didn't say why it need to be an atomic_t, while in v1 it was a
plain scalar.

Jan

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

[Andrew Cooper]

On 02/07/14 17:06, Jan Beulich wrote:
>>>> On 02.07.14 at 17:41, <andrew.cooper3@citrix.com> wrote:
>> On 02/07/14 16:29, Jan Beulich wrote:
>>>>>> On 02.07.14 at 17:03, <andrew.cooper3@citrix.com> wrote:
>>>> --- a/xen/include/xen/sched.h
>>>> +++ b/xen/include/xen/sched.h
>>>> @@ -366,7 +366,7 @@ struct domain
>>>>      /* 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;
>>>> +    atomic_t         controller_pause_count;
>>>>      /* Domain's VCPUs are pinned 1:1 to physical CPUs? */
>>>>      bool_t           is_pinned;
>>>>  
>>> Now why did you leave this be an atomic_t, sitting between two
>>> bool_t-s?
>> It is not sitting between two bool_t's.  It is sitting next to an enum
>> which has the same natural width and alignment.
> Oh, right, the first one is being replaced.
>
> But you didn't say why it need to be an atomic_t, while in v1 it was a
> plain scalar.
>
> Jan
>

v1 was a plain scalar because of the spinlock.  v2 was an atomic_t
because of atomic_{inc,dec}_bounded()

But given the hand rolled loops with cmpxchg(), this can indeed be a
plan scalar.

v4 on its way.

~Andrew