[PATCH v3 0/2] xen/sched: fix crashes when vcpu creation fails

[PATCH v3 0/2] xen/sched: fix crashes when vcpu creation fails

[Furkan Caliskan]

Furkan Caliskan (2):
  xen/common: add vcpus_create() and keep max_vcpus in sync
  xen/sched: core: kill unarmed timers on sched_init_vcpu() failure

 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       | 16 +++++++++-------
 xen/include/xen/domain.h      |  1 +
 6 files changed, 47 insertions(+), 39 deletions(-)

-- 
2.34.1

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

[Furkan Caliskan]

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@suse.com>
Signed-off-by: Furkan Caliskan <frn1furkan10@gmail.com>
---
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

[PATCH v3 2/2] xen/sched: core: kill unarmed timers on sched_init_vcpu() failure

[Furkan Caliskan]

sched_init_vcpu() calls init_timer() for a vcpu's periodic_timer,
singleshot_timer and poll_timer before it can fail -- these
become live, linked into their target pCPU's per-cpu timer list
regardless of what happens next. If the sched_alloc_udata() call
further down then fails, the function frees the sched_unit via
sched_free_unit() and returns 1, but never unlinks these three
timers.

The caller, vcpu_create(), makes this worse: on sched_init_vcpu()
returning nonzero it jumps to fail_wq, skipping fail_sched and
thus sched_destroy_vcpu() -- the only function on this path that
calls kill_timer() on them. vcpu_destroy() then frees the vcpu,
and the three timers embedded in it, while they are still linked
into that shared list.

This silently corrupts that list. It only shows up later, when
something else touches a neighboring timer: sched_move_domain()
crashed with "Assertion 'entry->prev->next == entry' failed" on a
completely unrelated, valid vcpu's timer.

Call sched_destroy_vcpu() in sched_init_vcpu()'s own failure
branch instead of sched_free_unit(), so it doesn't depend
on the caller reaching sched_destroy_vcpu() to undo what it set
up itself. sched_destroy_vcpu() assumes unit->priv is set, which
is not the case here, so make it only free the udata and remove
the unit if unit->priv in non-NULL.

Fixes: d884b1077817 ("Domain creation/destruction cleanups.")
Signed-off-by: Furkan Caliskan <frn1furkan10@gmail.com>
---
v3:
 - Call sched_destroy_vcpu() from sched_init_vcpu()'s failure
   branch.
 - Made sched_destroy_vcpu() tolerate unit->priv == NULL.
---
 xen/common/sched/core.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c
index 14069eed03..b65f728e77 100644
--- a/xen/common/sched/core.c
+++ b/xen/common/sched/core.c
@@ -589,7 +589,7 @@ int sched_init_vcpu(struct vcpu *v)
     unit->priv = sched_alloc_udata(dom_scheduler(d), unit, d->sched_priv);
     if ( unit->priv == NULL )
     {
-        sched_free_unit(unit, v);
+        sched_destroy_vcpu(v);
         rcu_read_unlock(&sched_res_rculock);
         return 1;
     }
@@ -869,8 +869,11 @@ void sched_destroy_vcpu(struct vcpu *v)
     {
         rcu_read_lock(&sched_res_rculock);
 
-        sched_remove_unit(vcpu_scheduler(v), unit);
-        sched_free_udata(vcpu_scheduler(v), unit->priv);
+        if ( unit->priv )
+        {
+            sched_remove_unit(vcpu_scheduler(v), unit);
+            sched_free_udata(vcpu_scheduler(v), unit->priv);
+        }
         sched_free_unit(unit, v);
 
         rcu_read_unlock(&sched_res_rculock);
-- 
2.34.1

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

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 5083 bytes --]

On 31.08.26 07:16, Furkan Caliskan wrote:
> 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@suse.com>
> Signed-off-by: Furkan Caliskan <frn1furkan10@gmail.com>
> ---
> 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;

I think this should be -ENOMEM.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH v3 2/2] xen/sched: core: kill unarmed timers on sched_init_vcpu() failure

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 1631 bytes --]

