LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [RFC PATCH] kobject: Clean up allocated memory on failure
@ 2019-05-16  0:07 Tobin C. Harding
  2019-05-16  6:40 ` Greg Kroah-Hartman
  0 siblings, 1 reply; 4+ messages in thread
From: Tobin C. Harding @ 2019-05-16  0:07 UTC (permalink / raw)
  To: Greg Kroah-Hartman; +Cc: Tobin C. Harding, Rafael J. Wysocki, linux-kernel

Currently kobject_add_varg() calls kobject_set_name_vargs() then returns
the return value of kobject_add_internal().  kobject_set_name_vargs()
allocates memory for the name string.  When kobject_add_varg() returns
an error we do not know if memory was allocated or not.  If we check the
return value of kobject_add_internal() instead of returning it directly
we can free the allocated memory if kobject_add_internal() fails.  Doing
this means that we now know that if kobject_add_varg() fails we do not
have to do any clean up, this benefit goes back up the call chain
meaning that we now do not need to do any cleanup if kobject_del()
fails.  Moving further back (in a theoretical kobject user callchain)
this means we now no longer need to call kobject_put() after calling
kobject_init_and_add(), we can just call kfree() on the enclosing
structure.  This makes the kobject API better follow the principle of
least surprise.

Check return value of kobject_add_internal() and free previously
allocated memory on failure.

Signed-off-by: Tobin C. Harding <tobin@kernel.org>
---

Hi Greg,

Pretty excited by this one, if this is correct it means that kobject
initialisation code, in the error path, can now use either kobject_put()
(to trigger the release method) OR kfree().  This means most of the
call sites of kobject_init_and_add() will get fixed for free!

I've been wrong before so I'll state here that this is based on the
assumption that kobject_init() does nothing that causes leaked memory.
This is _not_ what the function docs in kobject.c say but it _is_ what
the code seems to say since kobject_init() does nothing except
initialise kobject data member values?  Or have I got the dog by the
tail?

thanks,
Tobin.

 lib/kobject.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/lib/kobject.c b/lib/kobject.c
index f2ccdbac8ed9..bb0c0d374b13 100644
--- a/lib/kobject.c
+++ b/lib/kobject.c
@@ -387,7 +387,15 @@ static __printf(3, 0) int kobject_add_varg(struct kobject *kobj,
 		return retval;
 	}
 	kobj->parent = parent;
-	return kobject_add_internal(kobj);
+	retval = kobject_add_internal(kobj);
+
+	if (retval) {
+		if (kobj->name)
+			kfree_const(kobj->name);
+
+		return retval;
+	}
+	return 0;
 }
 
 /**
-- 
2.21.0


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [RFC PATCH] kobject: Clean up allocated memory on failure
  2019-05-16  0:07 [RFC PATCH] kobject: Clean up allocated memory on failure Tobin C. Harding
@ 2019-05-16  6:40 ` Greg Kroah-Hartman
  2019-05-16 12:01   ` Tobin C. Harding
  0 siblings, 1 reply; 4+ messages in thread
From: Greg Kroah-Hartman @ 2019-05-16  6:40 UTC (permalink / raw)
  To: Tobin C. Harding; +Cc: Rafael J. Wysocki, linux-kernel

On Thu, May 16, 2019 at 10:07:16AM +1000, Tobin C. Harding wrote:
> Currently kobject_add_varg() calls kobject_set_name_vargs() then returns
> the return value of kobject_add_internal().  kobject_set_name_vargs()
> allocates memory for the name string.  When kobject_add_varg() returns
> an error we do not know if memory was allocated or not.  If we check the
> return value of kobject_add_internal() instead of returning it directly
> we can free the allocated memory if kobject_add_internal() fails.  Doing
> this means that we now know that if kobject_add_varg() fails we do not
> have to do any clean up, this benefit goes back up the call chain
> meaning that we now do not need to do any cleanup if kobject_del()
> fails.  Moving further back (in a theoretical kobject user callchain)
> this means we now no longer need to call kobject_put() after calling
> kobject_init_and_add(), we can just call kfree() on the enclosing
> structure.  This makes the kobject API better follow the principle of
> least surprise.
> 
> Check return value of kobject_add_internal() and free previously
> allocated memory on failure.
> 
> Signed-off-by: Tobin C. Harding <tobin@kernel.org>
> ---
> 
> Hi Greg,
> 
> Pretty excited by this one, if this is correct it means that kobject
> initialisation code, in the error path, can now use either kobject_put()
> (to trigger the release method) OR kfree().  This means most of the
> call sites of kobject_init_and_add() will get fixed for free!
> 
> I've been wrong before so I'll state here that this is based on the
> assumption that kobject_init() does nothing that causes leaked memory.
> This is _not_ what the function docs in kobject.c say but it _is_ what
> the code seems to say since kobject_init() does nothing except
> initialise kobject data member values?  Or have I got the dog by the
> tail?

I think you are correct here.  In looking at the code paths, all should
be good and safe.

But, if you use your patch, then you have to call kfree, and you can not
call kobject_put(), otherwise kfree_const() will be called twice on the
same pointer, right?  So you will have to audit the kernel and change
everything again :)

Or, maybe this patch would prevent that:


diff --git a/lib/kobject.c b/lib/kobject.c
index f2ccdbac8ed9..03cdec1d450a 100644
--- a/lib/kobject.c
+++ b/lib/kobject.c
@@ -387,7 +387,14 @@ static __printf(3, 0) int kobject_add_varg(struct kobject *kobj,
 		return retval;
 	}
 	kobj->parent = parent;
-	return kobject_add_internal(kobj);
+
+	retval = kobject_add_internal(kobj);
+	if (retval && !is_kernel_rodata((unsigned long)(kobj->name))) {
+		kfree_const(kobj->name);
+		kobj->name = NULL;
+	}
+
+	return retval;
 }
 
 /**


But that feels like a huge hack to me.  I think, to be safe, we should
keep the existing lifetime rules, as it mirrors what happens with
'struct device', and that is what people _should_ be using, not "raw"
kobjects if at all possible.

Yeah, I know filesystems don't do that, my fault, I never thought a
filesystem would care about sysfs all those years ago :)

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [RFC PATCH] kobject: Clean up allocated memory on failure
  2019-05-16  6:40 ` Greg Kroah-Hartman
