Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1759727AbcLUOok (ORCPT ); Wed, 21 Dec 2016 09:44:40 -0500 Received: from mx2.suse.de ([195.135.220.15]:42492 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751506AbcLUOoi (ORCPT ); Wed, 21 Dec 2016 09:44:38 -0500 Date: Wed, 21 Dec 2016 15:44:35 +0100 From: Petr Mladek To: Josh Poimboeuf Cc: Jessica Yu , Jiri Kosina , Miroslav Benes , 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 Subject: Re: [PATCH v3 15/15] livepatch: allow removal of a disabled patch Message-ID: <20161221144435.GF25166@pathway.suse.cz> References: <21cb19ed5a44e876aeb830fdea1fad2c795a63f5.1481220077.git.jpoimboe@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <21cb19ed5a44e876aeb830fdea1fad2c795a63f5.1481220077.git.jpoimboe@redhat.com> 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 Content-Length: 2194 Lines: 54 On Thu 2016-12-08 12:08:40, Josh Poimboeuf wrote: > From: Miroslav Benes > > 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. [...] > Also all kobject_put(&patch->kobj) calls are moved outside of klp_mutex > lock protection to prevent a deadlock situation when > klp_unregister_patch is called and sysfs directories are removed. > is no need to do the same for other kobject_put() callsites as we > currently do not have their sysfs counterparts. Heh, we have spent huge amount of time on this. I think that it deserves a more precise description ;-). What about? 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 neiter 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 > Signed-off-by: Miroslav Benes > Signed-off-by: Josh Poimboeuf Otherwise, it seems correct. We have already analyzed this to the death. I do not see new problems with a fresh look. With the above, or comparable, change in the commit message: Reviewed-by: Petr Mladek Best Regards, Petr