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 + load_lp $mod +done + +mods=(/sys/kernel/livepatch/*) +nmods=${#mods[@]} +if [ "$nmods" -ne 3 ]; then + die "Expecting three modules listed, found $nmods" +fi + 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 +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/$MOD_REPLACE.ko replace=1 livepatch: enabling patch '$MOD_REPLACE' livepatch: '$MOD_REPLACE': initializing patching transition @@ -149,6 +185,8 @@ livepatch: '$MOD_REPLACE': starting patching transition livepatch: '$MOD_REPLACE': completing patching transition livepatch: '$MOD_REPLACE': patching complete $MOD_REPLACE: this has been live patched +% rmmod test_klp_callbacks_demo +% rmmod test_klp_syscall % rmmod $MOD_LIVEPATCH $MOD_REPLACE: this has been live patched % echo 0 > /sys/kernel/livepatch/$MOD_REPLACE/enabled
--- base-commit: 6d69b6c12fce479fde7bc06f686212451688a102 change-id: 20240525-lp-atomic-replace-90b33ed018dc
Best regards,
On Sat 2024-05-25 11:34:08, 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
I am not completely sure if it is a good idea to test so many aspects and use so many different test modules in a single test. It might be harder to maintain and analyze eventual problems.
But the change will help to catch more problems which is good. I am fine with it:
Reviewed-by: Petr Mladek pmladek@suse.com
Best Regards, Petr
On Sat, 25 May 2024, Marcos Paulo de Souza wrote:
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
s/addtional/additional/
Reviewed-by: Miroslav Benes mbenes@suse.cz
M
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?
- 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.
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.
Acked-by: Joe Lawrence joe.lawrence@redhat.com
-- Joe
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.
- 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.
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.
Acked-by: Joe Lawrence joe.lawrence@redhat.com
-- Joe
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.
Best Regards, Petr
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
linux-kselftest-mirror@lists.linaro.org
-
Joe Lawrence -
Marcos Paulo de Souza -
Miroslav Benes -
Petr Mladek