Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755668AbbLAON2 (ORCPT ); Tue, 1 Dec 2015 09:13:28 -0500 Received: from mx2.suse.de ([195.135.220.15]:40171 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755216AbbLAON1 (ORCPT ); Tue, 1 Dec 2015 09:13:27 -0500 Date: Tue, 1 Dec 2015 15:13:24 +0100 From: Petr Mladek To: Jiri Slaby Cc: Josh Poimboeuf , Li Bin , sjenning@redhat.com, jikos@kernel.org, vojtech@suse.com, live-patching@vger.kernel.org, linux-kernel@vger.kernel.org, guohanjun@huawei.com, dingtianhong@huawei.com, xiexiuqi@huawei.com, zhouchengming1@huawei.com, Miroslav Benes Subject: Re: [PATCH] livepatch: fix race between enabled_store() and klp_unregister_patch() Message-ID: <20151201141323.GB14230@pathway.suse.cz> References: <1448855677-8392-1-git-send-email-huawei.libin@huawei.com> <20151201011139.GB12513@treble.redhat.com> <565D5F4F.8020807@suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <565D5F4F.8020807@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 Content-Length: 6098 Lines: 145 On Tue 2015-12-01 09:50:23, Jiri Slaby wrote: > On 12/01/2015, 02:11 AM, Josh Poimboeuf wrote: > > When I try to recreate something similar by putting a delay in > > enabled_store(), klp_free_patch() just sleeps on its call to > > kobject_put() until enabled_store() returns. The unregister stack looks > > like: > > > > [] __kernfs_remove+0x1fb/0x380 > > [] kernfs_remove+0x23/0x40 > > [] sysfs_remove_dir+0x51/0x80 > > [] kobject_del+0x18/0x50 > > [] kobject_release+0x5a/0x190 > > [] kobject_put+0x27/0x50 > > What about _put outside of klp_mutex in klp_unregister_patch (and maybe > the other _put's as well)? Plus Li Bin's patch. This might work. But I am pretty sure that we would need to put also all the other kobject_puts outside of the lock. I wondered how the approach with mutex_trylock() would look like and got the patch below. It is not trivial but it still might be easier than moving all the kobject_put() calls. Also it should be easier to review because all the logic is on a single place. What do you think? >From 3eaec912f2700cd2a886ada1e7b4361ae192ef25 Mon Sep 17 00:00:00 2001 From: Petr Mladek Date: Tue, 1 Dec 2015 13:25:34 +0100 Subject: [PATCH] livepatch: Avoid deadlock when unregistering and enabling a patch There is a possible deadlock between kobject_put() calls and enabled_store(), see the lockdep report below. A solution would be to put all kobject without the klp_mutex and check if the patch is registered in enabled_store(). But this would make the unregister/free code even more scattered. This patch takes the other possible approach. It uses trylock in enabled_store(). It the lock is not available and the patch is not registered, it is probably being removed. Anyway, there is nothing to do and enable_store() returns -EINVAL. If the lock is not available and the patch is registered, it tries harder to get it. It uses mutex_is_locked() in the busy loop to avoid the cache bouncing. Lockdep report: [ 69.512196] ====================================================== [ 69.513139] [ INFO: possible circular locking dependency detected ] [ 69.513437] 4.4.0-rc3-4-default+ #2079 Tainted: G W E K [ 69.513437] ------------------------------------------------------- [ 69.513437] rmmod/3786 is trying to acquire lock: [ 69.513437] (s_active#99){++++.+}, at: [] kernfs_remove+0x23/0x40 [ 69.513437] but task is already holding lock: [ 69.513437] (klp_mutex){+.+.+.}, at: [] klp_unregister_patch+0x23/0xc0 [ 69.513437] which lock already depends on the new lock. [ 69.513437] the existing dependency chain (in reverse order) is: [ 69.513437] -> #1 (klp_mutex){+.+.+.}: [ 69.513437] [] lock_acquire+0xad/0x130 [ 69.513437] [] mutex_lock_nested+0x44/0x380 [ 69.513437] [] enabled_store+0x50/0xc0 [ 69.513437] [] kobj_attr_store+0xf/0x20 [ 69.513437] [] sysfs_kf_write+0x44/0x60 [ 69.513437] [] kernfs_fop_write+0x144/0x190 [ 69.513437] [] __vfs_write+0x28/0xe0 [ 69.513437] [] vfs_write+0xa2/0x1a0 [ 69.513437] [] SyS_write+0x49/0xa0 [ 69.513437] [] entry_SYSCALL_64_fastpath+0x12/0x76 [ 69.513437] -> #0 (s_active#99){++++.+}: [ 69.513437] [] __lock_acquire+0x14fd/0x1bd0 [ 69.513437] [] lock_acquire+0xad/0x130 [ 69.513437] [] __kernfs_remove+0x1f5/0x2b0 [ 69.513437] [] kernfs_remove+0x23/0x40 [ 69.513437] [] sysfs_remove_dir+0x51/0x80 [ 69.513437] [] kobject_del+0x18/0x50 [ 69.513437] [] kobject_cleanup+0x54/0x70 [ 69.513437] [] kobject_put+0x25/0x50 [ 69.513437] [] klp_unregister_patch+0x98/0xc0 [ 69.513437] [] livepatch_exit+0x25/0xf60 [livepatch_sample] [ 69.513437] [] SyS_delete_module+0x16c/0x1d0 [ 69.513437] [] entry_SYSCALL_64_fastpath+0x12/0x76 [ 69.513437] other info that might help us debug this: [ 69.513437] Possible unsafe locking scenario: [ 69.513437] CPU0 CPU1 [ 69.513437] ---- ---- [ 69.513437] lock(klp_mutex); [ 69.513437] lock(s_active#99); [ 69.513437] lock(klp_mutex); [ 69.513437] lock(s_active#99); [ 69.513437] *** DEADLOCK *** Signed-off-by: Petr Mladek --- kernel/livepatch/core.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c index 27712384c69e..e2f00f32bb00 100644 --- a/kernel/livepatch/core.c +++ b/kernel/livepatch/core.c @@ -612,7 +612,19 @@ static ssize_t enabled_store(struct kobject *kobj, struct kobj_attribute *attr, patch = container_of(kobj, struct klp_patch, kobj); - mutex_lock(&klp_mutex); + /* + * Avoid a deadlock with kobject_put(&patch->kobj) that is + * called under klp_mutex. Bail out when the patch is not + * longer registered. + */ + if (!mutex_trylock(&klp_mutex)) { + if (!klp_is_patch_registered(patch)) + return -EINVAL; + /* Do not spin with trylock that bounce cache lines. */ + while (mutex_is_locked(&klp_mutex) && + klp_is_patch_registered(patch)) + cond_resched(); + } if (val == patch->state) { /* already in requested state */ -- 1.8.5.6 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/