Re: [PATCH 2/2] module: deal with the failure of complete_formation

[Steven Rostedt <rostedt@goodmis.org>]

On Fri, 25 Dec 2015 15:03:13 +0800
"Qiu, PeiyangX" <peiyangx.qiu@intel.com> wrote:

> From: Qiu Peiyang <peiyangx.qiu@intel.com>
> 
> complete_formation might fail. kernel need clean up
> ftrace records of the module.
> 
> The patch fixes it by tuning the operation sequence in
> complete_formation. After complete_formation checks
> verify_export_symbols, call ftrace_module_init to init
> ftrace records.
> 
> Signed-off-by: Qiu Peiyang <peiyangx.qiu@intel.com>
> Signed-off-by: Zhang Yanmin <yanmin.zhang@intel.com>
> ---
>  kernel/module.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/module.c b/kernel/module.c
> index 8f051a1..0a67c4e 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -3373,6 +3373,11 @@ static int complete_formation(struct module *mod, struct load_info *info)
>  	/* This relies on module_mutex for list integrity. */
>  	module_bug_finalize(info->hdr, info->sechdrs, mod);
>  
> +	mutex_unlock(&module_mutex);
> +

First of all, this is buggy. You can't just move the locking of
module_mutex. That is needed to modify mod->state.

Second of all, you are solving this wrong. I guess we should add
ftrace_module_init_fail() function to cover the cases where the module
can fail before calling do_init_module(), as if that happens it does
not call the module going notifiers.

I'll be sending this to stable too.

-- Steve

---
 include/linux/ftrace.h |    2 ++
 kernel/module.c        |    1 +
 kernel/trace/ftrace.c  |    5 +++++
 3 files changed, 8 insertions(+)

Index: linux-trace.git/include/linux/ftrace.h
===================================================================
--- linux-trace.git.orig/include/linux/ftrace.h	2016-01-05 18:00:11.686173290 -0500
+++ linux-trace.git/include/linux/ftrace.h	2016-01-05 18:00:47.815631131 -0500
@@ -602,6 +602,7 @@ extern int ftrace_arch_read_dyn_info(cha
 
 extern int skip_trace(unsigned long ip);
 extern void ftrace_module_init(struct module *mod);
+extern void ftrace_module_init_fail(struct module *mod);
 
 extern void ftrace_disable_daemon(void);
 extern void ftrace_enable_daemon(void);
@@ -612,6 +613,7 @@ static inline void ftrace_disable_daemon
 static inline void ftrace_enable_daemon(void) { }
 static inline void ftrace_release_mod(struct module *mod) {}
 static inline void ftrace_module_init(struct module *mod) {}
+static inline void ftrace_module_init_fail(struct module *mod) {}
 static inline __init int register_ftrace_command(struct ftrace_func_command *cmd)
 {
 	return -EINVAL;
Index: linux-trace.git/kernel/module.c
===================================================================
--- linux-trace.git.orig/kernel/module.c	2016-01-05 18:00:11.687173275 -0500
+++ linux-trace.git/kernel/module.c	2016-01-05 18:00:47.816631117 -0500
@@ -3571,6 +3571,7 @@ static int load_module(struct load_info
 	synchronize_sched();
 	mutex_unlock(&module_mutex);
  free_module:
+	ftrace_module_init_fail(mod);
 	/* Free lock-classes; relies on the preceding sync_rcu() */
 	lockdep_free_key_range(mod->module_core, mod->core_size);
 
Index: linux-trace.git/kernel/trace/ftrace.c
===================================================================
--- linux-trace.git.orig/kernel/trace/ftrace.c	2016-01-05 18:00:47.817631101 -0500
+++ linux-trace.git/kernel/trace/ftrace.c	2016-01-05 18:01:10.418291635 -0500
@@ -4989,6 +4989,11 @@ void ftrace_module_init(struct module *m
 			    mod->ftrace_callsites + mod->num_ftrace_callsites);
 }
 
+void ftrace_module_init_fail(struct module *mod)
+{
+	ftrace_release_mod(mod);
+}
+
 static int ftrace_module_notify_exit(struct notifier_block *self,
 				     unsigned long val, void *data)
 {