| Message ID | 20230309-remove-holders-recursively-v1-1-c27cdba1edbf@avm.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | kmod: modprobe: Remove holders recursively | expand |
On Wed, Mar 15, 2023 at 02:48:16PM +0100, Nicolas Schier wrote: >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. 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@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 >
On Wed, Mar 15, 2023 at 11:16:08AM -0700, Lucas De Marchi wrote: > On Wed, Mar 15, 2023 at 02:48:16PM +0100, Nicolas Schier wrote: > > 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. > > > 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? For modules.dep that is exactly what I see during my tests on v6.1.8 and v4.9.276 (except the /modules1.ko:/module1.ko:/ typo). But with both kernel versions, holders exported via sysfs are only direct users: $ sudo modprobe module3 $ lsmod | grep module module3 16384 0 module2 16384 1 module3 module1 16384 1 module2 $ find /sys/module/module{1,2,3}/holders/ -ls [...] 0 Mar 16 08:45 /sys/module/module1/holders/ [...] 0 Mar 16 08:48 /sys/module/module1/holders/module2 -> ../../module2 [...] 0 Mar 16 08:47 /sys/module/module2/holders/ [...] 0 Mar 16 08:48 /sys/module/module2/holders/module3 -> ../../module3 [...] 0 Mar 16 08:47 /sys/module/module3/holders/ As far as I remember, that has always been this way; it is definitely not introduced by recent kernel changes. Current 'modprobe -r --remove-holders' does only analyse the reported holders, and does not consider anything from modules.dep. > 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. I am going to prepare a test for the testsuite and send a v2 within a few days. Kind regards, Nicolas
On Thu, Mar 16, 2023 at 09:24:40AM +0100, Nicolas Schier wrote: >On Wed, Mar 15, 2023 at 11:16:08AM -0700, Lucas De Marchi wrote: >> On Wed, Mar 15, 2023 at 02:48:16PM +0100, Nicolas Schier wrote: >> > 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. >> >> >> 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? > >For modules.dep that is exactly what I see during my tests on v6.1.8 and >v4.9.276 (except the /modules1.ko:/module1.ko:/ typo). But with both >kernel versions, holders exported via sysfs are only direct users: > > $ sudo modprobe module3 > $ lsmod | grep module > module3 16384 0 > module2 16384 1 module3 > module1 16384 1 module2 > $ find /sys/module/module{1,2,3}/holders/ -ls > [...] 0 Mar 16 08:45 /sys/module/module1/holders/ > [...] 0 Mar 16 08:48 /sys/module/module1/holders/module2 -> ../../module2 > [...] 0 Mar 16 08:47 /sys/module/module2/holders/ > [...] 0 Mar 16 08:48 /sys/module/module2/holders/module3 -> ../../module3 > [...] 0 Mar 16 08:47 /sys/module/module3/holders/ > >As far as I remember, that has always been this way; it is definitely >not introduced by recent kernel changes. > >Current 'modprobe -r --remove-holders' does only analyse the reported >holders, and does not consider anything from modules.dep. oh.. ok. It seems I mixed things up. If a module is holding another one due to a symbol dependency, then we would have the entry in modules.dep and when going through the module removal, it would work. considering just the holders, we have to go through then recursively indeed. > >> 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. > >I am going to prepare a test for the testsuite and send a v2 within a >few days. thanks Lucas De Marchi > >Kind regards, >Nicolas
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);