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

[Harshit Mogalapalli]

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