[PATCH] livepatch: Fix leak on klp_init_patch_early failure path

[PATCH] livepatch: Fix leak on klp_init_patch_early failure path

[David Vernet]

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. For
example, if we added the following object entry to the sample livepatch
code, it would cause us to leak the vmlinux `klp_object`, and its `struct
klp_func` which updates `cmdline_proc_show`:

```
static struct klp_object objs[] = {
        {
                .name = "kvm",
        }, { }
};
```

Without this change, if we enable `CONFIG_DEBUG_KOBJECT` and try to `kpatch
load livepatch-sample.ko`, we don't observe the kobjects being released
(though of course we do observe `insmod` failing to insert the module).
With the change, we do observe that the `kobject` for the patch and its
`vmlinux` object are released.

Signed-off-by: David Vernet <void@manifault.com>
---
 kernel/livepatch/core.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 335d988bd811..16e96836a825 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -1052,10 +1052,7 @@ int klp_enable_patch(struct klp_patch *patch)
 	}
 
 	ret = klp_init_patch_early(patch);
-	if (ret) {
-		mutex_unlock(&klp_mutex);
-		return ret;
-	}
+		goto err;
 
 	ret = klp_init_patch(patch);
 	if (ret)
-- 
2.30.2

Re: [PATCH] livepatch: Fix leak on klp_init_patch_early failure path

[Josh Poimboeuf]

On Mon, Dec 13, 2021 at 11:17:35AM -0800, 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. For
> example, if we added the following object entry to the sample livepatch
> code, it would cause us to leak the vmlinux `klp_object`, and its `struct
> klp_func` which updates `cmdline_proc_show`:
> 
> ```
> static struct klp_object objs[] = {
>         {
>                 .name = "kvm",
>         }, { }
> };
> ```
> 
> Without this change, if we enable `CONFIG_DEBUG_KOBJECT` and try to `kpatch
> load livepatch-sample.ko`, we don't observe the kobjects being released
> (though of course we do observe `insmod` failing to insert the module).
> With the change, we do observe that the `kobject` for the patch and its
> `vmlinux` object are released.
> 
> Signed-off-by: David Vernet <void@manifault.com>

Thanks for reporting the issue and submitting the patch!

The patch description needs a few tweaks.  In the kernel we don't use
Markdown for patch descriptions.

A function can be postfixed with a trailing pair of parentheses, like
klp_enable_patch().

Other symbols can be enclosed with single quotes, like 'struct
klp_object'.

I'd also recommend avoiding the excessive use of "we", in favor of more
imperative-type language.

See Documentation/process/submitting-patches.rst for more details.  It's
also a good idea to look at some kernel commit logs to get a general
idea of the kernel patch description style.

> @@ -1052,10 +1052,7 @@ int klp_enable_patch(struct klp_patch *patch)
>  	}
>  
>  	ret = klp_init_patch_early(patch);
> -	if (ret) {
> -		mutex_unlock(&klp_mutex);
> -		return ret;
> -	}
> +		goto err;
>  
>  	ret = klp_init_patch(patch);
>  	if (ret)

I don't think the fix will be quite that simple.  For example, if
klp_init_patch_early() fails, that means try_module_get() hasn't been
done, so klp_free_patch_finish() will wrongly do a module_put().

-- 
Josh

Re: [PATCH] livepatch: Fix leak on klp_init_patch_early failure path

[David Vernet]

Josh Poimboeuf <jpoimboe@redhat.com> wrote on Mon [2021-Dec-13 12:10:22 -0800]:
> The patch description needs a few tweaks.  In the kernel we don't use
> Markdown for patch descriptions.
> 
> A function can be postfixed with a trailing pair of parentheses, like
> klp_enable_patch().
> 
> Other symbols can be enclosed with single quotes, like 'struct
> klp_object'.
> 
> I'd also recommend avoiding the excessive use of "we", in favor of more
> imperative-type language.
> 
> See Documentation/process/submitting-patches.rst for more details.  It's
> also a good idea to look at some kernel commit logs to get a general
> idea of the kernel patch description style.

Understood, I'll take a read through and re-submit the patch to honor the
norms for Linux kernel patches. My sincere apologies for the noise, and
thank you for the positive and constructive suggestions.

> I don't think the fix will be quite that simple.  For example, if
> klp_init_patch_early() fails, that means try_module_get() hasn't been
> done, so klp_free_patch_finish() will wrongly do a module_put().

Ugh, good point and thank you for catching that. Another problem with the
current patch is that we'll call kobject_put() on the patch even if we
never call kobject_init on the patch due to patch->objs being NULL.

Perhaps we should pull try_module_get() and the NULL check for patch->objs
out of klp_init_patch_early()? It feels a bit more intuitive to me if
klp_init_patch_early() were only be responsible for initializing kobjects
for the patch and its objects / funcs anyways.

Testing it locally seems to work fine. Let me know if this sounds
reasonable to you, and I'll send out a v2 patch with the fixes to both the
patch description, and logic.

- David

Re: [PATCH] livepatch: Fix leak on klp_init_patch_early failure path

[Song Liu]

