From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758470AbbCDORn (ORCPT ); Wed, 4 Mar 2015 09:17:43 -0500 Received: from cantor2.suse.de ([195.135.220.15]:51486 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757595AbbCDORm (ORCPT ); Wed, 4 Mar 2015 09:17:42 -0500 Date: Wed, 4 Mar 2015 15:18:25 +0100 From: Petr Mladek To: Josh Poimboeuf Cc: Seth Jennings , Jiri Kosina , Vojtech Pavlik , live-patching@vger.kernel.org, linux-kernel@vger.kernel.org, Rusty Russell , Miroslav Benes , Masami Hiramatsu , mingo@kernel.org, mathieu.desnoyers@efficios.com, oleg@redhat.com, paulmck@linux.vnet.ibm.com, andi@firstfloor.org, rostedt@goodmis.org, tglx@linutronix.de Subject: Re: [PATCH 2/2] livepatch: fix patched module loading race Message-ID: <20150304141825.GE15177@pathway.suse.cz> References: <297cb260437910c43bf0ec85f53578f6cfef6d88.1425423640.git.jpoimboe@redhat.com> <20150304131752.GD15177@pathway.suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150304131752.GD15177@pathway.suse.cz> 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 Wed 2015-03-04 14:17:52, Petr Mladek wrote: > On Tue 2015-03-03 17:02:22, Josh Poimboeuf wrote: > > It's possible for klp_register_patch() to see a module before the COMING > > notifier is called, or after the GOING notifier is called. > > > > That can cause all kinds of ugly races. As Pter Mladek reported: > > > > "The problem is that we do not keep the klp_mutex lock all the time when > > the module is being added or removed. > > > > First, the module is visible even before ftrace is ready. If we enable a patch > > in this time frame, adding ftrace ops will fail and the patch will get rejected > > just because bad timing. > > Ah, this is not true after all. I did not properly check when > MODULE_STATE_COMING was set. I though that it was before ftrace was > initialized but it was not true. > > > > Second, if we are "lucky" and enable the patch for the coming module when the > > ftrace is ready but before the module notifier has been called. The notifier > > will try to enable the patch as well. It will detect that it is already patched, > > return error, and the module will get rejected just because bad > > timing. The more serious problem is that it will not call the notifier for > > going module, so that the mess will stay there and we wont be able to load > > the module later. > > Ah, the race is there but the effect is not that serious in the > end. It seems that errors from module notifiers are ignored. In fact, > we do not propagate the error from klp_module_notify_coming(). It means > that WARN() from klp_enable_object() will be printed but the module > will be loaded and patched. > > I am sorry, I was confused by kGraft where kgr_module_init() was > called directly from module_load(). The errors were propagated. It > means that kGraft rejects module when the patch cannot be applied. > > Note that the current solution is perfectly fine for the simple > consistency model. > > > > Third, similar problems are there for going module. If a patch is enabled after > > the notifier finishes but before the module is removed from the list of modules, > > the new patch will be applied to the module. The module might disappear at > > anytime when the patch enabling is in progress, so there might be an access out > > of memory. Or the whole patch might be applied and some mess will be left, > > so it will not be possible to load/patch the module again." > > This is true. > > > > Fix these races by letting the first one who sees the module do the > > needed work. > > > > Reported-by: Petr Mladek > > Signed-off-by: Josh Poimboeuf > > --- > > kernel/livepatch/core.c | 53 +++++++++++++++++++++++++++++++++++++++++++++---- > > @@ -965,10 +990,30 @@ static int klp_module_notify(struct notifier_block *nb, unsigned long action, > > continue; > > > > if (action == MODULE_STATE_COMING) { > > + > > + /* > > + * Check for a small window where the register > > + * path already initialized the object. > > + */ > s/path/patch/ > > > > > + if (obj->mod) > > + continue; > > This might break stacking. The recently registered patch might become > the last on the stack and thus unused. Going through the stack when registering new patch would be quite ugly. I am going to provide yet another solution. Best Regards, Petr