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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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
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 <[email protected]>
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 <[email protected]>
---
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
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 <[email protected]>
> 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 <[email protected]>
> ---
> 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
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 <[email protected]>
> > 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 <[email protected]>
> > ---
> > 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/[email protected]/T/#u
Thanks,
Ming