Re: [PATCH] module: add debugging auto-load duplicate module support

[Luis Chamberlain <mcgrof@kernel.org>]

On Fri, Apr 21, 2023 at 11:31:03AM -0700, Lucas De Marchi wrote:
> On Fri, Apr 21, 2023 at 10:38:49AM -0700, Luis Chamberlain wrote:
> > Just as with the kludge-of-concept I posted for kread [0], userspace
> > also should have similar issues in mapping module name to arbitrary
> > file names given:
> > 
> >  o a module can be in different paths and libkmod could for
> >     example at one point load a module in one path, then userspace
> >     removes it, and the next path is used.
> 
> no, it can't. Unless you are doing out of tree modules and loading them
> manually by path. There can only be one module with the same name in kmod's
> database. If you have duplicate modules, depmod will use the dir
> priority configured by the distro (see depmod.d(5)).
> 
> Since we are talking about *udev* it's not a real possibility as
> 1) the udev requests are serialized
> 2) there is only 1 kmod ctx, so they use the same configuration, no
> funky kmod_new("/another-rootfs", ...) type of thing.
> 
> >  o module names may differ from the filename slightly (in the kernel
> >    we replace dash with "_", refer to KBUILD_MODNAME
> 
> this is taken care by depmod/libkmod too. All the aliases are mapped to
> module names and then normalized. See modname_normalize() in kmod.

Great! So this should be much simpler in userspace.

> > [1] https://lore.kernel.org/all/ZDmAvwi+KNvie+OI@bombadil.infradead.org/T/#md172510af8fdf7e0f76f6caafee9c99f7a8b6de7
> > 
> > > libkmod only skips the call if the module is already in
> > > the live state.
> > 
> > It can do better, it can converge requests to avoid a kernel_read*()
> > from using vmalloc space. Note that this was not well known before,
> > but now it is clear.
> 
> in userspace, if using the same context and using init_module() rather
> than finit_module(), I **guess** we would have a similar thing due to
> the memory pool for modules: we don't read the module again. That is not
> true for finit_module() though as we just open and pass the fd.

I think we could not not care about init_module() races for now.

> > I realize though that this could mean sharing a context between all
> > loads thoughs in udev, and such a change could take significant time
> > and review to complete.
> 
> But there is only one context. There aren't multiple paralell requests
> from multiple sources. Probably need to Cc someone still changing
> udev's builtin...  but from a quick look, from what I remember about
> that the last time I touched it and without data to prove me wrong,
> it seems we are not looking at the right problem space to come up with a
> solution.

Data seems to indicate that somehow this might not be true.

> > If we *wanted* to do this in kernel instead, I have already shown it's
> > not hard.
> > 
> > > It seems systemd-udev also duplicates the check
> > > in src/shared/module-util.c:module_load_and_warn()
> > 
> > Evidence is showing that does not suffice for the races which are
> > currently possible.
> 
> can you raise the udev verbosity and share?

How do I do that?

> All the kmod-builtin
> calls will already be logged there. See
> src/udev/udev-event.c:udev_event_execute_run() leading to
> 
> 	log_device_debug(event->dev, "Running built-in command \"%s\"", command);
> 	r = udev_builtin_run(event->dev, &event->rtnl, builtin_cmd, command, false);
> 
> if you are rather seeing "Running command", ohh... then your udev was
> built without libkmod and it will just fork/exec. Not what we want.

I'm using debian testing everything vanilla packages except the kernel,
using modules-next.

> so it seems the easiest thing to do is collect the udev log.
> 
> hopefully you don't have CONFIG_UEVENT_HELPER_PATH set or anything
> mucking /sys/kernel/uevent_helper. Right?

No.

  Luis