From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S965470AbcCNPGs (ORCPT ); Mon, 14 Mar 2016 11:06:48 -0400 Received: from mx2.suse.de ([195.135.220.15]:34570 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S965378AbcCNPGq (ORCPT ); Mon, 14 Mar 2016 11:06:46 -0400 Date: Mon, 14 Mar 2016 16:06:43 +0100 From: Petr Mladek To: Jessica Yu Cc: Josh Poimboeuf , Seth Jennings , Jiri Kosina , Vojtech Pavlik , Miroslav Benes , Rusty Russell , Steven Rostedt , live-patching@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v2 2/2] livepatch/module: remove livepatch module notifier Message-ID: <20160314150643.GU10940@pathway.suse.cz> References: <1457726628-9171-1-git-send-email-jeyu@redhat.com> <1457726628-9171-3-git-send-email-jeyu@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1457726628-9171-3-git-send-email-jeyu@redhat.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri 2016-03-11 15:03:48, 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_coming() during module load, and that klp_module_going() 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. > > Signed-off-by: Jessica Yu > Acked-by: Josh Poimboeuf > Acked-by: Rusty Russell > --- > include/linux/livepatch.h | 13 ++++ > kernel/livepatch/core.c | 147 ++++++++++++++++++++++------------------------ > kernel/module.c | 10 ++++ > 3 files changed, 94 insertions(+), 76 deletions(-) > > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c > index 780f00c..4fafbae 100644 > --- a/kernel/livepatch/core.c > +++ b/kernel/livepatch/core.c > @@ -866,103 +866,108 @@ 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) > +int klp_module_coming(struct module *mod) > { > - struct module *pmod = patch->mod; > - struct module *mod = obj->mod; > int ret; > + struct klp_patch *patch; > + struct klp_object *obj; > > - ret = klp_init_object_loaded(patch, obj); > - if (ret) { > - pr_warn("failed to initialize patch '%s' for module '%s' (%d)\n", > - pmod->name, mod->name, ret); > - return ret; > - } > + if (WARN_ON(mod->state != MODULE_STATE_COMING)) > + return -EINVAL; > > - if (patch->state == KLP_DISABLED) > - return 0; > + mutex_lock(&klp_mutex); > + /* > + * Each module has to know that klp_module_coming() > + * has been called. We never know what module will > + * get patched by a new patch. > + */ > + mod->klp_alive = true; > > - pr_notice("applying patch '%s' to loading module '%s'\n", > - pmod->name, mod->name); > + list_for_each_entry(patch, &klp_patches, list) { > + klp_for_each_object(patch, obj) { > + if (!klp_is_module(obj) || strcmp(obj->name, mod->name)) > + continue; > > - ret = klp_enable_object(obj); > - if (ret) > - pr_warn("failed to apply patch '%s' to module '%s' (%d)\n", > - pmod->name, mod->name, ret); > - return ret; > -} > + obj->mod = mod; > > -static void klp_module_notify_going(struct klp_patch *patch, > - struct klp_object *obj) > -{ > - struct module *pmod = patch->mod; > - struct module *mod = obj->mod; > + ret = klp_init_object_loaded(patch, obj); > + if (ret) { > + pr_warn("failed to initialize patch '%s' for module '%s' (%d)\n", > + patch->mod->name, obj->mod->name, ret); > + goto err; > + } > > - if (patch->state == KLP_DISABLED) > - goto disabled; > + if (patch->state == KLP_DISABLED) > + break; > + > + pr_notice("applying patch '%s' to loading module '%s'\n", > + patch->mod->name, obj->mod->name); > + > + ret = klp_enable_object(obj); > + if (ret) { > + pr_warn("failed to apply patch '%s' to module '%s' (%d)\n", > + patch->mod->name, obj->mod->name, ret); > + goto err; > + } > + > + break; > + } > + } > > - pr_notice("reverting patch '%s' on unloading module '%s'\n", > - pmod->name, mod->name); > + mutex_unlock(&klp_mutex); > > - klp_disable_object(obj); > + return 0; > > -disabled: > +err: > + /* > + * If a patch is unsuccessfully applied, return > + * error to the module loader. > + */ > + pr_warn("patch '%s' failed for module '%s', refusing to load module '%s'\n", > + patch->mod->name, obj->mod->name, obj->mod->name); One more thing. We should add here: mod->klp_alive = true; Otherwise, there is a small race window when a new patch will try to patch the module. > klp_free_object_loaded(obj); > + mutex_unlock(&klp_mutex); > + > + return ret; > } With the above fix: Reviewed-by: Petr Mladek Best Regards, Petr