[PATCH 0/4] modprobe -r fixes and refactors

[PATCH 0/4] modprobe -r fixes and refactors

[Lucas De Marchi]

Fixes and refactors that I noticed were needed while preparing support
for "patient module removal[1][2]" in modprobe.

The added functionality will be sent on top of these.

[1] https://lore.kernel.org/linux-modules/20210803202417.462197-1-mcgrof@kernel.org/
[2] https://lore.kernel.org/linux-modules/20210810051602.3067384-1-mcgrof@kernel.org/

Lucas De Marchi (4):
  modprobe: Rename rmmod_do_deps_list
  modprobe: Fix holders removal
  modprobe: Move check for remove_holders to caller
  modprobe: Make rmmod_do_module() contain all the removal sequence

 tools/modprobe.c | 64 ++++++++++++++++++++++++++++--------------------
 1 file changed, 38 insertions(+), 26 deletions(-)

-- 
2.35.1

[PATCH 1/4] modprobe: Rename rmmod_do_deps_list

[Lucas De Marchi]

It's used not only for dependencies, but also for pre and post softdep.

Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 tools/modprobe.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/tools/modprobe.c b/tools/modprobe.c
index 9387537..eed951f 100644
--- a/tools/modprobe.c
+++ b/tools/modprobe.c
@@ -357,7 +357,8 @@ static int rmmod_do_remove_module(struct kmod_module *mod)
 #define RMMOD_FLAG_IGNORE_BUILTIN	0x2
 static int rmmod_do_module(struct kmod_module *mod, int flags);
 
-static int rmmod_do_deps_list(struct kmod_list *list, bool stop_on_errors)
+/* Remove modules in reverse order */
+static int rmmod_do_modlist(struct kmod_list *list, bool stop_on_errors)
 {
 	struct kmod_list *l;
 
@@ -413,12 +414,12 @@ static int rmmod_do_module(struct kmod_module *mod, int flags)
 		}
 	}
 
-	rmmod_do_deps_list(post, false);
+	rmmod_do_modlist(post, false);
 
 	if ((flags & RMMOD_FLAG_DO_DEPENDENCIES) && remove_dependencies) {
 		struct kmod_list *deps = kmod_module_get_dependencies(mod);
 
-		err = rmmod_do_deps_list(deps, true);
+		err = rmmod_do_modlist(deps, true);
 		if (err < 0)
 			goto error;
 	}
@@ -443,7 +444,7 @@ static int rmmod_do_module(struct kmod_module *mod, int flags)
 	if (err < 0)
 		goto error;
 
-	rmmod_do_deps_list(pre, false);
+	rmmod_do_modlist(pre, false);
 
 error:
 	kmod_module_unref_list(pre);
-- 
2.35.1

[PATCH 1/4] modprobe: rename rmmod_do_deps_list

[Lucas De Marchi]

It's used not only for dependencies, but also for pre and post softdep.

Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 tools/modprobe.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/tools/modprobe.c b/tools/modprobe.c
index 9387537..eed951f 100644
--- a/tools/modprobe.c
+++ b/tools/modprobe.c
@@ -357,7 +357,8 @@ static int rmmod_do_remove_module(struct kmod_module *mod)
 #define RMMOD_FLAG_IGNORE_BUILTIN	0x2
 static int rmmod_do_module(struct kmod_module *mod, int flags);
 
-static int rmmod_do_deps_list(struct kmod_list *list, bool stop_on_errors)
+/* Remove modules in reverse order */
+static int rmmod_do_modlist(struct kmod_list *list, bool stop_on_errors)
 {
 	struct kmod_list *l;
 
@@ -413,12 +414,12 @@ static int rmmod_do_module(struct kmod_module *mod, int flags)
 		}
 	}
 