On 31.08.26 07:16, Furkan Caliskan wrote:
> sched_init_vcpu() calls init_timer() for a vcpu's periodic_timer,
> singleshot_timer and poll_timer before it can fail -- these
> become live, linked into their target pCPU's per-cpu timer list
> regardless of what happens next. If the sched_alloc_udata() call
> further down then fails, the function frees the sched_unit via
> sched_free_unit() and returns 1, but never unlinks these three
> timers.
> 
> The caller, vcpu_create(), makes this worse: on sched_init_vcpu()
> returning nonzero it jumps to fail_wq, skipping fail_sched and
> thus sched_destroy_vcpu() -- the only function on this path that
> calls kill_timer() on them. vcpu_destroy() then frees the vcpu,
> and the three timers embedded in it, while they are still linked
> into that shared list.
> 
> This silently corrupts that list. It only shows up later, when
> something else touches a neighboring timer: sched_move_domain()
> crashed with "Assertion 'entry->prev->next == entry' failed" on a
> completely unrelated, valid vcpu's timer.
> 
> Call sched_destroy_vcpu() in sched_init_vcpu()'s own failure
> branch instead of sched_free_unit(), so it doesn't depend
> on the caller reaching sched_destroy_vcpu() to undo what it set
> up itself. sched_destroy_vcpu() assumes unit->priv is set, which
> is not the case here, so make it only free the udata and remove
> the unit if unit->priv in non-NULL.
> 
> Fixes: d884b1077817 ("Domain creation/destruction cleanups.")
> Signed-off-by: Furkan Caliskan <frn1furkan10@gmail.com>

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

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

[Furkan Çalışkan]

On 8/31/26 11:13, Jürgen Groß wrote:
> On 31.08.26 07:16, Furkan Caliskan wrote:
>> 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@suse.com>
>> Signed-off-by: Furkan Caliskan <frn1furkan10@gmail.com>
>> ---
>> 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;
> 
> I think this should be -ENOMEM.
> 
> 
> Juergen

Currently vcpu_create() can only fail because of memory errors, so
-ENOMEM would be correct today. But I've sent a patch series that
adds RTDS admission control, which would make vcpu_create() also
fail for a capacity issue, so I went with -EINVAL here to not only
tie it to the memory failure.

If you'd rather keep it as -ENOMEM, I'm happy to update it.

Furkan

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

[Furkan Caliskan]

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@suse.com>
Signed-off-by: Furkan Caliskan <frn1furkan10@gmail.com>
---
v4:
 - Return -ENOMEM instead of -EINVAL from vcpus_create() on failure.
---
 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..32b7fa34d1 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 -ENOMEM;
+        }
+    }
+
+    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

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

[Andrew Cooper]

On 31/08/2026 6:16 am, Furkan Caliskan wrote:
> 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

As I told you before, you must cope with this property in non-error
scenarios.

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

No, it really doesn't.

> , 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@suse.com>
> Signed-off-by: Furkan Caliskan <frn1furkan10@gmail.com>

For the avoidance of a long drawn-out argument, nack.  Under no
circumstances are you editing d->max_cpus after it's put into the domain
list.

You've chosen to do so at a point where the domain object is live,
visible in the system and able to be the target of other hypercalls.

Furthermore you have not fixed what your commit message claims. 
d->vcpu[...] is still NULL for an arbitrary period of time, including
being able to be the target of hypercalls, before vCPUs are created.

All code MUST be able to cope with d->vcpu[...] being NULL.  It's how
the object lifecycles must work, because creating vCPUs is not atomic
with respect to creating domains.

~Andrew

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

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 2445 bytes --]

On 31.08.26 11:45, Andrew Cooper wrote:
> On 31/08/2026 6:16 am, Furkan Caliskan wrote:
>> 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
> 
> As I told you before, you must cope with this property in non-error
> scenarios.
> 
>> , 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
> 
> No, it really doesn't.
> 
>> , 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@suse.com>
>> Signed-off-by: Furkan Caliskan <frn1furkan10@gmail.com>
> 
> For the avoidance of a long drawn-out argument, nack.  Under no
> circumstances are you editing d->max_cpus after it's put into the domain
> list.
> 
> You've chosen to do so at a point where the domain object is live,
> visible in the system and able to be the target of other hypercalls.
> 
> Furthermore you have not fixed what your commit message claims.
> d->vcpu[...] is still NULL for an arbitrary period of time, including
> being able to be the target of hypercalls, before vCPUs are created.
> 
> All code MUST be able to cope with d->vcpu[...] being NULL.  It's how
> the object lifecycles must work, because creating vCPUs is not atomic
> with respect to creating domains.

Would you be fine with me creating a patch series moving vcpu creation into
domain_create()?

This would at once solve all those problems, while even removing the need for
having the XEN_DOMCTL_max_vcpus domctl.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

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

[Jan Beulich]

