lkml.org 
[lkml]   [2021]   [Dec]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] livepatch: Fix leak on klp_init_patch_early failure path
Ah, found the thread...

On Tue, Dec 14, 2021 at 09:45:53AM +0100, Petr Mladek wrote:
> On Mon 2021-12-13 11:17:35, David Vernet wrote:
> > When enabling a KLP patch with `klp_enable_patch`, we invoke
> > `klp_init_patch_early` to initialize the kobjects for the patch itself, as
> > well as the `struct klp_object*`'s and `struct klp_func*`'s that comprise
> > it. However, there are some paths where we may fail to do an
> > early-initialization of an object or its functions if certain conditions
> > are not met, such as an object having a `NULL` funcs pointer. In these
> > paths, we may currently leak the `struct klp_patch*`'s kobject, as well as
> > any of its objects or functions, as we don't free the patch in
> > `klp_enable_patch` if `klp_init_patch_early` returns an error code.
>
> Could you please explain what exactly are we leaking?
>
> I do not see anything allocated in klp_init_*_early() functions.
> Also I do not see anything allocated in kobject_init().
>
> Documentation/core-api/kobject.rst says that kobject_put() must be
> used after calling kobject_add():
>
> "Once you registered your kobject via kobject_add(), you must never use
> kfree() to free it directly. The only safe way is to use kobject_put(). It
> is good practice to always use kobject_put() after kobject_init() to avoid
> errors creeping in."
>
>
> Hmm, the comment in lib/kobject.c says something else:
>
> /**
> * kobject_init() - Initialize a kobject structure.
> * @kobj: pointer to the kobject to initialize
> * @ktype: pointer to the ktype for this kobject.
> *
> * This function will properly initialize a kobject such that it can then
> * be passed to the kobject_add() call.
> *
> * After this function is called, the kobject MUST be cleaned up by a call
> * to kobject_put(), not by a call to kfree directly to ensure that all of
> * the memory is cleaned up properly.
> */

These say the same thing as "good practice" == "MUST" here. You can NOT
call kfree after calling kobject_init(). Bad things will happen if you
try to do so.

> I believe that this comment is misleading. IMHO, kobject_init() allows
> to call kobject_put().

You are FORCED TO call kobject_put() after kobject_init() is called.
Anything else is a bug.

> And it might be used to free memory that has
> already been allocated when initializing the structure where this
> kobject is bundled. But simple free() is perfectly fine when nothing
> else was allocated.

Nope, sorry, you have to call kobject_put().

thanks,

greg k-h

\
 
 \ /
  Last update: 2021-12-14 16:52    [W:0.142 / U:0.048 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site