Re: [PATCH v5 9/9] selftests: livepatch: Add test for function conflict across provides

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Malformed livepatch module missing `old_name` causes a kernel panic on load.

Pre-existing issues:
- [Medium] Race condition in sysfs polling causes flaky selftests.
--

--- Patch [9]: [PATCH v5 9/9] selftests: livepatch: Add test for function conflict across provides ---
commit eaca609db12a6842c4e94debcf9d9ef5049826db
Author: Yafang Shao <[email protected]>

selftests: livepatch: Add test for function conflict across provides
    
This commit adds a new test module test_klp_provides.c and three new test
scenarios to test-provides-obsoletes.sh. These tests verify the behavior
of livepatch function conflicts and replacement when using different
provides IDs with and without obsoletes.

> diff --git a/tools/testing/selftests/livepatch/test-provides-obsoletes.sh b/tools/testing/selftests/livepatch/test-provides-obsoletes.sh
> index 1b2c73bdd50b0..32885625401d1 100755
> --- a/tools/testing/selftests/livepatch/test-provides-obsoletes.sh
> +++ b/tools/testing/selftests/livepatch/test-provides-obsoletes.sh
[ ... ]
> @@ -278,4 +279,123 @@ $MOD_STATE2: free_loglevel_state: freeing space for the stored console_loglevel
>  livepatch: '$MOD_STATE2': unpatching complete
>  % rmmod $MOD_STATE2"
>  
> +
> +# - load a livepatch with provides=1 that modifies meminfo_proc_show
> +# - try to load another livepatch with provides=2 that modifies the
> +#   same function. The second one must be rejected because livepatches
> +#   with different provides ids and no obsoletes must not modify
> +#   the same function.
> +# - disable and unload the remaining livepatch
> +
> +start_test "function conflict across provides"
> +
> +load_lp $MOD_ATOMIC provides=1
> +load_failing_mod $MOD_PROVIDES provides=2
> +
> +disable_lp $MOD_ATOMIC
> +unload_lp $MOD_ATOMIC
> +
> +check_result "% insmod test_modules/$MOD_ATOMIC.ko provides=1
> +livepatch: enabling patch '$MOD_ATOMIC'
> +livepatch: '$MOD_ATOMIC': initializing patching transition
> +livepatch: '$MOD_ATOMIC': starting patching transition
> +livepatch: '$MOD_ATOMIC': completing patching transition
> +livepatch: '$MOD_ATOMIC': patching complete
> +% insmod test_modules/$MOD_PROVIDES.ko provides=2
> +livepatch: Livepatch patch ($MOD_PROVIDES) is not compatible with the already installed livepatches.
> +insmod: ERROR: could not insert module test_modules/$MOD_PROVIDES.ko: Invalid parameters
> +% echo 0 > $SYSFS_KLP_DIR/$MOD_ATOMIC/enabled
> +livepatch: '$MOD_ATOMIC': initializing unpatching transition
> +livepatch: '$MOD_ATOMIC': starting unpatching transition
> +livepatch: '$MOD_ATOMIC': completing unpatching transition
> +livepatch: '$MOD_ATOMIC': unpatching complete
> +% rmmod $MOD_ATOMIC"
> +
> +
> +# - load a livepatch with provides=1 that modifies meminfo_proc_show
> +# - load another livepatch with provides=1 that modifies the same
> +#   function. The second one loads successfully because livepatches
> +#   with the same provides id replace each other.
> +# - disable and unload the remaining livepatch
> +
> +start_test "function replace within same provides"
> +
> +load_lp $MOD_ATOMIC provides=1
> +load_lp $MOD_PROVIDES provides=1
> +
> +mods=($SYSFS_KLP_DIR/*)
> +nmods=${#mods[@]}
> +if [[ "$nmods" -ne 1 ]]; then

[Severity: Medium]
This is a pre-existing issue, but does this pattern cause a regression in the
form of flaky selftests?

The script synchronously counts the directories in the sysfs path immediately
after a transition. However, the old patch's sysfs directory removal is
deferred to an asynchronous workqueue:

kernel/livepatch/core.c:klp_free_replaced_patches_async()

Because of this, the script may momentarily see 2 directories instead of 1
before the asynchronous cleanup completes.

> +	die "Expecting one module listed, found $nmods"
> +fi

[ ... ]

> diff --git a/tools/testing/selftests/livepatch/test_modules/test_klp_provides.c b/tools/testing/selftests/livepatch/test_modules/test_klp_provides.c
> new file mode 100644
> index 0000000000000..9751a6f6c8518
> --- /dev/null
> +++ b/tools/testing/selftests/livepatch/test_modules/test_klp_provides.c
> @@ -0,0 +1,72 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
[ ... ]
> +static int test_klp_provides_init(void)
> +{
> +#ifdef KLP_HAS_REPLACE
> +	patch.replace = replace;
> +#else
> +	patch.provides = provides;
> +	if (nr_obsoletes > 0) {
> +		patch.obsoletes = obsoletes;
> +		patch.nr_obsoletes = nr_obsoletes;
> +	}
> +#endif
> +	return klp_enable_patch(&patch);

[Severity: High]
Does klp_enable_patch() introduce a regression where a malformed livepatch
module missing old_name causes a kernel panic on load?

If a livepatch module defines a klp_func with .old_name = NULL but provides
.new_func, it bypasses the klp_for_each_func_static loop termination
condition.

Since klp_is_patch_compatible() executes before klp_init_patch():

kernel/livepatch/core.c:klp_enable_patch() {
    ...
    if (!klp_is_patch_compatible(patch)) {
    ...
    ret = klp_init_patch(patch);
    ...
}

klp_has_function_conflict() iterates over this malformed function, which then
passes the malformed func to klp_find_func():

kernel/livepatch/state.c:klp_has_function_conflict() {
    ...
    if (klp_find_func(old_obj, func))
    ...
}

This then passes func->old_name to strcmp():

kernel/livepatch/core.c:klp_find_func() {
    ...
    if ((strcmp(obj_func->old_name, func->old_name) == 0) &&
    ...
}

resulting in an immediate null pointer dereference.

> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=9