From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752349AbbCJLPy (ORCPT ); Tue, 10 Mar 2015 07:15:54 -0400 Received: from cantor2.suse.de ([195.135.220.15]:33948 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750976AbbCJLPw (ORCPT ); Tue, 10 Mar 2015 07:15:52 -0400 Date: Tue, 10 Mar 2015 12:15:45 +0100 From: Petr Mladek To: Rusty Russell Cc: Seth Jennings , Josh Poimboeuf , Jiri Kosina , Miroslav Benes , Masami Hiramatsu , mingo@kernel.org, mathieu.desnoyers@efficios.com, oleg@redhat.com, paulmck@linux.vnet.ibm.com, live-patching@vger.kernel.org, linux-kernel@vger.kernel.org, andi@firstfloor.org, rostedt@goodmis.org, tglx@linutronix.de Subject: Re: [PATCH v2 2/2] livepatch/module: Correctly handle going modules Message-ID: <20150310111545.GB1813@dhcp128.suse.cz> References: <1425570314-23675-1-git-send-email-pmladek@suse.cz> <1425570314-23675-3-git-send-email-pmladek@suse.cz> <87a8zpy5wb.fsf@rustcorp.com.au> <20150309091656.GF9162@pathway.suse.cz> <87k2ypk2ue.fsf@rustcorp.com.au> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <87k2ypk2ue.fsf@rustcorp.com.au> 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 Tue 2015-03-10 12:53:21, Rusty Russell wrote: > Petr Mladek writes: > > On Sat 2015-03-07 11:34:36, Rusty Russell wrote: > >> I don't think you should handle going modules at all. Rarely happens, > >> and it should happen fast. > > > > I would like to handle it correctly. It would be pity to break a system > > just because of a module removal. Also the extra overhead will be > > very small and it will happen only very rarely. > > I don't understand why you don't just stop modules. I'm happy to write > "int stop_module_changes() / void restart_module_changes()" for you. > > This is far far simpler. Stop module changes before you start patching. > Restart after it's done. Interesting idea. stop_module_changes() would need to prevent anyone from entering COMING and GOING module states. Also it would need to wait for other modules to leave these states. As you mentioned later, we would need to exclude the current module because patches are added by a module and stop_module_changes() would need to be called from mod->init() script. In fact, we would need to exclude all modules that called stop_module_changes() to prevent a deadlock. > Is your intent to apply patches to modules which are applied (long) > after the original patch? Yes, we want to apply already loaded patches to coming modules. > Or leave that problem to userspace (ie. assume you've updated the > on-disk modules)? It would make things too complicated for our use case. We allow to install any kernel build in parallel with another kernel build. It makes it easier to reboot with the old working kernel if things went wrong. Most (default) modules are distributed with the kernel, so we would need to update the kernel as well and make it special to override the patched one. Also it would create a strange mix. The kernel would be able to load modules from different builds. Therefore crashdump would be much harder to analyze. Another problem will be with the module providing the patch. It will be needed for the original kernel but it might[*] be incompatible with the fixed one. [*] There is a possibility to hardcode relocation tables. It is handy when the patch is generated automatically or when the patch need to modify a module function with an ambiguous name. > >> If you can hold the module_lock, the easiest thing to do is have us wake > >> module_wq when a module is freed, then you can just: > > > > Unfortunately, we could not use a waitqueue easily. We would need to > > release klp_mutex to do not block going modules. But we could not > > do so in the middle of a patch adding. > > > > BTW: It seems that module_wq is used for coming modules. We could not > > use it for coming modules from the same reason. In addition, waiters > > are weaken after mod->init(). But we would need to apply the patch > > before mod->init() to avoid any inconsistency. > > You grab the module mutex using stop_module_changes() before anything > else. > > Or are you using the "failed module loading" hack to apply patches? > That would imply that the current module would have to be excluded > from the stop_module_changes() check, but should still be possible. Yes, we will need to be careful to avoid deadlocks. I think that stop_module_changes/restart_module_changes feature makes sense only if it will have more users. Otherwise, the approach with the extra flag looks much easier to me. Note that there is only one boolean/bit needed with the last version of the patch. Thanks a lot for review and feedback. I could try to implement it another way if you give me hints. Best Regards, Petr