Re: [PATCH] xen/domctl: Fix double domid_free in XEN_DOMCTL_createdomain error path

[Alejandro Vallejo <alejandro.garciavallejo@amd.com>]

On Sat Sep 13, 2025 at 1:56 PM CEST, Andrew Cooper wrote:
> On 13/09/2025 11:44 am, Oleksii Moisieiev wrote:
>> Remove redundant domid_free() call in the XEN_DOMCTL_createdomain error
>> handling path to prevent a double-free condition.
>>
>> When domain_create() fails, it internally calls _domain_destroy() during
>> its cleanup routine, which already invokes domid_free() to release the
>> allocated domain ID. The additional domid_free() call in the domctl error
>> path creates a double-free scenario, triggering an assertion failure in
>> domid.c:
>>
>>     Assertion 'rc' failed at common/domid.c:84
>>
>> The domain creation flow is:
>> 1. domid_alloc() allocates a domain ID
>> 2. domain_create() is called with the allocated ID
>> 3. If domain_create() fails:
>>    a) domain_create() calls _domain_destroy() internally
>>    b) _domain_destroy() calls domid_free() to release the ID
>>    c) domctl incorrectly calls domid_free() again
>>
>> This double-free violates the domain ID management invariants and causes
>> system instability. The fix ensures domid_free() is called exactly once
>> per allocated domain ID, maintaining proper resource cleanup
>> semantics.
>
> Fixes: 2d5065060710 ("xen/domain: unify domain ID allocation")
>
>> Signed-off-by: Oleksii Moisieiev <oleksii_moisieiev@epam.com>
>
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
>
> the tl;dr is that domain_create() either inserts the domain into the
> domlist, or cleans up after itself.
>
> The domid alloc infrastructure is problematic in multiple ways, not
> least because it now means there are two sources of truth for which
> domain's exist, and they are not interlocked.

The source of truth of existing domains is the domlist. The source of truth
of domids is the domid bitmap; and they need not match.

A domid being allocated doesn't mean the domain exists. Merely that the domid
is unavailable for allocation.

This is fairly important to allow fallible boots, where some boot-domains are
allowed to fail construction for one reason or another while preventing their
domids from being hijacked by a later domain, and thus prevent some resources
granted to those domains that failed construction from being usable by others.

The actual problem is that the lifetime of certain resources assigned to domains
extends past the lifetime of the domain, and the mere possesion of a domid
grants authority over resources assigned to the domid, even after its associated
domain died. This is wrong and bad, and has been wrong and bad since before the
time_t was 0.

If it wasn't for this messed up means of resource management we wouldn't have a
need for preventing certain domids from being used. We need saner semantics with
the lifetimes of event channels and grants. And possibly more resources.

Cheers,
Alejandro