> On Dec 13, 2021, at 2:58 PM, David Vernet <void@manifault.com> wrote:
> 
> Josh Poimboeuf <jpoimboe@redhat.com> wrote on Mon [2021-Dec-13 12:10:22 -0800]:
>> The patch description needs a few tweaks.  In the kernel we don't use
>> Markdown for patch descriptions.
>> 
>> A function can be postfixed with a trailing pair of parentheses, like
>> klp_enable_patch().
>> 
>> Other symbols can be enclosed with single quotes, like 'struct
>> klp_object'.
>> 
>> I'd also recommend avoiding the excessive use of "we", in favor of more
>> imperative-type language.
>> 
>> See Documentation/process/submitting-patches.rst for more details.  It's
>> also a good idea to look at some kernel commit logs to get a general
>> idea of the kernel patch description style.
> 
> Understood, I'll take a read through and re-submit the patch to honor the
> norms for Linux kernel patches. My sincere apologies for the noise, and
> thank you for the positive and constructive suggestions.
> 
>> I don't think the fix will be quite that simple.  For example, if
>> klp_init_patch_early() fails, that means try_module_get() hasn't been
>> done, so klp_free_patch_finish() will wrongly do a module_put().
> 
> Ugh, good point and thank you for catching that. Another problem with the
> current patch is that we'll call kobject_put() on the patch even if we
> never call kobject_init on the patch due to patch->objs being NULL.
> 
> Perhaps we should pull try_module_get() and the NULL check for patch->objs
> out of klp_init_patch_early()? It feels a bit more intuitive to me if
> klp_init_patch_early() were only be responsible for initializing kobjects
> for the patch and its objects / funcs anyways.

Pulling those logic out of klp_init_patch_early() makes sense to me. 
Alternatively, we may also have a cleanup section in klp_init_patch_early(), 
like below. I am not sure which way will be cleaner. 

Thanks,
Song



diff --git i/kernel/livepatch/core.c w/kernel/livepatch/core.c
index 335d988bd811..20b959c82204 100644
--- i/kernel/livepatch/core.c
+++ w/kernel/livepatch/core.c
@@ -864,7 +864,7 @@ static void klp_init_object_early(struct klp_patch *patch,

 static int klp_init_patch_early(struct klp_patch *patch)
 {
-       struct klp_object *obj;
+       struct klp_object *obj, *obj2;
        struct klp_func *func;

        if (!patch->objs)
@@ -880,7 +880,7 @@ static int klp_init_patch_early(struct klp_patch *patch)

        klp_for_each_object_static(patch, obj) {
                if (!obj->funcs)
-                       return -EINVAL;
+                       goto cleanup;

                klp_init_object_early(patch, obj);

@@ -890,9 +890,15 @@ static int klp_init_patch_early(struct klp_patch *patch)
        }

        if (!try_module_get(patch->mod))
-               return -ENODEV;
+               goto cleanup;

        return 0;
+cleanup:
+       klp_for_each_func_static(patch, obj2) {
+               if (obj2 == obj)
+                       break;  // done
+               /* do clean up */
+       }
 }

 static int klp_init_patch(struct klp_patch *patch)

Re: [PATCH] livepatch: Fix leak on klp_init_patch_early failure path

[Josh Poimboeuf]

On Mon, Dec 13, 2021 at 02:58:38PM -0800, David Vernet wrote:
> > I don't think the fix will be quite that simple.  For example, if
> > klp_init_patch_early() fails, that means try_module_get() hasn't been
> > done, so klp_free_patch_finish() will wrongly do a module_put().
> 
> Ugh, good point and thank you for catching that. Another problem with the
> current patch is that we'll call kobject_put() on the patch even if we
> never call kobject_init on the patch due to patch->objs being NULL.
> 
> Perhaps we should pull try_module_get() and the NULL check for patch->objs
> out of klp_init_patch_early()? It feels a bit more intuitive to me if
> klp_init_patch_early() were only be responsible for initializing kobjects
> for the patch and its objects / funcs anyways.
> 
> Testing it locally seems to work fine. Let me know if this sounds
> reasonable to you, and I'll send out a v2 patch with the fixes to both the
> patch description, and logic.

Sounds reasonable to me.  Thanks.

-- 
Josh

Re: [PATCH] livepatch: Fix leak on klp_init_patch_early failure path

[Petr Mladek]

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.
 */

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.

Adding Greg into Cc.

Best Regards,
Petr

> For example, if we added the following object entry to the sample livepatch
> code, it would cause us to leak the vmlinux `klp_object`, and its `struct
> klp_func` which updates `cmdline_proc_show`:
> 
> ```
> static struct klp_object objs[] = {
>         {
>                 .name = "kvm",
>         }, { }
> };
> ```
> 
> Without this change, if we enable `CONFIG_DEBUG_KOBJECT` and try to `kpatch
> load livepatch-sample.ko`, we don't observe the kobjects being released
> (though of course we do observe `insmod` failing to insert the module).
> With the change, we do observe that the `kobject` for the patch and its
> `vmlinux` object are released.
> 
> Signed-off-by: David Vernet <void@manifault.com>
> ---
>  kernel/livepatch/core.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index 335d988bd811..16e96836a825 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -1052,10 +1052,7 @@ int klp_enable_patch(struct klp_patch *patch)
>  	}
>  
>  	ret = klp_init_patch_early(patch);
> -	if (ret) {
> -		mutex_unlock(&klp_mutex);
> -		return ret;
> -	}
> +		goto err;
>  
>  	ret = klp_init_patch(patch);
>  	if (ret)
> -- 
> 2.30.2

Re: [PATCH] livepatch: Fix leak on klp_init_patch_early failure path

[Miroslav Benes]

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

Re: [PATCH] livepatch: Fix leak on klp_init_patch_early failure path

[David Vernet]

Miroslav Benes <mbenes@suse.cz> wrote on Tue [2021-Dec-14 10:17:08 +0100]:
> It would help to share warning outputs (or whatever) from DEBUG_KOBJECTS.

