[PATCH 0/2] fix ftrace initialization issue when a module is loaded

[PATCH 0/2] fix ftrace initialization issue when a module is loaded

[Qiu, PeiyangX]

When a module is loaded, current ftrace initialization around the new module
has some issues.

1) ftrace might race with insmod: Just after load_module calls
ftrace_module_init to add ftrace records of the module, ftrace_run_update_code
might jump in to change module codes. But load_module calls
complete_formation=>set_section_ro_nx to put the module TEXT attribute to RO.
Then, ftrace_run_update_code triggers ftrace_bug and fails.

2) complete_formation might fail and the module's ftrace records are not
cleaned up.

This patchset fixes above issues.

[PATCH 1/2] ftrace: fix the race between ftrace and insmod

[Qiu, PeiyangX]

From: Qiu Peiyang <peiyangx.qiu@intel.com>

We hit ftrace_bug report when booting Android on a 64bit ATOM SOC chip.
Basically, there is a race between insmod and ftrace_run_update_code.

After load_module=>ftrace_module_init, another thread jumps in to call
ftrace_run_update_code=>ftrace_arch_code_modify_prepare
                        =>set_all_modules_text_rw, to change all modules
as RW. Since the new module is at MODULE_STATE_UNFORMED, the text attribute
is not changed. Then, the 2nd thread goes ahead to change codes.
However, load_module continues to call complete_formation=>set_section_ro_nx,
then 2nd thread would fail when probing the module's TEXT.

The patch fixes it by using notifier to delay the enabling of ftrace
records to the time when module is at state MODULE_STATE_COMING.

Signed-off-by: Qiu Peiyang <peiyangx.qiu@intel.com>
Signed-off-by: Zhang Yanmin <yanmin.zhang@intel.com>
---
 kernel/trace/ftrace.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 64f865b..52d1908 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -4993,7 +4993,6 @@ static void ftrace_init_module(struct module *mod,
 	if (ftrace_disabled || start == end)
 		return;
 	ftrace_process_locs(mod, start, end);
-	ftrace_module_enable(mod);
 }
 
 void ftrace_module_init(struct module *mod)
@@ -5003,26 +5002,34 @@ void ftrace_module_init(struct module *mod)
 			   mod->num_ftrace_callsites);
 }
 
-static int ftrace_module_notify_exit(struct notifier_block *self,
+static int ftrace_module_notify(struct notifier_block *self,
 				     unsigned long val, void *data)
 {
 	struct module *mod = data;
 
-	if (val == MODULE_STATE_GOING)
+	switch (val) {
+	case MODULE_STATE_COMING:
+		ftrace_module_enable(mod);
+		break;
+	case MODULE_STATE_GOING:
 		ftrace_release_mod(mod);
+		break;
+	default:
+		break;
+	}
 
 	return 0;
 }
 #else
-static int ftrace_module_notify_exit(struct notifier_block *self,
+static int ftrace_module_notify(struct notifier_block *self,
 				     unsigned long val, void *data)
 {
 	return 0;
 }
 #endif /* CONFIG_MODULES */
 
-struct notifier_block ftrace_module_exit_nb = {
-	.notifier_call = ftrace_module_notify_exit,
+struct notifier_block ftrace_module_nb = {
+	.notifier_call = ftrace_module_notify,
 	.priority = INT_MIN,	/* Run after anything that can remove kprobes */
 };
 
@@ -5054,7 +5061,7 @@ void __init ftrace_init(void)
 				  __start_mcount_loc,
 				  __stop_mcount_loc);
 
-	ret = register_module_notifier(&ftrace_module_exit_nb);
+	ret = register_module_notifier(&ftrace_module_nb);
 	if (ret)
 		pr_warning("Failed to register trace ftrace module exit notifier\n");
 
-- 
1.9.1

[PATCH 1/2] ftrace: fix the race between ftrace and insmod

[Qiu, PeiyangX]

From: Qiu Peiyang <peiyangx.qiu@intel.com>

We hit ftrace_bug report when booting Android on a 64bit ATOM SOC chip.
Basically, there is a race between insmod and ftrace_run_update_code.

After load_module=>ftrace_module_init, another thread jumps in to call
ftrace_run_update_code=>ftrace_arch_code_modify_prepare
                        =>set_all_modules_text_rw, to change all modules
as RW. Since the new module is at MODULE_STATE_UNFORMED, the text attribute
is not changed. Then, the 2nd thread goes ahead to change codes.
However, load_module continues to call complete_formation=>set_section_ro_nx,
then 2nd thread would fail when probing the module's TEXT.

The patch fixes it by using notifier to delay the enabling of ftrace
records to the time when module is at state MODULE_STATE_COMING.

Signed-off-by: Qiu Peiyang <peiyangx.qiu@intel.com>
Signed-off-by: Zhang Yanmin <yanmin.zhang@intel.com>
---
 kernel/trace/ftrace.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 64f865b..52d1908 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -4993,7 +4993,6 @@ static void ftrace_init_module(struct module *mod,
 	if (ftrace_disabled || start == end)
 		return;
 	ftrace_process_locs(mod, start, end);
-	ftrace_module_enable(mod);
 }
  void ftrace_module_init(struct module *mod)
@@ -5003,26 +5002,34 @@ void ftrace_module_init(struct module *mod)
 			   mod->num_ftrace_callsites);
 }
 -static int ftrace_module_notify_exit(struct notifier_block *self,
+static int ftrace_module_notify(struct notifier_block *self,
 				     unsigned long val, void *data)
 {
 	struct module *mod = data;
 -	if (val == MODULE_STATE_GOING)
+	switch(val) {
+	case MODULE_STATE_COMING:
+		ftrace_module_enable(mod);
+		break;
+	case MODULE_STATE_GOING:
 		ftrace_release_mod(mod);
+		break;
+	default:
+		break;
+	}
  	return 0;
 }
 #else
-static int ftrace_module_notify_exit(struct notifier_block *self,
+static int ftrace_module_notify(struct notifier_block *self,
 				     unsigned long val, void *data)
 {
 	return 0;
 }
 #endif /* CONFIG_MODULES */
 -struct notifier_block ftrace_module_exit_nb = {
-	.notifier_call = ftrace_module_notify_exit,
+struct notifier_block ftrace_module_nb = {
+	.notifier_call = ftrace_module_notify,
 	.priority = INT_MIN,	/* Run after anything that can remove kprobes */
 };
 @@ -5054,7 +5061,7 @@ void __init ftrace_init(void)
 				  __start_mcount_loc,
 				  __stop_mcount_loc);
 -	ret = register_module_notifier(&ftrace_module_exit_nb);
+	ret = register_module_notifier(&ftrace_module_nb);
 	if (ret)
 		pr_warning("Failed to register trace ftrace module exit notifier\n");
 -- 1.9.1

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

[Qiu, PeiyangX]

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);
+
+	/* Ftrace init must be called in the MODULE_STATE_UNFORMED state */
+	ftrace_module_init(mod);
+
 	/* Set RO and NX regions for core */
 	set_section_ro_nx(mod->module_core,
 				mod->core_text_size,
@@ -3388,7 +3393,6 @@ static int complete_formation(struct module *mod, struct load_info *info)
 	/* Mark state as coming so strong_try_module_get() ignores us,
 	 * but kallsyms etc. can see us. */
 	mod->state = MODULE_STATE_COMING;
-	mutex_unlock(&module_mutex);
 
 	blocking_notifier_call_chain(&module_notify_list,
 				     MODULE_STATE_COMING, mod);
@@ -3505,9 +3509,6 @@ static int load_module(struct load_info *info, const char __user *uargs,
 
 	dynamic_debug_setup(info->debug, info->num_debug);
 
-	/* Ftrace init must be called in the MODULE_STATE_UNFORMED state */
-	ftrace_module_init(mod);
-
 	/* Finally it's fully formed, ready to start executing. */
 	err = complete_formation(mod, info);
 	if (err)
-- 
1.9.1

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

[Steven Rostedt]

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)
 {

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

[Zhang, Yanmin]

On 2016/1/6 9:01, Steven Rostedt wrote:
> 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.

It's a good idea although ftrace_module_init_fail might be complicated.

Thanks,
Yanmin

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

[Steven Rostedt]

On Wed, 06 Jan 2016 09:14:42 +0800
"Zhang, Yanmin" <yanmin_zhang@linux.intel.com> wrote:


> It's a good idea although ftrace_module_init_fail might be complicated.

How so? I posted the patch. All it does is to call
ftrace_release_mod(), which will only remove pages that are included
with the module. If the pages were not added yet, nothing is done.

-- Steve

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

[Zhang, Yanmin]

On 2016/1/6 9:29, Steven Rostedt wrote:
> On Wed, 06 Jan 2016 09:14:42 +0800
> "Zhang, Yanmin" <yanmin_zhang@linux.intel.com> wrote:
>
>
>> It's a good idea although ftrace_module_init_fail might be complicated.
> How so? I posted the patch. All it does is to call
> ftrace_release_mod(), which will only remove pages that are included
> with the module. If the pages were not added yet, nothing is done.

You are right. ftrace_release_mod can be used.

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

[Steven Rostedt]

On Wed, 06 Jan 2016 09:48:42 +0800
"Zhang, Yanmin" <yanmin_zhang@linux.intel.com> wrote:

> On 2016/1/6 9:29, Steven Rostedt wrote:
> > On Wed, 06 Jan 2016 09:14:42 +0800
> > "Zhang, Yanmin" <yanmin_zhang@linux.intel.com> wrote:
> >
> >  
> >> It's a good idea although ftrace_module_init_fail might be complicated.  
> > How so? I posted the patch. All it does is to call
> > ftrace_release_mod(), which will only remove pages that are included
> > with the module. If the pages were not added yet, nothing is done.  
> 
> You are right. ftrace_release_mod can be used.

I was going to call that directly, but for clarity, documentation and
robustness I added the new call. Hmm, but on second thought, I noticed
that it is already shown globally. I think I'll change that back to call
ftrace_release_mod() directly.

-- Steve