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

On 8/18/26 13:53, Jan Beulich wrote:
> On 18.08.2026 12:35, Andrew Cooper wrote:
>> 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.
> 
> Yet isn't the (separate) bug then that we allow a DomU to be launched when
> XEN_DOMCTL_max_vcpus didn't finish setting up all vCPU-s? Or is that what
> you were alluding to? Since you did say "..., and we may even want to
> schedule in this scenario" - perhaps not.
> 
> Jan

When vcpu_create() returns NULL, XEN_DOMCTL_max_vcpus returns an
error. Once the toolstack sees that, it immediately issues the 
kill hypercall. AFAIK, a domain in that state will simply be killed
and never actually launched.

Furkan