@ 2019-05-16 12:01   ` Tobin C. Harding
  2019-05-16 17:23     ` Greg Kroah-Hartman
  0 siblings, 1 reply; 4+ messages in thread
From: Tobin C. Harding @ 2019-05-16 12:01 UTC (permalink / raw)
  To: Greg Kroah-Hartman; +Cc: Tobin C. Harding, Rafael J. Wysocki, linux-kernel

On Thu, May 16, 2019 at 08:40:29AM +0200, Greg Kroah-Hartman wrote:
> On Thu, May 16, 2019 at 10:07:16AM +1000, Tobin C. Harding wrote:
> > Currently kobject_add_varg() calls kobject_set_name_vargs() then returns
> > the return value of kobject_add_internal().  kobject_set_name_vargs()
> > allocates memory for the name string.  When kobject_add_varg() returns
> > an error we do not know if memory was allocated or not.  If we check the
> > return value of kobject_add_internal() instead of returning it directly
> > we can free the allocated memory if kobject_add_internal() fails.  Doing
> > this means that we now know that if kobject_add_varg() fails we do not
> > have to do any clean up, this benefit goes back up the call chain
> > meaning that we now do not need to do any cleanup if kobject_del()
> > fails.  Moving further back (in a theoretical kobject user callchain)
> > this means we now no longer need to call kobject_put() after calling
> > kobject_init_and_add(), we can just call kfree() on the enclosing
> > structure.  This makes the kobject API better follow the principle of
> > least surprise.
> > 
> > Check return value of kobject_add_internal() and free previously
> > allocated memory on failure.
> > 
> > Signed-off-by: Tobin C. Harding <tobin@kernel.org>
> > ---
> > 
> > Hi Greg,
> > 
> > Pretty excited by this one, if this is correct it means that kobject
> > initialisation code, in the error path, can now use either kobject_put()
> > (to trigger the release method) OR kfree().  This means most of the
> > call sites of kobject_init_and_add() will get fixed for free!
> > 
> > I've been wrong before so I'll state here that this is based on the
> > assumption that kobject_init() does nothing that causes leaked memory.
> > This is _not_ what the function docs in kobject.c say but it _is_ what
> > the code seems to say since kobject_init() does nothing except
> > initialise kobject data member values?  Or have I got the dog by the
> > tail?
> 
> I think you are correct here.  In looking at the code paths, all should
> be good and safe.
> 
> But, if you use your patch, then you have to call kfree, and you can not
> call kobject_put(), otherwise kfree_const() will be called twice on the
> same pointer, right?  So you will have to audit the kernel and change
> everything again :)

Oh my bad, I got so excited by this I read the 'if (name) {' in kobject
to be guarding the double call to kfree_const(), which clearly it doesn't.

