Re: [PATCH v2 4/4] platform/x86: hp-bioscfg: Fix error handling in hp_add_other_attributes()

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

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

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).

-- 
 i.