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

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:

quoted

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.

quoted

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?

quoted

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

quoted

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

quoted

+	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

quoted

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