* [PATCH] livepatch: Fix leak on klp_init_patch_early failure path @ 2021-12-13 19:17 David Vernet 2021-12-13 20:10 ` Josh Poimboeuf 2021-12-14 8:45 ` Petr Mladek 0 siblings, 2 replies; 19+ messages in thread From: David Vernet @ 2021-12-13 19:17 UTC (permalink / raw) To: pmladek, linux-doc, live-patching, linux-kernel, jpoimboe, jikos, mbenes, joe.lawrence, corbet Cc: void, yhs, songliubraving 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 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH] livepatch: Fix leak on klp_init_patch_early failure path 2021-12-13 19:17 [PATCH] livepatch: Fix leak on klp_init_patch_early failure path David Vernet @ 2021-12-13 20:10 ` Josh Poimboeuf 2021-12-13 22:58 ` David Vernet 2021-12-14 8:45 ` Petr Mladek 1 sibling, 1 reply; 19+ messages in thread From: Josh Poimboeuf @ 2021-12-13 20:10 UTC (permalink / raw) To: David Vernet Cc: pmladek, linux-doc, live-patching, linux-kernel, jikos, mbenes, joe.lawrence, corbet, yhs, songliubraving 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] livepatch: Fix leak on klp_init_patch_early failure path 2021-12-13 20:10 ` Josh Poimboeuf @ 2021-12-13 22:58 ` David Vernet 2021-12-13 23:32 ` Song Liu 2021-12-14 3:13 ` Josh Poimboeuf 0 siblings, 2 replies; 19+ messages in thread From: David Vernet @ 2021-12-13 22:58 UTC (permalink / raw) To: Josh Poimboeuf Cc: pmladek, linux-doc, live-patching, linux-kernel, jikos, mbenes, joe.lawrence, corbet, yhs, songliubraving 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] livepatch: Fix leak on klp_init_patch_early failure path 2021-12-13 22:58 ` David Vernet @ 2021-12-13 23:32 ` Song Liu 2021-12-14 3:13 ` Josh Poimboeuf 1 sibling, 0 replies; 19+ messages in thread From: Song Liu @ 2021-12-13 23:32 UTC (permalink / raw) To: David Vernet Cc: Josh Poimboeuf, pmladek, linux-doc, live-patching, LKML, jikos, mbenes, joe.lawrence, corbet, Yonghong Song > 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) ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH] livepatch: Fix leak on klp_init_patch_early failure path 2021-12-13 22:58 ` David Vernet 2021-12-13 23:32 ` Song Liu @ 2021-12-14 3:13 ` Josh Poimboeuf 1 sibling, 0 replies; 19+ messages in thread From: Josh Poimboeuf @ 2021-12-14 3:13 UTC (permalink / raw) To: David Vernet Cc: pmladek, linux-doc, live-patching, linux-kernel, jikos, mbenes, joe.lawrence, corbet, yhs, songliubraving 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] livepatch: Fix leak on klp_init_patch_early failure path 2021-12-13 19:17 [PATCH] livepatch: Fix leak on klp_init_patch_early failure path David Vernet 2021-12-13 20:10 ` Josh Poimboeuf @ 2021-12-14 8:45 ` Petr Mladek 2021-12-14 9:17 ` Miroslav Benes 2021-12-14 15:52 ` Greg Kroah-Hartman 1 sibling, 2 replies; 19+ messages in thread From: Petr Mladek @ 2021-12-14 8:45 UTC (permalink / raw) To: David Vernet Cc: linux-doc, live-patching, linux-kernel, jpoimboe, jikos, mbenes, joe.lawrence, corbet, yhs, songliubraving, Greg Kroah-Hartman 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] livepatch: Fix leak on klp_init_patch_early failure path 2021-12-14 8:45 ` Petr Mladek @ 2021-12-14 9:17 ` Miroslav Benes 2021-12-14 15:26 ` David Vernet 2021-12-14 15:52 ` Greg Kroah-Hartman 1 sibling, 1 reply; 19+ messages in thread From: Miroslav Benes @ 2021-12-14 9:17 UTC (permalink / raw) To: Petr Mladek Cc: David Vernet, linux-doc, live-patching, linux-kernel, jpoimboe, jikos, joe.lawrence, corbet, yhs, songliubraving, Greg Kroah-Hartman 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] livepatch: Fix leak on klp_init_patch_early failure path 2021-12-14 9:17 ` Miroslav Benes @ 2021-12-14 15:26 ` David Vernet 2021-12-14 15:50 ` Greg Kroah-Hartman 2021-12-15 8:33 ` Miroslav Benes 0 siblings, 2 replies; 19+ messages in thread From: David Vernet @ 2021-12-14 15:26 UTC (permalink / raw) To: Miroslav Benes Cc: Petr Mladek, linux-doc, live-patching, linux-kernel, jpoimboe, jikos, joe.lawrence, corbet, yhs, songliubraving, Greg Kroah-Hartman 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] livepatch: Fix leak on klp_init_patch_early failure path 2021-12-14 15:26 ` David Vernet @ 2021-12-14 15:50 ` Greg Kroah-Hartman 2021-12-15 8:19 ` Petr Mladek 2021-12-15 8:33 ` Miroslav Benes 1 sibling, 1 reply; 19+ messages in thread From: Greg Kroah-Hartman @ 2021-12-14 15:50 UTC (permalink / raw) To: David Vernet Cc: Miroslav Benes, Petr Mladek, linux-doc, live-patching, linux-kernel, jpoimboe, jikos, joe.lawrence, corbet, yhs, songliubraving 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] livepatch: Fix leak on klp_init_patch_early failure path 2021-12-14 15:50 ` Greg Kroah-Hartman @ 2021-12-15 8:19 ` Petr Mladek 2021-12-15 15:35 ` Greg Kroah-Hartman 0 siblings, 1 reply; 19+ messages in thread From: Petr Mladek @ 2021-12-15 8:19 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: David Vernet, Miroslav Benes, linux-doc, live-patching, linux-kernel, jpoimboe, jikos, joe.lawrence, corbet, yhs, songliubraving 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] livepatch: Fix leak on klp_init_patch_early failure path 2021-12-15 8:19 ` Petr Mladek @ 2021-12-15 15:35 ` Greg Kroah-Hartman 2021-12-16 14:14 ` Petr Mladek 0 siblings, 1 reply; 19+ messages in thread From: Greg Kroah-Hartman @ 2021-12-15 15:35 UTC (permalink / raw) To: Petr Mladek Cc: David Vernet, Miroslav Benes, linux-doc, live-patching, linux-kernel, jpoimboe, jikos, joe.lawrence, corbet, yhs, songliubraving 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] livepatch: Fix leak on klp_init_patch_early failure path 2021-12-15 15:35 ` Greg Kroah-Hartman @ 2021-12-16 14:14 ` Petr Mladek 2021-12-16 14:35 ` Greg Kroah-Hartman 2021-12-16 15:14 ` David Vernet 0 siblings, 2 replies; 19+ messages in thread From: Petr Mladek @ 2021-12-16 14:14 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: David Vernet, Miroslav Benes, linux-doc, live-patching, linux-kernel, jpoimboe, jikos, joe.lawrence, corbet, yhs, songliubraving 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] livepatch: Fix leak on klp_init_patch_early failure path 2021-12-16 14:14 ` Petr Mladek @ 2021-12-16 14:35 ` Greg Kroah-Hartman 2021-12-17 13:10 ` Petr Mladek 2021-12-16 15:14 ` David Vernet 1 sibling, 1 reply; 19+ messages in thread From: Greg Kroah-Hartman @ 2021-12-16 14:35 UTC (permalink / raw) To: Petr Mladek Cc: David Vernet, Miroslav Benes, linux-doc, live-patching, linux-kernel, jpoimboe, jikos, joe.lawrence, corbet, yhs, songliubraving 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] livepatch: Fix leak on klp_init_patch_early failure path 2021-12-16 14:35 ` Greg Kroah-Hartman @ 2021-12-17 13:10 ` Petr Mladek 0 siblings, 0 replies; 19+ messages in thread From: Petr Mladek @ 2021-12-17 13:10 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: David Vernet, Miroslav Benes, linux-doc, live-patching, linux-kernel, jpoimboe, jikos, joe.lawrence, corbet, yhs, songliubraving, Ming Lei 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] livepatch: Fix leak on klp_init_patch_early failure path 2021-12-16 14:14 ` Petr Mladek 2021-12-16 14:35 ` Greg Kroah-Hartman @ 2021-12-16 15:14 ` David Vernet 2021-12-17 13:53 ` Petr Mladek 1 sibling, 1 reply; 19+ messages in thread From: David Vernet @ 2021-12-16 15:14 UTC (permalink / raw) To: Petr Mladek Cc: Greg Kroah-Hartman, Miroslav Benes, linux-doc, live-patching, linux-kernel, jpoimboe, jikos, joe.lawrence, corbet, yhs, songliubraving 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] livepatch: Fix leak on klp_init_patch_early failure path 2021-12-16 15:14 ` David Vernet @ 2021-12-17 13:53 ` Petr Mladek 0 siblings, 0 replies; 19+ messages in thread From: Petr Mladek @ 2021-12-17 13:53 UTC (permalink / raw) To: David Vernet Cc: Greg Kroah-Hartman, Miroslav Benes, linux-doc, live-patching, linux-kernel, jpoimboe, jikos, joe.lawrence, corbet, yhs, songliubraving 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] livepatch: Fix leak on klp_init_patch_early failure path 2021-12-14 15:26 ` David Vernet 2021-12-14 15:50 ` Greg Kroah-Hartman @ 2021-12-15 8:33 ` Miroslav Benes 1 sibling, 0 replies; 19+ messages in thread From: Miroslav Benes @ 2021-12-15 8:33 UTC (permalink / raw) To: David Vernet Cc: Petr Mladek, linux-doc, live-patching, linux-kernel, jpoimboe, jikos, joe.lawrence, corbet, yhs, songliubraving, Greg Kroah-Hartman 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] livepatch: Fix leak on klp_init_patch_early failure path 2021-12-14 8:45 ` Petr Mladek 2021-12-14 9:17 ` Miroslav Benes @ 2021-12-14 15:52 ` Greg Kroah-Hartman 2021-12-14 18:26 ` David Vernet 1 sibling, 1 reply; 19+ messages in thread From: Greg Kroah-Hartman @ 2021-12-14 15:52 UTC (permalink / raw) To: Petr Mladek Cc: David Vernet, linux-doc, live-patching, linux-kernel, jpoimboe, jikos, mbenes, joe.lawrence, corbet, yhs, songliubraving 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] livepatch: Fix leak on klp_init_patch_early failure path 2021-12-14 15:52 ` Greg Kroah-Hartman @ 2021-12-14 18:26 ` David Vernet 0 siblings, 0 replies; 19+ messages in thread From: David Vernet @ 2021-12-14 18:26 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Petr Mladek, linux-doc, live-patching, linux-kernel, jpoimboe, jikos, mbenes, joe.lawrence, corbet, yhs, songliubraving 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2021-12-17 13:53 UTC | newest] Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-12-13 19:17 [PATCH] livepatch: Fix leak on klp_init_patch_early failure path David Vernet 2021-12-13 20:10 ` Josh Poimboeuf 2021-12-13 22:58 ` David Vernet 2021-12-13 23:32 ` Song Liu 2021-12-14 3:13 ` Josh Poimboeuf 2021-12-14 8:45 ` Petr Mladek 2021-12-14 9:17 ` Miroslav Benes 2021-12-14 15:26 ` David Vernet 2021-12-14 15:50 ` Greg Kroah-Hartman 2021-12-15 8:19 ` Petr Mladek 2021-12-15 15:35 ` Greg Kroah-Hartman 2021-12-16 14:14 ` Petr Mladek 2021-12-16 14:35 ` Greg Kroah-Hartman 2021-12-17 13:10 ` Petr Mladek 2021-12-16 15:14 ` David Vernet 2021-12-17 13:53 ` Petr Mladek 2021-12-15 8:33 ` Miroslav Benes 2021-12-14 15:52 ` Greg Kroah-Hartman 2021-12-14 18:26 ` David Vernet
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.