Re: [PATCH] kmod: modprobe: Remove holders recursively

[Nicolas Schier <n.schier@avm.de>]

[-- Attachment #1: Type: text/plain, Size: 3775 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]