Re: [PATCH v10 2/9] selftests/livepatch: Adapt atomic replace tests to provides/obsoletes

[Petr Mladek <pmladek@suse.com>]

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 <laoar.shao@gmail.com>
> 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:

> --- 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.

> +	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.

> --- 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.

>  
>  # - load a target module that provides /proc/test_klp_mod_target with
> diff --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.

> +/*
> + * 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