live-patching.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3] livepatch: Skip livepatch tests if ftrace cannot be configured
@ 2022-02-16 16:11 David Vernet
  2022-02-18  8:54 ` Miroslav Benes
                   ` (2 more replies)
  0 siblings, 3 replies; 5+ messages in thread
From: David Vernet @ 2022-02-16 16:11 UTC (permalink / raw)
  To: live-patching, linux-kernel, jpoimboe, pmladek, jikos, mbenes,
	joe.lawrence, corbet
  Cc: void, kernel-team

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 <pmladek@suse.com>
Signed-off-by: David Vernet <void@manifault.com>
---
v3:
  - Fixed a bug in v1 and v2 pointed out by Petr in
    https://lore.kernel.org/all/20220209154934.5w5k5tqcqldbfjt3@dev0025.ash9.facebook.com/
    wherein we'd incorrectly skip() in test-ftrace.sh when we expected to
    fail to set sysctl due to ftrace being in-use by livepatch.
  - Applied Petr's suggestions for the patch, which address the
    aforementioned issue by adding a --fail flag to set_ftrace_enabled().
    This allows callers to signal that they expect an invocation to fail.
  - Updates the caller in test-ftrace.sh that expects the call to
    set_ftrace_enabled() to fail, to pass this --fail flag.
  - Slightly updates the commit summary to reflect that we only skip the
    test "when appropriate" rather than if the call to sysctl fails.

v2:
  - Fix typo in newly added comment (s/permament/permanent).
  - Adjust the location of the added newline to be before the new comment
    rather than that the end of the function.
  - Make the failure-path check a bit less brittle by checking for the
    exact expected string, rather than specifically for "Device or resource
    busy".

 .../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"
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH v3] livepatch: Skip livepatch tests if ftrace cannot be configured
  2022-02-16 16:11 [PATCH v3] livepatch: Skip livepatch tests if ftrace cannot be configured David Vernet
@ 2022-02-18  8:54 ` Miroslav Benes
  2022-02-21 14:13 ` Petr Mladek
  2022-02-24  9:08 ` Petr Mladek
  2 siblings, 0 replies; 5+ messages in thread
From: Miroslav Benes @ 2022-02-18  8:54 UTC (permalink / raw)
  To: David Vernet
  Cc: live-patching, linux-kernel, jpoimboe, pmladek, jikos,
	joe.lawrence, corbet, kernel-team

On Wed, 16 Feb 2022, David Vernet wrote:

> 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 <pmladek@suse.com>
> Signed-off-by: David Vernet <void@manifault.com>

Acked-by: Miroslav Benes <mbenes@suse.cz>

M

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v3] livepatch: Skip livepatch tests if ftrace cannot be configured
  2022-02-16 16:11 [PATCH v3] livepatch: Skip livepatch tests if ftrace cannot be configured David Vernet
  2022-02-18  8:54 ` Miroslav Benes
@ 2022-02-21 14:13 ` Petr Mladek
  2022-02-22 15:34   ` Joe Lawrence
  2022-02-24  9:08 ` Petr Mladek
  2 siblings, 1 reply; 5+ messages in thread
From: Petr Mladek @ 2022-02-21 14:13 UTC (permalink / raw)
  To: David Vernet
  Cc: live-patching, linux-kernel, jpoimboe, jikos, mbenes,
	joe.lawrence, corbet, kernel-team

On Wed 2022-02-16 08:11:01, David Vernet wrote:
> 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 <pmladek@suse.com>
> Signed-off-by: David Vernet <void@manifault.com>

Reviewed-by: Petr Mladek <pmladek@suse.com>
Tested-by: Petr Mladek <pmladek@suse.com>

I am going to push it after two days or so unless anyone is against.

Best Regards,
Petr

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v3] livepatch: Skip livepatch tests if ftrace cannot be configured
  2022-02-21 14:13 ` Petr Mladek
@ 2022-02-22 15:34   ` Joe Lawrence
  0 siblings, 0 replies; 5+ messages in thread
From: Joe Lawrence @ 2022-02-22 15:34 UTC (permalink / raw)
  To: Petr Mladek, David Vernet
  Cc: live-patching, linux-kernel, jpoimboe, jikos, mbenes, corbet,
	kernel-team

On 2/21/22 9:13 AM, Petr Mladek wrote:
> On Wed 2022-02-16 08:11:01, David Vernet wrote:
>> 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 <pmladek@suse.com>
>> Signed-off-by: David Vernet <void@manifault.com>
> 
> Reviewed-by: Petr Mladek <pmladek@suse.com>
> Tested-by: Petr Mladek <pmladek@suse.com>
> 
> I am going to push it after two days or so unless anyone is against.
> 

Sorry for late replies as I'm just back from PTO, but v3 looks good to
me.  Thanks Petr and David.

Acked-by: Joe Lawrence <joe.lawrence@redhat.com>

-- 
Joe


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v3] livepatch: Skip livepatch tests if ftrace cannot be configured
  2022-02-16 16:11 [PATCH v3] livepatch: Skip livepatch tests if ftrace cannot be configured David Vernet
  2022-02-18  8:54 ` Miroslav Benes
  2022-02-21 14:13 ` Petr Mladek
@ 2022-02-24  9:08 ` Petr Mladek
  2 siblings, 0 replies; 5+ messages in thread
From: Petr Mladek @ 2022-02-24  9:08 UTC (permalink / raw)
  To: David Vernet
  Cc: live-patching, linux-kernel, jpoimboe, jikos, mbenes,
	joe.lawrence, corbet, kernel-team

On Wed 2022-02-16 08:11:01, David Vernet wrote:
> 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 <pmladek@suse.com>
> Signed-off-by: David Vernet <void@manifault.com>

The patch has been committed into livepatch/livepatch.git,
branch for-5.18/selftests-fixes.

Best Regards,
Petr

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2022-02-24  9:08 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-16 16:11 [PATCH v3] livepatch: Skip livepatch tests if ftrace cannot be configured David Vernet
2022-02-18  8:54 ` Miroslav Benes
2022-02-21 14:13 ` Petr Mladek
2022-02-22 15:34   ` Joe Lawrence
2022-02-24  9:08 ` Petr Mladek

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).