This is the output when running kpatch load livepatch-sample.ko if an extra
'struct klp_obj' entry is added that has a name but no funcs:

Without patch:

[   67.285762] livepatch_sample: tainting kernel with TAINT_LIVEPATCH
[   67.286107] kobject: 'livepatch_sample' (00000000cf89f7b6): kobject_add_internal: parent: 'module', set: 'module'
[   67.286113] kobject: 'holders' (00000000858b03bf): kobject_add_internal: parent: 'livepatch_sample', set: '<NULL>'
[   67.286126] kobject: 'notes' (00000000f2a3a4ce): kobject_add_internal: parent: 'livepatch_sample', set: '<NULL>'
[   67.297856] kobject: 'holders' (00000000858b03bf): kobject_cleanup, parent 00000000cf89f7b6
[   67.297859] kobject: 'holders' (00000000858b03bf): auto cleanup kobject_del
[   67.297861] kobject: 'holders' (00000000858b03bf): calling ktype release
[   67.297862] kobject: (00000000858b03bf): dynamic_kobj_release
[   67.297863] kobject: 'holders': free name
[   67.297865] kobject: 'notes' (00000000f2a3a4ce): kobject_cleanup, parent 00000000cf89f7b6
[   67.297866] kobject: 'notes' (00000000f2a3a4ce): auto cleanup kobject_del
[   67.297867] kobject: 'notes' (00000000f2a3a4ce): calling ktype release
[   67.297868] kobject: (00000000f2a3a4ce): dynamic_kobj_release
[   67.297869] kobject: 'notes': free name
[   67.297874] kobject: 'livepatch_sample' (00000000cf89f7b6): kobject_cleanup, parent 000000002555fa2d
[   67.297876] kobject: 'livepatch_sample' (00000000cf89f7b6): auto cleanup kobject_del
[   67.297877] kobject: 'livepatch_sample' (00000000cf89f7b6): calling ktype release
[   67.297878] kobject: 'livepatch_sample': free name
[   99.445938] kobject: '0:40' (000000002a98d11d): kobject_add_internal: parent: 'bdi', set: 'devices'
[   99.445954] kobject: '0:40' (000000002a98d11d): kobject_uevent_env
[   99.445957] kobject: '0:40' (000000002a98d11d): fill_kobj_path: path = '/devices/virtual/bdi/0:40'

With patch:

[  162.275251] livepatch_sample: tainting kernel with TAINT_LIVEPATCH
[  162.275985] kobject: 'livepatch_sample' (00000000e688ee30): kobject_add_internal: parent: 'module', set: 'module'
[  162.275993] kobject: 'holders' (000000004eee7860): kobject_add_internal: parent: 'livepatch_sample', set: '<NULL>'
[  162.276010] kobject: 'notes' (00000000c4f390ab): kobject_add_internal: parent: 'livepatch_sample', set: '<NULL>'
[  162.276028] kobject: '(null)' (000000003acccf72): kobject_cleanup, parent 0000000000000000
[  162.276031] kobject: '(null)' (000000003acccf72): calling ktype release
[  162.276033] kobject: '(null)' (00000000aeae6326): kobject_cleanup, parent 0000000000000000
[  162.276035] kobject: '(null)' (00000000aeae6326): calling ktype release
[  162.276037] kobject: '(null)' (0000000093b68297): kobject_cleanup, parent 0000000000000000
[  162.276039] kobject: '(null)' (0000000093b68297): calling ktype release
[  162.294063] kobject: 'holders' (000000004eee7860): kobject_cleanup, parent 00000000e688ee30
[  162.294070] kobject: 'holders' (000000004eee7860): auto cleanup kobject_del
[  162.294074] kobject: 'holders' (000000004eee7860): calling ktype release
[  162.294078] kobject: (000000004eee7860): dynamic_kobj_release
[  162.294081] kobject: 'holders': free name
[  162.294086] kobject: 'notes' (00000000c4f390ab): kobject_cleanup, parent 00000000e688ee30
[  162.294090] kobject: 'notes' (00000000c4f390ab): auto cleanup kobject_del
[  162.294094] kobject: 'notes' (00000000c4f390ab): calling ktype release
[  162.294097] kobject: (00000000c4f390ab): dynamic_kobj_release
[  162.294100] kobject: 'notes': free name
[  162.294114] kobject: 'livepatch_sample' (00000000e688ee30): kobject_cleanup, parent 00000000f9317c72
[  162.294118] kobject: 'livepatch_sample' (00000000e688ee30): auto cleanup kobject_del
[  162.294123] kobject: 'livepatch_sample' (00000000e688ee30): calling ktype release
[  162.294126] kobject: 'livepatch_sample': free name

The extra lines are of course the kobject: '(null)' entries, for which we
*don't* see auto cleanup kobject_del being called. So it seems that what's
there now is probably not actually leaking memory, and the question is
really whether or not the documentation in kobject.c is the source of truth
(i.e. whether the code needs to be "fixed" to honor the API contract).

> I think that this might be, once again, a false positive. We use kobjects 
> differently than what the kobject implementation and its documentation 
> assume.

I'm curious to hear what Greg says. As Petr pointed out, it seems that the
documentation for kobjects is inconsistent. If we're going by the function
comment header in kobject.c then what we have now should probably be
considered a bug? If we're going by what's in
Documentation/core-api/kobject.rst, I think what we have now is correct.

