[PATCH v5] ftrace/module: remove ftrace module notifier

[PATCH v5] ftrace/module: remove ftrace module notifier

[Jessica Yu]

Remove the ftrace module notifier in favor of directly calling
ftrace_module_enable() and ftrace_release_mod() in the module loader.
Hard-coding the function calls directly in the module loader removes
dependence on the module notifier call chain and provides better
visibility and control over what gets called when, which is important
to kernel utilities such as livepatch.

This fixes a notifier ordering issue in which the ftrace module notifier
(and hence ftrace_module_enable()) for coming modules was being called
after klp_module_notify(), which caused livepatch modules to initialize
incorrectly. This patch removes dependence on the module notifier call
chain in favor of hard coding the corresponding function calls in the
module loader. This ensures that ftrace and livepatch code get called in
the correct order on patch module load and unload.

Fixes: 5156dca34a3e ("ftrace: Fix the race between ftrace and insmod")
Signed-off-by: Jessica Yu <jeyu@redhat.com>
Reviewed-by: Steven Rostedt <rostedt@goodmis.org>
Reviewed-by: Petr Mladek <pmladek@suse.cz>
Acked-by: Rusty Russell <rusty@rustcorp.com.au>
---
Patch based on linux-next-20160216.

v5:
 - Make the ftrace notifier patch standalone, without the module.c
   cleanups. This is basically reverting back to patch 1/2 from v2, found
   here: http://article.gmane.org/gmane.linux.kernel/2141648

The original patchset (v4) this patch was a part of can be found here:
https://lkml.kernel.org/g/1454993424-31031-1-git-send-email-jeyu@redhat.com 

 include/linux/ftrace.h |  6 ++++--
 kernel/module.c        |  4 ++++
 kernel/trace/ftrace.c  | 36 +-----------------------------------
 3 files changed, 9 insertions(+), 37 deletions(-)

diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index 81de712..c2b340e 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -603,6 +603,7 @@ 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_module_enable(struct module *mod);
 extern void ftrace_release_mod(struct module *mod);
 
 extern void ftrace_disable_daemon(void);
@@ -612,8 +613,9 @@ static inline int skip_trace(unsigned long ip) { return 0; }
 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 void ftrace_module_init(struct module *mod) { }