-	rmmod_do_deps_list(post, false);
+	rmmod_do_modlist(post, false);
 
 	if ((flags & RMMOD_FLAG_DO_DEPENDENCIES) && remove_dependencies) {
 		struct kmod_list *deps = kmod_module_get_dependencies(mod);
 
-		err = rmmod_do_deps_list(deps, true);
+		err = rmmod_do_modlist(deps, true);
 		if (err < 0)
 			goto error;
 	}
@@ -443,7 +444,7 @@ static int rmmod_do_module(struct kmod_module *mod, int flags)
 	if (err < 0)
 		goto error;
 
-	rmmod_do_deps_list(pre, false);
+	rmmod_do_modlist(pre, false);
 
 error:
 	kmod_module_unref_list(pre);
-- 
2.35.1

[PATCH 2/4] modprobe: Fix holders removal

[Lucas De Marchi]

The idea behind --remove-dependencies was to remove other modules that
depend on the current module being removed. It's the reverse
dependency list, not the dependency list of the current module: that
never works since the current module would still hold a ref on it.

Fix it by replacing the call to kmod_module_get_dependencies() with
kmod_module_get_holders() when using that option. Also try to cleanup
the confusion by renaming the option to --remove-holders: "holder" is
the name used in sysfs and by libkmod to refer to a "live" reverse
dependency like what we are interested in.

Before:
	./tools/modprobe -D -r --remove-dependencies video
	rmmod video

After:
	./tools/modprobe -D -r --remove-holders video
	rmmod i915
	rmmod thinkpad_acpi
	rmmod video

Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 tools/modprobe.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/tools/modprobe.c b/tools/modprobe.c
index eed951f..ceb4ff6 100644
--- a/tools/modprobe.c
+++ b/tools/modprobe.c
@@ -54,7 +54,7 @@ static int use_blacklist = 0;
 static int force = 0;
 static int strip_modversion = 0;
 static int strip_vermagic = 0;
-static int remove_dependencies = 0;
+static int remove_holders = 0;
 static int quiet_inuse = 0;
 
 static const char cmdopts_s[] = "arRibfDcnC:d:S:sqvVh";
@@ -62,6 +62,7 @@ static const struct option cmdopts[] = {
 	{"all", no_argument, 0, 'a'},
 	{"remove", no_argument, 0, 'r'},
 	{"remove-dependencies", no_argument, 0, 5},
+	{"remove-holders", no_argument, 0, 5},
 	{"resolve-alias", no_argument, 0, 'R'},
 	{"first-time", no_argument, 0, 3},
 	{"ignore-install", no_argument, 0, 'i'},
@@ -107,7 +108,8 @@ static void help(void)
 		"\t                            be a module name to be inserted\n"
 		"\t                            or removed (-r)\n"
 		"\t-r, --remove                Remove modules instead of inserting\n"
-		"\t    --remove-dependencies   Also remove modules depending on it\n"
+		"\t    --remove-dependencies   Deprecated: use --remove-holders\n"
+		"\t    --remove-holders        Also remove module holders (use together with -r)\n"
 		"\t-R, --resolve-alias         Only lookup and print alias and exit\n"
 		"\t    --first-time            Fail if module already inserted or removed\n"
 		"\t-i, --ignore-install        Ignore install commands\n"
@@ -353,7 +355,7 @@ static int rmmod_do_remove_module(struct kmod_module *mod)
 	return err;
 }
 
-#define RMMOD_FLAG_DO_DEPENDENCIES	0x1
+#define RMMOD_FLAG_REMOVE_HOLDERS	0x1
 #define RMMOD_FLAG_IGNORE_BUILTIN	0x2
 static int rmmod_do_module(struct kmod_module *mod, int flags);
 
@@ -416,10 +418,10 @@ static int rmmod_do_module(struct kmod_module *mod, int flags)
 
 	rmmod_do_modlist(post, false);
 
