Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 9 Feb 2022 07:49:34 -0800
From:   David Vernet <void@manifault.com>
To:     Petr Mladek <pmladek@suse.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
Message-ID: <20220209154934.5w5k5tqcqldbfjt3@dev0025.ash9.facebook.com>
References: <20220203233205.1554034-1-void@manifault.com>
 <20220204205625.2628328-1-void@manifault.com>
 <20220209130233.GB8279@pathway.suse.cz>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220209130233.GB8279@pathway.suse.cz>
User-Agent: NeoMutt/20211029
Precedence: bulk

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