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
On Tue, 14 Dec 2021, 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?

It would help to share warning outputs (or whatever) from DEBUG_KOBJECTS.

> 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.
> */
>
> I believe that this comment is misleading. IMHO, kobject_init() allows
> to call kobject_put(). 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.

I think that this might be, once again, a false positive. We use kobjects
differently than what the kobject implementation and its documentation
assume. So not doing anything after kobject_init() and kobject_add() in
_init_early stages could be perfectly fine. DEBUG_KOBJECTS output would be
really welcome.

And if it is not a false positive, we should implement some rollback for
processed klp_funcs and klp_objects if an error happens. It is not only
klp_patch kobject affected.

Regards
Miroslav

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