Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932607AbbELMTU (ORCPT ); Tue, 12 May 2015 08:19:20 -0400 Received: from mail-wg0-f54.google.com ([74.125.82.54]:35632 "EHLO mail-wg0-f54.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932412AbbELMTR (ORCPT ); Tue, 12 May 2015 08:19:17 -0400 Date: Tue, 12 May 2015 20:19:23 +0800 From: Minfei Huang To: Miroslav Benes Cc: jpoimboe@redhat.com, sjenning@redhat.com, jkosina@suse.cz, vojtech@suse.cz, live-patching@vger.kernel.org, linux-kernel@vger.kernel.org, mhuang@redhat.com Subject: Re: [PATCH v2] livepatch: Prevent livepatch to apply the uninitialized patch Message-ID: <20150512121923.GA42505@dhcp-129-218.nay.redhat.com> References: <1431418935-42227-1-git-send-email-mnfhuang@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: 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 Content-Length: 5528 Lines: 155 On 05/12/15 at 01:41P, Miroslav Benes wrote: > On Tue, 12 May 2015, Minfei Huang wrote: > > > The previous patches can be applied, once the corresponding module is > > loaded. In general, the patch will do relocation (if necessary) and > > obtain/verify function address before we start to enable patch. > > > > In some case, the uninitialized patch can be applied to the kernel. > > Following is the case to describe the scenario step by step. > > > > 1) Patch a patch to the kernel before the corresponding module being > > loaded. > > 2) Call klp_module_notify_coming to enable the patch, once the module > > is loaded. > > 3) Do the instruction "obj->mod = mod", whatever the result of > > klp_module_notify_coming is > > 4) Fail to call the klp_init_object_loaded or klp_enable_object > > 5) klp_module_notify_coming returns, now the module is working > > > > 6) Enable the patch from the userspace (disable patch firstly, then > > enable the patch via sysfs) > > 7) Call __klp_enable_patch to enable patch > > 8) Pass the limitation (klp_init_object_loaded), because the value > > of obj->mod is not NULL (obtain the value from step 3) > > 9) Patch is applied, though it is uninitialized (do not relocate > > and obtain old_addr) > > > > It is fatal to kernel, once the uninitialized patch is appled. To > > fix it, obj->mod will nerver obtain the value, if livepatch fails > > to call the klp_module_notify_coming. > > Hi, > > I still think the changelog needs to be improved a bit. > > There are three different situations in which the coming module notifier > can fail: > > 1. relocations are not applied for some reason. In this case kallsyms for > module symbol is not called at all. The patch is not applied to the > module. If the user disable and enable patch again, there is possible bug > in klp_enable_func. If the user specified func->old_addr for some function > in the module (and he shouldn't do that, but nevertheless) our warning > would not catch it, ftrace will fail in the process and the patch is > not enabled. The function may be registered successfully by ftrace, although the function address is incorrect. > > 2. relocations are applied successfully, but kallsyms lookup fails. In > this case func->old_addr can be correct for all previous lookups, 0 for > current failed one, and "unspecified" for the rest. If we undergo the same > scenario as in 1. the behaviour differs for three cases, but the patch is > not enabled anyway. > > 3. the object is initialized but klp_enable_object fails in the notifier > due to possible ftrace error. Since it is improbable that ftrace would > heal itself in the future, we would get those errors everytime the patch > is enabled. > > Your patch fixes all three cases, but consider to change the changelog to > describe it all. Thanks for your effort about the patch log. It is more clearly to the patch log as you suggested. I will modify the patch log, then post the next version. > > See few more remarks below. > > > > > Signed-off-by: Minfei Huang > > --- > > v1: > > - modify the commit log, describe the issue more details > > --- > > kernel/livepatch/core.c | 23 +++++++++++++---------- > > 1 file changed, 13 insertions(+), 10 deletions(-) > > > > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c > > index 284e269..4bbcdda 100644 > > --- a/kernel/livepatch/core.c > > +++ b/kernel/livepatch/core.c > > @@ -883,30 +883,30 @@ int klp_register_patch(struct klp_patch *patch) > > } > > EXPORT_SYMBOL_GPL(klp_register_patch); > > > > -static void klp_module_notify_coming(struct klp_patch *patch, > > +static int klp_module_notify_coming(struct klp_patch *patch, > > struct klp_object *obj) > > { > > struct module *pmod = patch->mod; > > struct module *mod = obj->mod; > > - int ret; > > + int ret = 0; > > > > ret = klp_init_object_loaded(patch, obj); > > if (ret) > > - goto err; > > + goto out; > > > > if (patch->state == KLP_DISABLED) > > - return; > > + goto out; > > > > pr_notice("applying patch '%s' to loading module '%s'\n", > > pmod->name, mod->name); > > > > ret = klp_enable_object(obj); > > - if (!ret) > > - return; > > > > -err: > > - pr_warn("failed to apply patch '%s' to module '%s' (%d)\n", > > - pmod->name, mod->name, ret); > > +out: > > + if (ret) > > + pr_warn("failed to apply patch '%s' to module '%s' (%d)\n", > > + pmod->name, mod->name, ret); > > I think this message should be extended. We failed to apply the patch to > this object and we won't ever try that again. The user should know that. I will modify the error message. > > Also note, that this warning is printed in two different situations. > Either the initialization failed, or klp_enable_object failed. Maybe it > would be nice to inform the user about that. > > > + return ret; > > } > > > > static void klp_module_notify_going(struct klp_patch *patch, > > @@ -930,6 +930,7 @@ disabled: > > static int klp_module_notify(struct notifier_block *nb, unsigned long action, > > void *data) > > { > > + int ret = 0; > > Do we really need the initialization? klp_module_notify_coming returns the > error or 0 by itself. > Will modify. Thanks Minfei -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/