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

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

[Ming Lei]

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.


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       | 63 +++++++++++++++--------------------
 kernel/livepatch/core.h       |  3 +-
 kernel/livepatch/transition.c | 23 +++++++++----
 kernel/livepatch/transition.h |  2 +-
 5 files changed, 46 insertions(+), 47 deletions(-)

-- 
2.31.1

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

[Ming Lei]

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 <ming.lei@redhat.com>
---
 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 2/3] livepatch: free klp_patch object without holding klp_mutex

[Ming Lei]

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 <ming.lei@redhat.com>
---
 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 3/3] livepatch: free klp_patch object synchronously

[Ming Lei]

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().

Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 include/linux/livepatch.h     |  1 -
 kernel/livepatch/core.c       | 29 ++++++++++-------------------
 kernel/livepatch/core.h       |  2 +-
 kernel/livepatch/transition.c |  2 +-
 4 files changed, 12 insertions(+), 22 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..7fba5d47ffdd 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,7 +370,11 @@ static ssize_t enabled_store(struct kobject *kobj, struct kobj_attribute *attr,
 out:
 	mutex_unlock(&klp_mutex);
 
-	klp_free_patches_async(&to_free);
+	kn = sysfs_break_active_protection(kobj, &attr->attr);
+	WARN_ON_ONCE(!kn);
+	klp_free_patches(&to_free);
+	if (kn)
+		sysfs_unbreak_active_protection(kn);
 
 	if (ret)
 		return ret;
@@ -684,32 +689,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 +865,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 0/3] livepatch: cleanup kpl_patch kobject release

[Joe Lawrence]

On Thu, Oct 28, 2021 at 08:57:31PM +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.
>

Hi Ming,

I gave the patchset a spin on top of linus tree @ 1fc596a56b33 and ended
up with a stuck task:

Test
----
Enable the livepatch selftests:
  $ grep CONFIG_TEST_LIVEPATCH .config
  CONFIG_TEST_LIVEPATCH=m

Run a continuous kernel build in the background:
  $ while (true); do make clean && make -j$(nproc); done

While continuously executing the selftests:
  $ while (true); do make -C tools/testing/selftests/livepatch/ run_tests; done

