[PATCH V3 0/3] livepatch: cleanup kpl_patch kobject release

STALE1774d

Revision v3 of 4 in this series.

6 messages, 2 authors, 2021-11-02 · open the first message on its own page

[PATCH V3 0/3] livepatch: cleanup kpl_patch kobject release

From: Ming Lei <hidden>
Date: 2021-11-01 11:26:08

Hello,

The 1st patch moves module_put() to release handler of klp_patch
kobject.

The 2nd patch changes to free klp_patch and other kobjects without
klp_mutex.

The 3rd patch switches to synchronous kobject release for klp_patch.


V3:
	- one line fix on check of list_empty() in enabled_store(), 3/3

V2:
	- remove enabled attribute before deleting this klp_patch kobject,
	for avoiding deadlock in deleting me


Ming Lei (3):
  livepatch: remove 'struct completion finish' from klp_patch
  livepatch: free klp_patch object without holding klp_mutex
  livepatch: free klp_patch object synchronously

 include/linux/livepatch.h     |  2 --
 kernel/livepatch/core.c       | 67 ++++++++++++++++-------------------
 kernel/livepatch/core.h       |  3 +-
 kernel/livepatch/transition.c | 23 ++++++++----
 kernel/livepatch/transition.h |  2 +-
 5 files changed, 50 insertions(+), 47 deletions(-)

-- 
2.31.1

[PATCH V3 1/3] livepatch: remove 'struct completion finish' from klp_patch

From: Ming Lei <hidden>
Date: 2021-11-01 11:26:37

The completion finish is just for waiting release of the klp_patch
object, then releases module refcnt. We can simply drop the module
refcnt in the kobject release handler of klp_patch.

This way also helps to support allocating klp_patch from heap.

Signed-off-by: Ming Lei <redacted>
---
 include/linux/livepatch.h |  1 -
 kernel/livepatch/core.c   | 12 +++---------
 2 files changed, 3 insertions(+), 10 deletions(-)
diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index 2614247a9781..9712818997c5 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -170,7 +170,6 @@ struct klp_patch {
 	bool enabled;
 	bool forced;
 	struct work_struct free_work;
-	struct completion finish;
 };
 
 #define klp_for_each_object_static(patch, obj) \
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 335d988bd811..b967b4b0071b 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -551,10 +551,10 @@ static int klp_add_nops(struct klp_patch *patch)
 
 static void klp_kobj_release_patch(struct kobject *kobj)
 {
-	struct klp_patch *patch;
+	struct klp_patch *patch = container_of(kobj, struct klp_patch, kobj);
 
-	patch = container_of(kobj, struct klp_patch, kobj);
-	complete(&patch->finish);
+	if (!patch->forced)
+		module_put(patch->mod);
 }
 
 static struct kobj_type klp_ktype_patch = {
@@ -678,11 +678,6 @@ static void klp_free_patch_finish(struct klp_patch *patch)
 	 * cannot get enabled again.
 	 */
 	kobject_put(&patch->kobj);
-	wait_for_completion(&patch->finish);
-
-	/* Put the module after the last access to struct klp_patch. */
-	if (!patch->forced)
-		module_put(patch->mod);
 }
 
 /*
@@ -876,7 +871,6 @@ static int klp_init_patch_early(struct klp_patch *patch)
 	patch->enabled = false;
 	patch->forced = false;
 	INIT_WORK(&patch->free_work, klp_free_patch_work_fn);
-	init_completion(&patch->finish);
 
 	klp_for_each_object_static(patch, obj) {
 		if (!obj->funcs)
-- 
2.31.1

[PATCH V3 2/3] livepatch: free klp_patch object without holding klp_mutex

From: Ming Lei <hidden>
Date: 2021-11-01 11:26:39

kobject_del() is called from kobject_put(), and after the klp_patch
kobject is deleted, any show()/store() are done.

Once the klp_patch object is removed from list and prepared for
releasing, no need to hold the global mutex of klp_mutex, so
move the freeing outside of klp_mutex.

Signed-off-by: Ming Lei <redacted>
---
 kernel/livepatch/core.c       | 30 ++++++++++++++++++------------
 kernel/livepatch/core.h       |  3 +--
 kernel/livepatch/transition.c | 23 +++++++++++++++++------
 kernel/livepatch/transition.h |  2 +-
 4 files changed, 37 insertions(+), 21 deletions(-)
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index b967b4b0071b..9ede093d699a 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -327,7 +327,8 @@ int klp_apply_section_relocs(struct module *pmod, Elf_Shdr *sechdrs,
  * /sys/kernel/livepatch/<patch>/<object>
  * /sys/kernel/livepatch/<patch>/<object>/<function,sympos>
  */
