Path: csiph.com!eternal-september.org!feeder.eternal-september.org!weretis.net!feeder4.news.weretis.net!storethat.news.telefonica.de!telefonica.de!news.panservice.it!bofh.it!news.nic.it!robomod From: Miroslav Benes Newsgroups: linux.kernel Subject: Re: [PATCH v5.1 15/15] livepatch: allow removal of a disabled patch Date: Tue, 07 Mar 2017 16:10:01 +0100 Message-ID: References: X-Original-To: Josh Poimboeuf User-Agent: Alpine 2.20 (LSU 67 2015-01-07) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Sender: robomod@news.nic.it List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Approved: robomod@news.nic.it Lines: 63 Organization: linux.* mail to news gateway X-Original-Cc: Jessica Yu , Jiri Kosina , Petr Mladek , linux-kernel@vger.kernel.org, live-patching@vger.kernel.org, Michael Ellerman , Heiko Carstens , x86@kernel.org, linuxppc-dev@lists.ozlabs.org, linux-s390@vger.kernel.org, Vojtech Pavlik , Jiri Slaby , Chris J Arges , Andy Lutomirski , Ingo Molnar , Peter Zijlstra , Kamalesh Babulal , Balbir Singh X-Original-Date: Tue, 7 Mar 2017 15:50:11 +0100 (CET) X-Original-Message-ID: X-Original-References: <95fe68239fc91360093f84aea97418ae88cfc634.1487036215.git.jpoimboe@redhat.com> <20170306172029.vk4fk7kzlrh2ty4m@treble> X-Original-Sender: linux-kernel-owner@vger.kernel.org Xref: csiph.com linux.kernel:1594326 On Mon, 6 Mar 2017, Josh Poimboeuf wrote: > > Currently we do not allow patch module to unload since there is no > method to determine if a task is still running in the patched code. > > The consistency model gives us the way because when the unpatching > finishes we know that all tasks were marked as safe to call an original > function. Thus every new call to the function calls the original code > and at the same time no task can be somewhere in the patched code, > because it had to leave that code to be marked as safe. > > We can safely let the patch module go after that. > > Completion is used for synchronization between module removal and sysfs > infrastructure in a similar way to commit 942e443127e9 ("module: Fix > mod->mkobj.kobj potentially freed too early"). > > Note that we still do not allow the removal for immediate model, that is > no consistency model. The module refcount may increase in this case if > somebody disables and enables the patch several times. This should not > cause any harm. > > With this change a call to try_module_get() is moved to > __klp_enable_patch from klp_register_patch to make module reference > counting symmetric (module_put() is in a patch disable path) and to > allow to take a new reference to a disabled module when being enabled. > > Finally, we need to be very careful about possible races between > klp_unregister_patch(), kobject_put() functions and operations > on the related sysfs files. > > kobject_put(&patch->kobj) must be called without klp_mutex. Otherwise, > it might be blocked by enabled_store() that needs the mutex as well. > In addition, enabled_store() must check if the patch was not > unregisted in the meantime. > > There is no need to do the same for other kobject_put() callsites > at the moment. Their sysfs operations neither take the lock nor > they access any data that might be freed in the meantime. > > There was an attempt to use kobjects the right way and prevent these > races by design. But it made the patch definition more complicated > and opened another can of worms. See > https://lkml.kernel.org/r/1464018848-4303-1-git-send-email-pmladek@suse.com > > [Thanks to Petr Mladek for improving the commit message.] > > Signed-off-by: Miroslav Benes > Signed-off-by: Josh Poimboeuf > Reviewed-by: Petr Mladek > --- > v5.1: improve error handling in enable path -- call module_put() in > klp_cancel_transition() Looks good. Acked-by: Miroslav Benes if it is needed for Josh's code. Regards, Miroslav