Re: [PATCH] selftests: livepatch: Test atomic replace against multiple modules

From: Joe Lawrence <joe.lawrence@redhat.com>
To: Marcos Paulo de Souza <mpdesouza@suse.com>,
	Josh Poimboeuf <jpoimboe@kernel.org>,
	Jiri Kosina <jikos@kernel.org>, Miroslav Benes <mbenes@suse.cz>,
	Petr Mladek <pmladek@suse.com>, Shuah Khan <shuah@kernel.org>
Cc: linux-kernel@vger.kernel.org, live-patching@vger.kernel.org,
	linux-kselftest@vger.kernel.org
Subject: Re: [PATCH] selftests: livepatch: Test atomic replace against multiple modules
Date: Thu, 21 Mar 2024 10:08:05 -0400	[thread overview]
Message-ID: <56bf6323-9e9b-a0e3-f505-d628aac793d4@redhat.com> (raw)
In-Reply-To: <20240312-lp-selftest-new-test-v1-1-9c843e25e38e@suse.com>

On 3/12/24 08:12, Marcos Paulo de Souza wrote:
> This new test checks if a livepatch with replace attribute set replaces
> all previously applied livepatches.
> 
> Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
> ---
>  tools/testing/selftests/livepatch/Makefile         |  3 +-
>  .../selftests/livepatch/test-atomic-replace.sh     | 71 ++++++++++++++++++++++
>  2 files changed, 73 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/livepatch/Makefile b/tools/testing/selftests/livepatch/Makefile
> index 35418a4790be..e92f61208d35 100644
> --- a/tools/testing/selftests/livepatch/Makefile
> +++ b/tools/testing/selftests/livepatch/Makefile
> @@ -10,7 +10,8 @@ TEST_PROGS := \
>  	test-state.sh \
>  	test-ftrace.sh \
>  	test-sysfs.sh \
> -	test-syscall.sh
> +	test-syscall.sh \
> +	test-atomic-replace.sh
>  
>  TEST_FILES := settings
>  
> diff --git a/tools/testing/selftests/livepatch/test-atomic-replace.sh b/tools/testing/selftests/livepatch/test-atomic-replace.sh
> new file mode 100755
> index 000000000000..09a3dcdcb8de
> --- /dev/null
> +++ b/tools/testing/selftests/livepatch/test-atomic-replace.sh
> @@ -0,0 +1,71 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +#
> +# Copyright (C) 2024 SUSE
> +# Author: Marcos Paulo de Souza <mpdesouza@suse.com>
> +
> +. $(dirname $0)/functions.sh
> +
> +MOD_REPLACE=test_klp_atomic_replace
> +
> +setup_config
> +
> +# - Load three livepatch modules.
> +# - Load one more livepatch with replace being set, and check that only one
> +#   livepatch module is being listed.
> +
> +start_test "apply one liveptach to replace multiple livepatches"
> +
> +for mod in test_klp_livepatch test_klp_syscall test_klp_callbacks_demo; do
> +	load_lp $mod
> +done
> +
> +nmods=$(ls /sys/kernel/livepatch | wc -l)
> +if [ $nmods -ne 3 ]; then
> +	die "Expecting three modules listed, found $nmods"
> +fi
> +
> +load_lp $MOD_REPLACE replace=1
> +
> +nmods=$(ls /sys/kernel/livepatch | wc -l)
> +if [ $nmods -ne 1 ]; then
> +	die "Expecting only one moduled listed, found $nmods"
> +fi
> +
> +disable_lp $MOD_REPLACE
> +unload_lp $MOD_REPLACE
> +
> +check_result "% insmod test_modules/test_klp_livepatch.ko
> +livepatch: enabling patch 'test_klp_livepatch'
> +livepatch: 'test_klp_livepatch': initializing patching transition
> +livepatch: 'test_klp_livepatch': starting patching transition
> +livepatch: 'test_klp_livepatch': completing patching transition
> +livepatch: 'test_klp_livepatch': patching complete
> +% insmod test_modules/test_klp_syscall.ko
> +livepatch: enabling patch 'test_klp_syscall'
> +livepatch: 'test_klp_syscall': initializing patching transition
> +livepatch: 'test_klp_syscall': starting patching transition
> +livepatch: 'test_klp_syscall': completing patching transition
> +livepatch: 'test_klp_syscall': patching complete
> +% insmod test_modules/test_klp_callbacks_demo.ko
> +livepatch: enabling patch 'test_klp_callbacks_demo'
> +livepatch: 'test_klp_callbacks_demo': initializing patching transition
> +test_klp_callbacks_demo: pre_patch_callback: vmlinux
> +livepatch: 'test_klp_callbacks_demo': starting patching transition
> +livepatch: 'test_klp_callbacks_demo': completing patching transition
> +test_klp_callbacks_demo: post_patch_callback: vmlinux
> +livepatch: 'test_klp_callbacks_demo': patching complete
> +% insmod test_modules/test_klp_atomic_replace.ko replace=1
> +livepatch: enabling patch 'test_klp_atomic_replace'
> +livepatch: 'test_klp_atomic_replace': initializing patching transition
> +livepatch: 'test_klp_atomic_replace': starting patching transition
> +livepatch: 'test_klp_atomic_replace': completing patching transition
> +livepatch: 'test_klp_atomic_replace': patching complete
> +% echo 0 > /sys/kernel/livepatch/test_klp_atomic_replace/enabled
> +livepatch: 'test_klp_atomic_replace': initializing unpatching transition
> +livepatch: 'test_klp_atomic_replace': starting unpatching transition
> +livepatch: 'test_klp_atomic_replace': completing unpatching transition
> +livepatch: 'test_klp_atomic_replace': unpatching complete
> +% rmmod test_klp_atomic_replace"
> +
> +exit 0
> 

Hi Marcos,

I'm not against adding a specific atomic replace test, but for a quick
tl/dr what is the difference between this new test and
test-livepatch.sh's "atomic replace livepatch" test?

If this one provides better coverage, should we follow up with removing
the existing one?

-- 
Joe