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

[Jan Beulich <jbeulich@xxxxxxxx>]

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)?

Jan