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@xxxxxxxxxxxxxxxxxxxxxx/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
