* [PATCH] livepatch: Skip livepatch tests if ftrace cannot be configured @ 2022-02-03 23:32 David Vernet 2022-02-04 20:30 ` Joe Lawrence 2022-02-04 20:56 ` [PATCH v2] " David Vernet 0 siblings, 2 replies; 10+ messages in thread From: David Vernet @ 2022-02-03 23:32 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 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] livepatch: Skip livepatch tests if ftrace cannot be configured 2022-02-03 23:32 [PATCH] livepatch: Skip livepatch tests if ftrace cannot be configured David Vernet @ 2022-02-04 20:30 ` Joe Lawrence 2022-02-04 20:43 ` David Vernet 2022-02-04 20:56 ` [PATCH v2] " David Vernet 1 sibling, 1 reply; 10+ messages in thread From: Joe Lawrence @ 2022-02-04 20:30 UTC (permalink / raw) To: David Vernet, live-patching, linux-kernel, jpoimboe, pmladek, jikos, mbenes, corbet Cc: kernel-team 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] livepatch: Skip livepatch tests if ftrace cannot be configured 2022-02-04 20:30 ` Joe Lawrence @ 2022-02-04 20:43 ` David Vernet 0 siblings, 0 replies; 10+ messages in thread From: David Vernet @ 2022-02-04 20:43 UTC (permalink / raw) To: Joe Lawrence Cc: live-patching, linux-kernel, jpoimboe, pmladek, jikos, mbenes, corbet, kernel-team 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2] livepatch: Skip livepatch tests if ftrace cannot be configured 2022-02-03 23:32 [PATCH] livepatch: Skip livepatch tests if ftrace cannot be configured David Vernet 2022-02-04 20:30 ` Joe Lawrence @ 2022-02-04 20:56 ` David Vernet 2022-02-08 15:45 ` Joe Lawrence 2022-02-09 13:02 ` Petr Mladek 1 sibling, 2 replies; 10+ messages in thread From: David Vernet @ 2022-02-04 20:56 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 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2] livepatch: Skip livepatch tests if ftrace cannot be configured 2022-02-04 20:56 ` [PATCH v2] " David Vernet @ 2022-02-08 15:45 ` Joe Lawrence 2022-02-09 13:02 ` Petr Mladek 1 sibling, 0 replies; 10+ messages in thread From: Joe Lawrence @ 2022-02-08 15:45 UTC (permalink / raw) To: David Vernet, live-patching, linux-kernel, jpoimboe, pmladek, jikos, mbenes, corbet Cc: kernel-team 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] livepatch: Skip livepatch tests if ftrace cannot be configured 2022-02-04 20:56 ` [PATCH v2] " David Vernet 2022-02-08 15:45 ` Joe Lawrence @ 2022-02-09 13:02 ` Petr Mladek 2022-02-09 15:49 ` David Vernet 1 sibling, 1 reply; 10+ messages in thread From: Petr Mladek @ 2022-02-09 13:02 UTC (permalink / raw) To: David Vernet Cc: live-patching, linux-kernel, jpoimboe, jikos, mbenes, joe.lawrence, corbet, kernel-team 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] livepatch: Skip livepatch tests if ftrace cannot be configured 2022-02-09 13:02 ` Petr Mladek @ 2022-02-09 15:49 ` David Vernet 2022-02-10 15:23 ` Petr Mladek 0 siblings, 1 reply; 10+ messages in thread From: David Vernet @ 2022-02-09 15:49 UTC (permalink / raw) To: Petr Mladek Cc: live-patching, linux-kernel, jpoimboe, jikos, mbenes, joe.lawrence, corbet, kernel-team 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] livepatch: Skip livepatch tests if ftrace cannot be configured 2022-02-09 15:49 ` David Vernet @ 2022-02-10 15:23 ` Petr Mladek 2022-02-10 16:22 ` David Vernet 0 siblings, 1 reply; 10+ messages in thread From: Petr Mladek @ 2022-02-10 15:23 UTC (permalink / raw) To: David Vernet Cc: live-patching, linux-kernel, jpoimboe, jikos, mbenes, joe.lawrence, corbet, kernel-team 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2] livepatch: Skip livepatch tests if ftrace cannot be configured 2022-02-10 15:23 ` Petr Mladek @ 2022-02-10 16:22 ` David Vernet 2022-02-10 20:58 ` Joe Lawrence 0 siblings, 1 reply; 10+ messages in thread From: David Vernet @ 2022-02-10 16:22 UTC (permalink / raw) To: Petr Mladek Cc: live-patching, linux-kernel, jpoimboe, jikos, mbenes, joe.lawrence, corbet, kernel-team 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] livepatch: Skip livepatch tests if ftrace cannot be configured 2022-02-10 16:22 ` David Vernet @ 2022-02-10 20:58 ` Joe Lawrence 0 siblings, 0 replies; 10+ messages in thread From: Joe Lawrence @ 2022-02-10 20:58 UTC (permalink / raw) To: David Vernet, Petr Mladek Cc: live-patching, linux-kernel, jpoimboe, jikos, mbenes, corbet, kernel-team 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> > ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2022-02-10 20:58 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-02-03 23:32 [PATCH] livepatch: Skip livepatch tests if ftrace cannot be configured David Vernet 2022-02-04 20:30 ` Joe Lawrence 2022-02-04 20:43 ` David Vernet 2022-02-04 20:56 ` [PATCH v2] " David Vernet 2022-02-08 15:45 ` Joe Lawrence 2022-02-09 13:02 ` Petr Mladek 2022-02-09 15:49 ` David Vernet 2022-02-10 15:23 ` Petr Mladek 2022-02-10 16:22 ` David Vernet 2022-02-10 20:58 ` Joe Lawrence
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).