Messages in this thread | | | Date | Mon, 13 Dec 2021 12:10:22 -0800 | From | Josh Poimboeuf <> | Subject | Re: [PATCH] livepatch: Fix leak on klp_init_patch_early failure path |
| |
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
| |