On Wed, Apr 12, 2023 at 12:21:51PM -0700, Lucas De Marchi wrote:
> 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;
Thanks! Yes, sure. I sent it as a separate patch at
https://lore.kernel.org/linux-modules/20230418-add-missing-kmod_module_unref_list-v1-1-ab5b554f15ee@xxxxxx/
>
> 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.
Yes, that sounds much nicer than the stuff I did. I will try and send a v3.
> > }
> >
> > /* 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;
No, as the module might already be removed and kmod_module_remove_module() thus
returns -ENOENT. I think, your suggestion to regarding introduction of
recursive_holders() should remove the need for checking the kmod state here.
FTR: I tested the current patch w/o the kmod state check; as expected, it leads to errors:
testsuite failure, when using --remove-holders:
rmmod mod_dep_chain_c
TESTSUITE: Added module to test delete_module:
TESTSUITE: name=mod_dep_chain_c ret=0 errcode=0
TESTSUITE: Added module to test delete_module:
TESTSUITE: name=mod_dep_chain_b ret=0 errcode=0
TESTSUITE: Added module to test delete_module:
TESTSUITE: name=mod_dep_chain_a ret=0 errcode=0
rmmod mod_dep_chain_b
libkmod: kmod_module_get_holders: could not open '/sys/module/mod_dep_chain_c/holders': No such file or directory
rmmod mod_dep_chain_a
libkmod: kmod_module_get_holders: could not open '/sys/module/mod_dep_chain_c/holders': No such file or directory
libkmod: kmod_module_get_holders: could not open '/sys/module/mod_dep_chain_b/holders': No such file or directory
rmmod mod_dep_chain_a
libkmod: kmod_module_get_holders: could not open '/sys/module/mod_dep_chain_c/holders': No such file or directory
libkmod: kmod_module_get_holders: could not open '/sys/module/mod_dep_chain_b/holders': No such file or directory
libkmod: kmod_module_get_holders: could not open '/sys/module/mod_dep_chain_a/holders': No such file or directory
modprobe: ERROR: could not remove 'mod_dep_chain_a': No such file or directory
TESTSUITE: ERR: TRAP delete_module(): /home/nicolas/src/kmod/testsuite/rootfs/test-modprobe/remove-holders/sys/module/mod_dep_chain_a: opendir: No such file or directory
TESTSUITE: ERR: TRAP delete_module(): unable to adjust sysfs tree
TESTSUITE: running modprobe_remove_with_holders, in forked context
TESTSUITE: ERR: 'modprobe_remove_with_holders' [2320200] exited with return code 1
TESTSUITE: ERR: FAILED: modprobe_remove_with_holders
TESTSUITE: ------
FAIL testsuite/test-modprobe (exit status: 1)
and on my dev maschine I also see attempts of module removal that fail:
The relevant snippets from lsmod output:
btrfs 1777664 0
zstd_compress 294912 1 btrfs
...
raid456 180224 0
async_raid6_recov 24576 1 raid456
async_memcpy 20480 2 raid456,async_raid6_recov
async_pq 20480 2 raid456,async_raid6_recov
async_xor 20480 3 async_pq,raid456,async_raid6_recov
async_tx 20480 5 async_pq,async_memcpy,async_xor,raid456,async_raid6_recov
xor 24576 2 async_xor,btrfs
raid6_pq 122880 4 async_pq,btrfs,raid456,async_raid6_recov
and the modprobe call, without the additional kmod state check:
sudo tools/modprobe -r --remove-holders xor -vv
modprobe: INFO: custom logging function 0x5564236f5700 registered
rmmod btrfs
rmmod zstd_compress
rmmod raid456
rmmod async_raid6_recov
rmmod async_pq
rmmod raid6_pq
rmmod async_xor
rmmod xor
rmmod async_memcpy
rmmod async_tx
rmmod xor
modprobe: ERROR: could not remove 'xor': No such file or directory
modprobe: INFO: context 0x556423e68440 released
[exit code 1]
Both, testsuite and the modprobe -r, succeed as w/o complaints with the kmod
state check included.
(Again, as written above: I hope the kmod state check insertion becomes
obsolete when switching to something like recursive_holders().)
> > + } 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
Yeah, I have no strong opinion to that. Currently, 'modprobe -r
--remove-holders MOD' removes only direct dependencies of MOD if they have a refcnt
of 0 after MOD has been removed. I think, removing MOD's dependencies
recursively, would make it more of an inverse operation:
modprobe mod-dep-chain-c (loads mod-dep-chain-a, -b and -c)
modprobe -r --remove-holders mod-dep-chain-c (unloads only mod-dep-chain-b ?)
(If option '--remove-dependencies' wasn't burned, it could have been a
switch to enable this?)
But I cannot say, if someone really needs that functionality; thus, I
will drop it in v3.
Thanks for review and suggestions!
Kind regards,
Nicolas
> > 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
> >
Attachment:
signature.asc
Description: PGP signature
