| Message ID | 20211119090327.12811-4-mbenes@suse.cz (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | livepatch: Allow user to specify functions to search for on a stack | expand |
On Fri 2021-11-19 10:03:27, Miroslav Benes wrote: > Add a test for the API which allows the user to specify functions which > are then searched for on any tasks's stack during a transition process. > > --- /dev/null > +++ b/lib/livepatch/test_klp_funcstack_mod.c > @@ -0,0 +1,72 @@ > +// SPDX-License-Identifier: GPL-2.0 > +// Copyright (C) 2021 Miroslav Benes <mbenes@suse.cz> > + > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > + > +#include <linux/module.h> > +#include <linux/kernel.h> > +#include <linux/debugfs.h> > +#include <linux/delay.h> > + > +static int sleep_length = 10000; > +module_param(sleep_length, int, 0644); > +MODULE_PARM_DESC(sleep_length, "length of sleep in seconds (default=10)"); > + > +static noinline void child_function(void) > +{ > + pr_info("%s enter\n", __func__); > + msleep(sleep_length); The hardcoded sleep is not ideal. It might be too low or non-necessary high. If I get it correctly, we are trying to achieve here the same as busymod_work_func() in test_klp_callbacks_busy.c. The approach with debugfs is an interesting trick. Though, I slightly prefer using the scheduled work. The workqueue API looks less tricky to me than sysfs/debugfs API. Also it does not block the module in the init() callback[*]. But I might be biased. Anyway, it might make sense to use the same trick in both situations. It would make it easier to maintain the test modules. [*] There is actually a race in the workqueue approach. The module init() callback should wait until the work is really scheduled and sleeping. It might be achieved by similar hand-shake like with @block_transition variable. Or completion API might be even more elegant. > + pr_info("%s exit\n", __func__); > +} > + > +static noinline void child2_function(void) > +{ > + pr_info("%s\n", __func__); > +} > + > +static noinline void parent_function(void) > +{ > + pr_info("%s enter\n", __func__); > + child_function(); > + child2_function(); This would deserve some explanation what we try to simulate here and how it is achieved. It is not easy for me even with the background that I have freshly in my mind. Also I think about more descriptive names ;-) What about something like this (using workqueue work and completion): /* * Simulate part of the caller code that is in another .elf section * and is reached via jump. It this was really the case then the stack * unwinder might not be able to detect that the process is sleeping * in the caller. */ static void simulate_jump_part(void) { pr_info("%s enter\n", __func__); /* Stay in the jump part unless told to leave. */ wait_for_completion(finish_jump); pr_info("%s exit\n", __func__); } /* * Simulate modified part of the caller code. It should never get * livepatched when the caller is sleeping in the just_part(). */ static void simulate_modified_part(void) { pr_info("%s\n", __func__); } static void test_not_on_stack_func_work(struct work_struct *work) { pr_info("%s enter\n", __func__); /* Simulation ready */ complete(work_started); simulate_jump_part(); simulate_modified_part(); pr_info("%s exit\n", __func__); } static int test_klp_no_on_stack_init(void) { pr_info("%s\n", __func__); schedule_work(&work); wait_for_completion(&work_started); return 0; } static void test_not_on_stack_exit(void) { complete(&finish_jump); flush_work(&work); pr_info("%s\n", __func__); } module_init(test_klp_not_on_stack_init); module_exit(test_klp_not_on_stack_exit); > + pr_info("%s exit\n", __func__); > +} > + Best Regards, Petr
On Thu, 25 Nov 2021, Petr Mladek wrote: > On Fri 2021-11-19 10:03:27, Miroslav Benes wrote: > > Add a test for the API which allows the user to specify functions which > > are then searched for on any tasks's stack during a transition process. > > > > --- /dev/null > > +++ b/lib/livepatch/test_klp_funcstack_mod.c > > @@ -0,0 +1,72 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +// Copyright (C) 2021 Miroslav Benes <mbenes@suse.cz> > > + > > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > > + > > +#include <linux/module.h> > > +#include <linux/kernel.h> > > +#include <linux/debugfs.h> > > +#include <linux/delay.h> > > + > > +static int sleep_length = 10000; > > +module_param(sleep_length, int, 0644); > > +MODULE_PARM_DESC(sleep_length, "length of sleep in seconds (default=10)"); > > + > > +static noinline void child_function(void) > > +{ > > + pr_info("%s enter\n", __func__); > > + msleep(sleep_length); > > The hardcoded sleep is not ideal. It might be too low or non-necessary high. It is not. > If I get it correctly, we are trying to achieve here the same as > busymod_work_func() in test_klp_callbacks_busy.c. Yes. > The approach with debugfs is an interesting trick. Though, I slightly > prefer using the scheduled work. The workqueue API looks less tricky > to me than sysfs/debugfs API. Also it does not block the module > in the init() callback[*]. But I might be biased. It seemed to me that debugfs gave us more control over the process than workqueues, but I do not really care. Could you explain the blocking in the init callback? I do not follow. > Anyway, it might make sense to use the same trick in both situations. > It would make it easier to maintain the test modules. True. So I will rewrite it to workqueues as you are proposing below. > [*] There is actually a race in the workqueue approach. The module > init() callback should wait until the work is really scheduled > and sleeping. It might be achieved by similar hand-shake like > with @block_transition variable. Or completion API might be > even more elegant. > > > > + pr_info("%s exit\n", __func__); > > +} > > + > > +static noinline void child2_function(void) > > +{ > > + pr_info("%s\n", __func__); > > +} > > + > > +static noinline void parent_function(void) > > +{ > > + pr_info("%s enter\n", __func__); > > + child_function(); > > + child2_function(); > > This would deserve some explanation what we try to simulate here > and how it is achieved. It is not easy for me even with the background > that I have freshly in my mind. > > Also I think about more descriptive names ;-) Hey, I thought it was self-explaining :). So, yes, I started with the example given in the .fixup thread, but it is not really tied to .cold section, jumps or whatever. The setup is just used to test a new API. Moreover, the .fixup example is just a one scenario the new API tries to solve. What you propose below, that is function names and comments, is a bit confusing for me. Especially if I did not know anything about the original issue (which will be the case in a couple of weeks when I forget everything). So I think it I will stick to brevity unless you or someone else really insist. I can improve tests description in tools/testing/selftests/livepatch/test-func-stack.sh if it helps anything. Miroslav > What about something like this (using workqueue work and completion): > > /* > * Simulate part of the caller code that is in another .elf section > * and is reached via jump. It this was really the case then the stack > * unwinder might not be able to detect that the process is sleeping > * in the caller. > */ > static void simulate_jump_part(void) > { > pr_info("%s enter\n", __func__); > > /* Stay in the jump part unless told to leave. */ > wait_for_completion(finish_jump); > > pr_info("%s exit\n", __func__); > } > > /* > * Simulate modified part of the caller code. It should never get > * livepatched when the caller is sleeping in the just_part(). > */ > static void simulate_modified_part(void) > { > pr_info("%s\n", __func__); > } > > static void test_not_on_stack_func_work(struct work_struct *work) > { > pr_info("%s enter\n", __func__); > > /* Simulation ready */ > complete(work_started); > > simulate_jump_part(); > simulate_modified_part(); > > pr_info("%s exit\n", __func__); > } > > static int test_klp_no_on_stack_init(void) > { > pr_info("%s\n", __func__); > > schedule_work(&work); > wait_for_completion(&work_started); > > return 0; > } > > static void test_not_on_stack_exit(void) > { > complete(&finish_jump); > flush_work(&work); > pr_info("%s\n", __func__); > } > > module_init(test_klp_not_on_stack_init); > module_exit(test_klp_not_on_stack_exit); > > > + pr_info("%s exit\n", __func__); > > +} > > +
On Fri 2021-11-26 10:20:54, Miroslav Benes wrote: > On Thu, 25 Nov 2021, Petr Mladek wrote: > > > On Fri 2021-11-19 10:03:27, Miroslav Benes wrote: > > > Add a test for the API which allows the user to specify functions which > > > are then searched for on any tasks's stack during a transition process. > > > > > The approach with debugfs is an interesting trick. Though, I slightly > > prefer using the scheduled work. The workqueue API looks less tricky > > to me than sysfs/debugfs API. Also it does not block the module > > in the init() callback[*]. But I might be biased. > > It seemed to me that debugfs gave us more control over the process than > workqueues, but I do not really care. Could you explain the blocking in > the init callback? I do not follow. Good question about the blocking! Please, forget it. I am not sure why I thought that the module might get blocked in the module_init() script. > > Anyway, it might make sense to use the same trick in both situations. > > It would make it easier to maintain the test modules. > > True. So I will rewrite it to workqueues as you are proposing below. > > > [*] There is actually a race in the workqueue approach. The module > > init() callback should wait until the work is really scheduled > > and sleeping. It might be achieved by similar hand-shake like > > with @block_transition variable. Or completion API might be > > even more elegant. > > > > > > > + pr_info("%s exit\n", __func__); > > > +} > > > + > > > +static noinline void child2_function(void) > > > +{ > > > + pr_info("%s\n", __func__); > > > +} > > > + > > > +static noinline void parent_function(void) > > > +{ > > > + pr_info("%s enter\n", __func__); > > > + child_function(); > > > + child2_function(); > > > > This would deserve some explanation what we try to simulate here > > and how it is achieved. It is not easy for me even with the background > > that I have freshly in my mind. > > > > Also I think about more descriptive names ;-) > > Hey, I thought it was self-explaining :). So, yes, I started with the > example given in the .fixup thread, but it is not really tied to .cold > section, jumps or whatever. The setup is just used to test a new API. > Moreover, the .fixup example is just a one scenario the new API tries to > solve. OK, single child() function should be enough for testing the behavior. We might sleep/wait in the parent(). I think that I was confused by the two child() functions. It looked like sleeping in a child function was important. And the "same" name of the two children did not help much to understand and distinguish the purpose. > What you propose below, that is function names and comments, is a bit > confusing for me. Especially if I did not know anything about the original > issue (which will be the case in a couple of weeks when I forget > everything). > > So I think it I will stick to brevity unless you or someone else really > insist. No, I do not resist on the complicated exmaple. When thinking about it, the easier test case the better. It should be enough to describe the real-life purpose of the API in the patch that introduces the API. > I can improve tests description in > tools/testing/selftests/livepatch/test-func-stack.sh if it helps anything. Yes, please. I miss some top-level descriptions of the tested functionality, something like: # Tests for "bla bla" feature. # It allows to block transition of a process when it is running # parent() function and only the child() function is livepatched. # This test does not use the feature. The transition finishes even # before parent() exits. # In this test case, the livepatch is instructed to check also # parent() on stack. The transition must not finish before # parent() exists. It would be nice to have these high-level descriptions even in the test modules. Best Regards, Petr
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug index 9ef7ce18b4f5..aa4c97098f41 100644 --- a/lib/Kconfig.debug +++ b/lib/Kconfig.debug @@ -2529,6 +2529,7 @@ config TEST_LIVEPATCH default n depends on DYNAMIC_DEBUG depends on LIVEPATCH + depends on DEBUG_FS depends on m help Test kernel livepatching features for correctness. The tests will diff --git a/lib/livepatch/Makefile b/lib/livepatch/Makefile index dcc912b3478f..584e3b8b5415 100644 --- a/lib/livepatch/Makefile +++ b/lib/livepatch/Makefile @@ -11,4 +11,6 @@ obj-$(CONFIG_TEST_LIVEPATCH) += test_klp_atomic_replace.o \ test_klp_shadow_vars.o \ test_klp_state.o \ test_klp_state2.o \ - test_klp_state3.o + test_klp_state3.o \ + test_klp_funcstack_mod.o \ + test_klp_funcstack_demo.o diff --git a/lib/livepatch/test_klp_funcstack_demo.c b/lib/livepatch/test_klp_funcstack_demo.c new file mode 100644 index 000000000000..902798077f05 --- /dev/null +++ b/lib/livepatch/test_klp_funcstack_demo.c @@ -0,0 +1,61 @@ +// SPDX-License-Identifier: GPL-2.0 +// Copyright (C) 2021 Miroslav Benes <mbenes@suse.cz> + +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt + +#include <linux/module.h> +#include <linux/kernel.h> +#include <linux/livepatch.h> + +static int funcstack; +module_param(funcstack, int, 0644); +MODULE_PARM_DESC(funcstack, "func_stack (default=0)"); + +static noinline void livepatch_child2_function(void) +{ + pr_info("%s\n", __func__); +} + +static struct klp_func funcs[] = { + { + .old_name = "child2_function", + .new_func = livepatch_child2_function, + }, {} +}; + +static struct klp_func funcs_stack[] = { + { + .old_name = "parent_function", + }, {} +}; + +static struct klp_object objs[] = { + { + .name = "test_klp_funcstack_mod", + .funcs = funcs, + }, {} +}; + +static struct klp_patch patch = { + .mod = THIS_MODULE, + .objs = objs, +}; + +static int test_klp_funcstack_demo_init(void) +{ + if (funcstack) + objs[0].funcs_stack = funcs_stack; + + return klp_enable_patch(&patch); +} + +static void test_klp_funcstack_demo_exit(void) +{ +} + +module_init(test_klp_funcstack_demo_init); +module_exit(test_klp_funcstack_demo_exit); +MODULE_LICENSE("GPL"); +MODULE_INFO(livepatch, "Y"); +MODULE_AUTHOR("Miroslav Benes <mbenes@suse.cz>"); +MODULE_DESCRIPTION("Livepatch test: func_stack demo"); diff --git a/lib/livepatch/test_klp_funcstack_mod.c b/lib/livepatch/test_klp_funcstack_mod.c new file mode 100644 index 000000000000..127c6093d890 --- /dev/null +++ b/lib/livepatch/test_klp_funcstack_mod.c @@ -0,0 +1,72 @@ +// SPDX-License-Identifier: GPL-2.0 +// Copyright (C) 2021 Miroslav Benes <mbenes@suse.cz> + +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt + +#include <linux/module.h> +#include <linux/kernel.h> +#include <linux/debugfs.h> +#include <linux/delay.h> + +static int sleep_length = 10000; +module_param(sleep_length, int, 0644); +MODULE_PARM_DESC(sleep_length, "length of sleep in seconds (default=10)"); + +static noinline void child_function(void) +{ + pr_info("%s enter\n", __func__); + msleep(sleep_length); + pr_info("%s exit\n", __func__); +} + +static noinline void child2_function(void) +{ + pr_info("%s\n", __func__); +} + +static noinline void parent_function(void) +{ + pr_info("%s enter\n", __func__); + child_function(); + child2_function(); + pr_info("%s exit\n", __func__); +} + +static int parent_function_get(void *data, u64 *val) +{ + *val = 0; + parent_function(); + + return 0; +} + +DEFINE_DEBUGFS_ATTRIBUTE(fops_parent_function, parent_function_get, NULL, "%llu\n"); + +static struct dentry *debugfs_dir; + +static int test_klp_funcstack_mod_init(void) +{ + struct dentry *d; + + debugfs_dir = debugfs_create_dir("test_klp_funcstack", NULL); + if (IS_ERR(debugfs_dir)) + return PTR_ERR(debugfs_dir); + + d = debugfs_create_file("parent_function", 0400, debugfs_dir, NULL, + &fops_parent_function); + if (IS_ERR(d)) + debugfs_remove_recursive(debugfs_dir); + + return 0; +} + +static void test_klp_funcstack_mod_exit(void) +{ + debugfs_remove_recursive(debugfs_dir); +} + +module_init(test_klp_funcstack_mod_init); +module_exit(test_klp_funcstack_mod_exit); +MODULE_LICENSE("GPL"); +MODULE_AUTHOR("Miroslav Benes <mbenes@suse.cz>"); +MODULE_DESCRIPTION("Livepatch test: func_stack module"); diff --git a/tools/testing/selftests/livepatch/Makefile b/tools/testing/selftests/livepatch/Makefile index 1acc9e1fa3fb..40f8a3a2e9aa 100644 --- a/tools/testing/selftests/livepatch/Makefile +++ b/tools/testing/selftests/livepatch/Makefile @@ -6,7 +6,8 @@ TEST_PROGS := \ test-callbacks.sh \ test-shadow-vars.sh \ test-state.sh \ - test-ftrace.sh + test-ftrace.sh \ + test-func-stack.sh TEST_FILES := settings diff --git a/tools/testing/selftests/livepatch/test-func-stack.sh b/tools/testing/selftests/livepatch/test-func-stack.sh new file mode 100755 index 000000000000..b7da62c9f5a1 --- /dev/null +++ b/tools/testing/selftests/livepatch/test-func-stack.sh @@ -0,0 +1,88 @@ +#!/bin/bash +# SPDX-License-Identifier: GPL-2.0 +# Copyright (C) 2021 Miroslav Benes <mbenes@suse.cz> + +. $(dirname $0)/functions.sh + +MOD_TARGET=test_klp_funcstack_mod +MOD_LIVEPATCH=test_klp_funcstack_demo + +setup_config + +# - load a target module and call its parent_function(). It will sleep in its +# child_function() callee. +# - load a live patch with new child2_function() called from parent_function() +# too. The patching does not wait for child_function() to return, because +# child2_function() is not on any stack. +# - clean up afterwards + +start_test "non-blocking patching without the function on a stack" + +load_mod $MOD_TARGET + +(cat /sys/kernel/debug/test_klp_funcstack/parent_function) >/dev/null & +PID=$! + +load_lp $MOD_LIVEPATCH + +wait $PID + +disable_lp $MOD_LIVEPATCH +unload_lp $MOD_LIVEPATCH +unload_mod $MOD_TARGET + +check_result "% modprobe $MOD_TARGET +$MOD_TARGET: parent_function enter +$MOD_TARGET: child_function enter +% modprobe $MOD_LIVEPATCH +livepatch: enabling patch '$MOD_LIVEPATCH' +livepatch: '$MOD_LIVEPATCH': initializing patching transition +livepatch: '$MOD_LIVEPATCH': starting patching transition +livepatch: '$MOD_LIVEPATCH': completing patching transition +livepatch: '$MOD_LIVEPATCH': patching complete +$MOD_TARGET: child_function exit +$MOD_LIVEPATCH: livepatch_child2_function +$MOD_TARGET: parent_function exit +% echo 0 > /sys/kernel/livepatch/$MOD_LIVEPATCH/enabled +livepatch: '$MOD_LIVEPATCH': initializing unpatching transition +livepatch: '$MOD_LIVEPATCH': starting unpatching transition +livepatch: '$MOD_LIVEPATCH': completing unpatching transition +livepatch: '$MOD_LIVEPATCH': unpatching complete +% rmmod $MOD_LIVEPATCH +% rmmod $MOD_TARGET" + +# Similar to the previous test but now the patching has to wait for +# child2_function() to return, because parent_function() is also checked for. + +start_test "patching delayed due to the function on a stack" + +load_mod $MOD_TARGET + +(cat /sys/kernel/debug/test_klp_funcstack/parent_function) >/dev/null & + +load_lp $MOD_LIVEPATCH funcstack=1 +disable_lp $MOD_LIVEPATCH +unload_lp $MOD_LIVEPATCH +unload_mod $MOD_TARGET + +check_result "% modprobe $MOD_TARGET +$MOD_TARGET: parent_function enter +$MOD_TARGET: child_function enter +% modprobe $MOD_LIVEPATCH funcstack=1 +livepatch: enabling patch '$MOD_LIVEPATCH' +livepatch: '$MOD_LIVEPATCH': initializing patching transition +livepatch: '$MOD_LIVEPATCH': starting patching transition +$MOD_TARGET: child_function exit +$MOD_TARGET: child2_function +$MOD_TARGET: parent_function exit +livepatch: '$MOD_LIVEPATCH': completing patching transition +livepatch: '$MOD_LIVEPATCH': patching complete +% echo 0 > /sys/kernel/livepatch/$MOD_LIVEPATCH/enabled +livepatch: '$MOD_LIVEPATCH': initializing unpatching transition +livepatch: '$MOD_LIVEPATCH': starting unpatching transition +livepatch: '$MOD_LIVEPATCH': completing unpatching transition +livepatch: '$MOD_LIVEPATCH': unpatching complete +% rmmod $MOD_LIVEPATCH +% rmmod $MOD_TARGET" + +exit 0
Add a test for the API which allows the user to specify functions which are then searched for on any tasks's stack during a transition process. Signed-off-by: Miroslav Benes <mbenes@suse.cz> --- lib/Kconfig.debug | 1 + lib/livepatch/Makefile | 4 +- lib/livepatch/test_klp_funcstack_demo.c | 61 +++++++++++++ lib/livepatch/test_klp_funcstack_mod.c | 72 +++++++++++++++ tools/testing/selftests/livepatch/Makefile | 3 +- .../selftests/livepatch/test-func-stack.sh | 88 +++++++++++++++++++ 6 files changed, 227 insertions(+), 2 deletions(-) create mode 100644 lib/livepatch/test_klp_funcstack_demo.c create mode 100644 lib/livepatch/test_klp_funcstack_mod.c create mode 100755 tools/testing/selftests/livepatch/test-func-stack.sh