Date:   Thu, 10 Feb 2022 16:23:15 +0100
From:   Petr Mladek <pmladek@...e.com>
To:     David Vernet <void@...ifault.com>
Cc:     live-patching@...r.kernel.org, linux-kernel@...r.kernel.org,
        jpoimboe@...hat.com, jikos@...nel.org, mbenes@...e.cz,
        joe.lawrence@...hat.com, corbet@....net, kernel-team@...com
Subject: Re: [PATCH v2] livepatch: Skip livepatch tests if ftrace cannot be
 configured

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

Powered by blists - more mailing lists