Re: [PATCH v8 8/9] selftests/livepatch: Add state test for provides/obsoletes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] Unloading independent livepatches in FIFO order permanently alters the global `console_loglevel`.
- [Medium] Mutable module parameter 'state' causes a memory leak and failure to restore kernel state if modified via sysfs after load.
- [Low] Missing sysfs value verification in test 'misc states, misc provides'.
--

commit 253fd167cb01a1b8aeead3420135a4b275836ef3
Author: Yafang Shao <laoar.shao@gmail.com>
Subject: selftests/livepatch: Add state test for provides/obsoletes

This commit adds `provides` and `obsoletes` module parameters to
`test_klp_state` and `test_klp_state2` selftests. It introduces testing 
coverage to ensure that reusing a state ID across different provides IDs 
is rejected, while it is allowed when the new livepatch obsoletes the old 
one.

> diff --git a/tools/testing/selftests/livepatch/test-provides-obsoletes.sh b/tools/testing/selftests/livepatch/test-provides-obsoletes.sh
> index c2fbac3d0e06f..8b7b5eded80c7 100755
> --- a/tools/testing/selftests/livepatch/test-provides-obsoletes.sh
> +++ b/tools/testing/selftests/livepatch/test-provides-obsoletes.sh
[ ... ]
> @@ -213,4 +215,238 @@ livepatch: '$MOD_CMDLINE': unpatching complete
[ ... ]
> +# - load two livepatches with different provides ids (provides=1 and
> +#   provides=2) that modify different system states (state=1 and
> +#   state=2). Neither of them replaces the other, because their
> +#   provides ids differ (and the obsoletes lists are empty), so they
> +#   can coexist.
> +# - verify the provides sysfs value of both livepatches
> +# - disable and unload both livepatches
> +
> +start_test "misc states, misc provides"
> +
> +load_lp $MOD_STATE state=1 provides=1
> +load_lp $MOD_STATE2 state=2 provides=2
> +

[Severity: Low]
The test documentation states it will "verify the provides sysfs value of both
livepatches". Is this verification missing here before we disable and unload
them?

> +disable_lp $MOD_STATE
> +unload_lp $MOD_STATE
> +disable_lp $MOD_STATE2
> +unload_lp $MOD_STATE2

[Severity: Medium]
Does unloading these livepatches in FIFO order permanently alter the global
console_loglevel?

If MOD_STATE saves the original loglevel and sets it to 15, and then
MOD_STATE2 saves 15 and sets it to 15, wouldn't unloading MOD_STATE first
restore the original loglevel, only for unloading MOD_STATE2 to subsequently
overwrite it with 15? Should the teardown be LIFO to properly unwind the
loglevel changes?

> +
> +check_result "% insmod test_modules/$MOD_STATE.ko state=1 provides=1
[ ... ]
> diff --git a/tools/testing/selftests/livepatch/test_modules/test_klp_state.c b/tools/testing/selftests/livepatch/test_modules/test_klp_state.c
> index 5c5872ff25665..1de63c2438d68 100644
> --- a/tools/testing/selftests/livepatch/test_modules/test_klp_state.c
> +++ b/tools/testing/selftests/livepatch/test_modules/test_klp_state.c
> @@ -9,10 +9,25 @@
>  #include <linux/printk.h>
>  #include <linux/livepatch.h>
>  
> -#define CONSOLE_LOGLEVEL_STATE 1
>  /* Version 1 does not support migration. */
>  #define CONSOLE_LOGLEVEL_STATE_VERSION 1
>  
> +static unsigned int state = 1;
> +module_param(state, uint, 0644);

[Severity: Medium]
Could this mutable module parameter cause a memory leak and failure to restore
the kernel state if modified via sysfs after load?

Since the permissions are set to 0644, if root modifies the state parameter
while the livepatch is loaded, subsequent callbacks like free_loglevel_state()
using klp_get_state(&patch, state) will return NULL. Does this silently skip
cleanup (kfree) and state restoration during patch unload?

This same issue also applies to test_klp_state2.c.

> +MODULE_PARM_DESC(state, "console loglevel state (default=1)");
> +

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909024324.16002-1-laoar.shao@gmail.com?part=8