Livepatch module notifier cleanup

PATCH Applied Jessica Yu 2 participants 4 messages Mar 17, 2016 Live patching

Share & tools

Jessica Yu

author

2016-03-17 00:55 UTC #0 lore raw

v3 cover Livepatch module notifier cleanup

These are the remaining 2 patches that came from the original ftrace/livepatch module notifier patchset found here: https://lkml.org/lkml/2016/2/8/1180

Basically, the patchset does a bit of module.c cleanup (patch 1) in preparation for the klp_module_{coming,going} calls (patch 2). We decided to stop relying on the module notifier callchain in favor of hard-coding the appropriate livepatch function calls that handle coming and going modules. Hard-coding these calls will guarantee that ftrace and livepatch exit/initialization routines are called in the correct order without relying on module notifiers.

The patches should be nearly exactly the same as those from the previous discussion, except in patch 2 I've added back the #if IS_ENABLED(CONFIG_LIVEPATCH) guard in livepatch.h.

Patches based on linux-next.

v2 discussion found here:
https://lkml.kernel.org/g/[email protected]

v3:
- Remove confusing comment above klp_module_going()
- Place prepare_coming_module() after complete_formation()
- Fix small race in klp_module_coming() by setting mod->klp_alive to false in the error path

v2:
- we don't need to change mod->state to GOING when cleaning up during failed module load
- modify mod->state check to allow klp_module_going() to be called in both COMING and GOING states

Jessica Yu (2):
modules: split part of complete_formation() into
prepare_coming_module()
livepatch/module: remove livepatch module notifier

 include/linux/livepatch.h |  13 ++++
 kernel/livepatch/core.c   | 147 ++++++++++++++++++++++------------------------
 kernel/module.c           |  36 +++++++++---
 3 files changed, 112 insertions(+), 84 deletions(-)

Signature · 1 line

2.4.3

Jessica Yu

author

2016-03-17 00:55 UTC #1 lore raw

PATCH v3 1/2 modules: split part of complete_formation() into prepare_coming_module()

Put all actions in complete_formation() that are performed after module->state is set to MODULE_STATE_COMING into a separate function prepare_coming_module(). This split prepares for the removal of the livepatch module notifiers in favor of hard-coding function calls to klp_module_{coming,going} in the module loader.

The complete_formation -> prepare_coming_module split will also make error handling easier since we can jump to the appropriate error label to do any module GOING cleanup after all the COMING-actions have completed.

Signed-off-by Jessica Yu <[email protected]>

Reviewed-by Josh Poimboeuf <[email protected]>

Reviewed-by Petr Mladek <[email protected]>

Acked-by Rusty Russell <[email protected]>

---
 kernel/module.c | 26 ++++++++++++++++++--------
 1 file changed, 18 insertions(+), 8 deletions(-)

kernel/module.c +18 -8

index 87cfeb2..a5e0089 100644
@@ -3335,9 +3335,6 @@ static int complete_formation(struct module *mod, struct load_info *info)
 	mod->state = MODULE_STATE_COMING;
 	mutex_unlock(&module_mutex);
 
-	ftrace_module_enable(mod);
-	blocking_notifier_call_chain(&module_notify_list,
-				     MODULE_STATE_COMING, mod);
 	return 0;
 
 out:

@@ -3345,6 +3342,14 @@ out:
 	return err;
 }
 
