Memory leak in spapr_machine_init()?

Memory leak in spapr_machine_init()?

[Markus Armbruster]

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

Re: Memory leak in spapr_machine_init()?

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2141 bytes --]

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.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: Memory leak in spapr_machine_init()?

[Markus Armbruster]

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!