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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 18/08/2026 8:53 am, Furkan Çalışkan wrote:
> On 8/18/26 10:11, Jürgen Groß wrote:
>> On 18.08.26 08:32, 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 that all of
>>> its vcpu slots are populated, and skip it if any are missing. The
>>> rest of the function walks the vcpus that actually exist, via
>>> for_each_vcpu() rather than n_units, so skipping a unit here
>>> does not leave anything else out of sync.
>>>
>>> Signed-off-by: Furkan Caliskan <frn1furkan10@xxxxxxxxx>
>>> ---
>>>   xen/common/sched/core.c | 19 +++++++++++++++++++
>>>   1 file changed, 19 insertions(+)
>>>
>>> diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c
>>> index d3a0a97e1d..d542c76543 100644
>>> --- a/xen/common/sched/core.c
>>> +++ b/xen/common/sched/core.c
>>> @@ -745,6 +745,25 @@ int sched_move_domain(struct domain *d, struct cpupool 
>>> *c)
>>>         for ( unit_idx = 0; unit_idx < n_units; unit_idx++ )
>>>       {
>>> +        /*
>>> +         * Skip this unit if any of its vcpus is missing. Bounded by
>>> +         * max_vcpus.
>>> +         */
>>> +        bool vcpu_failed = false;
>>> +
>>> +        for ( unsigned int i = 0;
>>> +              i < gran && unit_idx * gran + i < d->max_vcpus; i++ )
>>> +        {
>>> +            if ( !d->vcpu[unit_idx * gran + i] )
>>> +            {
>>> +                vcpu_failed = true;
>>> +                break;
>>> +            }
>>> +        }
>>> +
>>> +        if ( vcpu_failed )
>>> +            continue;
>> I don't think this is correct.
>>
>> If there are some vcpus in the unit you will loose them (i.e. make them no
>> longer be able to be scheduled), right?
>>
>> For a dying domain this might be okay, but not for one still active. So I 
>> think
>> you should at least verify the domain is dying, otherwise sched_move_domain()
>> should just fail.
>>
>> An alternative might be to fix the NULL dereferencing where needed, but this
>> could become tedious.
>>
>>
>> Juergen
> Right. My initial attempt only checked 'd->vcpu[unit_idx*gran]'
> for the head vCPU. The crash happens when unit->vcpu_list is set
> to d->vcpu[unit_idx*gran] (which is NULL) and passed to 'alloc_udata()',
> causing a NULL dereference. 
>
> I expanded the loop over 'gran' to handle core-scheduling cases where a 
> subsequent vCPU fails mid-unit, but as you pointed out, that drops the 
> whole unit for active domain.
>
> I'll update the patch to check d->is_dying to skip incomplete units 
> only for dying domains, and have sched_move_domain() fail if an active 
> domain has missing vCPUs

I'm afraid that wont fix everything.

Your scenario is rare, but sadly we have no interlock to kill the domain
if a setmaxvcpus hypercall finishes mid-way through.  Furthermore, for
dom0 at least, we actively do want to run in this configuration if we
end up in it, because that at least helps recovery of the system.

At all points in a domain's lifecycle, d->vcpu[0..max_vcpus] may be
NULL, and we may even want to schedule in this scenario.

~Andrew