Re: [PATCH v10 2/9] selftests/livepatch: Adapt atomic replace tests to provides/obsoletes
flat view
From: Petr Mladek <pmladek@suse.com>
Date: 2026-10-01 15:57:08
On Wed 2026-09-30 11:03:16, Yafang Shao wrote:
The legacy "replace" field in struct klp_patch will be replaced by the provides/obsoletes mechanism. As a result, the atomic replace selftests fail to build against kernels that only support provides/obsoletes. Adapt the selftests so that they build and run on both old and new kernels. On kernels without the legacy "replace" support, the replace-related test cases are skipped with a SKIP message instead of being run. Also introduce the CONFIG_KLP_HAS_PROVIDES compile-time marker in kernel/livepatch/Kconfig. It defaults to n and will be set to y once the provides/obsoletes support is implemented later in this series. The provides/obsoletes-based selftests will be added later in this series, after the legacy "replace" field has been substituted by the new mechanism. Suggested-by: Petr Mladek <pmladek@suse.com> Signed-off-by: Yafang Shao <redacted> Acked-by: Song Liu <song@kernel.org>
Looks good and passes selftests. Feel free to use: Reviewed-by: Petr Mladek <pmladek@suse.com> Tested-by: Petr Mladek <pmladek@suse.com> Thouhg, see few nits below:
quoted hunk ↗ jump to hunk
--- a/tools/testing/selftests/livepatch/test-callbacks.sh +++ b/tools/testing/selftests/livepatch/test-callbacks.sh@@ -458,16 +459,17 @@ $MOD_TARGET_BUSY: ${MOD_TARGET_BUSY}_exit" # execute as each patch progresses through its (un)patching # transition. -start_test "multiple livepatches" +function test_multiple_livepatches() {
I really like the approach with bash functions. It separates the test code from the skip-check.
quoted hunk ↗ jump to hunk
+ start_test "multiple livepatches" -load_lp $MOD_LIVEPATCH -load_lp $MOD_LIVEPATCH2 -disable_lp $MOD_LIVEPATCH2 -disable_lp $MOD_LIVEPATCH -unload_lp $MOD_LIVEPATCH2 -unload_lp $MOD_LIVEPATCH + load_lp $MOD_LIVEPATCH + load_lp $MOD_LIVEPATCH2 + disable_lp $MOD_LIVEPATCH2 + disable_lp $MOD_LIVEPATCH + unload_lp $MOD_LIVEPATCH2 + unload_lp $MOD_LIVEPATCH -check_result "% insmod test_modules/$MOD_LIVEPATCH.ko + check_result "% insmod test_modules/$MOD_LIVEPATCH.ko livepatch: enabling patch '$MOD_LIVEPATCH' livepatch: '$MOD_LIVEPATCH': initializing patching transition $MOD_LIVEPATCH: pre_patch_callback: vmlinux@@ -548,6 +552,15 @@ $MOD_LIVEPATCH2: post_unpatch_callback: vmlinux livepatch: '$MOD_LIVEPATCH2': unpatching complete % rmmod $MOD_LIVEPATCH2 % rmmod $MOD_LIVEPATCH" +} + +if [[ "$HAS_PROVIDES_ATTR" != "1" ]]; then + test_multiple_livepatches + test_atomic_replace +else + skip "multiple livepatches" "legacy replace attribute not present" + skip "atomic replace" "legacy replace attribute not present" +fi
I would revert the logic and ordering: + Positive checks are easier to read ;-) + It would make sense to have the obsolete checks in the "else" part. + It would be the same ordering as in the .c files.
quoted hunk ↗ jump to hunk
--- a/tools/testing/selftests/livepatch/test-kprobe.sh +++ b/tools/testing/selftests/livepatch/test-kprobe.sh@@ -196,6 +200,16 @@ livepatch: '$MOD_REPLACE': starting unpatching transition livepatch: '$MOD_REPLACE': completing unpatching transition livepatch: '$MOD_REPLACE': unpatching complete % rmmod $MOD_REPLACE" +} + + +if [[ "$HAS_PROVIDES_ATTR" != "1" ]]; then + test_multiple_livepatches + test_atomic_replace_livepatch +else + skip "multiple livepatches" "legacy replace attribute not present" + skip "atomic replace livepatch" "legacy replace attribute not present" +fi
Same here.
quoted hunk ↗ jump to hunk
# - load a target module that provides /proc/test_klp_mod_target withdiff --git a/tools/testing/selftests/livepatch/test_modules/test_klp_callbacks_demo2.c b/tools/testing/selftests/livepatch/test_modules/test_klp_callbacks_demo2.c index 5417573e80af..6c46ce575f5c 100644 --- a/tools/testing/selftests/livepatch/test_modules/test_klp_callbacks_demo2.c +++ b/tools/testing/selftests/livepatch/test_modules/test_klp_callbacks_demo2.c@@ -7,9 +7,16 @@ #include <linux/kernel.h> #include <linux/livepatch.h> +#ifdef CONFIG_KLP_HAS_PROVIDES
JFYI, this is the related check in the .c code.
quoted hunk ↗ jump to hunk
+/* + * TODO: Add provides/obsoletes module parameters for the + * provides/obsoletes based tests (to be added later). + */ +#else static int replace; module_param(replace, int, 0644); MODULE_PARM_DESC(replace, "replace (default=0)"); +#endif static const char *const module_state[] = { [MODULE_STATE_LIVE] = "[MODULE_STATE_LIVE] Normal state",
Best Regardss, Petr