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

Juergen Gross <[email protected]> Tue, 18 Aug 2026 12:47:33 +0200
Newsgroups	gmane.comp.emulators.xen.devel
Message-ID	<[email protected]>
On 18.08.26 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.

Please clarify what you mean with "this situation".

sched_move_domain() is being called either due to an admin action
("xl cpupool-migrate"), or during domain_kill() in order to move the
domain to cpupool0 for avoiding a zombie domain blocking cpupool
removal.

The first case is allowed to fail, which the suggested fix would do,
while the second case is happening only after DOMDYING_dying has been
set for the domain.


Juergen
OpenPGP_0xB0DE9DD628BF132F.asc (application/pgp-keys, 3.6 KB)
OpenPGP_signature.asc (application/pgp-signature, 495 B)