From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757054AbcBDUzP (ORCPT ); Thu, 4 Feb 2016 15:55:15 -0500 Received: from mx1.redhat.com ([209.132.183.28]:36439 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753195AbcBDUzN (ORCPT ); Thu, 4 Feb 2016 15:55:13 -0500 Date: Thu, 4 Feb 2016 14:55:11 -0600 From: Josh Poimboeuf To: Jessica Yu Cc: Seth Jennings , Jiri Kosina , Vojtech Pavlik , Miroslav Benes , Rusty Russell , Steven Rostedt , Ingo Molnar , live-patching@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v2 2/2] livepatch/module: remove livepatch module notifier Message-ID: <20160204205511.GO22854@treble.redhat.com> References: <1454375856-27757-1-git-send-email-jeyu@redhat.com> <1454375856-27757-3-git-send-email-jeyu@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <1454375856-27757-3-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 Mon, Feb 01, 2016 at 08:17:36PM -0500, Jessica Yu wrote: > Remove the livepatch module notifier in favor of directly enabling and > disabling patches to modules in the module loader. Hard-coding the > function calls ensures that ftrace_module_enable() is run before > klp_module_enable() during module load, and that klp_module_disable() is > run before ftrace_release_mod() during module unload. This way, ftrace > and livepatch code is run in the correct order during the module > load/unload sequence without dependence on the module notifier call chain. > > 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. > > Signed-off-by: Jessica Yu > --- > include/linux/livepatch.h | 9 +++ > kernel/livepatch/core.c | 144 ++++++++++++++++++++++------------------------ > kernel/module.c | 8 +++ > 3 files changed, 86 insertions(+), 75 deletions(-) > > diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h > index a882865..fdd5f1c 100644 > --- a/include/linux/livepatch.h > +++ b/include/linux/livepatch.h > @@ -134,6 +134,15 @@ int klp_unregister_patch(struct klp_patch *); > int klp_enable_patch(struct klp_patch *); > int klp_disable_patch(struct klp_patch *); > > +/* Called from the module loader during module coming/going states */ > +extern int klp_module_enable(struct module *mod); > +extern void klp_module_disable(struct module *mod); > + > +#else /* !CONFIG_LIVEPATCH */ > + > +static inline int klp_module_enable(struct module *mod) { return 0; } > +static inline void klp_module_disable(struct module *mod) { } > + > #endif /* CONFIG_LIVEPATCH */ > > #endif /* _LINUX_LIVEPATCH_H_ */ > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c > index bc2c85c..7aa975d 100644 > --- a/kernel/livepatch/core.c > +++ b/kernel/livepatch/core.c > @@ -103,7 +103,7 @@ static void klp_find_object_module(struct klp_object *obj) > */ > mod = find_module(obj->name); > /* > - * Do not mess work of the module coming and going notifiers. > + * Do not mess work of the klp module coming and going handlers. > * Note that the patch might still be needed before the going handler > * is called. Module functions can be called even in the GOING state > * until mod->exit() finishes. This is especially important for > @@ -866,103 +866,107 @@ int klp_register_patch(struct klp_patch *patch) > } > EXPORT_SYMBOL_GPL(klp_register_patch); > > -static int klp_module_notify_coming(struct klp_patch *patch, > - struct klp_object *obj) > +/* Called when module state is MODULE_STATE_COMING */ > +int klp_module_enable(struct module *mod) I think this function name was originally my idea. But now I'm thinking it could cause some confusion with the similarly named klp_enable_object(). How about naming it klp_module_coming()? That more accurately describes its purpose IMO and it would also make the comment above it no longer necessary. And similarly we could rename klp_module_disable() -> klp_module_going(). -- Josh