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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

Jan