Re: [PATCH] x86/hvm: Fix double free from vlapic_init() early error path

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 31/03/2021 15:03, Jan Beulich wrote:
> On 31.03.2021 15:31, Andrew Cooper wrote:
>> vlapic_init()'s caller calls vlapic_destroy() on error.  Therefore, the error
>> path from __map_domain_page_global() failing would doubly free
>> vlapic->regs_page.
> I'm having difficulty seeing this. What I find at present is
>
>     rc = vlapic_init(v);
>     if ( rc != 0 ) /* teardown: vlapic_destroy */
>         goto fail2;
>
> and then
>
>  fail3:
>     vlapic_destroy(v);
>  fail2:
>
> Am I missing some important aspect?

No - I'm blind.  (although I do blame the code comment for being
actively misleading.)

I retract the patch at this point.  It needs to be part of a bigger
series making changes like this consistently across the callers.

>> Rework vlapic_destroy() to be properly idempotent, introducing the necessary
>> UNMAP_DOMAIN_PAGE_GLOBAL() and FREE_DOMHEAP_PAGE() wrappers.
>>
>> Rearrange vlapic_init() to group all trivial initialisation, and leave all
>> cleanup to the caller, in line with our longer term plans.
> Cleanup functions becoming idempotent is what I understand is the
> longer term plan. I didn't think this necessarily included leaving
> cleanup after failure in a function to it caller(s).

That's literally the point of the exercise.

>  At least in the
> general case I think it would be quite a bit better if functions
> cleaned up after themselves - perhaps (using the example here) by
> vlapic_init() calling vlapic_destroy() in such a case.

That pattern is the cause of code duplication (not a problem per say),
and many bugs (the problem needing solving) caused by the duplicated
logic not being equivalent.

We've got the start of the top-level pattern in progress, with
{domain,vcpu}_create() calling {d,v}_teardown() then {d,v}_destroy() for
errors.

This top-level pattern is also necessary so we can remove the need to
put partially constructed objects into full view of the system, and wait
for a subsequent hypercall to clean them up.  This series of bugs is
still active, and there are still a load of NULL pointer deferences
reachable from out-of-order toolstack hypercalls in failure cases.

We've also got some subsystems (vmtrace) moved into the new pattern, and
some minor parts of existing mess moved over too, but the end goal is to
have exactly one create() path and exactly one cleanup path, so its
impossible to introduce mismatched duplicate cleanup logic.

~Andrew