> Or, maybe this patch would prevent that:
> 
> 
> diff --git a/lib/kobject.c b/lib/kobject.c
> index f2ccdbac8ed9..03cdec1d450a 100644
> --- a/lib/kobject.c
> +++ b/lib/kobject.c
> @@ -387,7 +387,14 @@ static __printf(3, 0) int kobject_add_varg(struct kobject *kobj,
>  		return retval;
>  	}
>  	kobj->parent = parent;
> -	return kobject_add_internal(kobj);
> +
> +	retval = kobject_add_internal(kobj);
> +	if (retval && !is_kernel_rodata((unsigned long)(kobj->name))) {
> +		kfree_const(kobj->name);
> +		kobj->name = NULL;
> +	}
> +
> +	return retval;
>  }
>
>  /**
> 
> 
> But that feels like a huge hack to me.

I agree, does the job but too ugly.

> I think, to be safe, we should
> keep the existing lifetime rules, as it mirrors what happens with
> 'struct device', and that is what people _should_ be using, not "raw"
> kobjects if at all possible.

Oh, I wasn't seeing this through the eyes of a driver developer, perhaps
I should have started in drivers/ not in fs/ 

> Yeah, I know filesystems don't do that, my fault, I never thought a
> filesystem would care about sysfs all those years ago :)

Tough business that, predicting the future.

Let's drop this and I'll keep plugging away.

Thanks,
Tobin.

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [RFC PATCH] kobject: Clean up allocated memory on failure
  2019-05-16 12:01   ` Tobin C. Harding
@ 2019-05-16 17:23     ` Greg Kroah-Hartman
  0 siblings, 0 replies; 4+ messages in thread
From: Greg Kroah-Hartman @ 2019-05-16 17:23 UTC (permalink / raw)
  To: Tobin C. Harding; +Cc: Tobin C. Harding, Rafael J. Wysocki, linux-kernel

On Thu, May 16, 2019 at 10:01:23PM +1000, Tobin C. Harding wrote:
> On Thu, May 16, 2019 at 08:40:29AM +0200, Greg Kroah-Hartman wrote:
> > On Thu, May 16, 2019 at 10:07:16AM +1000, Tobin C. Harding wrote:
> > > Currently kobject_add_varg() calls kobject_set_name_vargs() then returns
> > > the return value of kobject_add_internal().  kobject_set_name_vargs()
> > > allocates memory for the name string.  When kobject_add_varg() returns
> > > an error we do not know if memory was allocated or not.  If we check the
> > > return value of kobject_add_internal() instead of returning it directly
> > > we can free the allocated memory if kobject_add_internal() fails.  Doing
> > > this means that we now know that if kobject_add_varg() fails we do not
> > > have to do any clean up, this benefit goes back up the call chain
> > > meaning that we now do not need to do any cleanup if kobject_del()
> > > fails.  Moving further back (in a theoretical kobject user callchain)
> > > this means we now no longer need to call kobject_put() after calling
> > > kobject_init_and_add(), we can just call kfree() on the enclosing
> > > structure.  This makes the kobject API better follow the principle of
> > > least surprise.
> > > 
> > > Check return value of kobject_add_internal() and free previously
> > > allocated memory on failure.
> > > 
> > > Signed-off-by: Tobin C. Harding <tobin@kernel.org>
> > > ---
> > > 
> > > Hi Greg,
> > > 
> > > Pretty excited by this one, if this is correct it means that kobject
> > > initialisation code, in the error path, can now use either kobject_put()
> > > (to trigger the release method) OR kfree().  This means most of the
> > > call sites of kobject_init_and_add() will get fixed for free!
> > > 
> > > I've been wrong before so I'll state here that this is based on the
> > > assumption that kobject_init() does nothing that causes leaked memory.
> > > This is _not_ what the function docs in kobject.c say but it _is_ what
> > > the code seems to say since kobject_init() does nothing except
> > > initialise kobject data member values?  Or have I got the dog by the
> > > tail?
> > 
> > I think you are correct here.  In looking at the code paths, all should
> > be good and safe.
> > 
> > But, if you use your patch, then you have to call kfree, and you can not
> > call kobject_put(), otherwise kfree_const() will be called twice on the
> > same pointer, right?  So you will have to audit the kernel and change
> > everything again :)
> 
> Oh my bad, I got so excited by this I read the 'if (name) {' in kobject
> to be guarding the double call to kfree_const(), which clearly it doesn't.
> 
> > Or, maybe this patch would prevent that:
> > 
> > 
> > diff --git a/lib/kobject.c b/lib/kobject.c
> > index f2ccdbac8ed9..03cdec1d450a 100644
> > --- a/lib/kobject.c
> > +++ b/lib/kobject.c
> > @@ -387,7 +387,14 @@ static __printf(3, 0) int kobject_add_varg(struct kobject *kobj,
> >  		return retval;
> >  	}
> >  	kobj->parent = parent;
> > -	return kobject_add_internal(kobj);
> > +
> > +	retval = kobject_add_internal(kobj);
> > +	if (retval && !is_kernel_rodata((unsigned long)(kobj->name))) {
> > +		kfree_const(kobj->name);
> > +		kobj->name = NULL;
> > +	}
> > +
> > +	return retval;
> >  }
> >
> >  /**
> > 
> > 
> > But that feels like a huge hack to me.
> 
> I agree, does the job but too ugly.
> 
> > I think, to be safe, we should
> > keep the existing lifetime rules, as it mirrors what happens with
> > 'struct device', and that is what people _should_ be using, not "raw"
> > kobjects if at all possible.
> 
> Oh, I wasn't seeing this through the eyes of a driver developer, perhaps
> I should have started in drivers/ not in fs/ 

That's next on your list :)

Keep up the great work,

greg k-h

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2019-05-16 17:23 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-16  0:07 [RFC PATCH] kobject: Clean up allocated memory on failure Tobin C. Harding
2019-05-16  6:40 ` Greg Kroah-Hartman
2019-05-16 12:01   ` Tobin C. Harding
2019-05-16 17:23     ` Greg Kroah-Hartman

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).