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

[Furkan Çalışkan <frn1furkan10@xxxxxxxxx>]

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@xxxxxxxx>
>> Signed-off-by: Furkan Caliskan <frn1furkan10@xxxxxxxxx>
>> ---
>> v3:
>>   - Reworked per Juergen's suggestion: instead of guarding
>>     sched_move_domain() against a missing vcpu slot, keep d->max_vcpus
>>     in sync with the vcpus actually created. Added vcpus_create() and
>>     converted every vcpu_create() loop to use it.
>>   - Reverted the sched_move_domain() check from v2, now unneeded.
>> ---
>>   xen/arch/arm/domain_build.c   | 15 +++++++--------
>>   xen/arch/x86/mm/mem_sharing.c | 11 ++---------
>>   xen/common/domain.c           | 24 ++++++++++++++++++++++++
>>   xen/common/domctl.c           | 19 ++++---------------
>>   xen/common/sched/core.c       |  7 +++----
>>   xen/include/xen/domain.h      |  1 +
>>   6 files changed, 41 insertions(+), 36 deletions(-)
>>
>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>> index 72d5316180..e08ee21ee5 100644
>> --- a/xen/arch/arm/domain_build.c
>> +++ b/xen/arch/arm/domain_build.c
>> @@ -1774,6 +1774,7 @@ static void __init find_gnttab_region(struct domain *d,
>>   int __init construct_domain(struct domain *d, struct kernel_info *kinfo)
>>   {
>>       unsigned int i;
>> +    int rc;
>>       struct vcpu *v = d->vcpu[0];
>>       struct cpu_user_regs *regs = &v->arch.cpu_info->guest_cpu_user_regs;
>>   @@ -1842,17 +1843,15 @@ int __init construct_domain(struct domain *d, 
>> struct kernel_info *kinfo)
>>       }
>>   #endif
>>   -    for ( i = 1; i < d->max_vcpus; i++ )
>> +    if ( (rc = vcpus_create(d)) )
>>       {
>> -        if ( vcpu_create(d, i) == NULL )
>> -        {
>> -            printk("Failed to allocate d%dv%d\n", d->domain_id, i);
>> -            return -ENOMEM;
>> -        }
>> +        printk("Failed to allocate d%dv%d\n", d->domain_id, d->max_vcpus);
>> +        return rc;
>> +    }
>>   -        if ( is_64bit_domain(d) )
>> +    if ( is_64bit_domain(d) )
>> +        for ( i = 1; i < d->max_vcpus; i++ )
>>               vcpu_switch_to_aarch64_mode(d->vcpu[i]);
>> -    }
>>         domain_update_node_affinity(d);
>>   diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
>> index 5c7a0ff30e..cd7f747c80 100644
>> --- a/xen/arch/x86/mm/mem_sharing.c
>> +++ b/xen/arch/x86/mm/mem_sharing.c
>> @@ -1612,21 +1612,14 @@ int mem_sharing_fork_page(struct domain *d, gfn_t 
>> gfn, bool unsharing)
>>     static int bring_up_vcpus(struct domain *cd, struct domain *d)
>>   {
>> -    unsigned int i;
>>       int ret = -EINVAL;
>>         if ( d->max_vcpus != cd->max_vcpus ||
>>           (ret = cpupool_move_domain(cd, d->cpupool)) )
>>           return ret;
>>   -    for ( i = 0; i < cd->max_vcpus; i++ )
>> -    {
>> -        if ( !d->vcpu[i] || cd->vcpu[i] )
>> -            continue;
>> -
>> -        if ( !vcpu_create(cd, i) )
>> -            return -EINVAL;
>> -    }
>> +    if ( (ret = vcpus_create(cd)) )
>> +        return ret;
>>         domain_update_node_affinity(cd);
>>       return 0;
>> diff --git a/xen/common/domain.c b/xen/common/domain.c
>> index e16f1ac383..a0a3e51b15 100644
>> --- a/xen/common/domain.c
>> +++ b/xen/common/domain.c
>> @@ -539,6 +539,30 @@ struct vcpu *vcpu_create(struct domain *d, unsigned int 
>> vcpu_id)
>>       return NULL;
>>   }
>>   +/*
>> + * Create every not yet existing vcpu of d, up to d->max_vcpus. On failure,
>> + * d->max_vcpus is rolled back to the id that failed, keeping d->vcpu[i]
>> + * non-NULL for all i < d->max_vcpus.
>> + */
>> +int vcpus_create(struct domain *d)
>> +{
>> +    unsigned int i;
>> +
>> +    for ( i = 0; i < d->max_vcpus; i++ )
>> +    {
>> +        if ( d->vcpu[i] )
>> +            continue;
>> +
>> +        if ( vcpu_create(d, i) == NULL )
>> +        {
>> +            d->max_vcpus = i;
>> +            return -EINVAL;
> 
> 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