On 31.08.2026 14:59, Jürgen Groß wrote:
> On 31.08.26 11:45, Andrew Cooper wrote:
>> On 31/08/2026 6:16 am, Furkan Caliskan wrote:
>>> 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
>>
>> As I told you before, you must cope with this property in non-error
>> scenarios.
>>
>>> , 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
>>
>> No, it really doesn't.
>>
>>> , 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@suse.com>
>>> Signed-off-by: Furkan Caliskan <frn1furkan10@gmail.com>
>>
>> For the avoidance of a long drawn-out argument, nack.  Under no
>> circumstances are you editing d->max_cpus after it's put into the domain
>> list.
>>
>> You've chosen to do so at a point where the domain object is live,
>> visible in the system and able to be the target of other hypercalls.
>>
>> Furthermore you have not fixed what your commit message claims.
>> d->vcpu[...] is still NULL for an arbitrary period of time, including
>> being able to be the target of hypercalls, before vCPUs are created.
>>
>> All code MUST be able to cope with d->vcpu[...] being NULL.  It's how
>> the object lifecycles must work, because creating vCPUs is not atomic
>> with respect to creating domains.
> 
> Would you be fine with me creating a patch series moving vcpu creation into
> domain_create()?

This was discussed before, and however nice it would be for the issue at hand,
it would get in the way of us wanting to have CPU policy for domains put in
place before vCPU-s are created, such that on x86 the XSAVE area can be sized
once and for all.

Jan

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

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 3060 bytes --]

On 01.09.26 09:22, Jan Beulich wrote:
> On 31.08.2026 14:59, Jürgen Groß wrote:
>> On 31.08.26 11:45, Andrew Cooper wrote:
>>> On 31/08/2026 6:16 am, Furkan Caliskan wrote:
>>>> 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
>>>
>>> As I told you before, you must cope with this property in non-error
>>> scenarios.
>>>
>>>> , 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
>>>
>>> No, it really doesn't.
>>>
>>>> , 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@suse.com>
>>>> Signed-off-by: Furkan Caliskan <frn1furkan10@gmail.com>
>>>
>>> For the avoidance of a long drawn-out argument, nack.  Under no
>>> circumstances are you editing d->max_cpus after it's put into the domain
>>> list.
>>>
>>> You've chosen to do so at a point where the domain object is live,
>>> visible in the system and able to be the target of other hypercalls.
>>>
>>> Furthermore you have not fixed what your commit message claims.
>>> d->vcpu[...] is still NULL for an arbitrary period of time, including
>>> being able to be the target of hypercalls, before vCPUs are created.
>>>
>>> All code MUST be able to cope with d->vcpu[...] being NULL.  It's how
>>> the object lifecycles must work, because creating vCPUs is not atomic
>>> with respect to creating domains.
>>
>> Would you be fine with me creating a patch series moving vcpu creation into
>> domain_create()?
> 
> This was discussed before, and however nice it would be for the issue at hand,
> it would get in the way of us wanting to have CPU policy for domains put in
> place before vCPU-s are created, such that on x86 the XSAVE area can be sized
> once and for all.

This could be done when unpausing the domain initially.

OTOH I don't see xstate_alloc_save_area() looking at the domain's cpu policy
at all. Is this a plan for the future?

And additionally there is no guard for avoiding the vcpus being created before
the policy is being set.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

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

[Jan Beulich]

On 01.09.2026 10:07, Jürgen Groß wrote:
> On 01.09.26 09:22, Jan Beulich wrote:
>> On 31.08.2026 14:59, Jürgen Groß wrote:
>>> On 31.08.26 11:45, Andrew Cooper wrote:
>>>> On 31/08/2026 6:16 am, Furkan Caliskan wrote:
>>>>> 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
>>>>
>>>> As I told you before, you must cope with this property in non-error
>>>> scenarios.
>>>>
>>>>> , 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
>>>>
>>>> No, it really doesn't.
>>>>
>>>>> , 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@suse.com>
>>>>> Signed-off-by: Furkan Caliskan <frn1furkan10@gmail.com>
>>>>
>>>> For the avoidance of a long drawn-out argument, nack.  Under no
>>>> circumstances are you editing d->max_cpus after it's put into the domain
>>>> list.
>>>>
>>>> You've chosen to do so at a point where the domain object is live,
>>>> visible in the system and able to be the target of other hypercalls.
>>>>
>>>> Furthermore you have not fixed what your commit message claims.
>>>> d->vcpu[...] is still NULL for an arbitrary period of time, including
>>>> being able to be the target of hypercalls, before vCPUs are created.
>>>>
>>>> All code MUST be able to cope with d->vcpu[...] being NULL.  It's how
>>>> the object lifecycles must work, because creating vCPUs is not atomic
>>>> with respect to creating domains.
>>>
>>> Would you be fine with me creating a patch series moving vcpu creation into
>>> domain_create()?
>>
>> This was discussed before, and however nice it would be for the issue at hand,
>> it would get in the way of us wanting to have CPU policy for domains put in
>> place before vCPU-s are created, such that on x86 the XSAVE area can be sized
>> once and for all.
> 
> This could be done when unpausing the domain initially.

Imo unpausing shouldn't fail because of memory shortage.

> OTOH I don't see xstate_alloc_save_area() looking at the domain's cpu policy
> at all. Is this a plan for the future?

This is to better accommodate the AMX series (which has been pending for years),
and potentially also for architectural-LBR work (which has been posted once, but
was apparently abandoned).

> And additionally there is no guard for avoiding the vcpus being created before
> the policy is being set.

Addressing that is part of Andrew's plan, aiui.

Jan

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

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 3320 bytes --]

On 01.09.26 10:15, Jan Beulich wrote:
> On 01.09.2026 10:07, Jürgen Groß wrote:
>> On 01.09.26 09:22, Jan Beulich wrote:
>>> On 31.08.2026 14:59, Jürgen Groß wrote:
>>>> On 31.08.26 11:45, Andrew Cooper wrote:
>>>>> On 31/08/2026 6:16 am, Furkan Caliskan wrote:
>>>>>> 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
>>>>>
>>>>> As I told you before, you must cope with this property in non-error
>>>>> scenarios.
>>>>>
>>>>>> , 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
>>>>>
>>>>> No, it really doesn't.
>>>>>
>>>>>> , 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@suse.com>
>>>>>> Signed-off-by: Furkan Caliskan <frn1furkan10@gmail.com>
>>>>>
>>>>> For the avoidance of a long drawn-out argument, nack.  Under no
>>>>> circumstances are you editing d->max_cpus after it's put into the domain
>>>>> list.
>>>>>
>>>>> You've chosen to do so at a point where the domain object is live,
>>>>> visible in the system and able to be the target of other hypercalls.
>>>>>
>>>>> Furthermore you have not fixed what your commit message claims.
>>>>> d->vcpu[...] is still NULL for an arbitrary period of time, including
>>>>> being able to be the target of hypercalls, before vCPUs are created.
>>>>>
>>>>> All code MUST be able to cope with d->vcpu[...] being NULL.  It's how
>>>>> the object lifecycles must work, because creating vCPUs is not atomic
>>>>> with respect to creating domains.
>>>>
>>>> Would you be fine with me creating a patch series moving vcpu creation into
>>>> domain_create()?
>>>
>>> This was discussed before, and however nice it would be for the issue at hand,
>>> it would get in the way of us wanting to have CPU policy for domains put in
>>> place before vCPU-s are created, such that on x86 the XSAVE area can be sized
>>> once and for all.
>>
>> This could be done when unpausing the domain initially.
> 
> Imo unpausing shouldn't fail because of memory shortage.

As long as the domain hasn't started running I don't see why this would be
different to the case where not all vcpus could be created.

BTW, I will address scenarios like that in my Xen summit presentation. :-)


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

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

[Jan Beulich]

