[PATCH 0/2] livepatch: Fix usage of kobject_init_and_add()

[PATCH 0/2] livepatch: Fix usage of kobject_init_and_add()

[Tobin C. Harding]

Hi,

Currently there are a few places in kernel/livepatch/ which do not
correctly use kobject_init_and_add().

An error return from kobject_init_and_add() requires a call to
kobject_put().

The cleanup function after a successful call to kobject_init_and_add()
is kobject_del(). 

This set is part of an effort to check/fix all callsites of
kobject_init_and_add().


This set fixes all callsites under kernel/livepatch/


thanks,
Tobin.


Tobin C. Harding (2):
  livepatch: Fix kobject memleak
  livepatch: Use correct kobject cleanup function

 kernel/livepatch/core.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

-- 
2.21.0

[PATCH 1/2] livepatch: Fix kobject memleak

[Tobin C. Harding]

Currently error return from kobject_init_and_add() is not followed by a
call to kobject_put().  This means there is a memory leak.

Add call to kobject_put() in error path of kobject_init_and_add().

Signed-off-by: Tobin C. Harding <tobin@kernel.org>
---
 kernel/livepatch/core.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index eb0ee10a1981..98a7bec41faa 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -727,7 +727,9 @@ static int klp_init_func(struct klp_object *obj, struct klp_func *func)
 	ret = kobject_init_and_add(&func->kobj, &klp_ktype_func,
 				   &obj->kobj, "%s,%lu", func->old_name,
 				   func->old_sympos ? func->old_sympos : 1);
-	if (!ret)
+	if (ret)
+		kobject_put(&func->kobj);
+	else
 		func->kobj_added = true;
 
 	return ret;
@@ -803,8 +805,10 @@ static int klp_init_object(struct klp_patch *patch, struct klp_object *obj)
 	name = klp_is_module(obj) ? obj->name : "vmlinux";
 	ret = kobject_init_and_add(&obj->kobj, &klp_ktype_object,
 				   &patch->kobj, "%s", name);
