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

- From: Yafang Shao \<hidden\>
- Date: 2026-09-09 07:31:21 
- List: live-patching
- Page: https://ratatoskr.run/live-patching/2026/09/17535977
- Thread: https://ratatoskr.run/sashiko-reviews/2026/09/17535977/t.md

```text
On Wed, Sep 9, 2026 at 10:57 AM [ref] wrote:
>
> 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 [ref]
> 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?

will update it

--- a/tools/testing/selftests/livepatch/test-provides-obsoletes.sh
+++ b/tools/testing/selftests/livepatch/test-provides-obsoletes.sh
@@ -404,6 +404,9 @@ start_test "misc states, misc provides"
 load_lp $MOD_STATE state=1 provides=1
 load_lp $MOD_STATE2 state=2 provides=2

+check_sysfs_value "$MOD_STATE" "provides" "1"
+check_sysfs_value "$MOD_STATE2" "provides" "2"
+
 disable_lp $MOD_STATE
 unload_lp $MOD_STATE
 disable_lp $MOD_STATE2

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

Ah, it should be LIFO
I will update it.

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

will change it to 0444 to fix it.

-- 
Regards
Yafang
```