+static int prepare_coming_module(struct module *mod)
+{
+	ftrace_module_enable(mod);
+	blocking_notifier_call_chain(&module_notify_list,
+				     MODULE_STATE_COMING, mod);
+	return 0;
+}
+
 static int unknown_module_param_cb(char *param, char *val, const char *modname,
 				   void *arg)
 {

@@ -3459,13 +3464,17 @@ static int load_module(struct load_info *info, const char __user *uargs,
 	if (err)
 		goto ddebug_cleanup;
 
+	err = prepare_coming_module(mod);
+	if (err)
+		goto bug_cleanup;
+
 	/* Module is ready to execute: parsing args may do that. */
 	after_dashes = parse_args(mod->name, mod->args, mod->kp, mod->num_kp,
 				  -32768, 32767, mod,
 				  unknown_module_param_cb);
 	if (IS_ERR(after_dashes)) {
 		err = PTR_ERR(after_dashes);
-		goto bug_cleanup;
+		goto coming_cleanup;
 	} else if (after_dashes) {
 		pr_warn("%s: parameters '%s' after `--' ignored\n",
 		       mod->name, after_dashes);

@@ -3474,7 +3483,7 @@ static int load_module(struct load_info *info, const char __user *uargs,
 	/* Link in to syfs. */
 	err = mod_sysfs_setup(mod, info, mod->kp, mod->num_kp);
 	if (err < 0)
-		goto bug_cleanup;
+		goto coming_cleanup;
 
 	/* Get rid of temporary copy. */
 	free_copy(info);

@@ -3484,15 +3493,16 @@ static int load_module(struct load_info *info, const char __user *uargs,
 
 	return do_init_module(mod);
 
+ coming_cleanup:
+	blocking_notifier_call_chain(&module_notify_list,
+				     MODULE_STATE_GOING, mod);
+
  bug_cleanup:
 	/* module_bug_cleanup needs module_mutex protection */
 	mutex_lock(&module_mutex);
 	module_bug_cleanup(mod);
 	mutex_unlock(&module_mutex);
 
-	blocking_notifier_call_chain(&module_notify_list,
-				     MODULE_STATE_GOING, mod);
-
 	/* we can't deallocate the module until we clear memory protection */
 	module_disable_ro(mod);
 	module_disable_nx(mod);

Signature · 1 line

2.4.3

Jessica Yu

author

2016-03-17 00:56 UTC #2 lore raw

PATCH v3 2/2 livepatch/module: remove livepatch module notifier

In reply to Jessica Yu

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 <[email protected]>

Reviewed-by Petr Mladek <[email protected]>

Acked-by Josh Poimboeuf <[email protected]>

Acked-by Rusty Russell <[email protected]>

---
 include/linux/livepatch.h |  13 ++++
 kernel/livepatch/core.c   | 147 ++++++++++++++++++++++------------------------
 kernel/module.c           |  10 ++++
 3 files changed, 94 insertions(+), 76 deletions(-)

include/linux/livepatch.h +13 -0

index c056797..bd830d5 100644
@@ -24,6 +24,8 @@
 #include <linux/module.h>
 #include <linux/ftrace.h>
 
+#if IS_ENABLED(CONFIG_LIVEPATCH)
+
 #include <asm/livepatch.h>
 
 enum klp_state {

@@ -132,4 +134,15 @@ int klp_unregister_patch(struct klp_patch *);
 int klp_enable_patch(struct klp_patch *);
 int klp_disable_patch(struct klp_patch *);
 
+/* Called from the module loader during module coming/going states */
+int klp_module_coming(struct module *mod);
+void klp_module_going(struct module *mod);
+
+#else /* !CONFIG_LIVEPATCH */
+
+static inline int klp_module_coming(struct module *mod) { return 0; }
+static inline void klp_module_going(struct module *mod) { }
+
+#endif /* CONFIG_LIVEPATCH */
+
 #endif /* _LINUX_LIVEPATCH_H_ */

kernel/livepatch/core.c +71 ~~-76~~

207 lines not on this page · raw message

kernel/module.c +10 -0

index a5e0089..041200c 100644
@@ -53,6 +53,7 @@
 #include <asm/sections.h>
 #include <linux/tracepoint.h>
 #include <linux/ftrace.h>
+#include <linux/livepatch.h>
 #include <linux/async.h>
 #include <linux/percpu.h>
 #include <linux/kmemleak.h>

@@ -984,6 +985,7 @@ SYSCALL_DEFINE2(delete_module, const char __user *, name_user,
 		mod->exit();
 	blocking_notifier_call_chain(&module_notify_list,
 				     MODULE_STATE_GOING, mod);
+	klp_module_going(mod);
 	ftrace_release_mod(mod);
 
 	async_synchronize_full();

@@ -3258,6 +3260,7 @@ fail:
 	module_put(mod);
 	blocking_notifier_call_chain(&module_notify_list,
 				     MODULE_STATE_GOING, mod);
+	klp_module_going(mod);
 	ftrace_release_mod(mod);
 	free_module(mod);
 	wake_up_all(&module_wq);

@@ -3344,7 +3347,13 @@ out:
 
 static int prepare_coming_module(struct module *mod)
 {
+	int err;
+
 	ftrace_module_enable(mod);
+	err = klp_module_coming(mod);
+	if (err)
+		return err;
+
 	blocking_notifier_call_chain(&module_notify_list,
 				     MODULE_STATE_COMING, mod);
 	return 0;

@@ -3496,6 +3505,7 @@ static int load_module(struct load_info *info, const char __user *uargs,
  coming_cleanup:
 	blocking_notifier_call_chain(&module_notify_list,
 				     MODULE_STATE_GOING, mod);
+	klp_module_going(mod);
 
  bug_cleanup:
 	/* module_bug_cleanup needs module_mutex protection */

Signature · 1 line

2.4.3

Jiri Kosina

Applied

2016-03-17 08:50 UTC #3 lore raw

In reply to Jessica Yu

Quoted 17 lines · Jessica Yu

These are the remaining 2 patches that came from the original ftrace/livepatch module notifier patchset found here: https://lkml.org/lkml/2016/2/8/1180
Basically, the patchset does a bit of module.c cleanup (patch 1) in preparation for the klp_module_{coming,going} calls (patch 2). We decided to stop relying on the module notifier callchain in favor of hard-coding the appropriate livepatch function calls that handle coming and going modules. Hard-coding these calls will guarantee that ftrace and livepatch exit/initialization routines are called in the correct order without relying on module notifiers.
The patches should be nearly exactly the same as those from the previous discussion, except in patch 2 I've added back the #if IS_ENABLED(CONFIG_LIVEPATCH) guard in livepatch.h.
Patches based on linux-next.

Quoted 8 lines

v2 discussion found here:
https://lkml.kernel.org/g/[email protected]
v3:
- Remove confusing comment above klp_module_going()
- Place prepare_coming_module() after complete_formation()
- Fix small race in klp_module_coming() by setting mod->klp_alive to false in the error path

I've applied v3 to for-4.6/notifier-cleanup.

Thanks!

Signature · 2 lines

Jiri Kosina
SUSE Labs