I do think it's a bit of a leaky abstraction to assume that the
implementation doesn't allocate anything, but I also see Petr's point that
it could be useful to make it explicit that kobject_init() doesn't allocate
anything, and instead just affords callers the option of using
kobject_put() if they want to the objects' destructors to be invoked.

Either way, we should fix the documentation to be consistent, which I'm
happy to do in another patch.

> 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.

The patch (though it needs to be corrected in its current form, as Josh
pointed out) does already address this for the klp_funcs and klp_objects.
The 'err' label invokes klp_free_patch_start(), which eventually invokes
__klp_free_objects(), which itself invokes __klp_free_funcs().

Regards,
David

Re: [PATCH] livepatch: Fix leak on klp_init_patch_early failure path

[Greg Kroah-Hartman]

On Tue, Dec 14, 2021 at 07:26:33AM -0800, David Vernet wrote:
> Miroslav Benes <mbenes@suse.cz> wrote on Tue [2021-Dec-14 10:17:08 +0100]:
> > It would help to share warning outputs (or whatever) from DEBUG_KOBJECTS.
> 
> This is the output when running kpatch load livepatch-sample.ko if an extra
> 'struct klp_obj' entry is added that has a name but no funcs:
> 
> Without patch:
> 
> [   67.285762] livepatch_sample: tainting kernel with TAINT_LIVEPATCH
> [   67.286107] kobject: 'livepatch_sample' (00000000cf89f7b6): kobject_add_internal: parent: 'module', set: 'module'
> [   67.286113] kobject: 'holders' (00000000858b03bf): kobject_add_internal: parent: 'livepatch_sample', set: '<NULL>'
> [   67.286126] kobject: 'notes' (00000000f2a3a4ce): kobject_add_internal: parent: 'livepatch_sample', set: '<NULL>'
> [   67.297856] kobject: 'holders' (00000000858b03bf): kobject_cleanup, parent 00000000cf89f7b6
> [   67.297859] kobject: 'holders' (00000000858b03bf): auto cleanup kobject_del
> [   67.297861] kobject: 'holders' (00000000858b03bf): calling ktype release
> [   67.297862] kobject: (00000000858b03bf): dynamic_kobj_release
> [   67.297863] kobject: 'holders': free name
> [   67.297865] kobject: 'notes' (00000000f2a3a4ce): kobject_cleanup, parent 00000000cf89f7b6
> [   67.297866] kobject: 'notes' (00000000f2a3a4ce): auto cleanup kobject_del
> [   67.297867] kobject: 'notes' (00000000f2a3a4ce): calling ktype release
> [   67.297868] kobject: (00000000f2a3a4ce): dynamic_kobj_release
> [   67.297869] kobject: 'notes': free name
> [   67.297874] kobject: 'livepatch_sample' (00000000cf89f7b6): kobject_cleanup, parent 000000002555fa2d
> [   67.297876] kobject: 'livepatch_sample' (00000000cf89f7b6): auto cleanup kobject_del
> [   67.297877] kobject: 'livepatch_sample' (00000000cf89f7b6): calling ktype release
> [   67.297878] kobject: 'livepatch_sample': free name
> [   99.445938] kobject: '0:40' (000000002a98d11d): kobject_add_internal: parent: 'bdi', set: 'devices'
> [   99.445954] kobject: '0:40' (000000002a98d11d): kobject_uevent_env
> [   99.445957] kobject: '0:40' (000000002a98d11d): fill_kobj_path: path = '/devices/virtual/bdi/0:40'
> 
> With patch:
> 
> [  162.275251] livepatch_sample: tainting kernel with TAINT_LIVEPATCH
> [  162.275985] kobject: 'livepatch_sample' (00000000e688ee30): kobject_add_internal: parent: 'module', set: 'module'
> [  162.275993] kobject: 'holders' (000000004eee7860): kobject_add_internal: parent: 'livepatch_sample', set: '<NULL>'
> [  162.276010] kobject: 'notes' (00000000c4f390ab): kobject_add_internal: parent: 'livepatch_sample', set: '<NULL>'
> [  162.276028] kobject: '(null)' (000000003acccf72): kobject_cleanup, parent 0000000000000000
> [  162.276031] kobject: '(null)' (000000003acccf72): calling ktype release
> [  162.276033] kobject: '(null)' (00000000aeae6326): kobject_cleanup, parent 0000000000000000
> [  162.276035] kobject: '(null)' (00000000aeae6326): calling ktype release
> [  162.276037] kobject: '(null)' (0000000093b68297): kobject_cleanup, parent 0000000000000000
> [  162.276039] kobject: '(null)' (0000000093b68297): calling ktype release
> [  162.294063] kobject: 'holders' (000000004eee7860): kobject_cleanup, parent 00000000e688ee30
> [  162.294070] kobject: 'holders' (000000004eee7860): auto cleanup kobject_del
> [  162.294074] kobject: 'holders' (000000004eee7860): calling ktype release
> [  162.294078] kobject: (000000004eee7860): dynamic_kobj_release
> [  162.294081] kobject: 'holders': free name
> [  162.294086] kobject: 'notes' (00000000c4f390ab): kobject_cleanup, parent 00000000e688ee30
> [  162.294090] kobject: 'notes' (00000000c4f390ab): auto cleanup kobject_del
> [  162.294094] kobject: 'notes' (00000000c4f390ab): calling ktype release
> [  162.294097] kobject: (00000000c4f390ab): dynamic_kobj_release
> [  162.294100] kobject: 'notes': free name
> [  162.294114] kobject: 'livepatch_sample' (00000000e688ee30): kobject_cleanup, parent 00000000f9317c72
> [  162.294118] kobject: 'livepatch_sample' (00000000e688ee30): auto cleanup kobject_del
> [  162.294123] kobject: 'livepatch_sample' (00000000e688ee30): calling ktype release
> [  162.294126] kobject: 'livepatch_sample': free name
> 
> The extra lines are of course the kobject: '(null)' entries, for which we
> *don't* see auto cleanup kobject_del being called. So it seems that what's
> there now is probably not actually leaking memory, and the question is
> really whether or not the documentation in kobject.c is the source of truth
> (i.e. whether the code needs to be "fixed" to honor the API contract).
> 
> > I think that this might be, once again, a false positive. We use kobjects 
> > differently than what the kobject implementation and its documentation 
> > assume.
> 
> I'm curious to hear what Greg says. As Petr pointed out, it seems that the
> documentation for kobjects is inconsistent. If we're going by the function
> comment header in kobject.c then what we have now should probably be
> considered a bug? If we're going by what's in
> Documentation/core-api/kobject.rst, I think what we have now is correct.

