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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 01/04/2021 14:45, Jan Beulich wrote:
> On 01.04.2021 15:20, Andrew Cooper wrote:
>> 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.
> Hmm, in which case you mean to shift the responsibility not to "the
> caller" (many instances) but "the top level caller" (a single
> instance)?

Yes, but the route to managing that needs to move the responsibility up
one level at a time for us not to break things.

i.e. we'd next want to make {pv,hvm}_*_create/initialise() call the
appropriate {pv,hvm}_*_destroy() covering the entire sub-call-tree.

Moving the responsibility across the arch_{d,v}_*() boundaries in
particular needs careful coordination.

~Andrew