[PATCH v3 1/2] xen/common: add vcpus_create() and keep max_vcpus in sync

Every vcpu_create() call site that builds more than one vcpu loops
over ids up to d->max_vcpus and stops on the first failure, but none
of them roll max_vcpus back to match. This leaves d->vcpu[i] == NULL
for ids below max_vcpus, which anything walking d->vcpu[] can then
dereference. This is what caused the crash: sched_move_domain()
walks every vcpu slot up to max_vcpus without checking for empty
ones, so when a domain built in a non-default cpupool had vcpu
creation fail partway through, domain_kill() later moving it back
to the default cpupool handed one of its empty slots straight to
the new cpupool's scheduler, causing a NULL-pointer dereference
inside sched_alloc_udata().

Add vcpus_create(d): creates every vcpu of d up to max_vcpus and
rolls max_vcpus back to the failed id on error. This keeps
d->vcpu[i] is non-NULL for all i < d->max_vcpus, instead of guarding
every reader of d->vcpu[] agains holes individually.

Convert every site that builds vcpus in a loop to call this function
instead.

Fixes: 61649709421a ("xen/domain: Allocate d->vcpu[] in domain_create()")
Suggested-by: Juergen Gross <[email protected]>
Signed-off-by: Furkan Caliskan <[email protected]>
---
v3:
 - Reworked per Juergen's suggestion: instead of guarding
   sched_move_domain() against a missing vcpu slot, keep d->max_vcpus
   in sync with the vcpus actually created. Added vcpus_create() and
   converted every vcpu_create() loop to use it.
 - Reverted the sched_move_domain() check from v2, now unneeded.
---
 xen/arch/arm/domain_build.c   | 15 +++++++--------
 xen/arch/x86/mm/mem_sharing.c | 11 ++---------
 xen/common/domain.c           | 24 ++++++++++++++++++++++++
 xen/common/domctl.c           | 19 ++++---------------
 xen/common/sched/core.c       |  7 +++----
 xen/include/xen/domain.h      |  1 +
 6 files changed, 41 insertions(+), 36 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 72d5316180..e08ee21ee5 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1774,6 +1774,7 @@ static void __init find_gnttab_region(struct domain *d,
 int __init construct_domain(struct domain *d, struct kernel_info *kinfo)
 {
     unsigned int i;
+    int rc;
     struct vcpu *v = d->vcpu[0];
     struct cpu_user_regs *regs = &v->arch.cpu_info->guest_cpu_user_regs;
 
@@ -1842,17 +1843,15 @@ int __init construct_domain(struct domain *d, struct kernel_info *kinfo)
     }
 #endif
 
-    for ( i = 1; i < d->max_vcpus; i++ )
+    if ( (rc = vcpus_create(d)) )
     {
-        if ( vcpu_create(d, i) == NULL )
-        {
-            printk("Failed to allocate d%dv%d\n", d->domain_id, i);
-            return -ENOMEM;
-        }
+        printk("Failed to allocate d%dv%d\n", d->domain_id, d->max_vcpus);
+        return rc;
+    }
 
-        if ( is_64bit_domain(d) )
+    if ( is_64bit_domain(d) )
+        for ( i = 1; i < d->max_vcpus; i++ )
             vcpu_switch_to_aarch64_mode(d->vcpu[i]);
-    }
 
     domain_update_node_affinity(d);
 
diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
index 5c7a0ff30e..cd7f747c80 100644
--- a/xen/arch/x86/mm/mem_sharing.c
+++ b/xen/arch/x86/mm/mem_sharing.c
@@ -1612,21 +1612,14 @@ int mem_sharing_fork_page(struct domain *d, gfn_t gfn, bool unsharing)
 
 static int bring_up_vcpus(struct domain *cd, struct domain *d)
 {
-    unsigned int i;
     int ret = -EINVAL;
 
     if ( d->max_vcpus != cd->max_vcpus ||
         (ret = cpupool_move_domain(cd, d->cpupool)) )
         return ret;
 
-    for ( i = 0; i < cd->max_vcpus; i++ )
-    {
-        if ( !d->vcpu[i] || cd->vcpu[i] )
-            continue;
-
-        if ( !vcpu_create(cd, i) )
-            return -EINVAL;
-    }
+    if ( (ret = vcpus_create(cd)) )
+        return ret;
 
     domain_update_node_affinity(cd);
     return 0;
diff --git a/xen/common/domain.c b/xen/common/domain.c
index e16f1ac383..a0a3e51b15 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -539,6 +539,30 @@ struct vcpu *vcpu_create(struct domain *d, unsigned int vcpu_id)
     return NULL;
 }
 
+/*
+ * Create every not yet existing vcpu of d, up to d->max_vcpus. On failure,
+ * d->max_vcpus is rolled back to the id that failed, keeping d->vcpu[i]
+ * non-NULL for all i < d->max_vcpus.
+ */
+int vcpus_create(struct domain *d)
+{
+    unsigned int i;
+
+    for ( i = 0; i < d->max_vcpus; i++ )
+    {
+        if ( d->vcpu[i] )
+            continue;
+
+        if ( vcpu_create(d, i) == NULL )
+        {
+            d->max_vcpus = i;
+            return -EINVAL;
+        }
+    }
+
+    return 0;
+}
+
 static int late_hwdom_init(struct domain *d)
 {
 #ifdef CONFIG_LATE_HWDOM
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index a6210db4fb..39f3f219ca 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -698,7 +698,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
 
     case XEN_DOMCTL_max_vcpus:
     {
-        unsigned int i, max = op->u.max_vcpus.max;
+        unsigned int max = op->u.max_vcpus.max;
 
         ret = -EINVAL;
         if ( (d == current->domain) || /* no domain_pause() */
@@ -708,21 +708,10 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
         /* Needed, for example, to ensure writable p.t. state is synced. */
         domain_pause(d);
 
-        ret = -ENOMEM;
-
-        for ( i = 0; i < max; i++ )
-        {
-            if ( d->vcpu[i] != NULL )
-                continue;
-
-            if ( vcpu_create(d, i) == NULL )
-                goto maxvcpu_out;
-        }
-
-        domain_update_node_affinity(d);
-        ret = 0;
+        ret = vcpus_create(d);
+        if ( !ret )
+            domain_update_node_affinity(d);
 
-    maxvcpu_out:
         domain_unpause(d);
         break;
     }
diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c
index d3a0a97e1d..14069eed03 100644
--- a/xen/common/sched/core.c
+++ b/xen/common/sched/core.c
@@ -3497,10 +3497,9 @@ void wait(void)
 #ifdef CONFIG_X86
 void __init sched_setup_dom0_vcpus(struct domain *d)
 {
-    unsigned int i;
-
-    for ( i = 1; i < d->max_vcpus; i++ )
-        vcpu_create(d, i);
+    if ( vcpus_create(d) )
+        printk("Failed to create all vcpus of dom0 (max_vcpus now %u)\n",
+               d->max_vcpus);
 
     domain_update_node_affinity(d);
 }
diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h
index aeb8b36ad1..eaf406a814 100644
--- a/xen/include/xen/domain.h
+++ b/xen/include/xen/domain.h
@@ -34,6 +34,7 @@ typedef union {
 } vcpu_guest_context_u __attribute__((__transparent_union__));
 
 struct vcpu *vcpu_create(struct domain *d, unsigned int vcpu_id);
+int vcpus_create(struct domain *d);
 
 unsigned int dom0_max_vcpus(void);
 int parse_arch_dom0_param(const char *s, const char *e);
-- 
2.34.1