Received-SPF: pass (google.com: domain of linux-kernel+bounces-112014-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <9d4c5c6bd5b7fd0305f9ec26038f4afbea5fc166.camel@suse.com>
Subject: Re: [PATCH] selftests: livepatch: Test atomic replace against
 multiple modules
From: Marcos Paulo de Souza <mpdesouza@suse.com>
To: Joe Lawrence <joe.lawrence@redhat.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
Date: Fri, 22 Mar 2024 17:31:35 -0300
In-Reply-To: <56bf6323-9e9b-a0e3-f505-d628aac793d4@redhat.com>
References: <20240312-lp-selftest-new-test-v1-1-9c843e25e38e@suse.com>
	 <56bf6323-9e9b-a0e3-f505-d628aac793d4@redhat.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.50.4 (by Flathub.org) 
Precedence: bulk
MIME-Version: 1.0

On Thu, 2024-03-21 at 10:08 -0400, Joe Lawrence wrote:
> 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.
> >=20
> > Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
> > ---
> > =C2=A0tools/testing/selftests/livepatch/Makefile=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0 |=C2=A0 3 +-
> > =C2=A0.../selftests/livepatch/test-atomic-replace.sh=C2=A0=C2=A0=C2=A0=
=C2=A0 | 71
> > ++++++++++++++++++++++
> > =C2=A02 files changed, 73 insertions(+), 1 deletion(-)
> >=20
> > 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 :=3D \
> > =C2=A0	test-state.sh \
> > =C2=A0	test-ftrace.sh \
> > =C2=A0	test-sysfs.sh \
> > -	test-syscall.sh
> > +	test-syscall.sh \
> > +	test-atomic-replace.sh
> > =C2=A0
> > =C2=A0TEST_FILES :=3D settings
> > =C2=A0
> > 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=3Dtest_klp_atomic_replace
> > +
> > +setup_config
> > +
> > +# - Load three livepatch modules.
> > +# - Load one more livepatch with replace being set, and check that
> > only one
> > +#=C2=A0=C2=A0 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=3D$(ls /sys/kernel/livepatch | wc -l)
> > +if [ $nmods -ne 3 ]; then
> > +	die "Expecting three modules listed, found $nmods"
> > +fi
> > +
> > +load_lp $MOD_REPLACE replace=3D1
> > +
> > +nmods=3D$(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=3D1
> > +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
> >=20
>=20
> Hi Marcos,
>=20
> 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?
>=20
> If this one provides better coverage, should we follow up with
> removing
> the existing one?

Hi Joe,

thanks for looking at it. To be honest I haven't checked the current
use of atomic replace on test-livepatch.sh =3D/

yes, that's mostly the same case, but in mine I load three modules and
then load the third one replacing the others, while in the test-
livepatch.sh we have only one module that is loaded, replaced, and then
we unload the replaced one.

Do you see value in extending the test at test-livepatch.sh to load
more than one LP moduled and the replace all of them with another one?
I believe that it adds more coverage, while keeping the number of tests
the same.

Thanks!

>=20