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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 01.09.2026 10:24, Jürgen Groß wrote:
> On 01.09.26 10:15, Jan Beulich wrote:
>> On 01.09.2026 10:07, Jürgen Groß wrote:
>>> 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.
>>
>> Imo unpausing shouldn't fail because of memory shortage.
> 
> As long as the domain hasn't started running I don't see why this would be
> different to the case where not all vcpus could be created.

I do. One could create a domain ready to be unpaused, but being kept paused
until whatever event triggers its launching. That better wouldn't fail, except
in extraordinary situations.

Jan