Re: [PATCH v3 1/2] xen/common: add vcpus_create() and keep max_vcpus in sync

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

On 31.08.26 11:45, Andrew Cooper wrote:
> On 31/08/2026 6:16 am, Furkan Caliskan wrote:
> > Every vcpu_create() call site that builds more than one vcpu loops
> > over ids up to d->max_vcpus and stops on the first failure, but none
> > of them roll max_vcpus back to match. This leaves d->vcpu[i] == NULL
> > for ids below max_vcpus
> As I told you before, you must cope with this property in non-error
> scenarios.
>
> > , which anything walking d->vcpu[] can then
> > dereference. This is what caused the crash: sched_move_domain()
> > walks every vcpu slot up to max_vcpus without checking for empty
> > ones, so when a domain built in a non-default cpupool had vcpu
> > creation fail partway through, domain_kill() later moving it back
> > to the default cpupool handed one of its empty slots straight to
> > the new cpupool's scheduler, causing a NULL-pointer dereference
> > inside sched_alloc_udata().
> >
> > Add vcpus_create(d): creates every vcpu of d up to max_vcpus and
> > rolls max_vcpus back to the failed id on error. This keeps
> > d->vcpu[i] is non-NULL for all i < d->max_vcpus
> No, it really doesn't.
>
> > , instead of guarding
> > every reader of d->vcpu[] agains holes individually.
> >
> > Convert every site that builds vcpus in a loop to call this function
> > instead.
> >
> > Fixes: 61649709421a ("xen/domain: Allocate d->vcpu[] in domain_create()")
> > Suggested-by: Juergen Gross <jgross@xxxxxxxx>
> > Signed-off-by: Furkan Caliskan <frn1furkan10@xxxxxxxxx>
> For the avoidance of a long drawn-out argument, nack.  Under no
> circumstances are you editing d->max_cpus after it's put into the domain
> list.
>
> You've chosen to do so at a point where the domain object is live,
> visible in the system and able to be the target of other hypercalls.
>
> Furthermore you have not fixed what your commit message claims.
> d->vcpu[...] is still NULL for an arbitrary period of time, including
> being able to be the target of hypercalls, before vCPUs are created.
>
> All code MUST be able to cope with d->vcpu[...] being NULL.  It's how
> the object lifecycles must work, because creating vCPUs is not atomic
> with respect to creating domains.
Would you be fine with me creating a patch series moving vcpu creation into
domain_create()?

This would at once solve all those problems, while even removing the need for
having the XEN_DOMCTL_max_vcpus domctl.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature