* Re: [PATCH v3 1/2] xen/common: add vcpus_create() and keep max_vcpus in sync
2026-08-31 5:16 ` [PATCH v3 1/2] xen/common: add vcpus_create() and keep max_vcpus in sync Furkan Caliskan
@ 2026-08-31 8:13 ` Jürgen Groß
2026-08-31 9:13 ` Furkan Çalışkan
2026-08-31 9:32 ` [PATCH v4] " Furkan Caliskan
2026-08-31 9:45 ` [PATCH v3 1/2] " Andrew Cooper
2 siblings, 1 reply; 15+ messages in thread
From: Jürgen Groß @ 2026-08-31 8:13 UTC (permalink / raw)
To: Furkan Caliskan, xen-devel
Cc: jbeulich, andrew.cooper3, dfaggioli, gwd, roger, anthony.perard,
julien, bertrand.marquis, michal.orzel, Volodymyr_Babchuk,
teddy.astie
[-- 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 --]
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v3 1/2] xen/common: add vcpus_create() and keep max_vcpus in sync
2026-08-31 8:13 ` Jürgen Groß
@ 2026-08-31 9:13 ` Furkan Çalışkan
0 siblings, 0 replies; 15+ messages in thread
From: Furkan Çalışkan @ 2026-08-31 9:13 UTC (permalink / raw)
To: Jürgen Groß, xen-devel
Cc: jbeulich, andrew.cooper3, dfaggioli, gwd, roger, anthony.perard,
julien, bertrand.marquis, michal.orzel, Volodymyr_Babchuk,
teddy.astie
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v4] xen/common: add vcpus_create() and keep max_vcpus in sync
2026-08-31 5:16 ` [PATCH v3 1/2] xen/common: add vcpus_create() and keep max_vcpus in sync Furkan Caliskan
2026-08-31 8:13 ` Jürgen Groß
@ 2026-08-31 9:32 ` Furkan Caliskan
2026-08-31 9:45 ` [PATCH v3 1/2] " Andrew Cooper
2 siblings, 0 replies; 15+ messages in thread
From: Furkan Caliskan @ 2026-08-31 9:32 UTC (permalink / raw)
To: xen-devel; +Cc: jgross, jbeulich, andrew.cooper3, 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
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH v3 1/2] xen/common: add vcpus_create() and keep max_vcpus in sync
2026-08-31 5:16 ` [PATCH v3 1/2] xen/common: add vcpus_create() and keep max_vcpus in sync Furkan Caliskan
2026-08-31 8:13 ` Jürgen Groß
2026-08-31 9:32 ` [PATCH v4] " Furkan Caliskan
@ 2026-08-31 9:45 ` Andrew Cooper
2026-08-31 12:59 ` Jürgen Groß
2 siblings, 1 reply; 15+ messages in thread
From: Andrew Cooper @ 2026-08-31 9:45 UTC (permalink / raw)
To: Furkan Caliskan, xen-devel
Cc: Andrew Cooper, jgross, jbeulich, dfaggioli, gwd, roger,
anthony.perard, julien, bertrand.marquis, michal.orzel,
Volodymyr_Babchuk, teddy.astie
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
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v3 1/2] xen/common: add vcpus_create() and keep max_vcpus in sync
2026-08-31 9:45 ` [PATCH v3 1/2] " Andrew Cooper
@ 2026-08-31 12:59 ` Jürgen Groß
2026-09-01 7:22 ` Jan Beulich
0 siblings, 1 reply; 15+ messages in thread
From: Jürgen Groß @ 2026-08-31 12:59 UTC (permalink / raw)
To: Andrew Cooper, Furkan Caliskan, xen-devel
Cc: jbeulich, dfaggioli, gwd, roger, anthony.perard, julien,
bertrand.marquis, michal.orzel, Volodymyr_Babchuk, teddy.astie
[-- 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 --]
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v3 1/2] xen/common: add vcpus_create() and keep max_vcpus in sync
2026-08-31 12:59 ` Jürgen Groß
@ 2026-09-01 7:22 ` Jan Beulich
2026-09-01 8:07 ` Jürgen Groß
0 siblings, 1 reply; 15+ messages in thread
From: Jan Beulich @ 2026-09-01 7:22 UTC (permalink / raw)
To: Jürgen Groß
Cc: dfaggioli, gwd, roger, anthony.perard, julien, bertrand.marquis,
michal.orzel, Volodymyr_Babchuk, teddy.astie, Andrew Cooper,
Furkan Caliskan, xen-devel
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
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v3 1/2] xen/common: add vcpus_create() and keep max_vcpus in sync
2026-09-01 7:22 ` Jan Beulich
@ 2026-09-01 8:07 ` Jürgen Groß
2026-09-01 8:15 ` Jan Beulich
0 siblings, 1 reply; 15+ messages in thread
From: Jürgen Groß @ 2026-09-01 8:07 UTC (permalink / raw)
To: Jan Beulich
Cc: dfaggioli, gwd, roger, anthony.perard, julien, bertrand.marquis,
michal.orzel, Volodymyr_Babchuk, teddy.astie, Andrew Cooper,
Furkan Caliskan, xen-devel
[-- 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 --]
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v3 1/2] xen/common: add vcpus_create() and keep max_vcpus in sync
2026-09-01 8:07 ` Jürgen Groß
@ 2026-09-01 8:15 ` Jan Beulich
2026-09-01 8:24 ` Jürgen Groß
0 siblings, 1 reply; 15+ messages in thread
From: Jan Beulich @ 2026-09-01 8:15 UTC (permalink / raw)
To: Jürgen Groß, Andrew Cooper
Cc: dfaggioli, gwd, roger, anthony.perard, julien, bertrand.marquis,
michal.orzel, Volodymyr_Babchuk, teddy.astie, Furkan Caliskan,
xen-devel
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
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v3 1/2] xen/common: add vcpus_create() and keep max_vcpus in sync
2026-09-01 8:15 ` Jan Beulich
@ 2026-09-01 8:24 ` Jürgen Groß
2026-09-01 8:36 ` Jan Beulich
0 siblings, 1 reply; 15+ messages in thread
From: Jürgen Groß @ 2026-09-01 8:24 UTC (permalink / raw)
To: Jan Beulich, Andrew Cooper
Cc: dfaggioli, gwd, roger, anthony.perard, julien, bertrand.marquis,
michal.orzel, Volodymyr_Babchuk, teddy.astie, Furkan Caliskan,
xen-devel
[-- 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 --]
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v3 1/2] xen/common: add vcpus_create() and keep max_vcpus in sync
2026-09-01 8:24 ` Jürgen Groß
@ 2026-09-01 8:36 ` Jan Beulich
2026-09-01 9:03 ` Jürgen Groß
0 siblings, 1 reply; 15+ messages in thread
From: Jan Beulich @ 2026-09-01 8:36 UTC (permalink / raw)
To: Jürgen Groß
Cc: dfaggioli, gwd, roger, anthony.perard, julien, bertrand.marquis,
michal.orzel, Volodymyr_Babchuk, teddy.astie, Furkan Caliskan,
xen-devel, Andrew Cooper
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
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v3 1/2] xen/common: add vcpus_create() and keep max_vcpus in sync
2026-09-01 8:36 ` Jan Beulich
@ 2026-09-01 9:03 ` Jürgen Groß
0 siblings, 0 replies; 15+ messages in thread
From: Jürgen Groß @ 2026-09-01 9:03 UTC (permalink / raw)
To: Jan Beulich
Cc: dfaggioli, gwd, roger, anthony.perard, julien, bertrand.marquis,
michal.orzel, Volodymyr_Babchuk, teddy.astie, Furkan Caliskan,
xen-devel, Andrew Cooper
[-- 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 --]
^ permalink raw reply [flat|nested] 15+ messages in thread