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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20220209154934.5w5k5tqcqldbfjt3@dev0025.ash9.facebook.com>
Date:   Wed, 9 Feb 2022 07:49:34 -0800
From:   David Vernet <void@...ifault.com>
To:     Petr Mladek <pmladek@...e.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, 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