Re: livepatch/module: remove livepatch module notifier

[Jessica Yu]

+++ Petr Mladek [14/03/16 16:06 +0100]:

On Fri 2016-03-11 15:03:48, Jessica Yu wrote:

Remove the livepatch module notifier in favor of directly enabling and
disabling patches to modules in the module loader. Hard-coding the
function calls ensures that ftrace_module_enable() is run before
klp_module_coming() during module load, and that klp_module_going() is
run before ftrace_release_mod() during module unload. This way, ftrace
and livepatch code is run in the correct order during the module
load/unload sequence without dependence on the module notifier call chain.

Signed-off-by: Jessica Yu <j...@redhat.com>
Acked-by: Josh Poimboeuf <jpoim...@redhat.com>
Acked-by: Rusty Russell <ru...@rustcorp.com.au>
---
 include/linux/livepatch.h |  13 ++++
 kernel/livepatch/core.c   | 147 ++++++++++++++++++++++------------------------
 kernel/module.c           |  10 ++++
 3 files changed, 94 insertions(+), 76 deletions(-)

diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 780f00c..4fafbae 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -866,103 +866,108 @@ int klp_register_patch(struct klp_patch *patch)
 }
 EXPORT_SYMBOL_GPL(klp_register_patch);

-static int klp_module_notify_coming(struct klp_patch *patch,
-                                    struct klp_object *obj)
+int klp_module_coming(struct module *mod)
 {
-       struct module *pmod = patch->mod;
-       struct module *mod = obj->mod;
        int ret;
+       struct klp_patch *patch;
+       struct klp_object *obj;

-       ret = klp_init_object_loaded(patch, obj);
-       if (ret) {
-               pr_warn("failed to initialize patch '%s' for module '%s' 
(%d)\n",
-                       pmod->name, mod->name, ret);
-               return ret;
-       }
+       if (WARN_ON(mod->state != MODULE_STATE_COMING))
+               return -EINVAL;

-       if (patch->state == KLP_DISABLED)
-               return 0;
+       mutex_lock(&klp_mutex);
+       /*
+        * Each module has to know that klp_module_coming()
+        * has been called. We never know what module will
+        * get patched by a new patch.
+        */
+       mod->klp_alive = true;

-       pr_notice("applying patch '%s' to loading module '%s'\n",
-                 pmod->name, mod->name);
+       list_for_each_entry(patch, &klp_patches, list) {
+               klp_for_each_object(patch, obj) {
+                       if (!klp_is_module(obj) || strcmp(obj->name, mod->name))
+                               continue;

-       ret = klp_enable_object(obj);
-       if (ret)
-               pr_warn("failed to apply patch '%s' to module '%s' (%d)\n",
-                       pmod->name, mod->name, ret);
-       return ret;
-}
+                       obj->mod = mod;

-static void klp_module_notify_going(struct klp_patch *patch,
-                                   struct klp_object *obj)
-{
-       struct module *pmod = patch->mod;
-       struct module *mod = obj->mod;
+                       ret = klp_init_object_loaded(patch, obj);
+                       if (ret) {
+                               pr_warn("failed to initialize patch '%s' for module 
'%s' (%d)\n",
+                                       patch->mod->name, obj->mod->name, ret);
+                               goto err;
+                       }

-       if (patch->state == KLP_DISABLED)
-               goto disabled;
+                       if (patch->state == KLP_DISABLED)
+                               break;
+
+                       pr_notice("applying patch '%s' to loading module 
'%s'\n",
+                                 patch->mod->name, obj->mod->name);
+
+                       ret = klp_enable_object(obj);
+                       if (ret) {
+                               pr_warn("failed to apply patch '%s' to module '%s' 
(%d)\n",
+                                       patch->mod->name, obj->mod->name, ret);
+                               goto err;
+                       }
+
+                       break;
+               }
+       }

-       pr_notice("reverting patch '%s' on unloading module '%s'\n",
-                 pmod->name, mod->name);
+       mutex_unlock(&klp_mutex);

-       klp_disable_object(obj);
+       return 0;

-disabled:
+err:
+       /*
+        * If a patch is unsuccessfully applied, return
+        * error to the module loader.
+        */
+       pr_warn("patch '%s' failed for module '%s', refusing to load module 
'%s'\n",
+               patch->mod->name, obj->mod->name, obj->mod->name);

One more thing. We should add here:

        mod->klp_alive = true;

Otherwise, there is a small race window when a new patch will try to
patch the module.

Ah, you are right. I think you meant false, though :-) Will fix that.

        klp_free_object_loaded(obj);
+       mutex_unlock(&klp_mutex);
+
+       return ret;
 }

With the above fix:

Reviewed-by: Petr Mladek <pmla...@suse.cz>


Best Regards,
Petr