+static inline void ftrace_module_enable(struct module *mod) { }
+static inline void ftrace_release_mod(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 9537da3..794ebe8 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -984,6 +984,8 @@ SYSCALL_DEFINE2(delete_module, const char __user *, name_user,
 		mod->exit();
 	blocking_notifier_call_chain(&module_notify_list,
 				     MODULE_STATE_GOING, mod);
+	ftrace_release_mod(mod);
+
 	async_synchronize_full();
 
 	/* Store the name of the last unloaded module for diagnostic purposes */
@@ -3313,6 +3315,7 @@ fail:
 	module_put(mod);
 	blocking_notifier_call_chain(&module_notify_list,
 				     MODULE_STATE_GOING, mod);
+	ftrace_release_mod(mod);
 	free_module(mod);
 	wake_up_all(&module_wq);
 	return ret;
@@ -3389,6 +3392,7 @@ 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;
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index eca592f..57a6eea 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -4961,7 +4961,7 @@ void ftrace_release_mod(struct module *mod)
 	mutex_unlock(&ftrace_lock);
 }
 
-static void ftrace_module_enable(struct module *mod)
+void ftrace_module_enable(struct module *mod)
 {
 	struct dyn_ftrace *rec;
 	struct ftrace_page *pg;
@@ -5038,38 +5038,8 @@ void ftrace_module_init(struct module *mod)
 	ftrace_process_locs(mod, mod->ftrace_callsites,
 			    mod->ftrace_callsites + mod->num_ftrace_callsites);
 }
-
-static int ftrace_module_notify(struct notifier_block *self,
-				unsigned long val, void *data)
-{
-	struct module *mod = data;
-
-	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(struct notifier_block *self,
-				unsigned long val, void *data)
-{
-	return 0;
-}
 #endif /* CONFIG_MODULES */
 
-struct notifier_block ftrace_module_nb = {
-	.notifier_call = ftrace_module_notify,
-	.priority = INT_MIN,	/* Run after anything that can remove kprobes */
-};
-
 void __init ftrace_init(void)
 {
 	extern unsigned long __start_mcount_loc[];
@@ -5098,10 +5068,6 @@ void __init ftrace_init(void)
 				  __start_mcount_loc,
 				  __stop_mcount_loc);
 
-	ret = register_module_notifier(&ftrace_module_nb);
-	if (ret)
-		pr_warning("Failed to register trace ftrace module exit notifier\n");
-
 	set_ftrace_early_filters();
 
 	return;
-- 
2.4.3

Re: [PATCH v5] ftrace/module: remove ftrace module notifier

[Josh Poimboeuf]

On Tue, Feb 16, 2016 at 05:32:33PM -0500, Jessica Yu wrote:
> Remove the ftrace module notifier in favor of directly calling
> ftrace_module_enable() and ftrace_release_mod() in the module loader.
> Hard-coding the function calls directly in the module loader removes
> dependence on the module notifier call chain and provides better
> visibility and control over what gets called when, which is important
> to kernel utilities such as livepatch.
> 
> This fixes a notifier ordering issue in which the ftrace module notifier
> (and hence ftrace_module_enable()) for coming modules was being called
> after klp_module_notify(), which caused livepatch modules to initialize
> incorrectly. This patch removes dependence on the module notifier call
> chain in favor of hard coding the corresponding function calls in the
> module loader. This ensures that ftrace and livepatch code get called in
> the correct order on patch module load and unload.
> 
> Fixes: 5156dca34a3e ("ftrace: Fix the race between ftrace and insmod")
> Signed-off-by: Jessica Yu <jeyu@redhat.com>
> Reviewed-by: Steven Rostedt <rostedt@goodmis.org>
> Reviewed-by: Petr Mladek <pmladek@suse.cz>
> Acked-by: Rusty Russell <rusty@rustcorp.com.au>

Reviewed-by: Josh Poimboeuf <jpoimboe@redhat.com>

> ---
> Patch based on linux-next-20160216.
> 
> v5:
>  - Make the ftrace notifier patch standalone, without the module.c
>    cleanups. This is basically reverting back to patch 1/2 from v2, found
>    here: http://article.gmane.org/gmane.linux.kernel/2141648
> 
> The original patchset (v4) this patch was a part of can be found here:
> https://lkml.kernel.org/g/1454993424-31031-1-git-send-email-jeyu@redhat.com 
> 
>  include/linux/ftrace.h |  6 ++++--
>  kernel/module.c        |  4 ++++
>  kernel/trace/ftrace.c  | 36 +-----------------------------------
>  3 files changed, 9 insertions(+), 37 deletions(-)
> 
> diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> index 81de712..c2b340e 100644
> --- a/include/linux/ftrace.h
> +++ b/include/linux/ftrace.h
> @@ -603,6 +603,7 @@ 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_module_enable(struct module *mod);
>  extern void ftrace_release_mod(struct module *mod);
>  
>  extern void ftrace_disable_daemon(void);
> @@ -612,8 +613,9 @@ static inline int skip_trace(unsigned long ip) { return 0; }
>  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 void ftrace_module_init(struct module *mod) { }
> +static inline void ftrace_module_enable(struct module *mod) { }
> +static inline void ftrace_release_mod(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 9537da3..794ebe8 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -984,6 +984,8 @@ SYSCALL_DEFINE2(delete_module, const char __user *, name_user,
>  		mod->exit();
>  	blocking_notifier_call_chain(&module_notify_list,
>  				     MODULE_STATE_GOING, mod);
> +	ftrace_release_mod(mod);
> +
>  	async_synchronize_full();
>  
>  	/* Store the name of the last unloaded module for diagnostic purposes */
> @@ -3313,6 +3315,7 @@ fail:
>  	module_put(mod);
>  	blocking_notifier_call_chain(&module_notify_list,
>  				     MODULE_STATE_GOING, mod);
> +	ftrace_release_mod(mod);
>  	free_module(mod);
>  	wake_up_all(&module_wq);
>  	return ret;
> @@ -3389,6 +3392,7 @@ 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;
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index eca592f..57a6eea 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -4961,7 +4961,7 @@ void ftrace_release_mod(struct module *mod)
>  	mutex_unlock(&ftrace_lock);
>  }
>  
> -static void ftrace_module_enable(struct module *mod)
> +void ftrace_module_enable(struct module *mod)
>  {
>  	struct dyn_ftrace *rec;
>  	struct ftrace_page *pg;
> @@ -5038,38 +5038,8 @@ void ftrace_module_init(struct module *mod)
>  	ftrace_process_locs(mod, mod->ftrace_callsites,
>  			    mod->ftrace_callsites + mod->num_ftrace_callsites);
>  }
> -
> -static int ftrace_module_notify(struct notifier_block *self,
> -				unsigned long val, void *data)
> -{
> -	struct module *mod = data;
> -
> -	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(struct notifier_block *self,
> -				unsigned long val, void *data)
> -{
> -	return 0;
> -}
>  #endif /* CONFIG_MODULES */
>  
> -struct notifier_block ftrace_module_nb = {
> -	.notifier_call = ftrace_module_notify,
> -	.priority = INT_MIN,	/* Run after anything that can remove kprobes */
> -};
> -
>  void __init ftrace_init(void)
>  {
>  	extern unsigned long __start_mcount_loc[];
> @@ -5098,10 +5068,6 @@ void __init ftrace_init(void)
>  				  __start_mcount_loc,
>  				  __stop_mcount_loc);
>  
> -	ret = register_module_notifier(&ftrace_module_nb);
> -	if (ret)
> -		pr_warning("Failed to register trace ftrace module exit notifier\n");
> -
>  	set_ftrace_early_filters();
>  
>  	return;
> -- 
> 2.4.3
> 

-- 
Josh

Re: [PATCH v5] ftrace/module: remove ftrace module notifier

[Miroslav Benes]

On Tue, 16 Feb 2016, Jessica Yu wrote:

> Remove the ftrace module notifier in favor of directly calling
> ftrace_module_enable() and ftrace_release_mod() in the module loader.
> Hard-coding the function calls directly in the module loader removes
> dependence on the module notifier call chain and provides better
> visibility and control over what gets called when, which is important
> to kernel utilities such as livepatch.
> 
> This fixes a notifier ordering issue in which the ftrace module notifier
> (and hence ftrace_module_enable()) for coming modules was being called
> after klp_module_notify(), which caused livepatch modules to initialize
> incorrectly. This patch removes dependence on the module notifier call
> chain in favor of hard coding the corresponding function calls in the
> module loader. This ensures that ftrace and livepatch code get called in
> the correct order on patch module load and unload.
> 
> Fixes: 5156dca34a3e ("ftrace: Fix the race between ftrace and insmod")
> Signed-off-by: Jessica Yu <jeyu@redhat.com>
> Reviewed-by: Steven Rostedt <rostedt@goodmis.org>
> Reviewed-by: Petr Mladek <pmladek@suse.cz>
> Acked-by: Rusty Russell <rusty@rustcorp.com.au>

Reviewed-by: Miroslav Benes <mbenes@suse.cz>

Miroslav

Re: [PATCH v5] ftrace/module: remove ftrace module notifier

[Jiri Kosina]

On Tue, 16 Feb 2016, Jessica Yu wrote:

> Remove the ftrace module notifier in favor of directly calling
> ftrace_module_enable() and ftrace_release_mod() in the module loader.
> Hard-coding the function calls directly in the module loader removes
> dependence on the module notifier call chain and provides better
> visibility and control over what gets called when, which is important
> to kernel utilities such as livepatch.
> 
> This fixes a notifier ordering issue in which the ftrace module notifier
> (and hence ftrace_module_enable()) for coming modules was being called
> after klp_module_notify(), which caused livepatch modules to initialize
> incorrectly. This patch removes dependence on the module notifier call
> chain in favor of hard coding the corresponding function calls in the
> module loader. This ensures that ftrace and livepatch code get called in
> the correct order on patch module load and unload.
> 
> Fixes: 5156dca34a3e ("ftrace: Fix the race between ftrace and insmod")
> Signed-off-by: Jessica Yu <jeyu@redhat.com>
> Reviewed-by: Steven Rostedt <rostedt@goodmis.org>

Steven, I am going to assume that your Reviewed-by: still holds for the 
patch even though it has been removed from the 4-patch series; if you 
don't agree, please speak up. Otherwise, I am going to apply it and push 
it to Linus for -rc5 still (as it fixes our regression).

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH v5] ftrace/module: remove ftrace module notifier

[Steven Rostedt]

On Wed, 17 Feb 2016 21:35:29 +0100 (CET)
Jiri Kosina <jikos@kernel.org> wrote:

> On Tue, 16 Feb 2016, Jessica Yu wrote:
> 
> > Remove the ftrace module notifier in favor of directly calling
> > ftrace_module_enable() and ftrace_release_mod() in the module loader.
> > Hard-coding the function calls directly in the module loader removes
> > dependence on the module notifier call chain and provides better
> > visibility and control over what gets called when, which is important
> > to kernel utilities such as livepatch.
> > 
> > This fixes a notifier ordering issue in which the ftrace module notifier
> > (and hence ftrace_module_enable()) for coming modules was being called
> > after klp_module_notify(), which caused livepatch modules to initialize
> > incorrectly. This patch removes dependence on the module notifier call
> > chain in favor of hard coding the corresponding function calls in the
> > module loader. This ensures that ftrace and livepatch code get called in
> > the correct order on patch module load and unload.
> > 
> > Fixes: 5156dca34a3e ("ftrace: Fix the race between ftrace and insmod")
> > Signed-off-by: Jessica Yu <jeyu@redhat.com>
> > Reviewed-by: Steven Rostedt <rostedt@goodmis.org>  
> 
> Steven, I am going to assume that your Reviewed-by: still holds for the 
> patch even though it has been removed from the 4-patch series; if you 
> don't agree, please speak up. Otherwise, I am going to apply it and push 
> it to Linus for -rc5 still (as it fixes our regression).
> 

This week has been very busy. Did the patch change at all?

-- Steve

Re: [PATCH v5] ftrace/module: remove ftrace module notifier

[Jiri Kosina]

On Wed, 17 Feb 2016, Steven Rostedt wrote:

> This week has been very busy. Did the patch change at all?

It's the same as the one you've sent your Reviewed-by on Thu, 4 Feb 2016 
08:29:34 -0500 (20160204082934.5ff048bb@gandalf.local.home).

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH v5] ftrace/module: remove ftrace module notifier

[Steven Rostedt]

On Wed, 17 Feb 2016 21:44:42 +0100 (CET)
Jiri Kosina <jikos@kernel.org> wrote:

> On Wed, 17 Feb 2016, Steven Rostedt wrote:
> 
> > This week has been very busy. Did the patch change at all?  
> 
> It's the same as the one you've sent your Reviewed-by on Thu, 4 Feb 2016 
> 08:29:34 -0500 (20160204082934.5ff048bb@gandalf.local.home).
> 

OK, then keep the review, if nothing has changed.

-- Steve

Re: [PATCH v5] ftrace/module: remove ftrace module notifier

[Jiri Kosina]

On Tue, 16 Feb 2016, Jessica Yu wrote:

> Remove the ftrace module notifier in favor of directly calling
> ftrace_module_enable() and ftrace_release_mod() in the module loader.
> Hard-coding the function calls directly in the module loader removes
> dependence on the module notifier call chain and provides better
> visibility and control over what gets called when, which is important
> to kernel utilities such as livepatch.
> 
> This fixes a notifier ordering issue in which the ftrace module notifier
> (and hence ftrace_module_enable()) for coming modules was being called
> after klp_module_notify(), which caused livepatch modules to initialize
> incorrectly. This patch removes dependence on the module notifier call
> chain in favor of hard coding the corresponding function calls in the
> module loader. This ensures that ftrace and livepatch code get called in
> the correct order on patch module load and unload.

Applied to for-4.5/upstream-fixes. Thanks,

-- 
Jiri Kosina
SUSE Labs