-	if (ret)
+	if (ret) {
+		kobject_put(&func->kobj);
 		return ret;
+	}
 	obj->kobj_added = true;
 
 	klp_for_each_func(obj, func) {
@@ -862,8 +866,10 @@ static int klp_init_patch(struct klp_patch *patch)
 
 	ret = kobject_init_and_add(&patch->kobj, &klp_ktype_patch,
 				   klp_root_kobj, "%s", patch->mod->name);
-	if (ret)
+	if (ret) {
+		kobject_put(&func->kobj);
 		return ret;
+	}
 	patch->kobj_added = true;
 
 	if (patch->replace) {
-- 
2.21.0

[PATCH 2/2] livepatch: Use correct kobject cleanup function

[Tobin C. Harding]

The correct cleanup function after a call to kobject_init_and_add() has
succeeded is kobject_del() _not_ kobject_put().  kobject_del() calls
kobject_put().

Use correct cleanup function when removing a kobject.

Signed-off-by: Tobin C. Harding <tobin@kernel.org>
---
 kernel/livepatch/core.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 98a7bec41faa..4cce6bb6e073 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -589,9 +589,8 @@ static void __klp_free_funcs(struct klp_object *obj, bool nops_only)
 
 		list_del(&func->node);
 
-		/* Might be called from klp_init_patch() error path. */
 		if (func->kobj_added) {
-			kobject_put(&func->kobj);
+			kobject_del(&func->kobj);
 		} else if (func->nop) {
 			klp_free_func_nop(func);
 		}
@@ -625,9 +624,8 @@ static void __klp_free_objects(struct klp_patch *patch, bool nops_only)
 
 		list_del(&obj->node);
 
-		/* Might be called from klp_init_patch() error path. */
 		if (obj->kobj_added) {
-			kobject_put(&obj->kobj);
+			kobject_del(&obj->kobj);
 		} else if (obj->dynamic) {
 			klp_free_object_dynamic(obj);
 		}
@@ -676,7 +674,7 @@ static void klp_free_patch_finish(struct klp_patch *patch)
 	 * cannot get enabled again.
 	 */
 	if (patch->kobj_added) {
-		kobject_put(&patch->kobj);
+		kobject_del(&patch->kobj);
 		wait_for_completion(&patch->finish);
 	}
 
-- 
2.21.0

Re: [PATCH 1/2] livepatch: Fix kobject memleak

[Greg Kroah-Hartman]

On Tue, Apr 30, 2019 at 10:15:33AM +1000, Tobin C. Harding wrote:
> Currently error return from kobject_init_and_add() is not followed by a
> call to kobject_put().  This means there is a memory leak.
> 
> Add call to kobject_put() in error path of kobject_init_and_add().
> 
> Signed-off-by: Tobin C. Harding <tobin@kernel.org>

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [PATCH 2/2] livepatch: Use correct kobject cleanup function

[Greg Kroah-Hartman]

On Tue, Apr 30, 2019 at 10:15:34AM +1000, Tobin C. Harding wrote:
> The correct cleanup function after a call to kobject_init_and_add() has
> succeeded is kobject_del() _not_ kobject_put().  kobject_del() calls
> kobject_put().
> 
> Use correct cleanup function when removing a kobject.
> 
> Signed-off-by: Tobin C. Harding <tobin@kernel.org>

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [PATCH 1/2] livepatch: Fix kobject memleak

[Miroslav Benes]

On Tue, 30 Apr 2019, Greg Kroah-Hartman wrote:

> On Tue, Apr 30, 2019 at 10:15:33AM +1000, Tobin C. Harding wrote:
> > Currently error return from kobject_init_and_add() is not followed by a
> > call to kobject_put().  This means there is a memory leak.
> > 
> > Add call to kobject_put() in error path of kobject_init_and_add().
> > 
> > Signed-off-by: Tobin C. Harding <tobin@kernel.org>
> 
> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Well, it does not even compile...

On Tue, 30 Apr 2019, Tobin C. Harding wrote:

> Currently error return from kobject_init_and_add() is not followed by a
> call to kobject_put().  This means there is a memory leak.
> 
> Add call to kobject_put() in error path of kobject_init_and_add().
> 
> Signed-off-by: Tobin C. Harding <tobin@kernel.org>
> ---
>  kernel/livepatch/core.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index eb0ee10a1981..98a7bec41faa 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -727,7 +727,9 @@ static int klp_init_func(struct klp_object *obj, struct klp_func *func)
>  	ret = kobject_init_and_add(&func->kobj, &klp_ktype_func,
>  				   &obj->kobj, "%s,%lu", func->old_name,
>  				   func->old_sympos ? func->old_sympos : 1);
> -	if (!ret)
> +	if (ret)
> +		kobject_put(&func->kobj);
> +	else
>  		func->kobj_added = true;
>  
>  	return ret;
> @@ -803,8 +805,10 @@ static int klp_init_object(struct klp_patch *patch, struct klp_object *obj)
>  	name = klp_is_module(obj) ? obj->name : "vmlinux";
>  	ret = kobject_init_and_add(&obj->kobj, &klp_ktype_object,
>  				   &patch->kobj, "%s", name);
> -	if (ret)
> +	if (ret) {
> +		kobject_put(&func->kobj);

kobject_put(&obj->kobj); I suppose.

>  		return ret;
> +	}
>  	obj->kobj_added = true;
>  
>  	klp_for_each_func(obj, func) {
> @@ -862,8 +866,10 @@ static int klp_init_patch(struct klp_patch *patch)
>  
>  	ret = kobject_init_and_add(&patch->kobj, &klp_ktype_patch,
>  				   klp_root_kobj, "%s", patch->mod->name);
> -	if (ret)
> +	if (ret) {
> +		kobject_put(&func->kobj);

kobject_put(&patch->kobj);

Thanks,
Miroslav

Re: [PATCH 1/2] livepatch: Fix kobject memleak

[Greg Kroah-Hartman]

On Tue, Apr 30, 2019 at 12:44:55PM +0200, Miroslav Benes wrote:
> On Tue, 30 Apr 2019, Greg Kroah-Hartman wrote:
> 
> > On Tue, Apr 30, 2019 at 10:15:33AM +1000, Tobin C. Harding wrote:
> > > Currently error return from kobject_init_and_add() is not followed by a
> > > call to kobject_put().  This means there is a memory leak.
> > > 
> > > Add call to kobject_put() in error path of kobject_init_and_add().
> > > 
> > > Signed-off-by: Tobin C. Harding <tobin@kernel.org>
> > 
> > Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> 
> Well, it does not even compile...

The idea is correct :)

> On Tue, 30 Apr 2019, Tobin C. Harding wrote:
> 
> > Currently error return from kobject_init_and_add() is not followed by a
> > call to kobject_put().  This means there is a memory leak.
> > 
> > Add call to kobject_put() in error path of kobject_init_and_add().
> > 
> > Signed-off-by: Tobin C. Harding <tobin@kernel.org>
> > ---
> >  kernel/livepatch/core.c | 12 +++++++++---
> >  1 file changed, 9 insertions(+), 3 deletions(-)
> > 
> > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> > index eb0ee10a1981..98a7bec41faa 100644
> > --- a/kernel/livepatch/core.c
> > +++ b/kernel/livepatch/core.c
> > @@ -727,7 +727,9 @@ static int klp_init_func(struct klp_object *obj, struct klp_func *func)
> >  	ret = kobject_init_and_add(&func->kobj, &klp_ktype_func,
> >  				   &obj->kobj, "%s,%lu", func->old_name,
> >  				   func->old_sympos ? func->old_sympos : 1);
> > -	if (!ret)
> > +	if (ret)
> > +		kobject_put(&func->kobj);
> > +	else
> >  		func->kobj_added = true;
> >  
> >  	return ret;
> > @@ -803,8 +805,10 @@ static int klp_init_object(struct klp_patch *patch, struct klp_object *obj)
> >  	name = klp_is_module(obj) ? obj->name : "vmlinux";
> >  	ret = kobject_init_and_add(&obj->kobj, &klp_ktype_object,
> >  				   &patch->kobj, "%s", name);
> > -	if (ret)
> > +	if (ret) {
> > +		kobject_put(&func->kobj);
> 
> kobject_put(&obj->kobj); I suppose.

Yeah, that makes it better, sorry for not catching that in the review :(

greg k-h

Re: [PATCH 2/2] livepatch: Use correct kobject cleanup function

[Miroslav Benes]

On Tue, 30 Apr 2019, Tobin C. Harding wrote:

> The correct cleanup function after a call to kobject_init_and_add() has
> succeeded is kobject_del() _not_ kobject_put().  kobject_del() calls
> kobject_put().
> 
> Use correct cleanup function when removing a kobject.
> 
> Signed-off-by: Tobin C. Harding <tobin@kernel.org>
> ---
>  kernel/livepatch/core.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index 98a7bec41faa..4cce6bb6e073 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -589,9 +589,8 @@ static void __klp_free_funcs(struct klp_object *obj, bool nops_only)
>  
>  		list_del(&func->node);
>  
> -		/* Might be called from klp_init_patch() error path. */

Could you leave the comment as is? If I am not mistaken, it is still 
valid. func->kobj_added check is here exactly because the function may be 
called as mentioned.

One could argue that the comment is not so important, but the change does 
not belong to the patch anyway in my opinion.

>  		if (func->kobj_added) {
> -			kobject_put(&func->kobj);
> +			kobject_del(&func->kobj);
>  		} else if (func->nop) {
>  			klp_free_func_nop(func);
>  		}
> @@ -625,9 +624,8 @@ static void __klp_free_objects(struct klp_patch *patch, bool nops_only)
>  
>  		list_del(&obj->node);
>  
> -		/* Might be called from klp_init_patch() error path. */

Same here.

>  		if (obj->kobj_added) {
> -			kobject_put(&obj->kobj);
> +			kobject_del(&obj->kobj);
>  		} else if (obj->dynamic) {
>  			klp_free_object_dynamic(obj);
>  		}
> @@ -676,7 +674,7 @@ static void klp_free_patch_finish(struct klp_patch *patch)
>  	 * cannot get enabled again.
>  	 */
>  	if (patch->kobj_added) {
> -		kobject_put(&patch->kobj);
> +		kobject_del(&patch->kobj);
>  		wait_for_completion(&patch->finish);
>  	}

Miroslav

Re: [PATCH 1/2] livepatch: Fix kobject memleak

[Petr Mladek]

On Tue 2019-04-30 10:15:33, Tobin C. Harding wrote:
> Currently error return from kobject_init_and_add() is not followed by a
> call to kobject_put().  This means there is a memory leak.

I see, the ref count is always initialized to 1 via:

  + kobject_init_and_add()
    + kobject_init()
      + kobject_init_internal()
	+ kref_init()


> Signed-off-by: Tobin C. Harding <tobin@kernel.org>
> ---
>  kernel/livepatch/core.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index eb0ee10a1981..98a7bec41faa 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -727,7 +727,9 @@ static int klp_init_func(struct klp_object *obj, struct klp_func *func)
>  	ret = kobject_init_and_add(&func->kobj, &klp_ktype_func,
>  				   &obj->kobj, "%s,%lu", func->old_name,
>  				   func->old_sympos ? func->old_sympos : 1);
> -	if (!ret)
> +	if (ret)
> +		kobject_put(&func->kobj);
> +	else
>  		func->kobj_added = true;

We could actually get rid of the custom kobj_added. Intead, we could
check for kobj->state_initialized in the various klp_free* functions.

Best Regards,
Petr

Re: [PATCH 2/2] livepatch: Use correct kobject cleanup function

[Petr Mladek]

On Tue 2019-04-30 10:15:34, Tobin C. Harding wrote:
> The correct cleanup function after a call to kobject_init_and_add() has
> succeeded is kobject_del() _not_ kobject_put().  kobject_del() calls
> kobject_put().

Really? I see only kobject_put(kobj->parent) in kobject_del.
It decreases a reference of the _parent_ object and not
the given one.

Also the section "Kobject removal" in Documentation/kobject.txt
says that kobject_del() is for two-stage removal. kobject_put()
still needs to get called at a later time.

IMHO, this patch causes that kobject_put() would never get called.

That said, we could probably make the removal a bit cleaner
by using kobject_del() in klp_free_patch_start() and
kobject_put() in klp_free_patch_finish(). But I have
to think more about it.

Best Regards,
Petr

Re: [PATCH 1/2] livepatch: Fix kobject memleak

[Greg Kroah-Hartman]

On Tue, Apr 30, 2019 at 04:56:13PM +0200, Petr Mladek wrote:
> On Tue 2019-04-30 10:15:33, Tobin C. Harding wrote:
> > Currently error return from kobject_init_and_add() is not followed by a
> > call to kobject_put().  This means there is a memory leak.
> 
> I see, the ref count is always initialized to 1 via:
> 
>   + kobject_init_and_add()
>     + kobject_init()
>       + kobject_init_internal()
> 	+ kref_init()
> 
> 
> > Signed-off-by: Tobin C. Harding <tobin@kernel.org>
> > ---
> >  kernel/livepatch/core.c | 12 +++++++++---
> >  1 file changed, 9 insertions(+), 3 deletions(-)
> > 
> > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> > index eb0ee10a1981..98a7bec41faa 100644
> > --- a/kernel/livepatch/core.c
> > +++ b/kernel/livepatch/core.c
> > @@ -727,7 +727,9 @@ static int klp_init_func(struct klp_object *obj, struct klp_func *func)
> >  	ret = kobject_init_and_add(&func->kobj, &klp_ktype_func,
> >  				   &obj->kobj, "%s,%lu", func->old_name,
> >  				   func->old_sympos ? func->old_sympos : 1);
> > -	if (!ret)
> > +	if (ret)
> > +		kobject_put(&func->kobj);
> > +	else
> >  		func->kobj_added = true;
> 
> We could actually get rid of the custom kobj_added. Intead, we could
> check for kobj->state_initialized in the various klp_free* functions.

Why do you need to care about this at all anyway?  The kobject can
handle it's own lifetime just fine (that's what it is there for), why do
you need to also track it?

thanks,

greg k-h

Re: [PATCH 2/2] livepatch: Use correct kobject cleanup function

[Tobin C. Harding]

On Tue, Apr 30, 2019 at 05:08:11PM +0200, Petr Mladek wrote:
> On Tue 2019-04-30 10:15:34, Tobin C. Harding wrote:
> > The correct cleanup function after a call to kobject_init_and_add() has
> > succeeded is kobject_del() _not_ kobject_put().  kobject_del() calls
> > kobject_put().
> 
> Really? I see only kobject_put(kobj->parent) in kobject_del.
> It decreases a reference of the _parent_ object and not
> the given one.

Thanks Petr, you are right.  I misread kobject_del().  The story
thickens, so we need to call kobject_del() AND kobject_put().

> Also the section "Kobject removal" in Documentation/kobject.txt
> says that kobject_del() is for two-stage removal. kobject_put()
> still needs to get called at a later time.

Is this call sequence above what is meant by 'two-stage removal', I
didn't really understand that bit of the docs (and I almost always just
assume docs are stale and take them as a hint only :)

> IMHO, this patch causes that kobject_put() would never get called.

I'll do a v2 of this one and re-check all the patches on this I've
already sent (including the docs ones).

> That said, we could probably make the removal a bit cleaner
> by using kobject_del() in klp_free_patch_start() and
> kobject_put() in klp_free_patch_finish(). But I have
> to think more about it.

Noted, thanks for your review.

	Tobin
	

Re: [PATCH 2/2] livepatch: Use correct kobject cleanup function

[Tobin C. Harding]

On Tue, Apr 30, 2019 at 01:00:05PM +0200, Miroslav Benes wrote:
> On Tue, 30 Apr 2019, Tobin C. Harding wrote:
> 
> > The correct cleanup function after a call to kobject_init_and_add() has
> > succeeded is kobject_del() _not_ kobject_put().  kobject_del() calls
> > kobject_put().
> > 
> > Use correct cleanup function when removing a kobject.
> > 
> > Signed-off-by: Tobin C. Harding <tobin@kernel.org>
> > ---
> >  kernel/livepatch/core.c | 8 +++-----
> >  1 file changed, 3 insertions(+), 5 deletions(-)
> > 
> > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> > index 98a7bec41faa..4cce6bb6e073 100644
> > --- a/kernel/livepatch/core.c
> > +++ b/kernel/livepatch/core.c
> > @@ -589,9 +589,8 @@ static void __klp_free_funcs(struct klp_object *obj, bool nops_only)
> >  
> >  		list_del(&func->node);
> >  
> > -		/* Might be called from klp_init_patch() error path. */
> 
> Could you leave the comment as is? If I am not mistaken, it is still 
> valid. func->kobj_added check is here exactly because the function may be 
> called as mentioned.

Will put it back in for you on v2

thanks,
Tobin.

Re: [PATCH 1/2] livepatch: Fix kobject memleak

[Tobin C. Harding]

On Tue, Apr 30, 2019 at 12:44:55PM +0200, Miroslav Benes wrote:
> On Tue, 30 Apr 2019, Greg Kroah-Hartman wrote:
> 
> > On Tue, Apr 30, 2019 at 10:15:33AM +1000, Tobin C. Harding wrote:
> > > Currently error return from kobject_init_and_add() is not followed by a
> > > call to kobject_put().  This means there is a memory leak.
> > > 
> > > Add call to kobject_put() in error path of kobject_init_and_add().
> > > 
> > > Signed-off-by: Tobin C. Harding <tobin@kernel.org>
> > 
> > Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> 
> Well, it does not even compile...

My apologies, I did compile this but obviously I don't know how to
configure the kernel.

Thanks for the review.

	Tobin