From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933286AbbCDR5z (ORCPT ); Wed, 4 Mar 2015 12:57:55 -0500 Received: from mx1.redhat.com ([209.132.183.28]:37783 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933145AbbCDR5x (ORCPT ); Wed, 4 Mar 2015 12:57:53 -0500 Date: Wed, 4 Mar 2015 11:57:12 -0600 From: Josh Poimboeuf To: Petr Mladek 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: <20150304175712.GD5464@treble.hsd1.ky.comcast.net> References: <297cb260437910c43bf0ec85f53578f6cfef6d88.1425423640.git.jpoimboe@redhat.com> <20150304131752.GD15177@pathway.suse.cz> <20150304153415.GB5464@treble.hsd1.ky.comcast.net> <20150304163611.GF15177@pathway.suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20150304163611.GF15177@pathway.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 Wed, Mar 04, 2015 at 05:36:11PM +0100, Petr Mladek wrote: > For example, let's have three patches (P1, P2, P3) for the functions a() and b() > where a() is from vmcore and b() is from a module M. Something like: > > a() b() > P1 a1() b1() > P2 a2() b2() > P3 a3() b3(3) > > If you load the module M after all patches are registered and enabled. > The ftrace ops for function a() and b() has listed the functions in this > order > > ops_a->func_stack -> list(a3,a2,a1) > ops_b->func_stack -> list(b3,b2,b1) > > , so the pointer to b3() is the first and will be used. > > Then you might have the following scenario. Let's start with state > when patches P1 and P2 are registered and enabled but the module M > is not loaded. Then ftrace ops for b() does not exist. Then we > get into the following race: > > > CPU0 CPU1 > > load_module(M) > > complete_formation() > > mod->state = MODULE_STATE_COMING; > mutex_unlock(&module_mutex); > > klp_register_patch(P3); > klp_enable_patch(P3); > > # STATE 1 > > > klp_module_notify(M) > klp_module_notify_coming(P1); > klp_module_notify_coming(P2); > klp_module_notify_coming(P3); > > # STATE 2 > > > The ftrace ops for a() and b() then looks: > > STATE1: > > ops_a->func_stack -> list(a3,a2,a1); > ops_b->func_stack -> list(b3); > > STATE2: > ops_a->func_stack -> list(a3,a2,a1); > ops_b->func_stack -> list(b2,b1,b3); > > therefore, b2() is used for the module but a3() is used for vmcore > because they were the last added. Thanks for the excellent explanation. That makes sense. > My plan is to fix this problem by calling klp_module_init() directly > in load_module() just after ftrace_module_init(). It will solve this > problem because it will be called in MODULE_STATE_UNFORMED. Ok, looking forward to that. > It will have another big advantage. It will allow to pass the error > code and refuse loading modules that could not get patched. This will > be needed for the more complex patches anyway. We have to prevent > running module code that is inconsistent with the patched system. Yeah, that does need to be fixed up too. > I am still in doubts how to best solve the problem for going modules. > Your suggested solution is fine for now. But we will need a better fix > after adding the more complex consistency model. Well, we could just get a reference on all patched modules to prevent them from being unloaded. -- Josh