From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933691AbcBQCfG (ORCPT ); Tue, 16 Feb 2016 21:35:06 -0500 Received: from mx1.redhat.com ([209.132.183.28]:55324 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932311AbcBQCfE (ORCPT ); Tue, 16 Feb 2016 21:35:04 -0500 Date: Tue, 16 Feb 2016 20:35:01 -0600 From: Josh Poimboeuf To: Jessica Yu Cc: Seth Jennings , Jiri Kosina , Vojtech Pavlik , Miroslav Benes , Petr Mladek , Rusty Russell , Steven Rostedt , Ingo Molnar , live-patching@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v5] ftrace/module: remove ftrace module notifier Message-ID: <20160217023501.GF3018@treble.redhat.com> References: <1455661953-15838-1-git-send-email-jeyu@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <1455661953-15838-1-git-send-email-jeyu@redhat.com> User-Agent: Mutt/1.5.23.1-rc1 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > Reviewed-by: Steven Rostedt > Reviewed-by: Petr Mladek > Acked-by: Rusty Russell Reviewed-by: Josh Poimboeuf > --- > 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