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

[Jürgen Groß <jgross@xxxxxxxx>]

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?

> Your scenario is rare, but sadly we have no interlock to kill the domain
> if a setmaxvcpus hypercall finishes mid-way through.  Furthermore, for
> dom0 at least, we actively do want to run in this configuration if we
> end up in it, because that at least helps recovery of the system.
It is only about moving a domain to another cpupool. Dom0 can't be moved
anyway, and not being able to move a crippled domain is better than a crash.

So what is the problem then?


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature