live-patching.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Petr Mladek <pmladek@suse.com>
To: David Vernet <void@manifault.com>
Cc: live-patching@vger.kernel.org, linux-kernel@vger.kernel.org,
	jpoimboe@redhat.com, jikos@kernel.org, mbenes@suse.cz,
	joe.lawrence@redhat.com, corbet@lwn.net, kernel-team@fb.com
Subject: Re: [PATCH v2] livepatch: Skip livepatch tests if ftrace cannot be configured
Date: Wed, 9 Feb 2022 14:02:33 +0100	[thread overview]
Message-ID: <20220209130233.GB8279@pathway.suse.cz> (raw)
In-Reply-To: <20220204205625.2628328-1-void@manifault.com>

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

  parent reply	other threads:[~2022-02-09 13:02 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
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 [this message]
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

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20220209130233.GB8279@pathway.suse.cz \
    --to=pmladek@suse.com \
    --cc=corbet@lwn.net \
    --cc=jikos@kernel.org \
    --cc=joe.lawrence@redhat.com \
    --cc=jpoimboe@redhat.com \
    --cc=kernel-team@fb.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=live-patching@vger.kernel.org \
    --cc=mbenes@suse.cz \
    --cc=void@manifault.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).