-static int __klp_disable_patch(struct klp_patch *patch);
+static int __klp_disable_patch(struct klp_patch *patch,
+		struct list_head *to_free);
 
 static ssize_t enabled_store(struct kobject *kobj, struct kobj_attribute *attr,
 			     const char *buf, size_t count)
@@ -335,6 +336,7 @@ static ssize_t enabled_store(struct kobject *kobj, struct kobj_attribute *attr,
 	struct klp_patch *patch;
 	int ret;
 	bool enabled;
+	LIST_HEAD(to_free);
 
 	ret = kstrtobool(buf, &enabled);
 	if (ret)
@@ -360,13 +362,15 @@ static ssize_t enabled_store(struct kobject *kobj, struct kobj_attribute *attr,
 	if (patch == klp_transition_patch)
 		klp_reverse_transition();
 	else if (!enabled)
-		ret = __klp_disable_patch(patch);
+		ret = __klp_disable_patch(patch, &to_free);
 	else
 		ret = -EINVAL;
 
 out:
 	mutex_unlock(&klp_mutex);
 
+	klp_free_patches_async(&to_free);
+
 	if (ret)
 		return ret;
 	return count;
@@ -693,20 +697,19 @@ static void klp_free_patch_work_fn(struct work_struct *work)
 	klp_free_patch_finish(patch);
 }
 
-void klp_free_patch_async(struct klp_patch *patch)
+static void klp_free_patch_async(struct klp_patch *patch)
 {
 	klp_free_patch_start(patch);
 	schedule_work(&patch->free_work);
 }
 
-void klp_free_replaced_patches_async(struct klp_patch *new_patch)
+void klp_free_patches_async(struct list_head *to_free)
 {
-	struct klp_patch *old_patch, *tmp_patch;
+	struct klp_patch *patch, *tmp_patch;
 
-	klp_for_each_patch_safe(old_patch, tmp_patch) {
-		if (old_patch == new_patch)
-			return;
-		klp_free_patch_async(old_patch);
+	list_for_each_entry_safe(patch, tmp_patch, to_free, list) {
+		list_del_init(&patch->list);
+		klp_free_patch_async(patch);
 	}
 }
 
@@ -915,7 +918,8 @@ static int klp_init_patch(struct klp_patch *patch)
 	return 0;
 }
 
-static int __klp_disable_patch(struct klp_patch *patch)
+static int __klp_disable_patch(struct klp_patch *patch,
+		struct list_head *to_free)
 {
 	struct klp_object *obj;
 
@@ -942,7 +946,7 @@ static int __klp_disable_patch(struct klp_patch *patch)
 
 	klp_start_transition();
 	patch->enabled = false;
-	klp_try_complete_transition();
+	klp_try_complete_transition(to_free);
 
 	return 0;
 }
@@ -951,6 +955,7 @@ static int __klp_enable_patch(struct klp_patch *patch)
 {
 	struct klp_object *obj;
 	int ret;
+	LIST_HEAD(unused);
 
 	if (klp_transition_patch)
 		return -EBUSY;
@@ -992,7 +997,8 @@ static int __klp_enable_patch(struct klp_patch *patch)
 
 	klp_start_transition();
 	patch->enabled = true;
-	klp_try_complete_transition();
+	klp_try_complete_transition(&unused);
+	WARN_ON_ONCE(!list_empty(&unused));
 
 	return 0;
 err:
diff --git a/kernel/livepatch/core.h b/kernel/livepatch/core.h
index 38209c7361b6..8ff97745ba40 100644
--- a/kernel/livepatch/core.h
+++ b/kernel/livepatch/core.h
@@ -13,8 +13,7 @@ extern struct list_head klp_patches;
 #define klp_for_each_patch(patch)	\
 	list_for_each_entry(patch, &klp_patches, list)
 
-void klp_free_patch_async(struct klp_patch *patch);
-void klp_free_replaced_patches_async(struct klp_patch *new_patch);
+void klp_free_patches_async(struct list_head *to_free);
 void klp_unpatch_replaced_patches(struct klp_patch *new_patch);
 void klp_discard_nops(struct klp_patch *new_patch);
 
diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
index 291b857a6e20..a9ebc9c5db02 100644
--- a/kernel/livepatch/transition.c
+++ b/kernel/livepatch/transition.c
@@ -32,12 +32,16 @@ static unsigned int klp_signals_cnt;
  */
 static void klp_transition_work_fn(struct work_struct *work)
 {
+	LIST_HEAD(to_free);
+
 	mutex_lock(&klp_mutex);
 
 	if (klp_transition_patch)
-		klp_try_complete_transition();
+		klp_try_complete_transition(&to_free);
 
 	mutex_unlock(&klp_mutex);
+
+	klp_free_patches_async(&to_free);
 }
 static DECLARE_DELAYED_WORK(klp_transition_work, klp_transition_work_fn);
 
@@ -384,7 +388,7 @@ static void klp_send_signals(void)
  *
  * If any tasks are still stuck in the initial patch state, schedule a retry.
  */
-void klp_try_complete_transition(void)
+void klp_try_complete_transition(struct list_head *to_free)
 {
 	unsigned int cpu;
 	struct task_struct *g, *task;
@@ -449,10 +453,17 @@ void klp_try_complete_transition(void)
 	 * klp_complete_transition() but it is called also
 	 * from klp_cancel_transition().
 	 */
-	if (!patch->enabled)
-		klp_free_patch_async(patch);
-	else if (patch->replace)
-		klp_free_replaced_patches_async(patch);
+	if (!patch->enabled) {
+		list_move(&patch->list, to_free);
+	} else if (patch->replace) {
+		struct klp_patch *old_patch, *tmp_patch;
+
+		klp_for_each_patch_safe(old_patch, tmp_patch) {
+			if (old_patch == patch)
+				break;
+			list_move(&old_patch->list, to_free);
+		}
+	}
 }
 
 /*
diff --git a/kernel/livepatch/transition.h b/kernel/livepatch/transition.h
index 322db16233de..20e3a5a0cbce 100644
--- a/kernel/livepatch/transition.h
+++ b/kernel/livepatch/transition.h
@@ -9,7 +9,7 @@ extern struct klp_patch *klp_transition_patch;
 void klp_init_transition(struct klp_patch *patch, int state);
 void klp_cancel_transition(void);
 void klp_start_transition(void);
-void klp_try_complete_transition(void);
+void klp_try_complete_transition(struct list_head *to_free);
 void klp_reverse_transition(void);
 void klp_force_transition(void);
 
-- 
2.31.1

[PATCH V3 3/3] livepatch: free klp_patch object synchronously

From: Ming Lei <hidden>
Date: 2021-11-01 11:26:50

klp_mutex isn't acquired before calling kobject_put(klp_patch), so it is
fine to free klp_patch object synchronously.

One issue is that enabled store() method, in which the klp_patch kobject
itself is deleted & released. However, sysfs has provided APIs for dealing
with this corner case, so use sysfs_break_active_protection() and
sysfs_unbreak_active_protection() for releasing klp_patch kobject from
enabled_store(), meantime the enabled attribute has to be removed
before deleting the klp_patch kobject.

Signed-off-by: Ming Lei <redacted>
---
 include/linux/livepatch.h     |  1 -
 kernel/livepatch/core.c       | 35 +++++++++++++++--------------------
 kernel/livepatch/core.h       |  2 +-
 kernel/livepatch/transition.c |  2 +-
 4 files changed, 17 insertions(+), 23 deletions(-)
diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index 9712818997c5..4dcebf52fac5 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -169,7 +169,6 @@ struct klp_patch {
 	struct list_head obj_list;
 	bool enabled;
 	bool forced;
-	struct work_struct free_work;
 };
 
 #define klp_for_each_object_static(patch, obj) \
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 9ede093d699a..bb573abecb90 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -337,6 +337,7 @@ static ssize_t enabled_store(struct kobject *kobj, struct kobj_attribute *attr,
 	int ret;
 	bool enabled;
 	LIST_HEAD(to_free);
+	struct kernfs_node *kn = NULL;
 
 	ret = kstrtobool(buf, &enabled);
 	if (ret)
@@ -369,10 +370,18 @@ static ssize_t enabled_store(struct kobject *kobj, struct kobj_attribute *attr,
 out:
 	mutex_unlock(&klp_mutex);
 
-	klp_free_patches_async(&to_free);
-
 	if (ret)
 		return ret;
+
+	if (!list_empty(&to_free)) {
+		kn = sysfs_break_active_protection(kobj, &attr->attr);
+		WARN_ON_ONCE(!kn);
+		sysfs_remove_file(kobj, &attr->attr);
+		klp_free_patches(&to_free);
+		if (kn)
+			sysfs_unbreak_active_protection(kn);
+	}
+
 	return count;
 }
 
@@ -684,32 +693,19 @@ static void klp_free_patch_finish(struct klp_patch *patch)
 	kobject_put(&patch->kobj);
 }
 
-/*
- * The livepatch might be freed from sysfs interface created by the patch.
- * This work allows to wait until the interface is destroyed in a separate
- * context.
- */
-static void klp_free_patch_work_fn(struct work_struct *work)
-{
-	struct klp_patch *patch =
-		container_of(work, struct klp_patch, free_work);
-
-	klp_free_patch_finish(patch);
-}
-
-static void klp_free_patch_async(struct klp_patch *patch)
+static void klp_free_patch(struct klp_patch *patch)
 {
 	klp_free_patch_start(patch);
-	schedule_work(&patch->free_work);
+	klp_free_patch_finish(patch);
 }
 
-void klp_free_patches_async(struct list_head *to_free)
+void klp_free_patches(struct list_head *to_free)
 {
 	struct klp_patch *patch, *tmp_patch;
 
 	list_for_each_entry_safe(patch, tmp_patch, to_free, list) {
 		list_del_init(&patch->list);
-		klp_free_patch_async(patch);
+		klp_free_patch(patch);
 	}
 }
 
@@ -873,7 +869,6 @@ static int klp_init_patch_early(struct klp_patch *patch)
 	kobject_init(&patch->kobj, &klp_ktype_patch);
 	patch->enabled = false;
 	patch->forced = false;
-	INIT_WORK(&patch->free_work, klp_free_patch_work_fn);
 
 	klp_for_each_object_static(patch, obj) {
 		if (!obj->funcs)
diff --git a/kernel/livepatch/core.h b/kernel/livepatch/core.h
index 8ff97745ba40..ea593f370049 100644
--- a/kernel/livepatch/core.h
+++ b/kernel/livepatch/core.h
@@ -13,7 +13,7 @@ extern struct list_head klp_patches;
 #define klp_for_each_patch(patch)	\
 	list_for_each_entry(patch, &klp_patches, list)
 
-void klp_free_patches_async(struct list_head *to_free);
+void klp_free_patches(struct list_head *to_free);
 void klp_unpatch_replaced_patches(struct klp_patch *new_patch);
 void klp_discard_nops(struct klp_patch *new_patch);
 
diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
index a9ebc9c5db02..3eff5fc0deee 100644
--- a/kernel/livepatch/transition.c
+++ b/kernel/livepatch/transition.c
@@ -41,7 +41,7 @@ static void klp_transition_work_fn(struct work_struct *work)
 
 	mutex_unlock(&klp_mutex);
 
-	klp_free_patches_async(&to_free);
+	klp_free_patches(&to_free);
 }
 static DECLARE_DELAYED_WORK(klp_transition_work, klp_transition_work_fn);
 
-- 
2.31.1

Re: [PATCH V3 0/3] livepatch: cleanup kpl_patch kobject release

From: Joe Lawrence <joe.lawrence@redhat.com>
Date: 2021-11-01 16:44:10

On Mon, Nov 01, 2021 at 07:25:45PM +0800, Ming Lei wrote:
Hello,

The 1st patch moves module_put() to release handler of klp_patch
kobject.

The 2nd patch changes to free klp_patch and other kobjects without
klp_mutex.

The 3rd patch switches to synchronous kobject release for klp_patch.


V3:
	- one line fix on check of list_empty() in enabled_store(), 3/3

V2:
	- remove enabled attribute before deleting this klp_patch kobject,
	for avoiding deadlock in deleting me
Hi Ming,

Another interesting wedge (doesn't seem to happen w/o system load):

# different kernel tree build
$ while(true); do make clean && make -j$(nproc); done

vs.

# loop the basic tests
$ while(true); do ./tools/testing/selftests/livepatch/test-livepatch.sh || break; done
TEST: basic function patching ... ok
TEST: multiple livepatches ... ok
TEST: atomic replace livepatch ... ERROR: failed to unload module test_klp_livepatch (refcnt)

$ lsmod | grep test_klp
test_klp_atomic_replace    16384  1
test_klp_livepatch     16384  1

$ head /sys/kernel/livepatch/*/enabled
==> /sys/kernel/livepatch/test_klp_atomic_replace/enabled <==
1

==> /sys/kernel/livepatch/test_klp_livepatch/enabled <==
0

[  307.818060] livepatch: kernel.ftrace_enabled = 1
[  307.849759] ===== TEST: basic function patching =====
[  307.876520] % modprobe test_klp_livepatch
[  307.889014] test_klp_livepatch: tainting kernel with TAINT_LIVEPATCH
[  307.915450] livepatch: enabling patch 'test_klp_livepatch'
[  307.923991] livepatch: 'test_klp_livepatch': starting patching transition
[  308.768730] livepatch: 'test_klp_livepatch': patching complete
[  308.779476] % echo 0 > /sys/kernel/livepatch/test_klp_livepatch/enabled
[  308.787378] livepatch: 'test_klp_livepatch': starting unpatching transition
[  308.828968] livepatch: 'test_klp_livepatch': unpatching complete
[  308.838284] % rmmod test_klp_livepatch
[  308.921367] ===== TEST: multiple livepatches =====
[  308.936056] % modprobe test_klp_livepatch
[  308.968419] livepatch: enabling patch 'test_klp_livepatch'
[  308.976976] livepatch: 'test_klp_livepatch': starting patching transition
[  309.779754] livepatch: 'test_klp_livepatch': patching complete
[  309.827765] test_klp_livepatch: this has been live patched
[  309.853678] % modprobe test_klp_atomic_replace replace=0
[  309.887697] livepatch: enabling patch 'test_klp_atomic_replace'
[  309.897090] livepatch: 'test_klp_atomic_replace': starting patching transition
[  309.906310] livepatch: 'test_klp_atomic_replace': patching complete
[  309.919386] test_klp_livepatch: this has been live patched
[  309.928124] test_klp_atomic_replace: this has been live patched
[  309.935306] % echo 0 > /sys/kernel/livepatch/test_klp_atomic_replace/enabled
[  309.943882] livepatch: 'test_klp_atomic_replace': starting unpatching transition
[  310.842939] livepatch: 'test_klp_atomic_replace': unpatching complete
[  310.875590] % rmmod test_klp_atomic_replace
[  310.910489] test_klp_livepatch: this has been live patched
[  310.919444] % echo 0 > /sys/kernel/livepatch/test_klp_livepatch/enabled
[  310.927157] livepatch: 'test_klp_livepatch': starting unpatching transition
[  311.795931] livepatch: 'test_klp_livepatch': unpatching complete
[  311.858463] % rmmod test_klp_livepatch
[  311.941334] ===== TEST: atomic replace livepatch =====
[  311.959647] % modprobe test_klp_livepatch
[  311.992469] livepatch: enabling patch 'test_klp_livepatch'
[  312.001464] livepatch: 'test_klp_livepatch': starting patching transition
[  312.787758] livepatch: 'test_klp_livepatch': patching complete
[  312.854237] test_klp_livepatch: this has been live patched
[  312.872939] % modprobe test_klp_atomic_replace replace=1
[  312.926957] livepatch: enabling patch 'test_klp_atomic_replace'
[  312.936289] livepatch: 'test_klp_atomic_replace': starting patching transition
[  312.968876] livepatch: 'test_klp_atomic_replace': patching complete
[  312.975898] ------------[ cut here ]------------
[  312.981073] WARNING: CPU: 16 PID: 35260 at kernel/livepatch/core.c:996 __klp_enable_patch.cold+0xbd/0x132
[  312.991781] Modules linked in: test_klp_atomic_replace(K+) test_klp_livepatch(K) rfkill intel_rapl_msr intel_rapl_common sb_edac x86_pkg_]
[  313.060963] CPU: 16 PID: 35260 Comm: modprobe Kdump: loaded Tainted: G              K   5.15.0-rc7+ #1
[  313.071375] Hardware name: Dell Inc. PowerEdge T620/0MX4YF, BIOS 1.0.4 02/21/2012
[  313.079756] RIP: 0010:__klp_enable_patch.cold+0xbd/0x132
[  313.085718] Code: dc ff ff e8 2a d4 78 ff e9 a2 b1 78 ff e8 b0 d8 78 ff c6 85 80 00 00 00 01 48 89 df e8 91 d4 78 ff 48 8b 04 24 48 39 d8f
[  313.106703] RSP: 0018:ffffa99ac8e0fd68 EFLAGS: 00010216
[  313.112564] RAX: ffffffffc098d020 RBX: ffffa99ac8e0fd68 RCX: 0000000000000000
[  313.120554] RDX: ffffa99ac8e0fd68 RSI: ffffa99ac8e0fd68 RDI: ffffffffc098d020
[  313.128552] RBP: ffffffffc0a9d000 R08: ffffa99ac8e0fd68 R09: ffffa99ac8e0fd68
[  313.136545] R10: ffffa99ac8e0fb50 R11: ffffffffb45e5648 R12: 0000000000000000
[  313.144528] R13: ffffffffc0a9d070 R14: ffffffffc0a9cfe8 R15: ffffffffc0a9d2f8
[  313.152517] FS:  00007fa4a968e740(0000) GS:ffff8b469fc00000(0000) knlGS:0000000000000000
[  313.161576] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  313.168017] CR2: 00000000013fb008 CR3: 000000012b7da005 CR4: 00000000000606e0
[  313.176008] Call Trace:
[  313.178765]  klp_enable_patch+0x2d7/0x340
[  313.183272]  ? livepatch_meminfo_proc_show+0x30/0x30 [test_klp_atomic_replace]
[  313.191367]  do_one_initcall+0x44/0x1d0
[  313.195685]  ? load_module+0xab3/0xb60
[  313.199899]  ? __cond_resched+0x16/0x40
[  313.204207]  ? kmem_cache_alloc_trace+0x44/0x3d0
[  313.209391]  do_init_module+0x5c/0x270
[  313.213600]  __do_sys_finit_module+0xae/0x110
[  313.218490]  do_syscall_64+0x3b/0x90
[  313.222505]  entry_SYSCALL_64_after_hwframe+0x44/0xae
[  313.228167] RIP: 0033:0x7fa4a979e0cd
[  313.232180] Code: 5b 41 5c c3 66 0f 1f 84 00 00 00 00 00 f3 0f 1e fa 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 088
[  313.253166] RSP: 002b:00007fff515d0488 EFLAGS: 00000246 ORIG_RAX: 0000000000000139
[  313.261658] RAX: ffffffffffffffda RBX: 0000562e0d532900 RCX: 00007fa4a979e0cd
[  313.269654] RDX: 0000000000000000 RSI: 0000562e0d532d90 RDI: 0000000000000003
[  313.277648] RBP: 0000000000040000 R08: 0000000000000000 R09: 00007fff00000002
[  313.285628] R10: 0000000000000003 R11: 0000000000000246 R12: 0000562e0d532d90
[  313.293617] R13: 0000562e0d532b40 R14: 0000562e0d532900 R15: 0000562e0d532db0
[  313.301612] ---[ end trace d4ec8b943e8d24cc ]---
[  313.315675] test_klp_atomic_replace: this has been live patched
[  375.850221] ERROR: failed to unload module test_klp_livepatch (refcnt)

 949 static int __klp_enable_patch(struct klp_patch *patch)
 ...
 993         klp_start_transition();
 994         patch->enabled = true;
 995         klp_try_complete_transition(&unused);
 996         WARN_ON_ONCE(!list_empty(&unused));
 
-- Joe

Re: [PATCH V3 0/3] livepatch: cleanup kpl_patch kobject release

From: Ming Lei <hidden>
Date: 2021-11-02 04:21:04

On Mon, Nov 01, 2021 at 12:44:00PM -0400, Joe Lawrence wrote:
On Mon, Nov 01, 2021 at 07:25:45PM +0800, Ming Lei wrote:
quoted
Hello,

The 1st patch moves module_put() to release handler of klp_patch
kobject.

The 2nd patch changes to free klp_patch and other kobjects without
klp_mutex.

The 3rd patch switches to synchronous kobject release for klp_patch.


V3:
	- one line fix on check of list_empty() in enabled_store(), 3/3

V2:
	- remove enabled attribute before deleting this klp_patch kobject,
	for avoiding deadlock in deleting me
Hi Ming,

Another interesting wedge (doesn't seem to happen w/o system load):

# different kernel tree build
$ while(true); do make clean && make -j$(nproc); done

vs.

# loop the basic tests
$ while(true); do ./tools/testing/selftests/livepatch/test-livepatch.sh || break; done
TEST: basic function patching ... ok
TEST: multiple livepatches ... ok
TEST: atomic replace livepatch ... ERROR: failed to unload module test_klp_livepatch (refcnt)

$ lsmod | grep test_klp
test_klp_atomic_replace    16384  1
test_klp_livepatch     16384  1

$ head /sys/kernel/livepatch/*/enabled
==> /sys/kernel/livepatch/test_klp_atomic_replace/enabled <==
1

==> /sys/kernel/livepatch/test_klp_livepatch/enabled <==
0

[  307.818060] livepatch: kernel.ftrace_enabled = 1
[  307.849759] ===== TEST: basic function patching =====
[  307.876520] % modprobe test_klp_livepatch
[  307.889014] test_klp_livepatch: tainting kernel with TAINT_LIVEPATCH
[  307.915450] livepatch: enabling patch 'test_klp_livepatch'
[  307.923991] livepatch: 'test_klp_livepatch': starting patching transition
[  308.768730] livepatch: 'test_klp_livepatch': patching complete
[  308.779476] % echo 0 > /sys/kernel/livepatch/test_klp_livepatch/enabled
[  308.787378] livepatch: 'test_klp_livepatch': starting unpatching transition
[  308.828968] livepatch: 'test_klp_livepatch': unpatching complete
[  308.838284] % rmmod test_klp_livepatch
[  308.921367] ===== TEST: multiple livepatches =====
[  308.936056] % modprobe test_klp_livepatch
[  308.968419] livepatch: enabling patch 'test_klp_livepatch'
[  308.976976] livepatch: 'test_klp_livepatch': starting patching transition
[  309.779754] livepatch: 'test_klp_livepatch': patching complete
[  309.827765] test_klp_livepatch: this has been live patched
[  309.853678] % modprobe test_klp_atomic_replace replace=0
[  309.887697] livepatch: enabling patch 'test_klp_atomic_replace'
[  309.897090] livepatch: 'test_klp_atomic_replace': starting patching transition
[  309.906310] livepatch: 'test_klp_atomic_replace': patching complete
[  309.919386] test_klp_livepatch: this has been live patched
[  309.928124] test_klp_atomic_replace: this has been live patched
[  309.935306] % echo 0 > /sys/kernel/livepatch/test_klp_atomic_replace/enabled
[  309.943882] livepatch: 'test_klp_atomic_replace': starting unpatching transition
[  310.842939] livepatch: 'test_klp_atomic_replace': unpatching complete
[  310.875590] % rmmod test_klp_atomic_replace
[  310.910489] test_klp_livepatch: this has been live patched
[  310.919444] % echo 0 > /sys/kernel/livepatch/test_klp_livepatch/enabled
[  310.927157] livepatch: 'test_klp_livepatch': starting unpatching transition
[  311.795931] livepatch: 'test_klp_livepatch': unpatching complete
[  311.858463] % rmmod test_klp_livepatch
[  311.941334] ===== TEST: atomic replace livepatch =====
[  311.959647] % modprobe test_klp_livepatch
[  311.992469] livepatch: enabling patch 'test_klp_livepatch'
[  312.001464] livepatch: 'test_klp_livepatch': starting patching transition
[  312.787758] livepatch: 'test_klp_livepatch': patching complete
[  312.854237] test_klp_livepatch: this has been live patched
[  312.872939] % modprobe test_klp_atomic_replace replace=1
[  312.926957] livepatch: enabling patch 'test_klp_atomic_replace'
[  312.936289] livepatch: 'test_klp_atomic_replace': starting patching transition
[  312.968876] livepatch: 'test_klp_atomic_replace': patching complete
[  312.975898] ------------[ cut here ]------------
[  312.981073] WARNING: CPU: 16 PID: 35260 at kernel/livepatch/core.c:996 __klp_enable_patch.cold+0xbd/0x132
[  312.991781] Modules linked in: test_klp_atomic_replace(K+) test_klp_livepatch(K) rfkill intel_rapl_msr intel_rapl_common sb_edac x86_pkg_]
[  313.060963] CPU: 16 PID: 35260 Comm: modprobe Kdump: loaded Tainted: G              K   5.15.0-rc7+ #1
[  313.071375] Hardware name: Dell Inc. PowerEdge T620/0MX4YF, BIOS 1.0.4 02/21/2012
[  313.079756] RIP: 0010:__klp_enable_patch.cold+0xbd/0x132
[  313.085718] Code: dc ff ff e8 2a d4 78 ff e9 a2 b1 78 ff e8 b0 d8 78 ff c6 85 80 00 00 00 01 48 89 df e8 91 d4 78 ff 48 8b 04 24 48 39 d8f
[  313.106703] RSP: 0018:ffffa99ac8e0fd68 EFLAGS: 00010216
[  313.112564] RAX: ffffffffc098d020 RBX: ffffa99ac8e0fd68 RCX: 0000000000000000
[  313.120554] RDX: ffffa99ac8e0fd68 RSI: ffffa99ac8e0fd68 RDI: ffffffffc098d020
[  313.128552] RBP: ffffffffc0a9d000 R08: ffffa99ac8e0fd68 R09: ffffa99ac8e0fd68
[  313.136545] R10: ffffa99ac8e0fb50 R11: ffffffffb45e5648 R12: 0000000000000000
[  313.144528] R13: ffffffffc0a9d070 R14: ffffffffc0a9cfe8 R15: ffffffffc0a9d2f8
[  313.152517] FS:  00007fa4a968e740(0000) GS:ffff8b469fc00000(0000) knlGS:0000000000000000
[  313.161576] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  313.168017] CR2: 00000000013fb008 CR3: 000000012b7da005 CR4: 00000000000606e0
[  313.176008] Call Trace:
[  313.178765]  klp_enable_patch+0x2d7/0x340
[  313.183272]  ? livepatch_meminfo_proc_show+0x30/0x30 [test_klp_atomic_replace]
[  313.191367]  do_one_initcall+0x44/0x1d0
[  313.195685]  ? load_module+0xab3/0xb60
[  313.199899]  ? __cond_resched+0x16/0x40
[  313.204207]  ? kmem_cache_alloc_trace+0x44/0x3d0
[  313.209391]  do_init_module+0x5c/0x270
[  313.213600]  __do_sys_finit_module+0xae/0x110
[  313.218490]  do_syscall_64+0x3b/0x90
[  313.222505]  entry_SYSCALL_64_after_hwframe+0x44/0xae
[  313.228167] RIP: 0033:0x7fa4a979e0cd
[  313.232180] Code: 5b 41 5c c3 66 0f 1f 84 00 00 00 00 00 f3 0f 1e fa 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 088
[  313.253166] RSP: 002b:00007fff515d0488 EFLAGS: 00000246 ORIG_RAX: 0000000000000139
[  313.261658] RAX: ffffffffffffffda RBX: 0000562e0d532900 RCX: 00007fa4a979e0cd
[  313.269654] RDX: 0000000000000000 RSI: 0000562e0d532d90 RDI: 0000000000000003
[  313.277648] RBP: 0000000000040000 R08: 0000000000000000 R09: 00007fff00000002
[  313.285628] R10: 0000000000000003 R11: 0000000000000246 R12: 0000562e0d532d90
[  313.293617] R13: 0000562e0d532b40 R14: 0000562e0d532900 R15: 0000562e0d532db0
[  313.301612] ---[ end trace d4ec8b943e8d24cc ]---
[  313.315675] test_klp_atomic_replace: this has been live patched
[  375.850221] ERROR: failed to unload module test_klp_livepatch (refcnt)

 949 static int __klp_enable_patch(struct klp_patch *patch)
 ...
 993         klp_start_transition();
 994         patch->enabled = true;
 995         klp_try_complete_transition(&unused);
 996         WARN_ON_ONCE(!list_empty(&unused));
Got it, in case of replace mode, patch can be freed even during
enabling, we can address it here by calling klp_free_patches()
in klp_enable_patch() without holding klp_mutex.

I have run the test for one hour after applying the above change, not
see the issue any more.

Will post v4 soon.

thanks,
Ming
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help