[PATCH] livepatch: Skip livepatch tests if ftrace cannot be configured

[PATCH] livepatch: Skip livepatch tests if ftrace cannot be configured

[David Vernet]

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 adds a check to set_ftrace_enabled() to skip the tests
if the sysctl invocation fails.

Signed-off-by: David Vernet <void@manifault.com>
---
 tools/testing/selftests/livepatch/functions.sh | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/tools/testing/selftests/livepatch/functions.sh b/tools/testing/selftests/livepatch/functions.sh
index 846c7ed71556..6857fdcb6b45 100644
--- a/tools/testing/selftests/livepatch/functions.sh
+++ b/tools/testing/selftests/livepatch/functions.sh
@@ -78,6 +78,12 @@ function set_ftrace_enabled() {
 	result=$(sysctl -q kernel.ftrace_enabled="$1" 2>&1 && \
 		 sysctl kernel.ftrace_enabled 2>&1)
 	echo "livepatch: $result" > /dev/kmsg
+	# Skip the test if ftrace is busy.  This can happen under normal system
+	# conditions if a trace is marked as permament.
+	if [[ "$result" == *"Device or resource busy"* ]]; then
+		skip "failed to set kernel.ftrace_enabled=$1"
+	fi
+
 }
 
 function cleanup() {
-- 
2.30.2

Re: [PATCH] livepatch: Skip livepatch tests if ftrace cannot be configured

[Joe Lawrence]

On 2/3/22 6:32 PM, 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 adds a check to set_ftrace_enabled() to skip the tests
> if the sysctl invocation fails.
> 
> Signed-off-by: David Vernet <void@manifault.com>

Hi David,

Thanks for this test case, comments below...

> ---
>  tools/testing/selftests/livepatch/functions.sh | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/tools/testing/selftests/livepatch/functions.sh b/tools/testing/selftests/livepatch/functions.sh
> index 846c7ed71556..6857fdcb6b45 100644
> --- a/tools/testing/selftests/livepatch/functions.sh
> +++ b/tools/testing/selftests/livepatch/functions.sh
> @@ -78,6 +78,12 @@ function set_ftrace_enabled() {
>  	result=$(sysctl -q kernel.ftrace_enabled="$1" 2>&1 && \
>  		 sysctl kernel.ftrace_enabled 2>&1)
>  	echo "livepatch: $result" > /dev/kmsg
> +	# Skip the test if ftrace is busy.  This can happen under normal system
> +	# conditions if a trace is marked as permament.

sp: s/permament/permanent

> +	if [[ "$result" == *"Device or resource busy"* ]]; then
> +		skip "failed to set kernel.ftrace_enabled=$1"
> +	fi
> +

style nit: move the blank line from here to just before the new # Skip
comment

>  }
>  
>  function cleanup() {
> 

Can we be more paranoid and just look for the exact result that we expect:

if [[ "$result" != "kernel.ftrace_enabled = 1" ]]; then
	skip "failed to set kernel.ftrace_enabled=$1"
fi

and that way we catch any other faults.  What do you think?

Thanks,
-- 
Joe

Re: [PATCH] livepatch: Skip livepatch tests if ftrace cannot be configured

[David Vernet]

On Fri, Feb 04, 2022 at 03:30:02PM -0500, Joe Lawrence wrote:

Hi Joe,

Thanks for the review.

> sp: s/permament/permanent
> 

Ack, thanks for catching that.

> > +	if [[ "$result" == *"Device or resource busy"* ]]; then
> > +		skip "failed to set kernel.ftrace_enabled=$1"
> > +	fi
> > +
> 
> style nit: move the blank line from here to just before the new # Skip
> comment

Ack.

> Can we be more paranoid and just look for the exact result that we expect:
> 
> if [[ "$result" != "kernel.ftrace_enabled = 1" ]]; then
> 	skip "failed to set kernel.ftrace_enabled=$1"
> fi

Agreed, this is cleaner and less brittle.

I'll follow-up with a v2 patch shortly. Thanks!

- David

[PATCH v2] livepatch: Skip livepatch tests if ftrace cannot be configured

[David Vernet]

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 adds a check to set_ftrace_enabled() to skip the tests
if the sysctl invocation fails.

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

 tools/testing/selftests/livepatch/functions.sh | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/tools/testing/selftests/livepatch/functions.sh b/tools/testing/selftests/livepatch/functions.sh
index 846c7ed71556..32970324dd7e 100644
--- a/tools/testing/selftests/livepatch/functions.sh
+++ b/tools/testing/selftests/livepatch/functions.sh
@@ -78,6 +78,12 @@ function set_ftrace_enabled() {
 	result=$(sysctl -q kernel.ftrace_enabled="$1" 2>&1 && \
 		 sysctl kernel.ftrace_enabled 2>&1)
 	echo "livepatch: $result" > /dev/kmsg
+
+	# Skip the test if ftrace is busy.  This can happen under normal system
+	# conditions if a trace is marked as permanent.
+	if [[ "$result" != "kernel.ftrace_enabled = $1" ]]; then
+		skip "failed to set kernel.ftrace_enabled=$1"
+	fi
 }
 
 function cleanup() {
-- 
2.30.2

Re: [PATCH v2] livepatch: Skip livepatch tests if ftrace cannot be configured

[Joe Lawrence]

On 2/4/22 3:56 PM, 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 adds a check to set_ftrace_enabled() to skip the tests
> if the sysctl invocation fails.
> 
> Signed-off-by: David Vernet <void@manifault.com>
> ---
> 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".
> 
>  tools/testing/selftests/livepatch/functions.sh | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/tools/testing/selftests/livepatch/functions.sh b/tools/testing/selftests/livepatch/functions.sh
> index 846c7ed71556..32970324dd7e 100644
> --- a/tools/testing/selftests/livepatch/functions.sh
> +++ b/tools/testing/selftests/livepatch/functions.sh
> @@ -78,6 +78,12 @@ function set_ftrace_enabled() {
>  	result=$(sysctl -q kernel.ftrace_enabled="$1" 2>&1 && \
>  		 sysctl kernel.ftrace_enabled 2>&1)
>  	echo "livepatch: $result" > /dev/kmsg
> +
> +	# Skip the test if ftrace is busy.  This can happen under normal system
> +	# conditions if a trace is marked as permanent.
> +	if [[ "$result" != "kernel.ftrace_enabled = $1" ]]; then
> +		skip "failed to set kernel.ftrace_enabled=$1"
> +	fi
>  }
>  
>  function cleanup() {
> 

Thanks for updating.

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

-- 
Joe

Re: [PATCH v2] livepatch: Skip livepatch tests if ftrace cannot be configured

[Petr Mladek]

On Fri 2022-02-04 12:56:26, 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 adds a check to set_ftrace_enabled() to skip the tests
> if the sysctl invocation fails.
> 
> Signed-off-by: David Vernet <void@manifault.com>
> ---
> 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".
> 
>  tools/testing/selftests/livepatch/functions.sh | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/tools/testing/selftests/livepatch/functions.sh b/tools/testing/selftests/livepatch/functions.sh
> index 846c7ed71556..32970324dd7e 100644
> --- a/tools/testing/selftests/livepatch/functions.sh
> +++ b/tools/testing/selftests/livepatch/functions.sh
> @@ -78,6 +78,12 @@ function set_ftrace_enabled() {
>  	result=$(sysctl -q kernel.ftrace_enabled="$1" 2>&1 && \
>  		 sysctl kernel.ftrace_enabled 2>&1)
>  	echo "livepatch: $result" > /dev/kmsg
> +
> +	# Skip the test if ftrace is busy.  This can happen under normal system
> +	# conditions if a trace is marked as permanent.
> +	if [[ "$result" != "kernel.ftrace_enabled = $1" ]]; then
> +		skip "failed to set kernel.ftrace_enabled=$1"
> +	fi
>  }
>  

Hmm, test-ftrace.sh fails with this patch:

localhost:/prace/kernel/linux/tools/testing/selftests/livepatch # ./test-ftrace.sh 
TEST: livepatch interaction with ftrace_enabled sysctl ... SKIP: failed to set kernel.ftrace_enabled=0

This is the dmesg output:

[  436.176433] ===== TEST: livepatch interaction with ftrace_enabled sysctl =====
[  436.186942] livepatch: kernel.ftrace_enabled = 0
[  436.187854] % modprobe test_klp_livepatch
[  436.217657] livepatch: enabling patch 'test_klp_livepatch'
[  436.218467] livepatch: 'test_klp_livepatch': initializing patching transition
[  436.219679] livepatch: failed to register ftrace handler for function 'cmdline_proc_show' (-16)
[  436.221003] livepatch: failed to patch object 'vmlinux'
[  436.221786] livepatch: failed to enable patch 'test_klp_livepatch'
[  436.222716] livepatch: 'test_klp_livepatch': canceling patching transition, going to unpatch
[  436.223955] livepatch: 'test_klp_livepatch': completing unpatching transition
[  436.225476] livepatch: 'test_klp_livepatch': unpatching complete
[  436.273295] modprobe: ERROR: could not insert 'test_klp_livepatch': Device or resource busy
[  436.284522] livepatch: kernel.ftrace_enabled = 1
[  436.305353] % modprobe test_klp_livepatch
[  436.334929] livepatch: enabling patch 'test_klp_livepatch'
[  436.335781] livepatch: 'test_klp_livepatch': initializing patching transition
[  436.338099] livepatch: 'test_klp_livepatch': starting patching transition
[  438.082707] livepatch: 'test_klp_livepatch': completing patching transition
[  438.084106] livepatch: 'test_klp_livepatch': patching complete
[  438.199997] livepatch: sysctl: setting key "kernel.ftrace_enabled": Device or resource busy
[  438.201249] kernel.ftrace_enabled = 1
[  438.201929] SKIP: failed to set kernel.ftrace_enabled=0


The problem is the 2nd "set_ftrace_enabled 0" call in
tools/testing/selftests/livepatch/test-ftrace.sh

It is done when "test_klp_livepatch" is loaded. ftrace could not be
disabled because it is used by the livepatch. It is expected behavior
and the test should succeed in this case.

Best Regards,
Petr

Re: [PATCH v2] livepatch: Skip livepatch tests if ftrace cannot be configured

[David Vernet]

On Wed, Feb 09, 2022 at 02:02:33PM +0100, Petr Mladek wrote:
> Hmm, test-ftrace.sh fails with this patch:
> 
> localhost:/prace/kernel/linux/tools/testing/selftests/livepatch # ./test-ftrace.sh 
> TEST: livepatch interaction with ftrace_enabled sysctl ... SKIP: failed to set kernel.ftrace_enabled=0
> 
> This is the dmesg output:
> 
> [  436.176433] ===== TEST: livepatch interaction with ftrace_enabled sysctl =====
> [  436.186942] livepatch: kernel.ftrace_enabled = 0
> [  436.187854] % modprobe test_klp_livepatch
> [  436.217657] livepatch: enabling patch 'test_klp_livepatch'
> [  436.218467] livepatch: 'test_klp_livepatch': initializing patching transition
> [  436.219679] livepatch: failed to register ftrace handler for function 'cmdline_proc_show' (-16)
> [  436.221003] livepatch: failed to patch object 'vmlinux'
> [  436.221786] livepatch: failed to enable patch 'test_klp_livepatch'
> [  436.222716] livepatch: 'test_klp_livepatch': canceling patching transition, going to unpatch
> [  436.223955] livepatch: 'test_klp_livepatch': completing unpatching transition
> [  436.225476] livepatch: 'test_klp_livepatch': unpatching complete
> [  436.273295] modprobe: ERROR: could not insert 'test_klp_livepatch': Device or resource busy
> [  436.284522] livepatch: kernel.ftrace_enabled = 1
> [  436.305353] % modprobe test_klp_livepatch
> [  436.334929] livepatch: enabling patch 'test_klp_livepatch'
> [  436.335781] livepatch: 'test_klp_livepatch': initializing patching transition
> [  436.338099] livepatch: 'test_klp_livepatch': starting patching transition
> [  438.082707] livepatch: 'test_klp_livepatch': completing patching transition
> [  438.084106] livepatch: 'test_klp_livepatch': patching complete
> [  438.199997] livepatch: sysctl: setting key "kernel.ftrace_enabled": Device or resource busy
> [  438.201249] kernel.ftrace_enabled = 1
> [  438.201929] SKIP: failed to set kernel.ftrace_enabled=0
> 
> 
> The problem is the 2nd "set_ftrace_enabled 0" call in
> tools/testing/selftests/livepatch/test-ftrace.sh
> 
> It is done when "test_klp_livepatch" is loaded. ftrace could not be
> disabled because it is used by the livepatch. It is expected behavior
> and the test should succeed in this case.

Ah, I missed this because in my configuration I was expecting the test to
be skipped. What do we think is the preferred alternative?  Perhaps we
could add an optional argument to set_trace_enabled() that specifies the
expected value of kernel.ftrace_enabled after invoking sysctl?

What do you think of something like this:

+
+	if [[ "$result" != "kernel.ftrace_enabled = $1" ]]; then
+		if [[ -z "$2" || "$2" == "$1" ]]; then
+			skip "failed to set kernel.ftrace_enabled=$1"
+		fi
+	elif [[ -n "$2" && "$2" != "$1" ]]; then
+		die "Unexpectedly set kernel.ftrace_enabled=$1"
+	fi

test-ftrace.sh could then specify 'set_ftrace_enabled 0 1' when validating
that ftrace can't be disabled when it's being used by livepatch.  Not sure
if it makes sense to use skip() in the first case and die() in the latter
case, but being able to unexpectedly set kernel.ftrace_enabled seems more
problematic and within the control of the test than not being able to set
it. I could also just leave off the elif case as it's not needed for the
test suite as it exists today, but I think it makes the
set_ftrace_enabled() API more consistent and intuitive.

Thanks,
David

Re: [PATCH v2] livepatch: Skip livepatch tests if ftrace cannot be configured

[Petr Mladek]

On Wed 2022-02-09 07:49:34, David Vernet wrote:
> On Wed, Feb 09, 2022 at 02:02:33PM +0100, Petr Mladek wrote:
> > Hmm, test-ftrace.sh fails with this patch:
> > 
> > localhost:/prace/kernel/linux/tools/testing/selftests/livepatch # ./test-ftrace.sh 
> > TEST: livepatch interaction with ftrace_enabled sysctl ... SKIP: failed to set kernel.ftrace_enabled=0
> > 
> > This is the dmesg output:
> > 
> > [  436.176433] ===== TEST: livepatch interaction with ftrace_enabled sysctl =====
> > [  436.186942] livepatch: kernel.ftrace_enabled = 0
> > [  436.187854] % modprobe test_klp_livepatch
> > [  436.217657] livepatch: enabling patch 'test_klp_livepatch'
> > [  436.218467] livepatch: 'test_klp_livepatch': initializing patching transition
> > [  436.219679] livepatch: failed to register ftrace handler for function 'cmdline_proc_show' (-16)
> > [  436.221003] livepatch: failed to patch object 'vmlinux'
> > [  436.221786] livepatch: failed to enable patch 'test_klp_livepatch'
> > [  436.222716] livepatch: 'test_klp_livepatch': canceling patching transition, going to unpatch
> > [  436.223955] livepatch: 'test_klp_livepatch': completing unpatching transition
> > [  436.225476] livepatch: 'test_klp_livepatch': unpatching complete
> > [  436.273295] modprobe: ERROR: could not insert 'test_klp_livepatch': Device or resource busy
> > [  436.284522] livepatch: kernel.ftrace_enabled = 1
> > [  436.305353] % modprobe test_klp_livepatch
> > [  436.334929] livepatch: enabling patch 'test_klp_livepatch'
> > [  436.335781] livepatch: 'test_klp_livepatch': initializing patching transition
> > [  436.338099] livepatch: 'test_klp_livepatch': starting patching transition
> > [  438.082707] livepatch: 'test_klp_livepatch': completing patching transition
> > [  438.084106] livepatch: 'test_klp_livepatch': patching complete
> > [  438.199997] livepatch: sysctl: setting key "kernel.ftrace_enabled": Device or resource busy
> > [  438.201249] kernel.ftrace_enabled = 1
> > [  438.201929] SKIP: failed to set kernel.ftrace_enabled=0
> > 
> > 
> > The problem is the 2nd "set_ftrace_enabled 0" call in
> > tools/testing/selftests/livepatch/test-ftrace.sh
> > 
> > It is done when "test_klp_livepatch" is loaded. ftrace could not be
> > disabled because it is used by the livepatch. It is expected behavior
> > and the test should succeed in this case.
> 
> Ah, I missed this because in my configuration I was expecting the test to
> be skipped. What do we think is the preferred alternative?  Perhaps we
> could add an optional argument to set_trace_enabled() that specifies the
> expected value of kernel.ftrace_enabled after invoking sysctl?
> 
> What do you think of something like this:
> 
> +
> +	if [[ "$result" != "kernel.ftrace_enabled = $1" ]]; then
> +		if [[ -z "$2" || "$2" == "$1" ]]; then

Hmm, the check "$2" == "$1" is strange and pretty confusing.

It would be better to use "$result" == "$2" except that "$result"
includes also the variable name.

> +			skip "failed to set kernel.ftrace_enabled=$1"
> +		fi
> +	elif [[ -n "$2" && "$2" != "$1" ]]; then
> +		die "Unexpectedly set kernel.ftrace_enabled=$1"
> +	fi

This looks too paranoid to me. But I do not have strong opinion.

> test-ftrace.sh could then specify 'set_ftrace_enabled 0 1' when validating
> that ftrace can't be disabled when it's being used by livepatch.  Not sure
> if it makes sense to use skip() in the first case and die() in the latter
> case, but being able to unexpectedly set kernel.ftrace_enabled seems more
> problematic and within the control of the test than not being able to set
> it. I could also just leave off the elif case as it's not needed for the
> test suite as it exists today, but I think it makes the
> set_ftrace_enabled() API more consistent and intuitive.

OK, what about the following change:

   1. Store only the value (number) in $result
   2. Add optional --fail parameter

Hmm, the 1st step is not needed after several iterations here ;-)
Anyway, it should be easier to maintain it in the long term when the
sysctl output might change. We should probably do it in a separate patch.


diff --git a/tools/testing/selftests/livepatch/functions.sh b/tools/testing/selftests/livepatch/functions.sh
index 846c7ed71556..7b624d0fd7c0 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
+	can_fail=0
+	if [[ "$1" == "--fail" ]] ; then
+		can_fail=1
+		shift
+	fi
+
+	err=$(sysctl -q kernel.ftrace_enabled="$1" 2>&1)
+	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"



Best Regards,
Petr

Re: [PATCH v2] livepatch: Skip livepatch tests if ftrace cannot be configured

[David Vernet]

On Thu, Feb 10, 2022 at 04:23:15PM +0100, Petr Mladek wrote:
> OK, what about the following change:
> 
>    1. Store only the value (number) in $result
>    2. Add optional --fail parameter

I'm fine with this approach. I agree that it's probably less confusing than
"$2" == "$1". I don't think the elif check I proposed is strictly necessary
either, and in any case the way you've rewired the patch address that.

> 
> Hmm, the 1st step is not needed after several iterations here ;-)
> Anyway, it should be easier to maintain it in the long term when the
> sysctl output might change. We should probably do it in a separate patch.
>
>
> diff --git a/tools/testing/selftests/livepatch/functions.sh b/tools/testing/selftests/livepatch/functions.sh
> index 846c7ed71556..7b624d0fd7c0 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
> +	can_fail=0

Can you make this variable a local?

> +	if [[ "$1" == "--fail" ]] ; then
> +		can_fail=1
> +		shift
> +	fi
> +
> +	err=$(sysctl -q kernel.ftrace_enabled="$1" 2>&1)
> +	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"

Patch looks great otherwise and works fine after testing on my end. Do you
want me to send it out as a v3? Or would you prefer to just apply it as is?
Assuming it's the latter, then:

Reviewed-by: David Vernet <void@manifault.com>

Thanks,
David

Re: [PATCH v2] livepatch: Skip livepatch tests if ftrace cannot be configured

[Joe Lawrence]

Sorry for not catching this in v2. I think Petr's approach is pretty
reasonable, a few comments below.

On 2/10/22 11:22 AM, David Vernet wrote:
> On Thu, Feb 10, 2022 at 04:23:15PM +0100, Petr Mladek wrote:
>> OK, what about the following change:
>>
>>    1. Store only the value (number) in $result
>>    2. Add optional --fail parameter
> 
> I'm fine with this approach. I agree that it's probably less confusing than
> "$2" == "$1". I don't think the elif check I proposed is strictly necessary
> either, and in any case the way you've rewired the patch address that.
> 
>>
>> Hmm, the 1st step is not needed after several iterations here ;-)
>> Anyway, it should be easier to maintain it in the long term when the
>> sysctl output might change. We should probably do it in a separate patch.
>>
>>
>> diff --git a/tools/testing/selftests/livepatch/functions.sh b/tools/testing/selftests/livepatch/functions.sh
>> index 846c7ed71556..7b624d0fd7c0 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
>> +	can_fail=0

nit: I don't think the script is entirely consistent w/local variables, but:

local can_fail=0

> 
> Can you make this variable a local?
> 
>> +	if [[ "$1" == "--fail" ]] ; then
>> +		can_fail=1
>> +		shift
>> +	fi
>> +
>> +	err=$(sysctl -q kernel.ftrace_enabled="$1" 2>&1)
>> +	result=$(sysctl --values kernel.ftrace_enabled)

nit: likewise, local err and result variables.

>> +
>> +	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"

Unexpected failure: If the caller doesn't want this to fail but it does,
would it be helpful to report the original $err, too?  IIUC, the
original code captured that in $result.

>> +	fi
>> +

Unexpected success: Inversely, if the caller expects failure, but it
actually succeeds, I think this version only reports the new value, right?

In the case of the 2nd "set_ftrace_enabled 0" in test-ftrace.sh, that
turns out to be OK since it follows up with verifying that livepatch
redirection is still in effect.

Maybe this is too paranoid?  Just thought I'd mentioned it.

Thanks,

--
Joe

>> +	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"
> 
> Patch looks great otherwise and works fine after testing on my end. Do you
> want me to send it out as a v3? Or would you prefer to just apply it as is?
> Assuming it's the latter, then:
> 
> Reviewed-by: David Vernet <void@manifault.com>
>