Re: [PATCH v2 1/2] xen/sched: core: skip missing vcpu slots in sched_move_domain()

On 8/28/26 15:45, Juergen Gross wrote:
> On 19.08.26 07:15, Furkan Caliskan wrote:
>> sched_move_domain() derives the number of units to rebuild from
>> d->max_vcpus, which is fixed at domain creation and never rolled
>> back if vcpu_create() fails partway through building a domain. So
>> d->vcpu[i] can be NULL for some i even though max_vcpus still
>> counts it - this happens if sched_alloc_udata() returns NULL.
>>
>> The per-unit loop doesn't check for this: it sets
>> unit->vcpu_list = d->vcpu[unit_id] (NULL) and hands that broken
>> unit straight to the destination scheduler's alloc_udata(),
>> which assumes vcpu_list is always valid and crashes Xen when
>> it is not.
>>
>> Reproduced by building a domain in a non-default cpupool where
>> vcpu creation fails partway through, then destroying it.
>> domain_kill() moves the domain back to the default cpupool via
>> sched_move_domain() before actually destroying it, crashing
>> inside the destination scheduler's alloc_udata() (seen in
>> Credit2's csched2_alloc_udata() -> is_idle_unit() -> NULL deref).
>>
>> Before building a unit in sched_move_domain(), check whether all
>> vpcu slots belonging to that unit are populated. If any of its
>> vpcus is missing:
>>   - For a dying domain, skip the unit allocation.
>>   - For an active domain, abort the move and return -EINVAL to
>>     prevent running with dropped vCPUs.
>>
>> Fixes: 70fadc41635b ("xen/cpupool: support moving domain between cpupools with different granularity")
>> Signed-off-by: Furkan Caliskan <[email protected]>
> 
> Sorry for realizing this only now, but I think this problem should be
> solved completely differently.
> 
> Today there are multiple places in the hypervisor where vcpus are being
> setup via vcpu_create(). For vcpu-ids other than 0 this is always done
> in a loop with ascending ids, up to d->max_vcpus. Whenever vcpu_create()
> is failing, the loop is terminated. The only special case is the idle-domain,
> which will never have your problem.
> 
> So the easy fix would be to:
> 
> - have only one function vcpus_create() creating all the vcpus for a domain
>   and use this function everywhere instead of said loop
> 
> - in case a single vcpu can't be created by vcpus_create(), d->max_vcpus
>   should be reset to the id of the vcpu which couldn't be created
> 
> This will avoid any potential NULL derefs elsewhere, as vcpu loops will
> just be ending before hitting a NULL pointer.
> 
> In case you are not feeling comfortable writing this patch, just speak up
> and I will do it.
> 
> 
> Juergen

Agreed, that's the better approach. I'll take it on and send a new version.

Furkan