[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: Memory leak in spapr_machine_init()?
From: |
Markus Armbruster |
Subject: |
Re: Memory leak in spapr_machine_init()? |
Date: |
Mon, 22 Jun 2020 10:19:31 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) |
David Gibson <david@gibson.dropbear.id.au> writes:
> On Thu, Jun 18, 2020 at 08:55:53AM +0200, Markus Armbruster wrote:
>> Either I'm confused (quite possible), or kvmppc_check_papr_resize_hpt()
>> can leak an Error object on failure. Please walk through the code with
>> me:
>>
>> kvmppc_check_papr_resize_hpt(&resize_hpt_err);
>>
>> This sets @resize_hpt_err on failure.
>>
>> if (spapr->resize_hpt == SPAPR_RESIZE_HPT_DEFAULT) {
>> /*
>> * If the user explicitly requested a mode we should either
>> * supply it, or fail completely (which we do below). But if
>> * it's not set explicitly, we reset our mode to something
>> * that works
>> */
>> if (resize_hpt_err) {
>> spapr->resize_hpt = SPAPR_RESIZE_HPT_DISABLED;
>> error_free(resize_hpt_err);
>> resize_hpt_err = NULL;
>>
>> Case 1: failure and SPAPR_RESIZE_HPT_DEFAULT; we free @resize_hpt_err.
>> Good.
>>
>> } else {
>> spapr->resize_hpt = smc->resize_hpt_default;
>> }
>> }
>>
>> assert(spapr->resize_hpt != SPAPR_RESIZE_HPT_DEFAULT);
>>
>> if ((spapr->resize_hpt != SPAPR_RESIZE_HPT_DISABLED) &&
>> resize_hpt_err) {
>> /*
>> * User requested HPT resize, but this host can't supply it.
>> Bail out
>> */
>> error_report_err(resize_hpt_err);
>> exit(1);
>>
>> Case 2: failure and not SPAPR_RESIZE_HPT_DISABLED; fatal. Good.
>>
>> }
>>
>> What about case 3: failure and SPAPR_RESIZE_HPT_DISABLED?
>>
>> Good if we get here via case 1 (we freed @resize_hpt_err).
>>
>> Else, ???
>
> I think you're right, and we leak it in this case - I think I forgot
> that in the DISABLED case we still (unnecessarily) ask the kernel if
> it can do it.
>
> Of course, it will only happen once per run, so it's not like it's a
> particularly noticeable leak.
Understood. I'll post a patch. Thanks!