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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxx>
>
> 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@xxxxxx>
> ---
> 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





[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Big List of Linux Books]

  Powered by Linux