I do not understand, what is the problem here?  I have been ignoring
this thread :)

> I do think it's a bit of a leaky abstraction to assume that the
> implementation doesn't allocate anything, but I also see Petr's point that
> it could be useful to make it explicit that kobject_init() doesn't allocate
> anything, and instead just affords callers the option of using
> kobject_put() if they want to the objects' destructors to be invoked.

kobject_init() does allocate things internally, where does it say it
does not?  What is trying to be "fixed" here?

confused,

greg k-h

Re: [PATCH] livepatch: Fix leak on klp_init_patch_early failure path

[Petr Mladek]

On Tue 2021-12-14 16:50:15, Greg Kroah-Hartman wrote:
> 
> kobject_init() does allocate things internally, where does it say it
> does not?  What is trying to be "fixed" here?

Could you please show where things are allocated in kobject_init()?
I do not see it in the code!

It looks to me like a cargo cult claim to me.

Documentation/core-api/kobject.rst says:

   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().

kobject_add() makes perfect sense because it copies the name, takes
reference to the parent, etc.

kobject_init() just initializes the structure members and nothing else.

Best Regards,
Petr

Re: [PATCH] livepatch: Fix leak on klp_init_patch_early failure path

[Greg Kroah-Hartman]

On Wed, Dec 15, 2021 at 09:19:59AM +0100, Petr Mladek wrote:
> On Tue 2021-12-14 16:50:15, Greg Kroah-Hartman wrote:
> > 
> > kobject_init() does allocate things internally, where does it say it
> > does not?  What is trying to be "fixed" here?
> 
> Could you please show where things are allocated in kobject_init()?
> I do not see it in the code!
> 
> It looks to me like a cargo cult claim to me.

Hm, I thought I saw it yesterday when I reviewed the code.  Let me look
again...

> Documentation/core-api/kobject.rst says:
> 
>    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().
> 
> kobject_add() makes perfect sense because it copies the name, takes
> reference to the parent, etc.
> 
> kobject_init() just initializes the structure members and nothing else.

Now it does.  In the past, I think we did create some memory.  I know
when we hook debugobjects up to kobjects (there's an external patch for
that floating around somewhere), that is one reason to keep the
kobject_put() rule, and there might have been other reasons in the past
20+ years as well.

So yes, while you are correct today, the "normal" reference counted
object model patern is "after the object is initialized, it MUST only be
freed by handling its reference count."  So let's stick to that rule for
now.

If you want, I can put some code in the kobject_init() logic to force
this to be the case if it bothers you :)

thanks,

greg k-h

Re: [PATCH] livepatch: Fix leak on klp_init_patch_early failure path

[Petr Mladek]

On Wed 2021-12-15 16:35:24, Greg Kroah-Hartman wrote:
> On Wed, Dec 15, 2021 at 09:19:59AM +0100, Petr Mladek wrote:
> > On Tue 2021-12-14 16:50:15, Greg Kroah-Hartman wrote:
> > > 
> > > kobject_init() does allocate things internally, where does it say it
> > > does not?  What is trying to be "fixed" here?
> > 
> > Could you please show where things are allocated in kobject_init()?
> > I do not see it in the code!
> > 
> > It looks to me like a cargo cult claim to me.
> 
> Hm, I thought I saw it yesterday when I reviewed the code.  Let me look
> again...
> 
> > Documentation/core-api/kobject.rst says:
> > 
> >    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().
> > 
> > kobject_add() makes perfect sense because it copies the name, takes
> > reference to the parent, etc.
> > 
> > kobject_init() just initializes the structure members and nothing else.
> 
> Now it does.  In the past, I think we did create some memory.  I know
> when we hook debugobjects up to kobjects (there's an external patch for
> that floating around somewhere), that is one reason to keep the
> kobject_put() rule, and there might have been other reasons in the past
> 20+ years as well.
> 
> So yes, while you are correct today, the "normal" reference counted
> object model patern is "after the object is initialized, it MUST only be
> freed by handling its reference count."  So let's stick to that rule for
> now.

Good point.


> If you want, I can put some code in the kobject_init() logic to force
> this to be the case if it bothers you :)

I actually know about one case where this might be very useful.

There is the problem with kobject lifetime and module removal.
module is removed after mod->exit() callback finishes. But some
kobject release() callbacks might be delayed, especillay when
CONFIG_DEBUG_KOBJECT_RELEASE is enabled.

