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 <jgross@xxxxxxxx>
Signed-off-by: Furkan Caliskan <frn1furkan10@xxxxxxxxx>
---
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;
