From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756234AbbCETfr (ORCPT ); Thu, 5 Mar 2015 14:35:47 -0500 Received: from mx1.redhat.com ([209.132.183.28]:52834 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751500AbbCETfp (ORCPT ); Thu, 5 Mar 2015 14:35:45 -0500 Date: Thu, 5 Mar 2015 13:34:33 -0600 From: Josh Poimboeuf To: Petr Mladek Cc: Seth Jennings , Jiri Kosina , Rusty Russell , Miroslav Benes , Masami Hiramatsu , mingo@kernel.org, mathieu.desnoyers@efficios.com, oleg@redhat.com, paulmck@linux.vnet.ibm.com, live-patching@vger.kernel.org, linux-kernel@vger.kernel.org, andi@firstfloor.org, rostedt@goodmis.org, tglx@linutronix.de Subject: Re: [PATCH v2 1/2] livepatch/module: Apply patch when loaded module is unformed Message-ID: <20150305193433.GC1870@treble.redhat.com> References: <1425570314-23675-1-git-send-email-pmladek@suse.cz> <1425570314-23675-2-git-send-email-pmladek@suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <1425570314-23675-2-git-send-email-pmladek@suse.cz> 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 Thu, Mar 05, 2015 at 04:45:13PM +0100, Petr Mladek wrote: > Existing live patches are applied to loaded modules using a notify handler. > There are two problems with this approach. > > First, errors from module notifiers are ignored and could not stop the module > from being loaded. But we will need to refuse the module when there are > semantics dependencies between functions and there are some problems > to apply the patch to the module. Otherwise, the system might become > into an inconsistent state. > > Second, the module notifiers are called when the module is in > STATE_MODULE_COMING. It means that it is visible by find_module() > and can be detected by klp_find_object_module() when a new patch is > registered. > > Now, the timing is important. If the new patch is registered after the module > notifier has been called, it has to initialize the module object for the new > patch. Note that, in this case, the new patch has to see the module as loaded > even when it is still in the COMING state. > > But when the new patch is registered before the module notifier, it _should_ > not initialize the module object, see below for detailed explanation. > > This patch solves both problems by calling klp_module_init() directly in > load_module(). We could handle the error there. Also it is called in > MODULE_STATE_UNFORMED and therefore before the module is visible via > find_module(). > > The implementation creates three functions for module init and three > functions for going modules. We need to revert already initialized > patches when something fails and thus need to be able to call > the code for going modules without leaving klp_mutex. > > Detailed explanation of the last problem: > > Why should not we initialize the module object for a new patch when > the related module coming notifier has not been called yet? > > Note that the notifier could _not_ _simply_ ignore already initialized module > objects. The notifier initializes the module object for all existing patches. > If the new patch is registered and enabled before, it would crate wrong > order of patches in fops->func_stack. > > For example, let's have three patches (P1, P2, P3) for the functions a() > and b() where a() is from vmcore and b() is from a module M. Something > like: > > a() b() > P1 a1() b1() > P2 a2() b2() > P3 a3() b3(3) > > If you load the module M after all patches are registered and enabled. > The ftrace ops for function a() and b() has listed the functions in this > order > > ops_a->func_stack -> list(a3,a2,a1) > ops_b->func_stack -> list(b3,b2,b1) > > , so the pointer to b3() is the first and will be used. > > Then you might have the following scenario. Let's start with state > when patches P1 and P2 are registered and enabled but the module M > is not loaded. Then ftrace ops for b() does not exist. Then we > get into the following race: > > CPU0 CPU1 > > load_module(M) > > complete_formation() > > mod->state = MODULE_STATE_COMING; > mutex_unlock(&module_mutex); > > klp_register_patch(P3); > klp_enable_patch(P3); > > # STATE 1 > > klp_module_notify(M) > klp_module_notify_coming(P1); > klp_module_notify_coming(P2); > klp_module_notify_coming(P3); > > # STATE 2 > > The ftrace ops for a() and b() then looks: > > STATE1: > > ops_a->func_stack -> list(a3,a2,a1); > ops_b->func_stack -> list(b3); > > STATE2: > ops_a->func_stack -> list(a3,a2,a1); > ops_b->func_stack -> list(b2,b1,b3); > > therefore, b2() is used for the module but a3() is used for vmcore > because they were the last added. > > Signed-off-by: Petr Mladek > --- > include/linux/livepatch.h | 10 +++++ > kernel/livepatch/core.c | 94 +++++++++++++++++++++++++++++++++++------------ > kernel/module.c | 9 +++++ > 3 files changed, 89 insertions(+), 24 deletions(-) Not sure why, but "git am" seemed to think this patch was malformed. It applied ok for me after I removed the diffstat. > > diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h > index ee6dbb39a809..78ac10546160 100644 > --- a/include/linux/livepatch.h > +++ b/include/linux/livepatch.h > @@ -128,6 +128,16 @@ int klp_unregister_patch(struct klp_patch *); > int klp_enable_patch(struct klp_patch *); > int klp_disable_patch(struct klp_patch *); > > +int klp_module_init(struct module *mod); > + > +#else /* CONFIG_LIVEPATCH */ > + > +inline int klp_module_init(struct module *mod) Should it not be "static inline"? /me prays not to have to break out the C spec again ;-) > +{ > + return 0; > +} > + > #endif /* CONFIG_LIVEPATCH */ > > + > #endif /* _LINUX_LIVEPATCH_H_ */ > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c > index 7e0c83dc7215..198f7733604b 100644 > --- a/kernel/livepatch/core.c > +++ b/kernel/livepatch/core.c > @@ -869,8 +869,8 @@ int klp_register_patch(struct klp_patch *patch) > } > EXPORT_SYMBOL_GPL(klp_register_patch); > > -static void klp_module_notify_coming(struct klp_patch *patch, > - struct klp_object *obj) > +static int klp_module_coming_update_patch(struct klp_patch *patch, > + struct klp_object *obj) This function name confused me a little bit. Not sure what would be better, but it really updates the object, not the patch. Maybe klp_module_coming_object()? > { > struct module *pmod = patch->mod; > struct module *mod = obj->mod; > @@ -881,22 +881,62 @@ static void klp_module_notify_coming(struct klp_patch *patch, > goto err; > > if (patch->state == KLP_DISABLED) > - return; > + return 0; > > pr_notice("applying patch '%s' to loading module '%s'\n", > pmod->name, mod->name); > > ret = klp_enable_object(obj); > if (!ret) > - return; > + return 0; > > err: > pr_warn("failed to apply patch '%s' to module '%s' (%d)\n", > pmod->name, mod->name, ret); Does it still make sense to have this pr_warn() here now that we can return an error and stop the module from loading? I'm thinking it should be changed to pr_err() to be consistent with the other klp error printks, and should probably say that we're preventing the module from loading. > + return ret; > } > > -static void klp_module_notify_going(struct klp_patch *patch, > - struct klp_object *obj) > +static void klp_module_going(struct module *mod); It would probably be better to move klp_module_going() here so you don't have to forward declare it. > + > +int klp_module_coming(struct module *mod) > +{ > + struct klp_patch *patch; > + struct klp_object *obj; > + int ret = 0; > + > + list_for_each_entry(patch, &klp_patches, list) { > + for (obj = patch->objs; obj->funcs; obj++) { > + if (!klp_is_module(obj) || strcmp(obj->name, mod->name)) > + continue; > + > + obj->mod = mod; > + ret = klp_module_coming_update_patch(patch, obj); > + if (ret) > + goto err; > + } > + } > + > + return 0; > + > +err: > + klp_module_going(mod); > + return ret; > +} > + > + > +int klp_module_init(struct module *mod) > +{ > + int ret = 0; > + > + mutex_lock(&klp_mutex); > + ret = klp_module_coming(mod); > + mutex_unlock(&klp_mutex); > + > + return ret; > +} > + > +static void klp_module_going_update_patch(struct klp_patch *patch, > + struct klp_object *obj) > { > struct module *pmod = patch->mod; > struct module *mod = obj->mod; > @@ -913,40 +953,46 @@ disabled: > klp_free_object_loaded(obj); > } > > -static int klp_module_notify(struct notifier_block *nb, unsigned long action, > - void *data) > +static void klp_module_going(struct module *mod) > { > - struct module *mod = data; > struct klp_patch *patch; > struct klp_object *obj; > > - if (action != MODULE_STATE_COMING && action != MODULE_STATE_GOING) > - return 0; > - > - mutex_lock(&klp_mutex); > - > list_for_each_entry(patch, &klp_patches, list) { > for (obj = patch->objs; obj->funcs; obj++) { > - if (!klp_is_module(obj) || strcmp(obj->name, mod->name)) > + /* > + * Handle only loaded (initialized) modules. > + * This is needed when used in an error path. > + */ > + if (!klp_is_object_loaded(obj) || > + strcmp(obj->name, mod->name)) Also need a klp_is_module() check here so it doesn't send NULL to strcmp in the case of vmlinux. > continue; > > - if (action == MODULE_STATE_COMING) { > - obj->mod = mod; > - klp_module_notify_coming(patch, obj); > - } else /* MODULE_STATE_GOING */ > - klp_module_notify_going(patch, obj); > - > - break; > + klp_module_going_update_patch(patch, obj); > } > } > > - mutex_unlock(&klp_mutex); > + return; Redundant return. > +} > + > +static int klp_module_notify_going(struct notifier_block *nb, > + unsigned long action, > + void *data) > +{ > + struct module *mod = data; > + > + if (action != MODULE_STATE_GOING) > + return 0; > + > + mutex_lock(&klp_mutex); > + klp_module_going(mod); > + mutex_lock(&klp_mutex); > > return 0; > } > > static struct notifier_block klp_module_nb = { > - .notifier_call = klp_module_notify, > + .notifier_call = klp_module_notify_going, > .priority = INT_MIN+1, /* called late but before ftrace notifier */ > }; > > diff --git a/kernel/module.c b/kernel/module.c > index d856e96a3cce..f744a639460d 100644 > --- a/kernel/module.c > +++ b/kernel/module.c > @@ -53,6 +53,7 @@ > #include > #include > #include > +#include > #include > #include > #include > @@ -3321,6 +3322,14 @@ static int load_module(struct load_info *info, const char __user *uargs, > /* Ftrace init must be called in the MODULE_STATE_UNFORMED state */ > ftrace_module_init(mod); > > + /* > + * LivePatch init must be called in the MODULE_STATE_UNFORMED state > + * and it might reject the module to avoid a system inconsistency. > + */ nit: I thought we were calling it livepatch (all lowercase). > + err = klp_module_init(mod); > + if (err) > + goto ddebug_cleanup; > + > /* Finally it's fully formed, ready to start executing. */ > err = complete_formation(mod, info); > if (err) Hm, we still have a problem with the timing here. The kallsyms lookup functions ignore MODULE_STATE_UNFORMED modules. So klp_find_verify_func_addr() will fail to find the func address and the module will always fail to load. -- Josh