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

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

On 01.09.26 09:22, Jan Beulich wrote:
> On 31.08.2026 14:59, Jürgen Groß wrote:
> > 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 was discussed before, and however nice it would be for the issue at hand,
> it would get in the way of us wanting to have CPU policy for domains put in
> place before vCPU-s are created, such that on x86 the XSAVE area can be sized
> once and for all.
This could be done when unpausing the domain initially.

OTOH I don't see xstate_alloc_save_area() looking at the domain's cpu policy
at all. Is this a plan for the future?

And additionally there is no guard for avoiding the vcpus being created before
the policy is being set.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature