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

["Jürgen Groß" <jgross@suse.com>]

[-- Attachment #1.1.1: Type: text/plain, Size: 3987 bytes --]

On 01.09.26 10:36, Jan Beulich wrote:
> 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@suse.com>
>>>>>>>> Signed-off-by: Furkan Caliskan <frn1furkan10@gmail.com>
>>>>>>>
>>>>>>> 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.

Okay, then we could add XEN_DOMCTL_finalize doing the final allocations AND
doing the creation_finished handling. This would even catch today's domain
crashing in the VMX specific domain_creation_finished() callback.

At the same time XEN_DOMCTL_max_vcpus would be dropped, so there isn't more
work for the toolstack.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]