Re: [for-4.22][PATCH] xen/arm: Fail domain construction if a secondary vCPU cannot be created

["Orzel, Michal" <michal.orzel@xxxxxxx>]

On 08-Jul-26 13:30, Andrew Cooper wrote:
> On 08/07/2026 8:49 am, Michal Orzel wrote:
>> construct_domain() creates the secondary vCPUs in a loop, but on a
>> vcpu_create() failure it only prints a message and breaks out of the
>> loop returning success. As a result the domain can be constructed
>> with fewer vCPUs than d->max_vcpus, leaving NULL holes in d->vcpu[]
>> below max_vcpus.
> 
> I'd suggest phrasing this as "partially constructed".  "holes" isn't
> really what's going on, given the linear nature of allocation.
> 
> Because of the object visibility, and because constructing vCPUs isn't
> atomic, all code needs to cope with d->vcpu[] having no, some or all of
> d->max_vcpus constructed.
> 
>>
>> When the guest probes the redistributor of a vCPU that was never created,
>> get_vcpu_from_rdist() only checks vcpu_id against d->max_vcpus and then
>> dereferences the NULL d->vcpu[vcpu_id], resulting in a data abort.
> 
> That's unsafe, especially as vcpu_id is calculated from an MMIO access.
> 
> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
> index c1c4d6f71ea8..c01cc596d593 100644
> --- a/xen/arch/arm/vgic-v3.c
> +++ b/xen/arch/arm/vgic-v3.c
> @@ -1111,10 +1111,10 @@ static struct vcpu *get_vcpu_from_rdist(struct
> domain *d,
>      unsigned int vcpu_id;
>  
>      vcpu_id = region->first_cpu + ((gpa - region->base) / GICV3_GICR_SIZE);
> -    if ( unlikely(vcpu_id >= d->max_vcpus) )
> -        return NULL;
>  
> -    v = d->vcpu[vcpu_id];
> +    v = domain_vcpu(d, vcpu_id);
> +    if ( !v )
> +        return NULL;
>  
>      *offset = gpa - v->arch.vgic.rdist_base;
>  
> 
> Do you want me to submit this separately?
If you have time, yes. Otherwise I can do that too tomorrow.

> 
> 
>>
>> Return an error instead of breaking out of the loop. Both callers
>> (construct_domU() and construct_hwdom()) already propagate a negative
>> return value and fail domain construction, which is the correct
>> behaviour: a domain that cannot provide the requested number of vCPUs
>> should not be brought up.
>>
>> Fixes: 6b0e8e43348a ("xen/arm: allocate secondaries dom0 vcpus")
>> Signed-off-by: Michal Orzel <michal.orzel@xxxxxxx>
>> ---
>>  xen/arch/arm/domain_build.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>> index 550617f152bb..b46574fd32aa 100644
>> --- a/xen/arch/arm/domain_build.c
>> +++ b/xen/arch/arm/domain_build.c
>> @@ -1847,7 +1847,7 @@ int __init construct_domain(struct domain *d, struct 
>> kernel_info *kinfo)
>>          if ( vcpu_create(d, i) == NULL )
>>          {
>>              printk("Failed to allocate d%dv%d\n", d->domain_id, i);
>> -            break;
>> +            return -EINVAL;
>>          }
>>  
>>          if ( is_64bit_domain(d) )
> 
> On x86, we explicitly tolerate a failure to build all of dom0's CPUs, if
> at least one did get constructed.  This is to increase the chances that
> the server can boot and at least let an admin in to look at things.
> 
> However, I can see why such a behaviour is not wanted in a "single
> pre-packaged system" as used by automotive.
Yes, that is a known difference in behavior between x86 and Arm. We decided to
bail out asap.

~Michal