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

["Jürgen Groß" <jgross@suse.com>]

[-- 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 --]