I have proposed there a solution where kobject_add_internal() takes reference
on the module. It would make sure that the module will stay in the
memory until the release callbacks is called, see
https://lore.kernel.org/all/Ya84O2%2FnYCyNb%2Ffp@alley/

But kobject_add_internal() is not the right place. The reference on
the module should be taken already in kobject_init() because the
release callbacks might be used after this point.

Best Regards,
Petr

Re: [PATCH] livepatch: Fix leak on klp_init_patch_early failure path

[Greg Kroah-Hartman]

On Thu, Dec 16, 2021 at 03:14:38PM +0100, Petr Mladek wrote:
> On Wed 2021-12-15 16:35:24, Greg Kroah-Hartman wrote:
> > On Wed, Dec 15, 2021 at 09:19:59AM +0100, Petr Mladek wrote:
> > > On Tue 2021-12-14 16:50:15, Greg Kroah-Hartman wrote:
> > > > 
> > > > kobject_init() does allocate things internally, where does it say it
> > > > does not?  What is trying to be "fixed" here?
> > > 
> > > Could you please show where things are allocated in kobject_init()?
> > > I do not see it in the code!
> > > 
> > > It looks to me like a cargo cult claim to me.
> > 
> > Hm, I thought I saw it yesterday when I reviewed the code.  Let me look
> > again...
> > 
> > > Documentation/core-api/kobject.rst says:
> > > 
> > >    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().
> > > 
> > > kobject_add() makes perfect sense because it copies the name, takes
> > > reference to the parent, etc.
> > > 
> > > kobject_init() just initializes the structure members and nothing else.
> > 
> > Now it does.  In the past, I think we did create some memory.  I know
> > when we hook debugobjects up to kobjects (there's an external patch for
> > that floating around somewhere), that is one reason to keep the
> > kobject_put() rule, and there might have been other reasons in the past
> > 20+ years as well.
> > 
> > So yes, while you are correct today, the "normal" reference counted
> > object model patern is "after the object is initialized, it MUST only be
> > freed by handling its reference count."  So let's stick to that rule for
> > now.
> 
> Good point.
> 
> 
> > If you want, I can put some code in the kobject_init() logic to force
> > this to be the case if it bothers you :)
> 
> I actually know about one case where this might be very useful.
> 
> There is the problem with kobject lifetime and module removal.
> module is removed after mod->exit() callback finishes. But some
> kobject release() callbacks might be delayed, especillay when
> CONFIG_DEBUG_KOBJECT_RELEASE is enabled.

Ick, modules and kobjects, just say no :)

> I have proposed there a solution where kobject_add_internal() takes reference
> on the module. It would make sure that the module will stay in the
> memory until the release callbacks is called, see
> https://lore.kernel.org/all/Ya84O2%2FnYCyNb%2Ffp@alley/

I don't want to add module pointers to kobjects.

kobjects are data.  modules are code.  Module references control code
lifespans.  Kobject references control data lifespans.  They are
different, so let us never mix the two please.

Yes, release callbacks are code, but if you really need to worry about
your release callback being unloaded, then just refuse to unload your
module until your data is all released!

The huge majority of kobject users never touch them directly, they use
the driver model, which should not have this issue.  Only code that
wants to touch kobjects in the "raw" have problems like this, and if you
want to mess with them at that level, you can handle the release data
issue.

Then there's the issue of static kobjects that are defined in code
(modules), but are treated as a reference count of data.  ick...

thanks,

greg k-h

Re: [PATCH] livepatch: Fix leak on klp_init_patch_early failure path

[Petr Mladek]

On Thu 2021-12-16 15:35:42, Greg Kroah-Hartman wrote:
> On Thu, Dec 16, 2021 at 03:14:38PM +0100, Petr Mladek wrote:
> > There is the problem with kobject lifetime and module removal.
> > module is removed after mod->exit() callback finishes. But some
> > kobject release() callbacks might be delayed, especillay when
> > CONFIG_DEBUG_KOBJECT_RELEASE is enabled.
> 
> Ick, modules and kobjects, just say no :)

I will try to persuade you that it is not that uncommon scenario,
see below.


> > I have proposed there a solution where kobject_add_internal() takes reference
> > on the module. It would make sure that the module will stay in the
> > memory until the release callbacks is called, see
> > https://lore.kernel.org/all/Ya84O2%2FnYCyNb%2Ffp@alley/
> 
> I don't want to add module pointers to kobjects.
> 
> kobjects are data.  modules are code.  Module references control code
> lifespans.  Kobject references control data lifespans.  They are
> different, so let us never mix the two please.

I do not undestand this argument. The data are useless without the
code. The code is needed to remove the data. Therefore the code should
stay until the data are released.

I am talking about data using statically defined kobj_type in modules.


> Yes, release callbacks are code, but if you really need to worry about
> your release callback being unloaded, then just refuse to unload your
> module until your data is all released!

This is exactly what I am proposing. If the kobject release callbacks
are defined in the module then the module should stay as long as
they are needed.


> The huge majority of kobject users never touch them directly, they use
> the driver model, which should not have this issue.  Only code that
> wants to touch kobjects in the "raw" have problems like this, and if you
> want to mess with them at that level, you can handle the release data
> issue.

There seems to be 14 simple modules that define static strcut
kobj_type:

$> for file in `git grep "static struct kobj_type" | cut -d : -f1 | sort -u` ; \
       do grep -q "module_init" $file && echo $file ; \
   done