On 01.09.2026 10:24, Jürgen Groß wrote:
> On 01.09.26 10:15, Jan Beulich wrote:
>> On 01.09.2026 10:07, Jürgen Groß wrote:
>>> On 01.09.26 09:22, Jan Beulich wrote:
>>>> On 31.08.2026 14:59, Jürgen Groß wrote:
>>>>> On 31.08.26 11:45, Andrew Cooper wrote:
>>>>>> On 31/08/2026 6:16 am, Furkan Caliskan wrote:
>>>>>>> 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
>>>>>>
>>>>>> As I told you before, you must cope with this property in non-error
>>>>>> scenarios.
>>>>>>
>>>>>>> , 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
>>>>>>
>>>>>> No, it really doesn't.
>>>>>>
>>>>>>> , 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@suse.com>
>>>>>>> Signed-off-by: Furkan Caliskan <frn1furkan10@gmail.com>
>>>>>>
>>>>>> For the avoidance of a long drawn-out argument, nack.  Under no
>>>>>> circumstances are you editing d->max_cpus after it's put into the domain
>>>>>> list.
>>>>>>
>>>>>> You've chosen to do so at a point where the domain object is live,
>>>>>> visible in the system and able to be the target of other hypercalls.
>>>>>>
>>>>>> Furthermore you have not fixed what your commit message claims.
>>>>>> d->vcpu[...] is still NULL for an arbitrary period of time, including
>>>>>> being able to be the target of hypercalls, before vCPUs are created.
>>>>>>
>>>>>> All code MUST be able to cope with d->vcpu[...] being NULL.  It's how
>>>>>> the object lifecycles must work, because creating vCPUs is not atomic
>>>>>> with respect to creating domains.
>>>>>
>>>>> Would you be fine with me creating a patch series moving vcpu creation into
>>>>> domain_create()?
>>>>
>>>> This was discussed before, and however nice it would be for the issue at hand,
>>>> it would get in the way of us wanting to have CPU policy for domains put in
>>>> place before vCPU-s are created, such that on x86 the XSAVE area can be sized
>>>> once and for all.
>>>
>>> This could be done when unpausing the domain initially.
>>
>> Imo unpausing shouldn't fail because of memory shortage.
> 
> As long as the domain hasn't started running I don't see why this would be
> different to the case where not all vcpus could be created.

I do. One could create a domain ready to be unpaused, but being kept paused
until whatever event triggers its launching. That better wouldn't fail, except
in extraordinary situations.

Jan

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

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 3987 bytes --]

On 01.09.26 10:36, Jan Beulich wrote:
> On 01.09.2026 10:24, Jürgen Groß wrote:
>> On 01.09.26 10:15, Jan Beulich wrote:
>>> On 01.09.2026 10:07, Jürgen Groß wrote:
>>>> On 01.09.26 09:22, Jan Beulich wrote:
>>>>> On 31.08.2026 14:59, Jürgen Groß wrote:
>>>>>> On 31.08.26 11:45, Andrew Cooper wrote:
>>>>>>> On 31/08/2026 6:16 am, Furkan Caliskan wrote:
>>>>>>>> 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
>>>>>>>
>>>>>>> As I told you before, you must cope with this property in non-error
>>>>>>> scenarios.
>>>>>>>
>>>>>>>> , 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
>>>>>>>
>>>>>>> No, it really doesn't.
>>>>>>>
>>>>>>>> , 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@suse.com>
>>>>>>>> Signed-off-by: Furkan Caliskan <frn1furkan10@gmail.com>
>>>>>>>
>>>>>>> For the avoidance of a long drawn-out argument, nack.  Under no
>>>>>>> circumstances are you editing d->max_cpus after it's put into the domain
>>>>>>> list.
>>>>>>>
>>>>>>> You've chosen to do so at a point where the domain object is live,
>>>>>>> visible in the system and able to be the target of other hypercalls.
>>>>>>>
>>>>>>> Furthermore you have not fixed what your commit message claims.
>>>>>>> d->vcpu[...] is still NULL for an arbitrary period of time, including
>>>>>>> being able to be the target of hypercalls, before vCPUs are created.
>>>>>>>
>>>>>>> All code MUST be able to cope with d->vcpu[...] being NULL.  It's how
>>>>>>> the object lifecycles must work, because creating vCPUs is not atomic
>>>>>>> with respect to creating domains.
>>>>>>
>>>>>> Would you be fine with me creating a patch series moving vcpu creation into
>>>>>> domain_create()?
>>>>>
>>>>> This was discussed before, and however nice it would be for the issue at hand,
>>>>> it would get in the way of us wanting to have CPU policy for domains put in
>>>>> place before vCPU-s are created, such that on x86 the XSAVE area can be sized
>>>>> once and for all.
>>>>
>>>> This could be done when unpausing the domain initially.
>>>
>>> Imo unpausing shouldn't fail because of memory shortage.
>>
>> As long as the domain hasn't started running I don't see why this would be
>> different to the case where not all vcpus could be created.
> 
> I do. One could create a domain ready to be unpaused, but being kept paused
> until whatever event triggers its launching. That better wouldn't fail, except
> in extraordinary situations.

Okay, then we could add XEN_DOMCTL_finalize doing the final allocations AND
doing the creation_finished handling. This would even catch today's domain
crashing in the VMX specific domain_creation_finished() callback.

At the same time XEN_DOMCTL_max_vcpus would be dropped, so there isn't more
work for the toolstack.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]