| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20141209185141.GA3955@treble.redhat.com>
Date: Tue, 9 Dec 2014 12:51:41 -0600
From: Josh Poimboeuf <jpoimboe@...hat.com>
To: Petr Mladek <pmladek@...e.cz>
Cc: Seth Jennings <sjenning@...hat.com>, Jiri Kosina <jkosina@...e.cz>,
Vojtech Pavlik <vojtech@...e.cz>,
Steven Rostedt <rostedt@...dmis.org>,
Miroslav Benes <mbenes@...e.cz>,
Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>,
Christoph Hellwig <hch@...radead.org>,
Greg KH <gregkh@...uxfoundation.org>,
Andy Lutomirski <luto@...capital.net>,
live-patching@...r.kernel.org, x86@...nel.org, kpatch@...hat.com,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH 5/6] livepatch v5: split init and free code that is done
only for loaded modules
On Tue, Dec 09, 2014 at 07:05:06PM +0100, Petr Mladek wrote:
> This patch makes it clear what initialization and freeing steps need to be done
> when an object (module) is being loaded or removed. It will help to maintain
> the module coming and going handlers. Also it will remove duplicated
> code from these handlers.
>
> Signed-off-by: Petr Mladek <pmladek@...e.cz>
> ---
> kernel/livepatch/core.c | 92 ++++++++++++++++++++++++++++++++-----------------
> 1 file changed, 61 insertions(+), 31 deletions(-)
>
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index 97a8d4a3d6d8..fe312b9ada78 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -590,6 +590,12 @@ static struct kobj_type klp_ktype_func = {
> .sysfs_ops = &kobj_sysfs_ops,
> };
>
> +/* Clean up when a patched object is unloaded */
> +static void klp_free_func_loaded(struct klp_func *func)
> +{
> + func->old_addr = 0;
> +}
> +
> /*
> * Free all functions' kobjects in the array up to some limit. When limit is
> * NULL, all kobjects are freed.
> @@ -603,6 +609,17 @@ static void klp_free_funcs_limited(struct klp_object *obj,
> kobject_put(&func->kobj);
> }
>
> +/* Clean up when a patched object is unloaded */
> +static void klp_free_object_loaded(struct klp_object *obj)
> +{
> + struct klp_func *func;
> +
> + obj->mod = NULL;
> +
> + for (func = obj->funcs; func->old_name; func++)
> + klp_free_func_loaded(func);
> +}
> +
> /*
> * Free all objects' kobjects in the array up to some limit. When limit is
> * NULL, all kobjects are freed.
> @@ -626,6 +643,12 @@ static void klp_free_patch(struct klp_patch *patch)
> kobject_put(&patch->kobj);
> }
>
> +/* parts of the initialization that is done only when the object is loaded */
> +static int klp_init_func_loaded(struct klp_object *obj, struct klp_func *func)
> +{
> + return klp_find_verify_func_addr(obj, func);
> +}
> +
Creating a new function here for one line of code, which is only called
once, seems excessive, and makes the code harder to understand IMO.
Ditto for klp_free_func_loaded.
> static int klp_init_func(struct klp_object *obj, struct klp_func *func)
> {
> struct ftrace_ops *ops;
> @@ -633,10 +656,6 @@ static int klp_init_func(struct klp_object *obj, struct klp_func *func)
>
> func->state = KLP_DISABLED;
>
> - ret = klp_find_verify_func_addr(obj, func);
> - if (ret)
> - return ret;
> -
> ops = kzalloc(sizeof(*ops), GFP_KERNEL);
> if (!ops)
> ret = -ENOMEM;
> @@ -656,6 +675,28 @@ static int klp_init_func(struct klp_object *obj, struct klp_func *func)
> return 0;
> }
>
> +/* parts of the initialization that is done only when the object is loaded */
> +static int klp_init_object_loaded(struct klp_patch *patch,
> + struct klp_object *obj)
> +{
> + struct klp_func *func;
> + int ret;
> +
> + if (obj->relocs) {
> + ret = klp_write_object_relocations(patch->mod, obj);
> + if (ret)
> + return ret;
> + }
> +
> + for (func = obj->funcs; func->old_name; func++) {
> + ret = klp_init_func_loaded(obj, func);
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> static int klp_init_object(struct klp_patch *patch, struct klp_object *obj)
> {
> struct klp_func *func;
> @@ -669,12 +710,6 @@ static int klp_init_object(struct klp_patch *patch, struct klp_object *obj)
>
> klp_find_object_module(obj);
>
> - if (obj->relocs && klp_is_object_loaded(obj)) {
> - ret = klp_write_object_relocations(patch->mod, obj);
> - if (ret)
> - return ret;
> - }
> -
> name = klp_is_module(obj) ? obj->name : "vmlinux";
> obj->kobj = kobject_create_and_add(name, &patch->kobj);
> if (!obj->kobj)
> @@ -686,6 +721,12 @@ static int klp_init_object(struct klp_patch *patch, struct klp_object *obj)
> goto free;
> }
>
> + if (klp_is_object_loaded(obj)) {
> + ret = klp_init_object_loaded(patch, obj);
> + if (ret)
> + goto free;
> + }
> +
> return 0;
>
> free:
> @@ -802,27 +843,19 @@ int klp_register_patch(struct klp_patch *patch)
> }
> EXPORT_SYMBOL_GPL(klp_register_patch);
>
> -static void klp_module_notify_coming(struct module *pmod,
> +static void klp_module_notify_coming(struct klp_patch *patch,
> struct klp_object *obj)
> {
> - struct klp_func *func;
> + struct module *pmod = patch->mod;
> struct module *mod = obj->mod;
> int ret;
>
> pr_notice("applying patch '%s' to loading module '%s'\n",
> pmod->name, mod->name);
>
> - if (obj->relocs) {
> - ret = klp_write_object_relocations(pmod, obj);
> - if (ret)
> - goto err;
> - }
> -
> - for (func = obj->funcs; func->old_name; func++) {
> - ret = klp_find_verify_func_addr(obj, func);
> - if (ret)
> - goto err;
> - }
> + ret = klp_init_object_loaded(patch, obj);
> + if (ret)
> + goto err;
>
> ret = klp_enable_object(obj);
> if (!ret)
> @@ -833,10 +866,10 @@ err:
> pmod->name, mod->name, ret);
> }
>
> -static void klp_module_notify_going(struct module *pmod,
> +static void klp_module_notify_going(struct klp_patch *patch,
> struct klp_object *obj)
> {
> - struct klp_func *func;
> + struct module *pmod = patch->mod;
> struct module *mod = obj->mod;
> int ret;
>
> @@ -848,10 +881,7 @@ static void klp_module_notify_going(struct module *pmod,
> pr_warn("failed to revert patch '%s' on module '%s' (%d)\n",
> pmod->name, mod->name, ret);
>
> - for (func = obj->funcs; func->old_name; func++)
> - func->old_addr = 0;
> -
> - obj->mod = NULL;
> + klp_free_object_loaded(obj);
> }
>
> static int klp_module_notify(struct notifier_block *nb, unsigned long action,
> @@ -876,9 +906,9 @@ static int klp_module_notify(struct notifier_block *nb, unsigned long action,
>
> if (action == MODULE_STATE_COMING) {
> obj->mod = mod;
> - klp_module_notify_coming(patch->mod, obj);
> + klp_module_notify_coming(patch, obj);
> } else /* MODULE_STATE_GOING */
> - klp_module_notify_going(patch->mod, obj);
> + klp_module_notify_going(patch, obj);
>
> break;
> }
> --
> 1.8.5.2
>
--
Josh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists