From 4327b9eaf8a4ddd2c534e4f4ed7c949cf3d2be1e Mon Sep 17 00:00:00 2001 From: David Vernet Date: Wed, 16 Feb 2022 08:11:01 -0800 Subject: [PATCH 1/3] livepatch: Skip livepatch tests if ftrace cannot be configured livepatch has a set of selftests that are used to validate the behavior of the livepatching subsystem. One of the testcases in the livepatch testsuite is test-ftrace.sh, which among other things, validates that livepatching gracefully fails when ftrace is disabled. In the event that ftrace cannot be disabled using 'sysctl kernel.ftrace_enabled=0', the test will fail later due to it unexpectedly successfully loading the test_klp_livepatch module. While the livepatch selftests are careful to remove any of the livepatch test modules between testcases to avoid this situation, ftrace may still fail to be disabled if another trace is active on the system that was enabled with FTRACE_OPS_FL_PERMANENT. For example, any active BPF programs that use trampolines will cause this test to fail due to the trampoline being implemented with register_ftrace_direct(). The following is an example of such a trace: tcp_drop (1) R I D tramp: ftrace_regs_caller+0x0/0x58 (call_direct_funcs+0x0/0x30) direct-->bpf_trampoline_6442550536_0+0x0/0x1000 In order to make the test more resilient to system state that is out of its control, this patch updates set_ftrace_enabled() to detect sysctl failures, and skip the testrun when appropriate. Suggested-by: Petr Mladek Signed-off-by: David Vernet Acked-by: Miroslav Benes Reviewed-by: Petr Mladek Tested-by: Petr Mladek Acked-by: Joe Lawrence Signed-off-by: Petr Mladek Link: https://lore.kernel.org/r/20220216161100.3243100-1-void@manifault.com --- .../testing/selftests/livepatch/functions.sh | 22 ++++++++++++++++--- .../selftests/livepatch/test-ftrace.sh | 3 ++- 2 files changed, 21 insertions(+), 4 deletions(-) diff --git a/tools/testing/selftests/livepatch/functions.sh b/tools/testing/selftests/livepatch/functions.sh index 846c7ed71556..9230b869371d 100644 --- a/tools/testing/selftests/livepatch/functions.sh +++ b/tools/testing/selftests/livepatch/functions.sh @@ -75,9 +75,25 @@ function set_dynamic_debug() { } function set_ftrace_enabled() { - result=$(sysctl -q kernel.ftrace_enabled="$1" 2>&1 && \ - sysctl kernel.ftrace_enabled 2>&1) - echo "livepatch: $result" > /dev/kmsg + local can_fail=0 + if [[ "$1" == "--fail" ]] ; then + can_fail=1 + shift + fi + + local err=$(sysctl -q kernel.ftrace_enabled="$1" 2>&1) + local result=$(sysctl --values kernel.ftrace_enabled) + + if [[ "$result" != "$1" ]] ; then + if [[ $can_fail -eq 1 ]] ; then + echo "livepatch: $err" > /dev/kmsg + return + fi + + skip "failed to set kernel.ftrace_enabled = $1" + fi + + echo "livepatch: kernel.ftrace_enabled = $result" > /dev/kmsg } function cleanup() { diff --git a/tools/testing/selftests/livepatch/test-ftrace.sh b/tools/testing/selftests/livepatch/test-ftrace.sh index 552e165512f4..825540a5194d 100755 --- a/tools/testing/selftests/livepatch/test-ftrace.sh +++ b/tools/testing/selftests/livepatch/test-ftrace.sh @@ -25,7 +25,8 @@ if [[ "$(cat /proc/cmdline)" != "$MOD_LIVEPATCH: this has been live patched" ]] die "livepatch kselftest(s) failed" fi -set_ftrace_enabled 0 +# Check that ftrace could not get disabled when a livepatch is enabled +set_ftrace_enabled --fail 0 if [[ "$(cat /proc/cmdline)" != "$MOD_LIVEPATCH: this has been live patched" ]] ; then echo -e "FAIL\n\n" die "livepatch kselftest(s) failed" From 2957308343fa7c621df9f342fab88cb970b8d5f3 Mon Sep 17 00:00:00 2001 From: Chengming Zhou Date: Sat, 12 Mar 2022 23:22:20 +0800 Subject: [PATCH 2/3] livepatch: Don't block removal of patches that are safe to unload module_put() is not called for a patch with "forced" flag. It should block the removal of the livepatch module when the code might still be in use after forced transition. klp_force_transition() currently sets "forced" flag for all patches on the list. In fact, any patch can be safely unloaded when it passed through the consistency model in KLP_UNPATCHED transition. In other words, the "forced" flag must be set only for livepatches that are being removed. In particular, set the "forced" flag: + only for klp_transition_patch when the transition to KLP_UNPATCHED state was forced. + all replaced patches when the transition to KLP_PATCHED state was forced and the patch was replacing the existing patches. Signed-off-by: Chengming Zhou Acked-by: Joe Lawrence Reviewed-by: Petr Mladek Tested-by: Petr Mladek [mbenes@suse.cz: wording improvements] Signed-off-by: Petr Mladek Link: https://lore.kernel.org/r/20220312152220.88127-1-zhouchengming@bytedance.com --- kernel/livepatch/transition.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c index 5683ac0d2566..77ef45a1e0a3 100644 --- a/kernel/livepatch/transition.c +++ b/kernel/livepatch/transition.c @@ -641,6 +641,13 @@ void klp_force_transition(void) for_each_possible_cpu(cpu) klp_update_patch_state(idle_task(cpu)); - klp_for_each_patch(patch) - patch->forced = true; + /* Set forced flag for patches being removed. */ + if (klp_target_state == KLP_UNPATCHED) + klp_transition_patch->forced = true; + else if (klp_transition_patch->replace) { + klp_for_each_patch(patch) { + if (patch != klp_transition_patch) + patch->forced = true; + } + } } From 5e6ded2e7a5d9c71186acc8f51989ef6e6addda4 Mon Sep 17 00:00:00 2001 From: Tom Rix Date: Sat, 19 Mar 2022 18:51:43 -0700 Subject: [PATCH 3/3] livepatch: Reorder to use before freeing a pointer Clang static analysis reports this issue livepatch-shadow-fix1.c:113:2: warning: Use of memory after it is freed pr_info("%s: dummy @ %p, prevented leak @ %p\n", ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ The pointer is freed in the previous statement. Reorder the pr_info to report before the free. Similar issue in livepatch-shadow-fix2.c Note that it is a false positive. pr_info() just prints the address. The freed memory is not accessed. Well, the static analyzer could not know this easily. Signed-off-by: Tom Rix Reviewed-by: Petr Mladek Acked-by: David Vernet Acked-by: Joe Lawrence [pmladek@suse.com: Note about that it was false positive.] Signed-off-by: Petr Mladek Link: https://lore.kernel.org/r/20220320015143.2208591-1-trix@redhat.com --- samples/livepatch/livepatch-shadow-fix1.c | 2 +- samples/livepatch/livepatch-shadow-fix2.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/samples/livepatch/livepatch-shadow-fix1.c b/samples/livepatch/livepatch-shadow-fix1.c index 918ce17b43fd..6701641bf12d 100644 --- a/samples/livepatch/livepatch-shadow-fix1.c +++ b/samples/livepatch/livepatch-shadow-fix1.c @@ -109,9 +109,9 @@ static void livepatch_fix1_dummy_leak_dtor(void *obj, void *shadow_data) void *d = obj; int **shadow_leak = shadow_data; - kfree(*shadow_leak); pr_info("%s: dummy @ %p, prevented leak @ %p\n", __func__, d, *shadow_leak); + kfree(*shadow_leak); } static void livepatch_fix1_dummy_free(struct dummy *d) diff --git a/samples/livepatch/livepatch-shadow-fix2.c b/samples/livepatch/livepatch-shadow-fix2.c index 29fe5cd42047..361046a4f10c 100644 --- a/samples/livepatch/livepatch-shadow-fix2.c +++ b/samples/livepatch/livepatch-shadow-fix2.c @@ -61,9 +61,9 @@ static void livepatch_fix2_dummy_leak_dtor(void *obj, void *shadow_data) void *d = obj; int **shadow_leak = shadow_data; - kfree(*shadow_leak); pr_info("%s: dummy @ %p, prevented leak @ %p\n", __func__, d, *shadow_leak); + kfree(*shadow_leak); } static void livepatch_fix2_dummy_free(struct dummy *d)