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
