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

On 18.08.2026 14:12, Furkan Çalışkan wrote:
> 
> 
> 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.
> 
> When vcpu_create() returns NULL, XEN_DOMCTL_max_vcpus returns an
> error. Once the toolstack sees that, it immediately issues the 
> kill hypercall.

That's what the one toolstack you look at does. At the hypervisor level, what
a toolstack may do after a failure is entirely unknown.

Jan