[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ZELZwms4wrc419gt@bombadil.infradead.org>
Date: Fri, 21 Apr 2023 11:45:22 -0700
From: Luis Chamberlain <mcgrof@...nel.org>
To: Lucas De Marchi <lucas.demarchi@...el.com>
Cc: Greg KH <gregkh@...uxfoundation.org>, david@...hat.com,
patches@...ts.linux.dev, linux-modules@...r.kernel.org,
linux-mm@...ck.org, linux-kernel@...r.kernel.org, pmladek@...e.com,
petr.pavlu@...e.com, prarit@...hat.com,
torvalds@...ux-foundation.org, rafael@...nel.org,
christophe.leroy@...roup.eu, tglx@...utronix.de,
peterz@...radead.org, song@...nel.org, rppt@...nel.org,
dave@...olabs.net, willy@...radead.org, vbabka@...e.cz,
mhocko@...e.com, dave.hansen@...ux.intel.com,
colin.i.king@...il.com, jim.cromie@...il.com,
catalin.marinas@....com, jbaron@...mai.com,
rick.p.edgecombe@...el.com, j.granados@...sung.com
Subject: Re: [PATCH] module: add debugging auto-load duplicate module support
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
Powered by blists - more mailing lists