On Mon, 2024-06-03 at 14:52 +0200, Petr Mladek wrote:
On Fri 2024-05-31 18:06:48, Marcos Paulo de Souza wrote:
On Fri, 2024-05-31 at 15:44 -0400, Joe Lawrence wrote:
On Sat, May 25, 2024 at 11:34:08AM -0300, Marcos Paulo de Souza wrote:
Adapt the current test-livepatch.sh script to account the number of applied livepatches and ensure that an atomic replace livepatch disables all previously applied livepatches.
Signed-off-by: Marcos Paulo de Souza mpdesouza@suse.com
Changes since v1:
- Added checks in the existing test-livepatch.sh instead of
creating a new test file. (Joe)
- Fixed issues reported by ShellCheck (Joe)
.../testing/selftests/livepatch/test-livepatch.sh | 46 ++++++++++++++++++++-- 1 file changed, 42 insertions(+), 4 deletions(-)
diff --git a/tools/testing/selftests/livepatch/test- livepatch.sh b/tools/testing/selftests/livepatch/test-livepatch.sh index e3455a6b1158..d85405d18e54 100755 --- a/tools/testing/selftests/livepatch/test-livepatch.sh +++ b/tools/testing/selftests/livepatch/test-livepatch.sh @@ -107,9 +107,12 @@ livepatch: '$MOD_LIVEPATCH': unpatching complete # - load a livepatch that modifies the output from /proc/cmdline and # verify correct behavior -# - load an atomic replace livepatch and verify that only the second is active -# - remove the first livepatch and verify that the atomic replace livepatch -# is still active +# - load two addtional livepatches and check the number of livepatch modules +# applied +# - load an atomic replace livepatch and check that the other three modules were +# disabled +# - remove all livepatches besides the atomic replace one and verify that the +# atomic replace livepatch is still active # - remove the atomic replace livepatch and verify that none are active start_test "atomic replace livepatch" @@ -119,12 +122,31 @@ load_lp $MOD_LIVEPATCH grep 'live patched' /proc/cmdline > /dev/kmsg grep 'live patched' /proc/meminfo > /dev/kmsg +for mod in test_klp_syscall test_klp_callbacks_demo; do
Slightly nitpicky here, but the tests were originally written with the livepatch module names via variables like $MOD_LIVEPATCH. Would using $MOD_LIVEPATCH{1,2,3} help indicate that their specifics aren't really interesting, that we just need 3 of them?
Makes sense. I thought about it when I was changing the code, but I didn't want to change it too much, so it was the result. But that makes sense to have the modules better named.
I like this.
- load_lp $mod
+done
+mods=(/sys/kernel/livepatch/*) +nmods=${#mods[@]} +if [ "$nmods" -ne 3 ]; then
- die "Expecting three modules listed, found $nmods"
+fi
I was going to suggest that we might protect against a situation where other livepatch modules were active, that a simple count wouldn't be sufficient. But then I thought about this test, atomic replace! Anything previously loaded is going to be pushed aside anyway.
So maybe (in another patch or set) it would be worth enhancing functions.sh :: start_test() do a quick sanity check to see that the initial conditions are safe? That might also prevent some collateral damage when test A fails and leaves the world a strange place for tests B, C, etc.
We have been discussing about start/end functions that would check for leftover modules... maybe should be a good think to implement soon as we land more tests.
Makes sense :-)
load_lp $MOD_REPLACE replace=1 grep 'live patched' /proc/cmdline > /dev/kmsg grep 'live patched' /proc/meminfo > /dev/kmsg -unload_lp $MOD_LIVEPATCH +mods=(/sys/kernel/livepatch/*) +nmods=${#mods[@]} +if [ "$nmods" -ne 1 ]; then
- die "Expecting only one moduled listed, found $nmods"
+fi
+# These modules were disabled by the atomic replace +for mod in test_klp_callbacks_demo test_klp_syscall $MOD_LIVEPATCH; do
- unload_lp "$mod"
+done grep 'live patched' /proc/cmdline > /dev/kmsg grep 'live patched' /proc/meminfo > /dev/kmsg @@ -142,6 +164,20 @@ livepatch: '$MOD_LIVEPATCH': starting patching transition livepatch: '$MOD_LIVEPATCH': completing patching transition livepatch: '$MOD_LIVEPATCH': patching complete $MOD_LIVEPATCH: this has been live patched +% insmod test_modules/test_klp_syscall.ko
Similar minor nit here, too. If we think copy/pasting all the $MOD_FOO is annoying, I am fine with leaving this as is. I don't have a strong opinion other than following some convention.
With that, I'm happy to ack as-is or with variable names.
Thanks Joe! I think that is Petr's call, either way I can rework this patch, or send additional ones to adjust the tests.
I would prefer if you did respin this patch. The use of $MOD_LIVEPATCH{1,2,3} would make even the patch easier to follow.
Done in v3. About the pre-check, I discussed with Miroslav about having an easier way to skip tests. The idea was to split each "test" into a different file, like fstests already does. Using this approach, each start_test function will be placed in a different file to test specifically one functionality. This way we can skip a test if we don't have some requirements (like a sysfs attribute for example, or the there were leftover modules).
I plan to send a patch starting this move when the v3 of this patchset is accepted.
Best Regards, Petr