Results
-------
...
[  366.862278] ===== TEST: multiple target modules =====
[  366.877470] % modprobe test_klp_callbacks_busy block_transition=N
[  366.890468] test_klp_callbacks_busy: test_klp_callbacks_busy_init
[  366.897280] test_klp_callbacks_busy: busymod_work_func enter
[  366.903602] test_klp_callbacks_busy: busymod_work_func exit
[  366.920311] % modprobe test_klp_callbacks_demo
[  366.931737] livepatch: enabling patch 'test_klp_callbacks_demo'
[  366.938466] test_klp_callbacks_demo: pre_patch_callback: vmlinux
[  366.945173] test_klp_callbacks_demo: pre_patch_callback: test_klp_callbacks_busy -> [MODULE_STATE_LIVE] Normal state
[  366.959322] livepatch: 'test_klp_callbacks_demo': starting patching transition
[  369.699278] test_klp_callbacks_demo: post_patch_callback: vmlinux
[  369.706118] test_klp_callbacks_demo: post_patch_callback: test_klp_callbacks_busy -> [MODULE_STATE_LIVE] Normal state
[  369.718079] livepatch: 'test_klp_callbacks_demo': patching complete
[  369.786485] % modprobe test_klp_callbacks_mod
[  369.806918] livepatch: applying patch 'test_klp_callbacks_demo' to loading module 'test_klp_callbacks_mod'
[  369.818005] test_klp_callbacks_demo: pre_patch_callback: test_klp_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init
[  369.831826] test_klp_callbacks_demo: post_patch_callback: test_klp_callbacks_mod -> [MODULE_STATE_COMING] Full formed, running module_init
[  369.846259] test_klp_callbacks_mod: test_klp_callbacks_mod_init
[  369.865115] % rmmod test_klp_callbacks_mod
[  369.881713] test_klp_callbacks_mod: test_klp_callbacks_mod_exit
[  369.888790] test_klp_callbacks_demo: pre_unpatch_callback: test_klp_callbacks_mod -> [MODULE_STATE_GOING] Going away
[  369.900583] livepatch: reverting patch 'test_klp_callbacks_demo' on unloading module 'test_klp_callbacks_mod'
[  369.911696] test_klp_callbacks_demo: post_unpatch_callback: test_klp_callbacks_mod -> [MODULE_STATE_GOING] Going away
[  369.946082] % echo 0 > /sys/kernel/livepatch/test_klp_callbacks_demo/enabled
[  369.954544] test_klp_callbacks_demo: pre_unpatch_callback: vmlinux
[  369.962117] test_klp_callbacks_demo: pre_unpatch_callback: test_klp_callbacks_busy -> [MODULE_STATE_LIVE] Normal state
[  369.974099] livepatch: 'test_klp_callbacks_demo': starting unpatching transition
[  370.022730] test_klp_callbacks_demo: post_unpatch_callback: vmlinux
[  370.029763] test_klp_callbacks_demo: post_unpatch_callback: test_klp_callbacks_busy -> [MODULE_STATE_LIVE] Normal state
[  370.042065] livepatch: 'test_klp_callbacks_demo': unpatching complete
[  494.498310] INFO: task test-callbacks.:10039 blocked for more than 122 seconds.
[  494.506489]       Tainted: G              K   5.15.0-rc7+ #2
[  494.512834] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
[  494.521601] task:test-callbacks. state:D stack:    0 pid:10039 ppid: 10036 flags:0x00004000
[  494.530958] Call Trace:
[  494.533706]  __schedule+0x200/0x540
[  494.537628]  schedule+0x44/0xa0
[  494.541161]  __kernfs_remove.part.0+0x21e/0x2a0
[  494.546251]  ? do_wait_intr_irq+0xa0/0xa0
[  494.550761]  kernfs_remove_by_name_ns+0x50/0x90
[  494.555852]  remove_files+0x2b/0x60
[  494.559783]  sysfs_remove_group+0x38/0x80
[  494.564300]  sysfs_remove_groups+0x29/0x40
[  494.568908]  __kobject_del+0x1b/0x80
[  494.572933]  kobject_cleanup+0x9c/0x130
[  494.577251]  enabled_store+0xdc/0x1a0
[  494.581379]  kernfs_fop_write_iter+0x11c/0x1b0
[  494.586374]  new_sync_write+0x11f/0x1b0
[  494.590690]  ? msr_build_context.constprop.0+0x5d/0xbe
[  494.596462]  vfs_write+0x1ce/0x260
[  494.600291]  ksys_write+0x5f/0xe0
[  494.604024]  do_syscall_64+0x3b/0x90
[  494.608049]  entry_SYSCALL_64_after_hwframe+0x44/0xae
[  494.613719] RIP: 0033:0x7f66cd5aea37
[  494.617733] RSP: 002b:00007ffe6a5e16c8 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
[  494.626209] RAX: ffffffffffffffda RBX: 0000000000000002 RCX: 00007f66cd5aea37
[  494.634196] RDX: 0000000000000002 RSI: 0000562e8101ba60 RDI: 0000000000000001
[  494.642177] RBP: 0000562e8101ba60 R08: 0000000000000000 R09: 00007f66cd6634e0
[  494.650166] R10: 00007f66cd6633e0 R11: 0000000000000246 R12: 0000000000000002
[  494.658156] R13: 00007f66cd6a85a0 R14: 0000000000000002 R15: 00007f66cd6a87a0
...
[ 1600.420533] INFO: task test-callbacks.:10039 blocked for more than 1228 seconds.

Let me know if you have any questions about the tests.  If you look at
the "^%" prefixed kernel messages in the above log, you can get a rough
idea of the commands that the test ran.

Regards,

-- Joe

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

[Ming Lei]

On Fri, Oct 29, 2021 at 09:51:54AM -0400, Joe Lawrence wrote:
> On Thu, Oct 28, 2021 at 08:57:31PM +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.
> >
> 
> Hi Ming,
> 
> I gave the patchset a spin on top of linus tree @ 1fc596a56b33 and ended
> up with a stuck task:
> 
> Test
> ----
> Enable the livepatch selftests:
>   $ grep CONFIG_TEST_LIVEPATCH .config
>   CONFIG_TEST_LIVEPATCH=m
> 
> Run a continuous kernel build in the background:
>   $ while (true); do make clean && make -j$(nproc); done
> 
> While continuously executing the selftests:
>   $ while (true); do make -C tools/testing/selftests/livepatch/ run_tests; done
> 
> Results
> -------

