Re: [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:

quoted

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