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@xxxxxxxxxx>
> Reviewed-by: Steven Rostedt <rostedt@xxxxxxxxxxx>
> Reviewed-by: Petr Mladek <pmladek@xxxxxxx>
> Acked-by: Rusty Russell <rusty@xxxxxxxxxxxxxxx>

Reviewed-by: Josh Poimboeuf <jpoimboe@xxxxxxxxxx>

> ---
> 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@xxxxxxxxxx 
> 
>  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