Hello Joe,

Thanks for the test!

Can you replace the 3rd patch with the following one then running the test again?

From 599e96f79aebc388ef3854134312c6039a7884bf Mon Sep 17 00:00:00 2001
From: Ming Lei <ming.lei@redhat.com>
Date: Thu, 28 Oct 2021 20:11:23 +0800
Subject: [PATCH 3/3] livepatch: free klp_patch object synchronously

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().

Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 include/linux/livepatch.h     |  1 -
 kernel/livepatch/core.c       | 32 +++++++++++++-------------------
 kernel/livepatch/core.h       |  2 +-
 kernel/livepatch/transition.c |  2 +-
 4 files changed, 15 insertions(+), 22 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..6cfc54f6bdcc 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,7 +370,14 @@ static ssize_t enabled_store(struct kobject *kobj, struct kobj_attribute *attr,
 out:
 	mutex_unlock(&klp_mutex);
 
-	klp_free_patches_async(&to_free);
+	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);
+	}
 
 	if (ret)
 		return ret;
@@ -684,32 +692,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 +868,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


Thanks,
Ming

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

[Joe Lawrence]

On Sat, Oct 30, 2021 at 12:36:28AM +0800, Ming Lei wrote:
> On Fri, Oct 29, 2021 at 09:51:54AM -0400, Joe Lawrence wrote:
> > On Thu, Oct 28, 2021 at 08:57:31PM +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.
> > >
> > 
> > Hi Ming,
> > 
> > I gave the patchset a spin on top of linus tree @ 1fc596a56b33 and ended
> > up with a stuck task:
> > 
> > Test
> > ----
> > Enable the livepatch selftests:
> >   $ grep CONFIG_TEST_LIVEPATCH .config
> >   CONFIG_TEST_LIVEPATCH=m
> > 
> > Run a continuous kernel build in the background:
> >   $ while (true); do make clean && make -j$(nproc); done
> > 
> > While continuously executing the selftests:
> >   $ while (true); do make -C tools/testing/selftests/livepatch/ run_tests; done
> > 
> > Results
> > -------
> 
> Hello Joe,
> 
> Thanks for the test!
> 
> Can you replace the 3rd patch with the following one then running the test again?
> 
> From 599e96f79aebc388ef3854134312c6039a7884bf Mon Sep 17 00:00:00 2001
> From: Ming Lei <ming.lei@redhat.com>
> Date: Thu, 28 Oct 2021 20:11:23 +0800
> Subject: [PATCH 3/3] livepatch: free klp_patch object synchronously
> 
> 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().
> 
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
>  include/linux/livepatch.h     |  1 -
>  kernel/livepatch/core.c       | 32 +++++++++++++-------------------
>  kernel/livepatch/core.h       |  2 +-
>  kernel/livepatch/transition.c |  2 +-
>  4 files changed, 15 insertions(+), 22 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..6cfc54f6bdcc 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,7 +370,14 @@ static ssize_t enabled_store(struct kobject *kobj, struct kobj_attribute *attr,
>  out:
>  	mutex_unlock(&klp_mutex);
>  
> -	klp_free_patches_async(&to_free);
> +	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);
> +	}
>  
>  	if (ret)
>  		return ret;
> @@ -684,32 +692,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 +868,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
> 
> 

Hi Ming,

The previous test runs without hung tasks, but I noticed that is
probably because the selftests quickly wedge.  A simple reproducer for
that:

# (background load)
% while(true); do make clean && make -j$(nproc); done
vs.
# (selftests)
% 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 ... ok
TEST: basic function patching ... ok
TEST: multiple livepatches ... ok
TEST: atomic replace livepatch ... ok
TEST: basic function patching ... ok
TEST: multiple livepatches ... ERROR: failed to disable livepatch test_klp_livepatch

% lsmod | grep test_klp
test_klp_livepatch     16384  1

% cat /sys/kernel/livepatch/test_klp_livepatch/enabled 
0

