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

[Juergen Gross <jgross@xxxxxxxx>]

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 <frn1furkan10@xxxxxxxxx>
> > > > > > ---
> > > > > >     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

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature