From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753545AbcCNRwb (ORCPT ); Mon, 14 Mar 2016 13:52:31 -0400 Received: from mx1.redhat.com ([209.132.183.28]:50116 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752799AbcCNRw0 (ORCPT ); Mon, 14 Mar 2016 13:52:26 -0400 Date: Mon, 14 Mar 2016 13:50:09 -0400 From: Jessica Yu To: Petr Mladek 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: livepatch/module: remove livepatch module notifier Message-ID: <20160314175008.GA1180@packer-debian-8-amd64.digitalocean.com> References: <1457726628-9171-1-git-send-email-jeyu@redhat.com> <1457726628-9171-3-git-send-email-jeyu@redhat.com> <20160314150643.GU10940@pathway.suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii; format=flowed Content-Disposition: inline In-Reply-To: <20160314150643.GU10940@pathway.suse.cz> X-OS: Linux eisen.io 3.16.0-4-amd64 x86_64 User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org +++ Petr Mladek [14/03/16 16:06 +0100]: >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. Ah, you are right. I think you meant false, though :-) Will fix that. >> klp_free_object_loaded(obj); >> + mutex_unlock(&klp_mutex); >> + >> + return ret; >> } > >With the above fix: > >Reviewed-by: Petr Mladek > > >Best Regards, >Petr