Re: [PATCH 1/4] selftests: livepatch: Introduce _remove_mod function

[Petr Mladek <pmladek@suse.com>]

On Mon 2026-05-25 08:49:10, Marcos Paulo de Souza wrote:
> On Sun, 2026-05-24 at 23:59 +0000, sashiko-bot@kernel.org wrote:
> > Thank you for your contribution! Sashiko AI review found 1 potential
> > issue(s) to consider:
> > - [Medium] `_remove_mod` uses `die()` which will terminate the script
> > during cleanup operations.
> > --
> > 
> > commit 7b1591f4e5c3b351136e7fe2821eb2e15ae59ad0
> > Author: Marcos Paulo de Souza <mpdesouza@suse.com>
> > 
> > selftests: livepatch: Introduce _remove_mod function
> > 
> > This commit introduces the _remove_mod internal function to remove a
> > loaded
> > module, intended to be used during cleanup when a testcase fails. It
> > also
> > refactors unload_mod to use this new helper function.
> > 
> > > diff --git a/tools/testing/selftests/livepatch/functions.sh
> > > b/tools/testing/selftests/livepatch/functions.sh
> > > index 2bc50271729c6..3ec0b7962fc5c 100644
> > > --- a/tools/testing/selftests/livepatch/functions.sh
> > > +++ b/tools/testing/selftests/livepatch/functions.sh
> > 
> > [ ... ]
> > 
> > > @@ -241,9 +241,10 @@ function load_failing_mod() {
> > >  	log "$ret"
> > >  }
> > >  
> > > -# unload_mod(modname) - unload a kernel module
> > > +# _remove_mod(modname) - Internal function to remove a loaded
> > > module.
> > > +#                        Use unload_mod() instead, which also
> > > updates TEST_MODS tracking.
> > >  #	modname - module name to unload
> > > -function unload_mod() {
> > > +function _remove_mod() {
> > >  	local mod="$1"
> > >  
> > >  	# Wait for module reference count to clear ...
> > > @@ -261,6 +262,14 @@ function unload_mod() {
> > >  		die "failed to unload module $mod (/sys/module)"
> > 
> > [Severity: Medium]
> > The commit message notes this function will be used to remove loaded
> > modules
> > when a testcase fails. If used in a cleanup path (such as a bash EXIT
> > trap),
> > will calling die() here terminate the script prematurely?
> > 
> > If a module fails to unload (for example, if its refcount doesn't
> > drop),
> > die() executes exit 1. Inside a trap, this aborts the rest of the
> > cleanup process. This could skip unloading any remaining modules and
> > prevent final cleanup operations like pop_config() from running,
> > potentially
> > leaving kernel tracing persistently enabled and polluting the system
> > state
> > for subsequent tests.
> 
> The system is already polluted when a testcase fails nowadays, since a
> module is kept loaded either way, making the next tests to fail if they
> also try to lead the same modules.
> 
> The current approach is trying to make it more reliable.

I believe that Sashiko suggested that the clean up path should not
use "die" or "exit" when something fails. It should try to continue
with the next cleanup task.

I remember that we even used the following in rpm post install scripts:

    command || true

It is useful when the script does several independent actions
and we would like to process as many of them as possible.

For example, in our case, if one module can't be unloaded
then we might still try to remove other modules and
call pop_config.

So, we should somehow distinguish when some code paths are
called in the script and when in the clean up part.

I would export some variable in the cleanup() function and
check it in the called code paths, for example:

diff --git a/tools/testing/selftests/livepatch/functions.sh b/tools/testing/selftests/livepatch/functions.sh
index 9c98bbb8b725..eebc7f193d98 100644
--- a/tools/testing/selftests/livepatch/functions.sh
+++ b/tools/testing/selftests/livepatch/functions.sh
@@ -62,7 +62,9 @@ function has_kdir() {
 function die() {
 	log "ERROR: $1"
 	echo "ERROR: $1" >&2
-	exit 1
+	if [ -z "$in_klp_cleanup" ] ; then
+	    exit 1
+	fi
 }
 
 function push_config() {
@@ -128,6 +130,8 @@ function set_ftrace_enabled() {
 }
 
 function cleanup() {
+	export in_klp_cleanup=1
+
 	# Remove leftover modules in reverse order to handle dependencies
 	for mod_item in "${TEST_MODS[@]}"; do
 		if is_livepatch_mod "$mod_item"; then


Finally, I think how to split the changes into patches.

I think that the above changes which add "in_klp_cleanup" might be a
separate patch,

But the original 1st patch looked weird. It did split _remove_mod()
and talked about TEST_MODS tracking but the tracking was added in
2nd patch. I would prefer to split the function and add the tracking
in the same patch so that it is obvious why it is split and what
is the difference.

Best Regards,
Petr