% rmmod test_klp_livepatch
rmmod: ERROR: Module test_klp_livepatch is in use

[  249.870587] ===== TEST: multiple livepatches =====
[  249.885520] % modprobe test_klp_livepatch
[  249.916987] livepatch: enabling patch 'test_klp_livepatch'
[  249.923131] livepatch: 'test_klp_livepatch': initializing patching transition
[  249.925575] livepatch: 'test_klp_livepatch': starting patching transition
[  249.934215] livepatch: 'test_klp_livepatch': completing patching transition
[  249.934337] livepatch: 'test_klp_livepatch': patching complete
[  249.946294] test_klp_livepatch: this has been live patched
[  249.963337] % modprobe test_klp_atomic_replace replace=0
[  249.996371] livepatch: enabling patch 'test_klp_atomic_replace'
[  250.002998] livepatch: 'test_klp_atomic_replace': initializing patching transition
[  250.005333] livepatch: 'test_klp_atomic_replace': starting patching transition
[  250.014364] livepatch: 'test_klp_atomic_replace': completing patching transition
[  250.014471] livepatch: 'test_klp_atomic_replace': patching complete
[  250.027259] test_klp_livepatch: this has been live patched
[  250.036050] test_klp_atomic_replace: this has been live patched
[  250.046347] % echo 0 > /sys/kernel/livepatch/test_klp_atomic_replace/enabled
[  250.054403] livepatch: 'test_klp_atomic_replace': initializing unpatching transition
[  250.054574] livepatch: 'test_klp_atomic_replace': starting unpatching transition
[  251.764849] livepatch: 'test_klp_atomic_replace': completing unpatching transition
[  251.799266] livepatch: 'test_klp_atomic_replace': unpatching complete
[  251.911706] % rmmod test_klp_atomic_replace
[  251.970438] test_klp_livepatch: this has been live patched
[  251.980347] % echo 0 > /sys/kernel/livepatch/test_klp_livepatch/enabled
[  251.987936] livepatch: 'test_klp_livepatch': initializing unpatching transition
[  251.988115] livepatch: 'test_klp_livepatch': starting unpatching transition
[  251.997033] livepatch: 'test_klp_livepatch': completing unpatching transition
[  252.027090] livepatch: 'test_klp_livepatch': unpatching complete
[  313.289932] ERROR: failed to disable livepatch test_klp_livepatch

In this case, the "failed to disable" msg occurs because the sysfs
interface / module remain present.

-- Joe

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

[Ming Lei]

