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

On 18/08/2026 11:13 am, Jürgen Groß wrote:
> On 18.08.26 12:04, Andrew Cooper wrote:
>> 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 <[email protected]>
>>>>> ---
>>>>>    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.
>
> Why not?

domU's in this situation do not have is_dying set.

~Andrew