arch/sh/kernel/cpu/sh4/sq.c
drivers/block/pktcdvd.c
drivers/firmware/dmi-sysfs.c
drivers/firmware/efi/efivars.c
drivers/firmware/qemu_fw_cfg.c
drivers/net/bonding/bond_main.c
drivers/net/ethernet/ibm/ibmveth.c
drivers/parisc/pdc_stable.c
drivers/platform/x86/dell/dell-wmi-sysman/sysman.c
drivers/platform/x86/intel/uncore-frequency.c
drivers/platform/x86/uv_sysfs.c
drivers/uio/uio.c
kernel/livepatch/core.c
samples/kobject/kset-example.c


The other struct kobj_type are fined in 81 source files:

$> for file in `git grep "static struct kobj_type" | cut -d : -f1 | sort -u` ;  \
      do grep -q "module_init" $file || echo $file ; \
   done | wc -l
81


I believe that most of them might be compiled as modules as well.
There are many non-trivial drivers and file systes. From just a quick
look:

drivers/infiniband/hw/mlx4/sysfs.c
fs/btrfs/sysfs.c
fs/ext4/sysfs.c


Well, we should probably discuss this in the original thread
https://lore.kernel.org/r/20211129034509.2646872-3-ming.lei@redhat.com

Best Regards,
Petr

Re: [PATCH] livepatch: Fix leak on klp_init_patch_early failure path

[David Vernet]

Petr Mladek <pmladek@suse.com> wrote on Thu [2021-Dec-16 15:14:38 +0100]:
> > Now it does.  In the past, I think we did create some memory.  I know
> > when we hook debugobjects up to kobjects (there's an external patch for
> > that floating around somewhere), that is one reason to keep the
> > kobject_put() rule, and there might have been other reasons in the past
> > 20+ years as well.
> > 
> > So yes, while you are correct today, the "normal" reference counted
> > object model patern is "after the object is initialized, it MUST only be
> > freed by handling its reference count."  So let's stick to that rule for
> > now.
> 
> Good point.

Thanks for the discussion all. I think we've landed on the fact that this
is a refcounting bug that needs to be fixed, but isn't a leak in how the
kobject implementation exists today.

Petr - are you OK with me sending out a v3 of the patch with the following
changes:
  - The patch description is updated to not claim that a leak is being
    fixed, but rather that a kobject reference counting bug is being fixed.
  - All of the NULL checking in klp_init_patch_early() is brought into
    klp_enable_patch(), and klp_init_patch_early() is updated to be void,
    per Josh's suggestion. This would address the refcounting issue and IMO
    also simplifies and improves the code. I know you were onboard with
    moving try_module_get() into klp_enable_patch(), but I don't think we
    ever resolved the point about moving the rest of the NULL checking out
    as well.

Regards,
David

Re: [PATCH] livepatch: Fix leak on klp_init_patch_early failure path

[Petr Mladek]

On Thu 2021-12-16 07:14:27, David Vernet wrote:
> Petr Mladek <pmladek@suse.com> wrote on Thu [2021-Dec-16 15:14:38 +0100]:
> > > Now it does.  In the past, I think we did create some memory.  I know
> > > when we hook debugobjects up to kobjects (there's an external patch for
> > > that floating around somewhere), that is one reason to keep the
> > > kobject_put() rule, and there might have been other reasons in the past
> > > 20+ years as well.
> > > 
> > > So yes, while you are correct today, the "normal" reference counted
> > > object model patern is "after the object is initialized, it MUST only be
> > > freed by handling its reference count."  So let's stick to that rule for
> > > now.
> > 
> > Good point.
> 
> Thanks for the discussion all. I think we've landed on the fact that this
> is a refcounting bug that needs to be fixed, but isn't a leak in how the
> kobject implementation exists today.
> 
> Petr - are you OK with me sending out a v3 of the patch with the following
> changes:
>   - The patch description is updated to not claim that a leak is being
>     fixed, but rather that a kobject reference counting bug is being fixed.
>   - All of the NULL checking in klp_init_patch_early() is brought into
>     klp_enable_patch(), and klp_init_patch_early() is updated to be void,
>     per Josh's suggestion. This would address the refcounting issue and IMO
>     also simplifies and improves the code. I know you were onboard with
>     moving try_module_get() into klp_enable_patch(), but I don't think we
>     ever resolved the point about moving the rest of the NULL checking out
>     as well.

Just for record. I have answered this in the other thread where it was
discussed, see https://lore.kernel.org/r/YbyV7nsLXbQ6/44S@alley

Best Regards,
Petr

Re: [PATCH] livepatch: Fix leak on klp_init_patch_early failure path

[Miroslav Benes]

On Tue, 14 Dec 2021, David Vernet wrote:

