Re: [Xen-devel] [PATCH for-next 5/8] x86/domain: factor out pv_domain_destroy

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 10/04/17 16:22, Wei Liu wrote:
> On Mon, Apr 10, 2017 at 04:16:12PM +0100, Andrew Cooper wrote:
>> On 10/04/17 16:12, Wei Liu wrote:
>>> On Mon, Apr 10, 2017 at 04:04:22PM +0100, Andrew Cooper wrote:
>>>> On 10/04/17 14:27, Wei Liu wrote:
>>>>> No functional change.
>>>>>
>>>>> Signed-off-by: Wei Liu <wei.liu2@xxxxxxxxxx>
>>>> Throughout this series, please make sure you add in proper NULL'ing of
>>>> freed data.
>>>>
>>>> While this patch is no functional change at the moment, you have
>>>> introduced a latent double-free bug for if (/when) pv_domain_destroy()
>>>> gets used on a failed create path.
>>>>
>>> No it won't. I made pv_domain_initialise idempotent.
>> Ah - I hadn't spotted that.
>>
>> My long-term goal is to reorder the init/destroy logic such that init
>> has no cleanup in it, and any failure comes out and calls destroy at the
>> top level.
> Just like what we do in libxl? :-)
>
>> This will avoid us opencoding the teardown logic twice;  we have had
>> bugs in the past where the two paths are different (the most common of
>> which is missing cleanup in the destroy path, leading to a memory leak).
>>
> Given there is going to be a lot of code churn, I think the best bet at
> the moment is to have the teardown logic twice, with each setting
> respective fields to their "NULL" state. Although that's a bit
> excessive, but it would prevent accidental memory leak or double free
> errors. And then after we are happy with the overall structures of
> hypervisor code, we can then audit the code to remove the clean up code
> in _init.
>
> What do you think?

For now, keep the patches along their current line, but making the
teardown idempotent.  I wasn't trying to lumber you with all the work. ;)

I have a series starting from the common code, but it will require
coordinated changes between all architectures.  This will be far easier
to reason about when all the underlying functions are idempotent.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel