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

[Juergen Gross <jgross@xxxxxxxx>]

On 19.08.26 07:15, 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 whether all
> vpcu slots belonging to that unit are populated. If any of its
> vpcus is missing:
>   - For a dying domain, skip the unit allocation.
>   - For an active domain, abort the move and return -EINVAL to
>     prevent running with dropped vCPUs.
>
> Fixes: 70fadc41635b ("xen/cpupool: support moving domain between cpupools with 
> different granularity")
> Signed-off-by: Furkan Caliskan <frn1furkan10@xxxxxxxxx>
Sorry for realizing this only now, but I think this problem should be
solved completely differently.

Today there are multiple places in the hypervisor where vcpus are being
setup via vcpu_create(). For vcpu-ids other than 0 this is always done
in a loop with ascending ids, up to d->max_vcpus. Whenever vcpu_create()
is failing, the loop is terminated. The only special case is the idle-domain,
which will never have your problem.

So the easy fix would be to:

- have only one function vcpus_create() creating all the vcpus for a domain
  and use this function everywhere instead of said loop

- in case a single vcpu can't be created by vcpus_create(), d->max_vcpus
  should be reset to the id of the vcpu which couldn't be created

This will avoid any potential NULL derefs elsewhere, as vcpu loops will
just be ending before hitting a NULL pointer.

In case you are not feeling comfortable writing this patch, just speak up
and I will do it.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature