[PATCH] ftrace/module: Hardcode ftrace_module_init() call into load_module()

[PATCH] ftrace/module: Hardcode ftrace_module_init() call into load_module()

[Steven Rostedt]

[
  Rusty, you can take this patch, or if you want, you can give me 
  an Acked-by, and I'll push this through my tree.
]


>From 3ad4487ccecb8eb799c8e96309f256a1c9296685 Mon Sep 17 00:00:00 2001
From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
Date: Thu, 24 Apr 2014 10:40:12 -0400
Subject: [PATCH] ftrace/module: Hardcode ftrace_module_init() call into
 load_module()

A race exists between module loading and enabling of function tracer.

	CPU 1				CPU 2
	-----				-----
  load_module()
   module->state = MODULE_STATE_COMING

				register_ftrace_function()
				 mutex_lock(&ftrace_lock);
				 ftrace_startup()
				  update_ftrace_function();
				   ftrace_arch_code_modify_prepare()
				    set_all_module_text_rw();
				   <enables-ftrace>
				    ftrace_arch_code_modify_post_process()
				     set_all_module_text_ro();

				[ here all module text is set to RO,
				  including the module that is
				  loading!! ]

   blocking_notifier_call_chain(MODULE_STATE_COMING);
    ftrace_init_module()

     [ tries to modify code, but it's RO, and fails!
       ftrace_bug() is called]

When this race happens, ftrace_bug() will produces a nasty warning and
all of the function tracing features will be disabled until reboot.

The simple solution is to treate module load the same way the core
kernel is treated at boot. To hardcode the ftrace function modification
of converting calls to mcount into nops. This is done in init/main.c
there's no reason it could not be done in load_module(). This gives
a better control of the changes and doesn't tie the state of the
module to its notifiers as much. Ftrace is special, it needs to be
treated as such.

The reason this would work, is that the ftrace_module_init() would be
called while the module is in MODULE_STATE_UNFORMED, which is ignored
by the set_all_module_text_ro() call.

Link: http://lkml.kernel.org/r/1395637826-3312-1-git-send-email-indou.takao@jp.fujitsu.com

Reported-by: Takao Indoh <indou.takao@jp.fujitsu.com>
Cc: stable@vger.kernel.org # 2.6.38+
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 include/linux/ftrace.h |  2 ++
 kernel/module.c        |  3 +++
 kernel/trace/ftrace.c  | 27 ++++-----------------------
 3 files changed, 9 insertions(+), 23 deletions(-)

diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index 9212b01..ae9504b 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -535,6 +535,7 @@ static inline int ftrace_modify_call(struct dyn_ftrace *rec, unsigned long old_a
 extern int ftrace_arch_read_dyn_info(char *buf, int size);
 
 extern int skip_trace(unsigned long ip);
+extern void ftrace_module_init(struct module *mod);
 
 extern void ftrace_disable_daemon(void);
 extern void ftrace_enable_daemon(void);
@@ -544,6 +545,7 @@ static inline int ftrace_force_update(void) { return 0; }
 static inline void ftrace_disable_daemon(void) { }
 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 __init int register_ftrace_command(struct ftrace_func_command *cmd)
 {
 	return -EINVAL;
diff --git a/kernel/module.c b/kernel/module.c
index 1186940..5f14fec 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -3271,6 +3271,9 @@ 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)
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 1fd4b94..4a54a25 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -4330,16 +4330,11 @@ static void ftrace_init_module(struct module *mod,
 	ftrace_process_locs(mod, start, end);
 }
 
-static int ftrace_module_notify_enter(struct notifier_block *self,
-				      unsigned long val, void *data)
+void ftrace_module_init(struct module *mod)
 {
-	struct module *mod = data;
-
-	if (val == MODULE_STATE_COMING)
-		ftrace_init_module(mod, mod->ftrace_callsites,
-				   mod->ftrace_callsites +
-				   mod->num_ftrace_callsites);
-	return 0;
+	ftrace_init_module(mod, mod->ftrace_callsites,
+			   mod->ftrace_callsites +
+			   mod->num_ftrace_callsites);
 }
 
 static int ftrace_module_notify_exit(struct notifier_block *self,
@@ -4353,11 +4348,6 @@ static int ftrace_module_notify_exit(struct notifier_block *self,
 	return 0;
 }
 #else
-static int ftrace_module_notify_enter(struct notifier_block *self,
-				      unsigned long val, void *data)
-{
-	return 0;
-}
 static int ftrace_module_notify_exit(struct notifier_block *self,
 				     unsigned long val, void *data)
 {
@@ -4365,11 +4355,6 @@ static int ftrace_module_notify_exit(struct notifier_block *self,
 }
 #endif /* CONFIG_MODULES */
 
-struct notifier_block ftrace_module_enter_nb = {
-	.notifier_call = ftrace_module_notify_enter,
-	.priority = INT_MAX,	/* Run before anything that can use kprobes */
-};
-
 struct notifier_block ftrace_module_exit_nb = {
 	.notifier_call = ftrace_module_notify_exit,
 	.priority = INT_MIN,	/* Run after anything that can remove kprobes */
@@ -4403,10 +4388,6 @@ void __init ftrace_init(void)
 				  __start_mcount_loc,
 				  __stop_mcount_loc);
 
-	ret = register_module_notifier(&ftrace_module_enter_nb);
-	if (ret)
-		pr_warning("Failed to register trace ftrace module enter notifier\n");
-
 	ret = register_module_notifier(&ftrace_module_exit_nb);
 	if (ret)
 		pr_warning("Failed to register trace ftrace module exit notifier\n");
-- 
1.8.1.4

Re: [PATCH] ftrace/module: Hardcode ftrace_module_init() call into load_module()

[Masami Hiramatsu]

(2014/04/24 23:54), Steven Rostedt wrote:
> [
>   Rusty, you can take this patch, or if you want, you can give me 
>   an Acked-by, and I'll push this through my tree.
> ]
> 
> 
>>From 3ad4487ccecb8eb799c8e96309f256a1c9296685 Mon Sep 17 00:00:00 2001
> From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
> Date: Thu, 24 Apr 2014 10:40:12 -0400
> Subject: [PATCH] ftrace/module: Hardcode ftrace_module_init() call into
>  load_module()
> 
> A race exists between module loading and enabling of function tracer.
> 
> 	CPU 1				CPU 2
> 	-----				-----
>   load_module()
>    module->state = MODULE_STATE_COMING
> 
> 				register_ftrace_function()
> 				 mutex_lock(&ftrace_lock);
> 				 ftrace_startup()
> 				  update_ftrace_function();
> 				   ftrace_arch_code_modify_prepare()
> 				    set_all_module_text_rw();
> 				   <enables-ftrace>
> 				    ftrace_arch_code_modify_post_process()
> 				     set_all_module_text_ro();
> 
> 				[ here all module text is set to RO,
> 				  including the module that is
> 				  loading!! ]
> 
>    blocking_notifier_call_chain(MODULE_STATE_COMING);
>     ftrace_init_module()
> 
>      [ tries to modify code, but it's RO, and fails!
>        ftrace_bug() is called]
> 
> When this race happens, ftrace_bug() will produces a nasty warning and
> all of the function tracing features will be disabled until reboot.
> 
> The simple solution is to treate module load the same way the core
> kernel is treated at boot. To hardcode the ftrace function modification
> of converting calls to mcount into nops. This is done in init/main.c
> there's no reason it could not be done in load_module(). This gives
> a better control of the changes and doesn't tie the state of the
> module to its notifiers as much. Ftrace is special, it needs to be
> treated as such.
> 
> The reason this would work, is that the ftrace_module_init() would be
> called while the module is in MODULE_STATE_UNFORMED, which is ignored
> by the set_all_module_text_ro() call.
> 
> Link: http://lkml.kernel.org/r/1395637826-3312-1-git-send-email-indou.takao@jp.fujitsu.com
> 
> Reported-by: Takao Indoh <indou.takao@jp.fujitsu.com>
> Cc: stable@vger.kernel.org # 2.6.38+
> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>

Looks good to me.

Reviewed-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>

Thank you,

> ---
>  include/linux/ftrace.h |  2 ++
>  kernel/module.c        |  3 +++
>  kernel/trace/ftrace.c  | 27 ++++-----------------------
>  3 files changed, 9 insertions(+), 23 deletions(-)
> 
> diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> index 9212b01..ae9504b 100644
> --- a/include/linux/ftrace.h
> +++ b/include/linux/ftrace.h
> @@ -535,6 +535,7 @@ static inline int ftrace_modify_call(struct dyn_ftrace *rec, unsigned long old_a
>  extern int ftrace_arch_read_dyn_info(char *buf, int size);
>  
>  extern int skip_trace(unsigned long ip);
> +extern void ftrace_module_init(struct module *mod);
>  
>  extern void ftrace_disable_daemon(void);
>  extern void ftrace_enable_daemon(void);
> @@ -544,6 +545,7 @@ static inline int ftrace_force_update(void) { return 0; }
>  static inline void ftrace_disable_daemon(void) { }
>  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 __init int register_ftrace_command(struct ftrace_func_command *cmd)
>  {
>  	return -EINVAL;
> diff --git a/kernel/module.c b/kernel/module.c
> index 1186940..5f14fec 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -3271,6 +3271,9 @@ 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)
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index 1fd4b94..4a54a25 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -4330,16 +4330,11 @@ static void ftrace_init_module(struct module *mod,
>  	ftrace_process_locs(mod, start, end);
>  }
>  
> -static int ftrace_module_notify_enter(struct notifier_block *self,
> -				      unsigned long val, void *data)
> +void ftrace_module_init(struct module *mod)
>  {
> -	struct module *mod = data;
> -
> -	if (val == MODULE_STATE_COMING)
> -		ftrace_init_module(mod, mod->ftrace_callsites,
> -				   mod->ftrace_callsites +
> -				   mod->num_ftrace_callsites);
> -	return 0;
> +	ftrace_init_module(mod, mod->ftrace_callsites,
> +			   mod->ftrace_callsites +
> +			   mod->num_ftrace_callsites);
>  }
>  
>  static int ftrace_module_notify_exit(struct notifier_block *self,
> @@ -4353,11 +4348,6 @@ static int ftrace_module_notify_exit(struct notifier_block *self,
>  	return 0;
>  }
>  #else
> -static int ftrace_module_notify_enter(struct notifier_block *self,
> -				      unsigned long val, void *data)
> -{
> -	return 0;
> -}
>  static int ftrace_module_notify_exit(struct notifier_block *self,
>  				     unsigned long val, void *data)
>  {
> @@ -4365,11 +4355,6 @@ static int ftrace_module_notify_exit(struct notifier_block *self,
>  }
>  #endif /* CONFIG_MODULES */
>  
> -struct notifier_block ftrace_module_enter_nb = {
> -	.notifier_call = ftrace_module_notify_enter,
> -	.priority = INT_MAX,	/* Run before anything that can use kprobes */
> -};
> -
>  struct notifier_block ftrace_module_exit_nb = {
>  	.notifier_call = ftrace_module_notify_exit,
>  	.priority = INT_MIN,	/* Run after anything that can remove kprobes */
> @@ -4403,10 +4388,6 @@ void __init ftrace_init(void)
>  				  __start_mcount_loc,
>  				  __stop_mcount_loc);
>  
> -	ret = register_module_notifier(&ftrace_module_enter_nb);
> -	if (ret)
> -		pr_warning("Failed to register trace ftrace module enter notifier\n");
> -
>  	ret = register_module_notifier(&ftrace_module_exit_nb);
>  	if (ret)
>  		pr_warning("Failed to register trace ftrace module exit notifier\n");
> 


-- 
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Research Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com

Re: [PATCH] ftrace/module: Hardcode ftrace_module_init() call into load_module()

[Rusty Russell]

Steven Rostedt <rostedt@goodmis.org> writes:
> [
>   Rusty, you can take this patch, or if you want, you can give me 
>   an Acked-by, and I'll push this through my tree.
> ]

Assuming you want this in the current kernel, I'll just ack:

Acked-by: Rusty Russell <rusty@rustcorp.com.au>

Since I've got my "make RO/NX earlier" patch queued for *next*
window, and yours need to go in before that...

Thanks,
Rusty.

Re: [PATCH] ftrace/module: Hardcode ftrace_module_init() call into load_module()

[Steven Rostedt]

On Mon, 28 Apr 2014 10:54:20 +0930
Rusty Russell <rusty@rustcorp.com.au> wrote:

> Steven Rostedt <rostedt@goodmis.org> writes:
> > [
> >   Rusty, you can take this patch, or if you want, you can give me 
> >   an Acked-by, and I'll push this through my tree.
> > ]
> 
> Assuming you want this in the current kernel, I'll just ack:
> 
> Acked-by: Rusty Russell <rusty@rustcorp.com.au>
> 
> Since I've got my "make RO/NX earlier" patch queued for *next*
> window, and yours need to go in before that...
> 

OK, thanks!

I'll start running it through my full test suite and then send it off
with a stable tag to Linus.

-- Steve