Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH] soc: fsl: guts: Fix a resource leak in the error handling
 path of 'fsl_guts_probe()'
To:     Li Yang <leoyang.li@nxp.com>,
        Christophe JAILLET <christophe.jaillet@wanadoo.fr>
Cc:     kernel-janitors@vger.kernel.org,
        linuxppc-dev <linuxppc-dev@lists.ozlabs.org>,
        lkml <linux-kernel@vger.kernel.org>,
        "moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE" 
        <linux-arm-kernel@lists.infradead.org>
References: <b12e8c5c5d6ab3061d9504de8fbaefcad6bbc385.1629321668.git.christophe.jaillet@wanadoo.fr>
 <CADRPPNTHKuV9eernJS6ZV_+i-xtPXHQnS64GSx=ubwWE+nbLYw@mail.gmail.com>
From:   Tyrel Datwyler <tyreld@linux.ibm.com>
Message-ID: <20ecf373-75a4-97e5-baa4-24b8871da546@linux.ibm.com>
Date:   Fri, 22 Oct 2021 13:16:30 -0700
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.13.0
MIME-Version: 1.0
In-Reply-To: <CADRPPNTHKuV9eernJS6ZV_+i-xtPXHQnS64GSx=ubwWE+nbLYw@mail.gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 10/21/21 5:26 PM, Li Yang wrote:
> On Wed, Aug 18, 2021 at 4:23 PM Christophe JAILLET
> <christophe.jaillet@wanadoo.fr> wrote:
>>
>> If an error occurs after 'of_find_node_by_path()', the reference taken for
>> 'root' will never be released and some memory will leak.
> 
> Thanks for finding this.  This truly is a problem.
> 
>>
>> Instead of adding an error handling path and modifying all the
>> 'return -SOMETHING' into 'goto errorpath', use 'devm_add_action_or_reset()'
>> to release the reference when needed.
>>
>> Simplify the remove function accordingly.
>>
>> As an extra benefit, the 'root' global variable can now be removed as well.
>>
>> Fixes: 3c0d64e867ed ("soc: fsl: guts: reuse machine name from device tree")
>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
>> ---
>> Compile tested only
>> ---
>>  drivers/soc/fsl/guts.c | 16 ++++++++++++++--
>>  1 file changed, 14 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/soc/fsl/guts.c b/drivers/soc/fsl/guts.c
>> index d5e9a5f2c087..4d9476c7b87c 100644
>> --- a/drivers/soc/fsl/guts.c
>> +++ b/drivers/soc/fsl/guts.c
>> @@ -28,7 +28,6 @@ struct fsl_soc_die_attr {
>>  static struct guts *guts;
>>  static struct soc_device_attribute soc_dev_attr;
>>  static struct soc_device *soc_dev;
>> -static struct device_node *root;
>>
>>
>>  /* SoC die attribute definition for QorIQ platform */
>> @@ -136,14 +135,23 @@ static u32 fsl_guts_get_svr(void)
>>         return svr;
>>  }
>>
>> +static void fsl_guts_put_root(void *data)
>> +{
>> +       struct device_node *root = data;
>> +
>> +       of_node_put(root);
>> +}
>> +
>>  static int fsl_guts_probe(struct platform_device *pdev)
>>  {
>>         struct device_node *np = pdev->dev.of_node;
>>         struct device *dev = &pdev->dev;
>> +       struct device_node *root;
>>         struct resource *res;
>>         const struct fsl_soc_die_attr *soc_die;
>>         const char *machine;
>>         u32 svr;
>> +       int ret;
>>
>>         /* Initialize guts */
>>         guts = devm_kzalloc(dev, sizeof(*guts), GFP_KERNEL);
>> @@ -159,6 +167,10 @@ static int fsl_guts_probe(struct platform_device *pdev)
>>
>>         /* Register soc device */
>>         root = of_find_node_by_path("/");
>> +       ret = devm_add_action_or_reset(dev, fsl_guts_put_root, root);
>> +       if (ret)
>> +               return ret;
> 
> We probably only need to hold the reference when we do get "machine"
> from the device tree, otherwise we can put it directly.

To be pedantic since you are using the a properties string value of the root
node directly you need to hold the reference of that node for the lifetime of
the device. Realistically, its not like the root node and its model/compatible
properties are going to need to get modified at runtime so you could also argue
that holding the reference is unnecessary.

> 
> Or maybe we just maintain a local copy of string machine which means
> we can release the reference right away?

Looks like that is the original behavior that commit 3c0d64e867ed changed.
Although it looks like that behavior neglected to handle a memory allocation
failure during the string copy. How much memory is really being saved by not
keeping a local copy? Maybe revert 3c0d64e867ed and add the memory allocation check?

-Tyrel

> 
>> +
>>         if (of_property_read_string(root, "model", &machine))
>>                 of_property_read_string_index(root, "compatible", 0, &machine);
>>         if (machine)
>> @@ -197,7 +209,7 @@ static int fsl_guts_probe(struct platform_device *pdev)
>>  static int fsl_guts_remove(struct platform_device *dev)
>>  {
>>         soc_device_unregister(soc_dev);
>> -       of_node_put(root);
>> +
>>         return 0;
>>  }
>>
>> --
>> 2.30.2
>>