| Message ID | 20231110142921.3398072-4-harshit.m.mogalapalli@oracle.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [v2,1/4] platform/x86: hp-bioscfg: Remove unused obj in hp_add_other_attributes() | expand |
Hi Ilpo, 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. >> 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(). >> 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. Thank you very much. Regards, Harshit >
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. 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. 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).
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). >
On Mon, 13 Nov 2023, Harshit Mogalapalli wrote: > 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] This relates to the 2nd problem (missing kobject_put()) and will be covered by the other patch. Don't try to solve this in the first patch at all! There are two indepedent problems: - Before kobject_init_and_add(), kfree() is missing - After kobject_init_and_add(), kobject_put() is missing Solve each in own patch and don't mix the solutions. I know both patches are needed to solve _both_ problems so it's fine to have "still broken" intermediate state as long as you didn't make things worse in the first patch which you didn't. > 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)
On Mon, Nov 13, 2023 at 04:15:50PM +0200, Ilpo Järvinen wrote: > This relates to the 2nd problem (missing kobject_put()) and will be > covered by the other patch. Don't try to solve this in the first patch > at all! > > There are two indepedent problems: > - Before kobject_init_and_add(), kfree() is missing > - After kobject_init_and_add(), kobject_put() is missing It's the same problem, though. The attr_name_kobj is leaked on all the error paths. It's just that it needs to be freed different ways depending on where you are. To me splitting it up makes it harder to review and I would not allow it in Staging. You can't fix half the problem. regards, dan carpenter
On Mon, 13 Nov 2023, Dan Carpenter wrote: > On Mon, Nov 13, 2023 at 04:15:50PM +0200, Ilpo Järvinen wrote: > > This relates to the 2nd problem (missing kobject_put()) and will be > > covered by the other patch. Don't try to solve this in the first patch > > at all! > > > > There are two indepedent problems: > > - Before kobject_init_and_add(), kfree() is missing > > - After kobject_init_and_add(), kobject_put() is missing > > It's the same problem, though. The attr_name_kobj is leaked on all the > error paths. I'll have politely disagree beyond that the symptoms are indeed about the same, the problem is clearly different like you immediately admit even yourself by stating this: ;-) > It's just that it needs to be freed different ways depending on where > you are. ...And that's because "it" actually changed in between so the problem became a different one. > To me splitting it up makes it harder to review This has already been proven incorrect in the context of this patch so your argument is rather weak... While reviewing it I clearly noted that the different way of handling things was not properly covered, and that was because what needs to be "freed" was changed by kobject_init_and_add(). If one would have done them separately, each commit message would have been more to the point and it would have been simpler to review which is exactly the opposite to your claim. But I guess we'll end up disagreing on this too :-). > and I would not allow it in Staging. You can't fix half the problem. I don't have that strong opinion on this so Harshit please follow what Dan is suggesting, just fix the changelog to clearly cover both cases.
diff --git a/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c b/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c index 351d782f3e96..8c9f4f3227fc 100644 --- a/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c +++ b/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c @@ -575,75 +575,77 @@ static void release_attributes_data(void) /** * hp_add_other_attributes() - Initialize HP custom attributes not * reported by BIOS and required to support Secure Platform and Sure * Start. * * @attr_type: Custom HP attribute not reported by BIOS * * Initialize all 2 types of attributes: Platform and Sure Start * object. Populates each attribute types respective properties * under sysfs files. * * Returns zero(0) if successful. Otherwise, a negative value. */ static int hp_add_other_attributes(int attr_type) { struct kobject *attr_name_kobj; int ret; char *attr_name; attr_name_kobj = kzalloc(sizeof(*attr_name_kobj), GFP_KERNEL); if (!attr_name_kobj) return -ENOMEM; mutex_lock(&bioscfg_drv.mutex); /* Check if attribute type is supported */ switch (attr_type) { case HPWMI_SECURE_PLATFORM_TYPE: attr_name_kobj->kset = bioscfg_drv.authentication_dir_kset; attr_name = SPM_STR; break; case HPWMI_SURE_START_TYPE: attr_name_kobj->kset = bioscfg_drv.main_dir_kset; attr_name = SURE_START_STR; break; default: pr_err("Error: Unknown attr_type: %d\n", attr_type); ret = -EINVAL; - goto err_other_attr_init; + kfree(attr_name_kobj); + goto unlock_drv_mutex; } ret = kobject_init_and_add(attr_name_kobj, &attr_name_ktype, NULL, "%s", attr_name); if (ret) { pr_err("Error encountered [%d]\n", ret); - kobject_put(attr_name_kobj); goto err_other_attr_init; } /* Populate attribute data */ switch (attr_type) { case HPWMI_SECURE_PLATFORM_TYPE: ret = hp_populate_secure_platform_data(attr_name_kobj); break; case HPWMI_SURE_START_TYPE: ret = hp_populate_sure_start_data(attr_name_kobj); break; default: ret = -EINVAL; } if (ret) goto err_other_attr_init; mutex_unlock(&bioscfg_drv.mutex); return 0; err_other_attr_init: + kobject_put(attr_name_kobj); +unlock_drv_mutex: mutex_unlock(&bioscfg_drv.mutex); return ret; }
We have two issues: 1. Memory leak of 'attr_name_kobj' in the error handling path. 2. When kobject_init_and_add() fails on every subsequent error path call kobject_put() to cleanup. Both of these issues will be fixed when we add kobject_put() in the goto label, as kfree() is already part of kobject_put(). Fixes: a34fc329b189 ("platform/x86: hp-bioscfg: bioscfg") Reported-by: kernel test robot <lkp@intel.com> Reported-by: Dan Carpenter <error27@gmail.com> Closes: https://lore.kernel.org/r/202309201412.on0VXJGo-lkp@intel.com/ Signed-off-by: Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com> --- Only compile tested, based on static analysis v1-> v2: Split this into mutliple patches doing one thing in a patch. --- drivers/platform/x86/hp/hp-bioscfg/bioscfg.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)