(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)" <rost...@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.ta...@jp.fujitsu.com
>
> Reported-by: Takao Indoh <indou.ta...@jp.fujitsu.com>
> Cc: sta...@vger.kernel.org # 2.6.38+
> Signed-off-by: Steven Rostedt <rost...@goodmis.org>
Looks good to me.
Reviewed-by: Masami Hiramatsu <masami.hiramatsu...@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...@hitachi.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