-	if ((flags & RMMOD_FLAG_DO_DEPENDENCIES) && remove_dependencies) {
-		struct kmod_list *deps = kmod_module_get_dependencies(mod);
+	if ((flags & RMMOD_FLAG_REMOVE_HOLDERS) && remove_holders) {
+		struct kmod_list *holders = kmod_module_get_holders(mod);
 
-		err = rmmod_do_modlist(deps, true);
+		err = rmmod_do_modlist(holders, true);
 		if (err < 0)
 			goto error;
 	}
@@ -469,7 +471,7 @@ static int rmmod(struct kmod_ctx *ctx, const char *alias)
 
 	kmod_list_foreach(l, list) {
 		struct kmod_module *mod = kmod_module_get_module(l);
-		err = rmmod_do_module(mod, RMMOD_FLAG_DO_DEPENDENCIES);
+		err = rmmod_do_module(mod, RMMOD_FLAG_REMOVE_HOLDERS);
 		kmod_module_unref(mod);
 		if (err < 0)
 			break;
@@ -787,7 +789,7 @@ static int do_modprobe(int argc, char **orig_argv)
 			do_remove = 1;
 			break;
 		case 5:
-			remove_dependencies = 1;
+			remove_holders = 1;
 			break;
 		case 'R':
 			lookup_only = 1;
-- 
2.35.1

[PATCH 3/4] modprobe: Move check for remove_holders to caller

[Lucas De Marchi]

Do not mix the flags with and additional boolean from arguments.

Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 tools/modprobe.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/tools/modprobe.c b/tools/modprobe.c
index ceb4ff6..0d9b805 100644
--- a/tools/modprobe.c
+++ b/tools/modprobe.c
@@ -418,7 +418,7 @@ static int rmmod_do_module(struct kmod_module *mod, int flags)
 
 	rmmod_do_modlist(post, false);
 
-	if ((flags & RMMOD_FLAG_REMOVE_HOLDERS) && remove_holders) {
+	if (flags & RMMOD_FLAG_REMOVE_HOLDERS) {
 		struct kmod_list *holders = kmod_module_get_holders(mod);
 
 		err = rmmod_do_modlist(holders, true);
@@ -471,7 +471,9 @@ static int rmmod(struct kmod_ctx *ctx, const char *alias)
 
 	kmod_list_foreach(l, list) {
 		struct kmod_module *mod = kmod_module_get_module(l);
-		err = rmmod_do_module(mod, RMMOD_FLAG_REMOVE_HOLDERS);
+		int flags = remove_holders ? RMMOD_FLAG_REMOVE_HOLDERS : 0;
+
+		err = rmmod_do_module(mod, flags);
 		kmod_module_unref(mod);
 		if (err < 0)
 			break;
-- 
2.35.1

[PATCH 3/4] modprobe: move check for remove_holders to caller

[Lucas De Marchi]

Do not mix the flags with and additional boolean from arguments.

Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 tools/modprobe.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/tools/modprobe.c b/tools/modprobe.c
index ceb4ff6..0d9b805 100644
--- a/tools/modprobe.c
+++ b/tools/modprobe.c
@@ -418,7 +418,7 @@ static int rmmod_do_module(struct kmod_module *mod, int flags)
 
 	rmmod_do_modlist(post, false);
 
-	if ((flags & RMMOD_FLAG_REMOVE_HOLDERS) && remove_holders) {
+	if (flags & RMMOD_FLAG_REMOVE_HOLDERS) {
 		struct kmod_list *holders = kmod_module_get_holders(mod);
 
 		err = rmmod_do_modlist(holders, true);
@@ -471,7 +471,9 @@ static int rmmod(struct kmod_ctx *ctx, const char *alias)
 
 	kmod_list_foreach(l, list) {
 		struct kmod_module *mod = kmod_module_get_module(l);
-		err = rmmod_do_module(mod, RMMOD_FLAG_REMOVE_HOLDERS);
+		int flags = remove_holders ? RMMOD_FLAG_REMOVE_HOLDERS : 0;
+
+		err = rmmod_do_module(mod, flags);
 		kmod_module_unref(mod);
 		if (err < 0)
 			break;
-- 
2.35.1

[PATCH 4/4] modprobe: Make rmmod_do_module() contain all the removal sequence

[Lucas De Marchi]

Move the remaining part of the removal sequence dangling in
rmmod_do_remove_module() to rmmod_do_module() so we can consider this
function is the one controlling all the module removals.

While at it, add some comments about the removal order and normalize
coding style in this function.

Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 tools/modprobe.c | 37 ++++++++++++++++++++++---------------
 1 file changed, 22 insertions(+), 15 deletions(-)

diff --git a/tools/modprobe.c b/tools/modprobe.c
index 0d9b805..34ef8da 100644
--- a/tools/modprobe.c
+++ b/tools/modprobe.c
@@ -322,7 +322,6 @@ end:
 static int rmmod_do_remove_module(struct kmod_module *mod)
 {
 	const char *modname = kmod_module_get_name(mod);
-	struct kmod_list *deps, *itr;
 	int flags = 0, err;
 
 	SHOW("rmmod %s\n", kmod_module_get_name(mod));
@@ -341,17 +340,6 @@ static int rmmod_do_remove_module(struct kmod_module *mod)
 			LOG("Module %s is not in kernel.\n", modname);
 	}
 
-	deps = kmod_module_get_dependencies(mod);
-	if (deps != NULL) {
-		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);
-			kmod_module_unref(dep);
-		}
-		kmod_module_unref_list(deps);
-	}
-
 	return err;
 }
 
@@ -394,7 +382,8 @@ static int rmmod_do_module(struct kmod_module *mod, int flags)
 		cmd = kmod_module_get_remove_commands(mod);
 	}
 
-	if (cmd == NULL && !ignore_loaded) {
+	/* Quick check if module is loaded, otherwise there's nothing to do */
+	if (!cmd && !ignore_loaded) {
 		int state = kmod_module_get_initstate(mod);
 
 		if (state < 0) {
@@ -416,8 +405,10 @@ static int rmmod_do_module(struct kmod_module *mod, int flags)
 		}
 	}
 
+	/* 1. @mod's post-softdeps in reverse order */
 	rmmod_do_modlist(post, false);
 
+	/* 2. Other modules holding @mod */
 	if (flags & RMMOD_FLAG_REMOVE_HOLDERS) {
 		struct kmod_list *holders = kmod_module_get_holders(mod);
 
@@ -426,7 +417,8 @@ static int rmmod_do_module(struct kmod_module *mod, int flags)
 			goto error;
 	}
 
-	if (!ignore_loaded && !cmd) {
+	/* 3. @mod itself, but check for refcnt first */
+	if (!cmd && !ignore_loaded) {
 		int usage = kmod_module_get_refcnt(mod);
 
 		if (usage > 0) {
@@ -438,7 +430,7 @@ static int rmmod_do_module(struct kmod_module *mod, int flags)
 		}
 	}
 
-	if (cmd == NULL)
+	if (!cmd)
 		err = rmmod_do_remove_module(mod);
 	else
 		err = command_do(mod, "remove", cmd, NULL);
@@ -446,6 +438,21 @@ static int rmmod_do_module(struct kmod_module *mod, int flags)
 	if (err < 0)
 		goto error;
 
+	/* 4. Other modules that became unused: errors are non-fatal */
+	if (!cmd) {
+		struct kmod_list *deps, *itr;
+
+		deps = kmod_module_get_dependencies(mod);
+		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);
+			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);
 
 error:
-- 
2.35.1

Re: [PATCH 0/4] modprobe -r fixes and refactors

[Lucas De Marchi]

On Tue, Mar 29, 2022 at 02:05:34AM -0700, Lucas De Marchi wrote:
>Fixes and refactors that I noticed were needed while preparing support
>for "patient module removal[1][2]" in modprobe.
>
>The added functionality will be sent on top of these.
>
>[1] https://lore.kernel.org/linux-modules/20210803202417.462197-1-mcgrof@kernel.org/
>[2] https://lore.kernel.org/linux-modules/20210810051602.3067384-1-mcgrof@kernel.org/
>
>Lucas De Marchi (4):
>  modprobe: Rename rmmod_do_deps_list
>  modprobe: Fix holders removal
>  modprobe: Move check for remove_holders to caller
>  modprobe: Make rmmod_do_module() contain all the removal sequence

patches 1 and 3 were duplicate as I reworded the commit message.
Please consider the ones matching subjects above.

thanks
Lucas De Marchi

>
> tools/modprobe.c | 64 ++++++++++++++++++++++++++++--------------------
> 1 file changed, 38 insertions(+), 26 deletions(-)
>
>-- 
>2.35.1
>

Re: [PATCH 1/4] modprobe: Rename rmmod_do_deps_list

[Luis Chamberlain]

On Tue, Mar 29, 2022 at 02:05:35AM -0700, Lucas De Marchi wrote:
> It's used not only for dependencies, but also for pre and post softdep.
> 
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>

Reviewed-by: Luis Chamberlain <mcgrof@kernel.org>

  Luis

Re: [PATCH 2/4] modprobe: Fix holders removal

[Luis Chamberlain]

On Tue, Mar 29, 2022 at 02:05:37AM -0700, Lucas De Marchi wrote:
> The idea behind --remove-dependencies was to remove other modules that
> depend on the current module being removed. It's the reverse
> dependency list, not the dependency list of the current module: that
> never works since the current module would still hold a ref on it.
> 
> Fix it by replacing the call to kmod_module_get_dependencies() with
> kmod_module_get_holders() when using that option. Also try to cleanup
> the confusion by renaming the option to --remove-holders: "holder" is
> the name used in sysfs and by libkmod to refer to a "live" reverse
> dependency like what we are interested in.
> 
> Before:
> 	./tools/modprobe -D -r --remove-dependencies video
> 	rmmod video
> 
> After:
> 	./tools/modprobe -D -r --remove-holders video
> 	rmmod i915
> 	rmmod thinkpad_acpi
> 	rmmod video
> 
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>

Reviewed-by: Luis Chamberlain <mcgrof@kernel.org>

  Luis

Re: [PATCH 3/4] modprobe: Move check for remove_holders to caller

[Luis Chamberlain]

On Tue, Mar 29, 2022 at 02:05:38AM -0700, Lucas De Marchi wrote:
> Do not mix the flags with and additional boolean from arguments.
> 
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>

Reviewed-by: Luis Chamberlain <mcgrof@kernel.org>

  Luis

Re: [PATCH 4/4] modprobe: Make rmmod_do_module() contain all the removal sequence

[Luis Chamberlain]

On Tue, Mar 29, 2022 at 02:05:40AM -0700, Lucas De Marchi wrote:
> Move the remaining part of the removal sequence dangling in
> rmmod_do_remove_module() to rmmod_do_module() so we can consider this
> function is the one controlling all the module removals.
> 
> While at it, add some comments about the removal order and normalize
> coding style in this function.
> 
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>

Reviewed-by: Luis Chamberlain <mcgrof@kernel.org>

  Luis

Re: [PATCH 4/4] modprobe: Make rmmod_do_module() contain all the removal sequence

[Lucas De Marchi]

On Wed, Apr 06, 2022 at 03:22:47PM -0700, Luis Chamberlain wrote:
>On Tue, Mar 29, 2022 at 02:05:40AM -0700, Lucas De Marchi wrote:
>> Move the remaining part of the removal sequence dangling in
>> rmmod_do_remove_module() to rmmod_do_module() so we can consider this
>> function is the one controlling all the module removals.
>>
>> While at it, add some comments about the removal order and normalize
>> coding style in this function.
>>
>> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
>
>Reviewed-by: Luis Chamberlain <mcgrof@kernel.org>

thanks, pushed.

Lucas De Marchi

>
>  Luis