On Wed, Mar 15, 2023 at 02:48:16PM +0100, Nicolas Schier wrote:
From: Nicolas Schier <n.schier@xxxxxx>
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, 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>
---
While commit 42b32d30c38e ("modprobe: Fix holders removal", 2022-03-29) already
implements removing first-level holders, indirect holders were not evaluated.
In a simple module dependency chain like
module3 depends on module2
module2 depends on module1
'modprobe -r --remove-holders module1' was only considering module2 and module1
and thus had to fail as module3 was still loaded and blocking removal of
module2.
By doing recursive depth-first removal this can be fixed for such simple
dependency.
the dep exported by the kernel didn't require it to be
recursive AFAIR because they were always expanded.
For your case modules.dep should have:
module3.ko: module2.ko module1.ko
module2.ko: module1.ko
modules1.ko:
Is that not the case anymore? Was it due to a change in the kernel build
system or something we missed? What kernel are you testing this with?
We will need a test in the testsuite for that and if it's a change in
the kernel rather than a bug in kmod, probably revert that change until
we have things figured out.
+Luis
thanks
Lucas De Marchi
---
To: linux-modules@xxxxxxxxxxxxxxx
Cc: Lucas De Marchi <lucas.de.marchi@xxxxxxxxx>
---
I am a bit unhappy about the introduction of the 'recursive' parameter
to rmmod_do_modlist() as it always holds the same value as
stop_on_errors; is re-using (and renaming) possibly a better option?
---
tools/modprobe.c | 37 ++++++++++++++++++++++++++++---------
1 file changed, 28 insertions(+), 9 deletions(-)
diff --git a/tools/modprobe.c b/tools/modprobe.c
index 3b7897c..9cbb236 100644
--- a/tools/modprobe.c
+++ b/tools/modprobe.c
@@ -390,13 +390,25 @@ 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) {
+ struct kmod_list *holders = kmod_module_get_holders(m);
+
+ r = rmmod_do_modlist(holders, stop_on_errors,
+ recursive);
+ }
+
+ if (!r)
+ r = rmmod_do_module(m, RMMOD_FLAG_IGNORE_BUILTIN);
+
kmod_module_unref(m);
if (r < 0 && stop_on_errors)
@@ -448,13 +460,13 @@ 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;
}
@@ -472,9 +484,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;
+ } else {
+ err = rmmod_do_remove_module(mod);
+ }
+ } else
err = command_do(mod, "remove", cmd, NULL);
if (err < 0)
@@ -488,14 +507,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);
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);
---
base-commit: 3d1bd339ab942ea47e60f053f4b11b0c47ff082b
change-id: 20230309-remove-holders-recursively-f667f32e2a7d
Best regards,
--
Nicolas Schier