On Sun, Oct 31, 2021 at 10:26:18PM -0400, Joe Lawrence wrote:
> On Sat, Oct 30, 2021 at 12:36:28AM +0800, Ming Lei wrote:
> > On Fri, Oct 29, 2021 at 09:51:54AM -0400, Joe Lawrence wrote:
> > > On Thu, Oct 28, 2021 at 08:57:31PM +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.
> > > >
> > > 
> > > Hi Ming,
> > > 
> > > I gave the patchset a spin on top of linus tree @ 1fc596a56b33 and ended
> > > up with a stuck task:
> > > 
> > > Test
> > > ----
> > > Enable the livepatch selftests:
> > >   $ grep CONFIG_TEST_LIVEPATCH .config
> > >   CONFIG_TEST_LIVEPATCH=m
> > > 
> > > Run a continuous kernel build in the background:
> > >   $ while (true); do make clean && make -j$(nproc); done
> > > 
> > > While continuously executing the selftests:
> > >   $ while (true); do make -C tools/testing/selftests/livepatch/ run_tests; done
> > > 
> > > Results
> > > -------
> > 
> > Hello Joe,
> > 
> > Thanks for the test!
> > 
> > Can you replace the 3rd patch with the following one then running the test again?
> > 
> > From 599e96f79aebc388ef3854134312c6039a7884bf Mon Sep 17 00:00:00 2001
> > From: Ming Lei <ming.lei@redhat.com>
> > Date: Thu, 28 Oct 2021 20:11:23 +0800
> > Subject: [PATCH 3/3] livepatch: free klp_patch object synchronously
> > 
> > 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().
> > 
> > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > ---
> >  include/linux/livepatch.h     |  1 -
> >  kernel/livepatch/core.c       | 32 +++++++++++++-------------------
> >  kernel/livepatch/core.h       |  2 +-
> >  kernel/livepatch/transition.c |  2 +-
> >  4 files changed, 15 insertions(+), 22 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..6cfc54f6bdcc 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,7 +370,14 @@ static ssize_t enabled_store(struct kobject *kobj, struct kobj_attribute *attr,
> >  out:
> >  	mutex_unlock(&klp_mutex);
> >  
> > -	klp_free_patches_async(&to_free);
> > +	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);
> > +	}
> >  
> >  	if (ret)
> >  		return ret;
> > @@ -684,32 +692,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 +868,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
> > 
> > 
> 
> Hi Ming,
> 
> The previous test runs without hung tasks, but I noticed that is
> probably because the selftests quickly wedge.  A simple reproducer for
> that:
> 
> # (background load)
> % while(true); do make clean && make -j$(nproc); done
> vs.
> # (selftests)
> % 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 ... ok
> TEST: basic function patching ... ok
> TEST: multiple livepatches ... ok
> TEST: atomic replace livepatch ... ok
> TEST: basic function patching ... ok
> TEST: multiple livepatches ... ERROR: failed to disable livepatch test_klp_livepatch
> 
> % lsmod | grep test_klp
> test_klp_livepatch     16384  1
> 
> % cat /sys/kernel/livepatch/test_klp_livepatch/enabled 
> 0
> 
> % rmmod test_klp_livepatch
> rmmod: ERROR: Module test_klp_livepatch is in use
> 
> [  249.870587] ===== TEST: multiple livepatches =====
> [  249.885520] % modprobe test_klp_livepatch
> [  249.916987] livepatch: enabling patch 'test_klp_livepatch'
> [  249.923131] livepatch: 'test_klp_livepatch': initializing patching transition
> [  249.925575] livepatch: 'test_klp_livepatch': starting patching transition
> [  249.934215] livepatch: 'test_klp_livepatch': completing patching transition
> [  249.934337] livepatch: 'test_klp_livepatch': patching complete
> [  249.946294] test_klp_livepatch: this has been live patched
> [  249.963337] % modprobe test_klp_atomic_replace replace=0
> [  249.996371] livepatch: enabling patch 'test_klp_atomic_replace'
> [  250.002998] livepatch: 'test_klp_atomic_replace': initializing patching transition
> [  250.005333] livepatch: 'test_klp_atomic_replace': starting patching transition
> [  250.014364] livepatch: 'test_klp_atomic_replace': completing patching transition
> [  250.014471] livepatch: 'test_klp_atomic_replace': patching complete
> [  250.027259] test_klp_livepatch: this has been live patched
> [  250.036050] test_klp_atomic_replace: this has been live patched
> [  250.046347] % echo 0 > /sys/kernel/livepatch/test_klp_atomic_replace/enabled
> [  250.054403] livepatch: 'test_klp_atomic_replace': initializing unpatching transition
> [  250.054574] livepatch: 'test_klp_atomic_replace': starting unpatching transition
> [  251.764849] livepatch: 'test_klp_atomic_replace': completing unpatching transition
> [  251.799266] livepatch: 'test_klp_atomic_replace': unpatching complete
> [  251.911706] % rmmod test_klp_atomic_replace
> [  251.970438] test_klp_livepatch: this has been live patched
> [  251.980347] % echo 0 > /sys/kernel/livepatch/test_klp_livepatch/enabled
> [  251.987936] livepatch: 'test_klp_livepatch': initializing unpatching transition
> [  251.988115] livepatch: 'test_klp_livepatch': starting unpatching transition
> [  251.997033] livepatch: 'test_klp_livepatch': completing unpatching transition
> [  252.027090] livepatch: 'test_klp_livepatch': unpatching complete
> [  313.289932] ERROR: failed to disable livepatch test_klp_livepatch
> 
> In this case, the "failed to disable" msg occurs because the sysfs
> interface / module remain present.

Hi Joe,

Thanks for your test!

Hammm, the check of list_empty() in enabled_store() is inverted, please try the
V3:

https://lore.kernel.org/lkml/20211101112548.3364086-4-ming.lei@redhat.com/T/#u


Thanks,
Ming