> Miroslav Benes <mbenes@suse.cz> wrote on Tue [2021-Dec-14 10:17:08 +0100]:
> > It would help to share warning outputs (or whatever) from DEBUG_KOBJECTS.
> 
> This is the output when running kpatch load livepatch-sample.ko if an extra
> 'struct klp_obj' entry is added that has a name but no funcs:
> 
> Without patch:
> 
> [   67.285762] livepatch_sample: tainting kernel with TAINT_LIVEPATCH
> [   67.286107] kobject: 'livepatch_sample' (00000000cf89f7b6): kobject_add_internal: parent: 'module', set: 'module'
> [   67.286113] kobject: 'holders' (00000000858b03bf): kobject_add_internal: parent: 'livepatch_sample', set: '<NULL>'
> [   67.286126] kobject: 'notes' (00000000f2a3a4ce): kobject_add_internal: parent: 'livepatch_sample', set: '<NULL>'
> [   67.297856] kobject: 'holders' (00000000858b03bf): kobject_cleanup, parent 00000000cf89f7b6
> [   67.297859] kobject: 'holders' (00000000858b03bf): auto cleanup kobject_del
> [   67.297861] kobject: 'holders' (00000000858b03bf): calling ktype release
> [   67.297862] kobject: (00000000858b03bf): dynamic_kobj_release
> [   67.297863] kobject: 'holders': free name
> [   67.297865] kobject: 'notes' (00000000f2a3a4ce): kobject_cleanup, parent 00000000cf89f7b6
> [   67.297866] kobject: 'notes' (00000000f2a3a4ce): auto cleanup kobject_del
> [   67.297867] kobject: 'notes' (00000000f2a3a4ce): calling ktype release
> [   67.297868] kobject: (00000000f2a3a4ce): dynamic_kobj_release
> [   67.297869] kobject: 'notes': free name
> [   67.297874] kobject: 'livepatch_sample' (00000000cf89f7b6): kobject_cleanup, parent 000000002555fa2d
> [   67.297876] kobject: 'livepatch_sample' (00000000cf89f7b6): auto cleanup kobject_del
> [   67.297877] kobject: 'livepatch_sample' (00000000cf89f7b6): calling ktype release
> [   67.297878] kobject: 'livepatch_sample': free name
> [   99.445938] kobject: '0:40' (000000002a98d11d): kobject_add_internal: parent: 'bdi', set: 'devices'
> [   99.445954] kobject: '0:40' (000000002a98d11d): kobject_uevent_env
> [   99.445957] kobject: '0:40' (000000002a98d11d): fill_kobj_path: path = '/devices/virtual/bdi/0:40'
> 
> With patch:
> 
> [  162.275251] livepatch_sample: tainting kernel with TAINT_LIVEPATCH
> [  162.275985] kobject: 'livepatch_sample' (00000000e688ee30): kobject_add_internal: parent: 'module', set: 'module'
> [  162.275993] kobject: 'holders' (000000004eee7860): kobject_add_internal: parent: 'livepatch_sample', set: '<NULL>'
> [  162.276010] kobject: 'notes' (00000000c4f390ab): kobject_add_internal: parent: 'livepatch_sample', set: '<NULL>'
> [  162.276028] kobject: '(null)' (000000003acccf72): kobject_cleanup, parent 0000000000000000
> [  162.276031] kobject: '(null)' (000000003acccf72): calling ktype release
> [  162.276033] kobject: '(null)' (00000000aeae6326): kobject_cleanup, parent 0000000000000000
> [  162.276035] kobject: '(null)' (00000000aeae6326): calling ktype release
> [  162.276037] kobject: '(null)' (0000000093b68297): kobject_cleanup, parent 0000000000000000
> [  162.276039] kobject: '(null)' (0000000093b68297): calling ktype release
> [  162.294063] kobject: 'holders' (000000004eee7860): kobject_cleanup, parent 00000000e688ee30
> [  162.294070] kobject: 'holders' (000000004eee7860): auto cleanup kobject_del
> [  162.294074] kobject: 'holders' (000000004eee7860): calling ktype release
> [  162.294078] kobject: (000000004eee7860): dynamic_kobj_release
> [  162.294081] kobject: 'holders': free name
> [  162.294086] kobject: 'notes' (00000000c4f390ab): kobject_cleanup, parent 00000000e688ee30
> [  162.294090] kobject: 'notes' (00000000c4f390ab): auto cleanup kobject_del
> [  162.294094] kobject: 'notes' (00000000c4f390ab): calling ktype release
> [  162.294097] kobject: (00000000c4f390ab): dynamic_kobj_release
> [  162.294100] kobject: 'notes': free name
> [  162.294114] kobject: 'livepatch_sample' (00000000e688ee30): kobject_cleanup, parent 00000000f9317c72
> [  162.294118] kobject: 'livepatch_sample' (00000000e688ee30): auto cleanup kobject_del
> [  162.294123] kobject: 'livepatch_sample' (00000000e688ee30): calling ktype release
> [  162.294126] kobject: 'livepatch_sample': free name
> 
> The extra lines are of course the kobject: '(null)' entries, for which we
> *don't* see auto cleanup kobject_del being called. So it seems that what's
> there now is probably not actually leaking memory, and the question is
> really whether or not the documentation in kobject.c is the source of truth
> (i.e. whether the code needs to be "fixed" to honor the API contract).

Indeed.
 
[...]

> > 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.
> 
> The patch (though it needs to be corrected in its current form, as Josh
> pointed out) does already address this for the klp_funcs and klp_objects.
> The 'err' label invokes klp_free_patch_start(), which eventually invokes
> __klp_free_objects(), which itself invokes __klp_free_funcs().

I was just thinking out loud whether it would make sense in the 
_init_early stage to actually clean up (call kobject_put()) immediately, 
but you are right that the code in klp_enable_patch() and elsewhere is 
already prepared for that and it would not make sense to change the 
approach just because.

Thanks
Miroslav

Re: [PATCH] livepatch: Fix leak on klp_init_patch_early failure path

[Greg Kroah-Hartman]

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

Re: [PATCH] livepatch: Fix leak on klp_init_patch_early failure path

[David Vernet]

Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote on Tue [2021-Dec-14 16:52:41 +0100]:
> You are FORCED TO call kobject_put() after kobject_init() is called.
> Anything else is a bug.

Ack, thanks for confirming, Greg. I'll send out a v2 of the patch that fixes the
leak and addresses the issue that Josh identified.

- David