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: 2891 bytes --]

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)

-- 
 i.