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

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

Furkan