On Wed, Mar 29, 2023 at 03:51:35PM +0200, Nicolas Schier wrote:
Remove holders recursively when removal of module holders is requested.
Extend commit 42b32d30c38e ("modprobe: Fix holders removal") by removing
also indirect holders if --remove-holders is set. For a simple module
dependency chain like
module3 depends on module2
module2 depends on module1
'modprobe -r --remove-holders module1' will remove module3, module2 and
module1 in correct order:
$ modprobe -r --remove-holders module1 --verbose
rmmod module3
rmmod module2
rmmod module1
(Actually, it will do the same when specifying module2 or module3 for
removal instead of module1.)
As a side-effect, 'modprobe -r module3' (w/o --remove-holders) will also
remove all three modules from the example above, as after removal of
module3, module2 does not have any holders and the same holds for
module1 after removal of module2:
$ modprobe -r module3 --verbose
rmmod module3
rmmod module2
rmmod module1
Without recursive evaluation of holders, modprobe leaves module1 loaded.
Unfortunately, '--dry-run --remove-holders' breaks with indirect
dependencies.
Signed-off-by: Nicolas Schier <n.schier@xxxxxx>
---
I am a bit unhappy about the introduction of the 'recursive' parameter
yeah... it makes it slightly harder to read.
to rmmod_do_modlist() as it always holds the same value as
stop_on_errors; is re-using (and renaming) possibly a better option?
modprobe --remove --remove-holders --dry-run now ignores current
refcounts of loaded modules when simulating removal of holders.
Changes in v2:
* Handle modules that have just been removed, gently
* Fix --dry-run by ignoring module refcounts (_only_ for --dry-run)
* Add missing kmod_module_unref_list() calls
---
tools/modprobe.c | 44 +++++++++++++++++++++++++++++++++++---------
1 file changed, 35 insertions(+), 9 deletions(-)
diff --git a/tools/modprobe.c b/tools/modprobe.c
index 3b7897c..a705f88 100644
--- a/tools/modprobe.c
+++ b/tools/modprobe.c
@@ -390,13 +390,27 @@ static int rmmod_do_remove_module(struct kmod_module *mod)
static int rmmod_do_module(struct kmod_module *mod, int flags);
/* Remove modules in reverse order */
-static int rmmod_do_modlist(struct kmod_list *list, bool stop_on_errors)
+static int rmmod_do_modlist(struct kmod_list *list, bool stop_on_errors,
+ bool recursive)
{
struct kmod_list *l;
kmod_list_foreach_reverse(l, list) {
struct kmod_module *m = kmod_module_get_module(l);
- int r = rmmod_do_module(m, RMMOD_FLAG_IGNORE_BUILTIN);
+ int r = 0;
+
+ if (recursive && kmod_module_get_initstate(m) >= 0) {
+ struct kmod_list *holders = kmod_module_get_holders(m);
+
+ r = rmmod_do_modlist(holders, stop_on_errors,
+ recursive);
+
+ kmod_module_unref_list(holders);
+ }
+
+ if (!r)
+ r = rmmod_do_module(m, RMMOD_FLAG_IGNORE_BUILTIN);
+
kmod_module_unref(m);
if (r < 0 && stop_on_errors)
@@ -448,15 +462,17 @@ static int rmmod_do_module(struct kmod_module *mod, int flags)
}
/* 1. @mod's post-softdeps in reverse order */
- rmmod_do_modlist(post, false);
+ rmmod_do_modlist(post, false, false);
/* 2. Other modules holding @mod */
if (flags & RMMOD_FLAG_REMOVE_HOLDERS) {
struct kmod_list *holders = kmod_module_get_holders(mod);
- err = rmmod_do_modlist(holders, true);
+ err = rmmod_do_modlist(holders, true, true);
if (err < 0)
goto error;
+
+ kmod_module_unref_list(holders);
this is a separate fix. We also need to unref it on error, so probably
best to do:
err = rmmod_do_modlist(holders, true, true);
kmod_module_unref_list(holders);
if (err < 0)
goto error;
I think the alternative to the recursive approach would be to make only
the kmod_module_get_holders() be recursive:
struct kmod_list *holders = recursive_holders(mod);
And let recursive holders do recurse on modules passing the list as
argument to be augmented. Then the rest remains the same.
}
/* 3. @mod itself, but check for refcnt first */
@@ -472,9 +488,16 @@ static int rmmod_do_module(struct kmod_module *mod, int flags)
}
}
- if (!cmd)
- err = rmmod_do_remove_module(mod);
- else
+ if (!cmd) {
+ int state = kmod_module_get_initstate(mod);
+
+ if (state < 0) {
+ /* Module was removed during recursive holder removal */
+ err = 0;
wouldn't this fall in this case inside rmmod_do_remove_module()?
err = kmod_module_remove_module(mod, flags);
if (err == -EEXIST) {
if (!first_time)
err = 0;
+ } else {
+ err = rmmod_do_remove_module(mod);
+ }
+ } else
err = command_do(mod, "remove", cmd, NULL);
if (err < 0)
@@ -488,14 +511,14 @@ static int rmmod_do_module(struct kmod_module *mod, int flags)
kmod_list_foreach(itr, deps) {
struct kmod_module *dep = kmod_module_get_module(itr);
if (kmod_module_get_refcnt(dep) == 0)
- rmmod_do_remove_module(dep);
+ rmmod_do_module(dep, flags);
not sure also recursing the holders of the modules left is what we want.
If there are holders, then the module's refcnt should not be 0 anyway.
Lucas De Marchi
kmod_module_unref(dep);
}
kmod_module_unref_list(deps);
}
/* 5. @mod's pre-softdeps in reverse order: errors are non-fatal */
- rmmod_do_modlist(pre, false);
+ rmmod_do_modlist(pre, false, false);
error:
kmod_module_unref_list(pre);
@@ -975,6 +998,9 @@ static int do_modprobe(int argc, char **orig_argv)
fstat(fileno(stderr), &stat_buf)))
use_syslog = 1;
+ if (remove_holders && dry_run)
+ ignore_loaded = 1;
+
log_open(use_syslog);
if (!do_show_config) {
--
2.40.0
