From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932109AbbCCTcP (ORCPT ); Tue, 3 Mar 2015 14:32:15 -0500 Received: from mx1.redhat.com ([209.132.183.28]:41619 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756104AbbCCTcN (ORCPT ); Tue, 3 Mar 2015 14:32:13 -0500 Date: Tue, 3 Mar 2015 13:31:28 -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, linux-kernel@vger.kernel.org, andi@firstfloor.org, rostedt@goodmis.org, tglx@linutronix.de Subject: Re: [RFC PATCH] livepatch/module: Do not patch modules that are not ready Message-ID: <20150303193128.GA31987@treble.redhat.com> References: <1425382709-9934-1-git-send-email-pmladek@suse.cz> <20150303145517.GA16889@treble.redhat.com> <20150303173456.GH3703@dhcp128.suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20150303173456.GH3703@dhcp128.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 Tue, Mar 03, 2015 at 06:34:57PM +0100, Petr Mladek wrote: > Ah, I have missed the comments in the code in the first reply. > > Why do we need two flags for this? The notifer already > > sets/clears obj->mod, so can we rely on the value obj->mod to determine > > if the notifier already ran? > > We need this for new patches. There we do not know if the module > notifier has already been called or not, see also below. > > Of course, we might use more optimal storage for the two flags > if requested. For example, two bits in an unsigned char. > > > > For example: > > > > @@ -89,7 +89,7 @@ static bool klp_is_object_loaded(struct klp_object *obj) > > /* sets obj->mod if object is not vmlinux and module is found */ > > static void klp_find_object_module(struct klp_object *obj) > > { > > - if (!klp_is_module(obj)) > > + if (!klp_is_module(obj) || obj->mod) > > return; > > > > mutex_lock(&module_mutex); > > @@ -98,7 +98,9 @@ static void klp_find_object_module(struct klp_object *obj) > > * the klp_mutex, which is also taken by the module notifier. This > > * prevents any module from unloading until we release the klp_mutex. > > */ > > - obj->mod = find_module(obj->name); > > + mod = find_module(obj->name); > > + if (mod->state == MODULE_STATE_LIVE) > > The check of the MODULE_STATE_LIVE is not enough. We need to init > modules when a new patch is registered, the module is still in > MODULE_STATE_COMING and the module notify handler has already > been called. Ok, thanks for explaining. I think I got it now. But the two module flags aren't ideal. MODULE_STATE_COMING means that ftrace is already initialized, so I think it doesn't _have_ to be the notifier which does the work. Instead, can we just let it be done by whoever gets there first? Like this: diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c index 01ca088..05390ab 100644 --- a/kernel/livepatch/core.c +++ b/kernel/livepatch/core.c @@ -89,16 +89,29 @@ static bool klp_is_object_loaded(struct klp_object *obj) /* sets obj->mod if object is not vmlinux and module is found */ static void klp_find_object_module(struct klp_object *obj) { - if (!klp_is_module(obj)) + struct module *mod; + + if (!klp_is_module(obj) || obj->mod) return; mutex_lock(&module_mutex); + /* * We don't need to take a reference on the module here because we have * the klp_mutex, which is also taken by the module notifier. This * prevents any module from unloading until we release the klp_mutex. */ - obj->mod = find_module(obj->name); + mod = find_module(obj->name); + + /* + * MODULE_STATE_COMING means we got to the module first before the + * notifier did. ftrace is already initialized, so it's fine to go + * ahead and start using it. + */ + if (mod->state == MODULE_STATE_COMING || + mod->state == MODULE_STATE_LIVE) + obj->mod = mod; + mutex_unlock(&module_mutex); } @@ -697,8 +710,6 @@ static void klp_free_object_loaded(struct klp_object *obj) { struct klp_func *func; - obj->mod = NULL; - for (func = obj->funcs; func->old_name; func++) func->old_addr = 0; } @@ -967,10 +978,31 @@ static int klp_module_notify(struct notifier_block *nb, unsigned long action, continue; if (action == MODULE_STATE_COMING) { + + /* + * There's a small window where a register or + * enable call may have already done this + * initialization. First one there wins. + */ + if (obj->mod) + continue; + obj->mod = mod; klp_module_notify_coming(patch, obj); - } else /* MODULE_STATE_GOING */ + } else { + /* MODULE_STATE_GOING */ + + /* + * There's a small window where a register or + * enable call may have already done this + * teardown. First one there wins. + */ + if (!obj->mod) + continue; + klp_module_notify_going(patch, obj); + obj->mod = NULL; + } break; }