Date:   Mon, 13 Nov 2023 19:27:56 +0530
From:   Harshit Mogalapalli <harshit.m.mogalapalli@...cle.com>
To:     Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc:     Jorge Lopez <jorge.lopez2@...com>,
        Hans de Goede <hdegoede@...hat.com>,
        Mark Gross <markgross@...nel.org>,
        Thomas Weißschuh <linux@...ssschuh.net>,
        platform-driver-x86@...r.kernel.org,
        LKML <linux-kernel@...r.kernel.org>, dan.carpenter@...aro.org,
        kernel-janitors@...r.kernel.org, error27@...il.com,
        vegard.nossum@...cle.com, darren.kenny@...cle.com,
        kernel test robot <lkp@...el.com>
Subject: Re: [PATCH v2 4/4] platform/x86: hp-bioscfg: Fix error handling in
 hp_add_other_attributes()

Hi Ilpo,

On 13/11/23 7:01 pm, Ilpo Järvinen wrote:
> On Sat, 11 Nov 2023, Harshit Mogalapalli wrote:
>> On 10/11/23 8:14 pm, Ilpo Järvinen wrote:
>>> On Fri, 10 Nov 2023, Harshit Mogalapalli wrote:
>>>
>>
>> Thanks for the review.
>>
>>> This changelog needs to be rewritten, it contains multiple errors. I
>>> suppose even this patch could be split into two but I'll not be too picky
>>> here if you insist on fixing them in the same patch.
>>>
>>
>> Any thoughts on how to split this into two patches ?
>>
>> I thought of fixing memory leak in separate patch, but that would add more
>> code which should be removed when we move kobject_put() to the end.
> 
Thanks for the suggestions.

> I meant that in the first patch you fix add the missing kfree(). Then in
> the second one, you correct kobject_put() + play with goto labels. There's
> no extra code that needs to be added and then removed AFAICT.
> 

This is the problem I am trying to explain:

Let us say we do memory leak fixing in first patch:

diff --git a/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c 
b/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
index 351d782f3e96..7f29a746210e 100644
--- a/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
+++ b/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
@@ -612,6 +612,7 @@ static int hp_add_other_attributes(int attr_type)
         default:
                 pr_err("Error: Unknown attr_type: %d\n", attr_type);
                 ret = -EINVAL;
+               kfree(attr_name_kobj);
                 goto err_other_attr_init;
         }

@@ -637,8 +638,10 @@ static int hp_add_other_attributes(int attr_type)
                 ret = -EINVAL;
         }

-       if (ret)
+       if (ret) {
+               kfree(attr_name_kobj); ///// [1]
                 goto err_other_attr_init;
+       }

         mutex_unlock(&bioscfg_drv.mutex);
         return 0;

Now when we want to move kobject_put() to goto label in next patch,
we should remove the kfree [1], as kobject_put() already does a kfree().

If that sounds okay, I will split this into two patches, (first one, 
only fixing memory leak; and second one fixing missing kobject_put() 
calls on error paths)

Thanks,
Harshit

> That way, you can make the commit messages more to the point too per
> patch.
> 
>>>> We have two issues:
>>>> 1. Memory leak of 'attr_name_kobj' in the error handling path.
>>>
>>> True, but not specific enough to be useful.
>>>
>>
>> Should I mention something like:
>>
>> 'attr_name_kobj' is allocated using kzalloc, but on all the error paths we
>> don't free it, hence we have a memory leak.
>>
>>>> 2. When kobject_init_and_add() fails on every subsequent error path call
>>>>      kobject_put() to cleanup.
>>>
>>> This makes no sense. The only case when there old code had no issue is
>>> "when kobject_init_and_add() fails" but now your wording claims it to be
>>> source of problem. Please rephrase this.
>>>
>>
>> Does this look better:
>>
>> kobject_put() must be called to cleanup memory associated with the object if
>> kobject_init_and_add() returns an error , before this patch only the error
>> path which is immediately next to kobject_init_and_add() has a kobject_put()
>> not any other error paths after it. Fix this by moving the kobject_put() into
>> a goto label "err_other_attr_init:" and use that for error paths after
>> kobject_init_and_add().
> 
> This is easier to understand I think:
> 
> kobject_put() must be always called after passing the object to
> kobject_init_and_add(). Only the error path which is immediately next
> to kobject_init_and_add() calls kobject_put() and not any other error
> path after it.
> 
> Fix the error handling by moving the kobject_put() into the goto label
> err_other_attr_init that is already used by all the error paths after
> kobject_init_and_add().
> 
>>>> Both of these issues will be fixed when we add kobject_put() in the goto
>>>> label, as kfree() is already part of kobject_put().
>>>
>>> No, you're fixing a problem in the patch which is not covered by moving
>>> kobject_put()!
>>
>> Sure, I will try to rephrase it to:
>>
>> 1. Add a new label "unlock_drv_mutex"
>> 2. Add a kfree() in the default statement of switch before going to
>> "unlock_drv_mutex" to free up the memory allocated with kzalloc.
>> 2. Move kobject_put() to goto "err_other_attr_init:" and use this goto label
>> in every error path after kobject_init_and_add().
>>
>> Please let me know if you have any comments.
> 
> I think none of this is needed for this patch after you move the other fix
> into a separate patch. Those two paragraphs I suggest above would explain
> the problem and solution (no need to have these numbered bullets or
> anything else besides those 2 paragraphs).
> 

Powered by blists - more mailing lists