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

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

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.