[PATCH] kmod: modprobe: Remove holders recursively

* [PATCH] kmod: modprobe: Remove holders recursively
@ 2023-03-15 13:48 Nicolas Schier
  2023-03-15 18:16 ` Lucas De Marchi
  0 siblings, 1 reply; 4+ messages in thread
From: Nicolas Schier @ 2023-03-15 13:48 UTC (permalink / raw)
  To: linux-modules; +Cc: Nicolas Schier, Lucas De Marchi

From: Nicolas Schier <n.schier@avm.de>

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@avm.de>
---
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.
---
To: linux-modules@vger.kernel.org
Cc: Lucas De Marchi <lucas.de.marchi@gmail.com>
---
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


^ permalink raw reply related	[flat|nested] 4+ messages in thread