[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <c3b821fb-5df1-4c58-99bc-f3e99a6d1d94@oracle.com>
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