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

[Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com>]

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
>