[PATCH] livepatch: Avoid deadlock when unregistering and enabling a patch

[Petr Mladek]

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: [<ffffffff8127d4a3>] kernfs_remove+0x23/0x40
[   69.513437]
but task is already holding lock:
[   69.513437]  (klp_mutex){+.+.+.}, at: [<ffffffff810de383>] 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]        [<ffffffff810bfd5d>] lock_acquire+0xad/0x130
[   69.513437]        [<ffffffff81916f04>] mutex_lock_nested+0x44/0x380
[   69.513437]        [<ffffffff810debe0>] enabled_store+0x50/0xc0
[   69.513437]        [<ffffffff813fa4ef>] kobj_attr_store+0xf/0x20
[   69.513437]        [<ffffffff8127ef44>] sysfs_kf_write+0x44/0x60
[   69.513437]        [<ffffffff8127e564>] kernfs_fop_write+0x144/0x190
[   69.513437]        [<ffffffff811fb648>] __vfs_write+0x28/0xe0
[   69.513437]        [<ffffffff811fbd02>] vfs_write+0xa2/0x1a0
[   69.513437]        [<ffffffff811fca19>] SyS_write+0x49/0xa0
[   69.513437]        [<ffffffff8191a2f2>] entry_SYSCALL_64_fastpath+0x12/0x76
[   69.513437]
-> #0 (s_active#99){++++.+}:
[   69.513437]        [<ffffffff810beead>] __lock_acquire+0x14fd/0x1bd0
[   69.513437]        [<ffffffff810bfd5d>] lock_acquire+0xad/0x130
[   69.513437]        [<ffffffff8127c745>] __kernfs_remove+0x1f5/0x2b0
[   69.513437]        [<ffffffff8127d4a3>] kernfs_remove+0x23/0x40
[   69.513437]        [<ffffffff8127f881>] sysfs_remove_dir+0x51/0x80
[   69.513437]        [<ffffffff813fa6e8>] kobject_del+0x18/0x50
[   69.513437]        [<ffffffff813fa774>] kobject_cleanup+0x54/0x70
[   69.513437]        [<ffffffff813fa645>] kobject_put+0x25/0x50
[   69.513437]        [<ffffffff810de3f8>] klp_unregister_patch+0x98/0xc0
[   69.513437]        [<ffffffffa00000c5>] livepatch_exit+0x25/0xf60 [livepatch_sample]
[   69.513437]        [<ffffffff810fd21c>] SyS_delete_module+0x16c/0x1d0
[   69.513437]        [<ffffffff8191a2f2>] 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 <pmladek@xxxxxxxx>
---
 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/