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

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

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

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature