On Tue, Sep 07, 2021 at 08:00:03PM -0700, Andrew Morton wrote:
> From: Rasmus Villemoes <linux@xxxxxxxxxxxxxxxxxx>
> Subject: init: move usermodehelper_enable() to populate_rootfs()
>
> Currently, usermodehelper is enabled right before PID1 starts going
> through the initcalls. However, any call of a usermodehelper from a
> pure_, core_, postcore_, arch_, subsys_ or fs_ initcall is futile, as
> there is no filesystem contents yet.
>
> Up until commit e7cb072eb988 ("init/initramfs.c: do unpacking
> asynchronously"), such calls, whether via some request_module(), a
> legacy uevent "/sbin/hotplug" notification or something else, would
> just fail silently with (presumably) -ENOENT from
> kernel_execve(). However, that commit introduced the
> wait_for_initramfs() synchronization hook which must be called from
> the usermodehelper exec path right before the kernel_execve, in order
> that request_module() et al done from *after* rootfs_initcall()
> time (i.e. device_ and late_ initcalls) would continue to find a
> populated initramfs as they used to.
>
> Any call of wait_for_initramfs() done before the unpacking has been
> scheduled (i.e. before rootfs_initcall time) must just return
> immediately [and let the caller find an empty file system] in order
> not to deadlock the machine. I mistakenly thought, and my limited
> testing confirmed, that there were no such calls, so I added a
> pr_warn_once() in wait_for_initramfs(). It turns out that one can
> indeed hit request_module() as well as kobject_uevent_env() during
> those early init calls, leading to a user-visible warning in the
> kernel log emitted consistently for certain configurations.
Further proof that the semantics for init is still loose. Formalizing
dependencies on init is something we should strive to. Eventualy with a
DAG. The linker-tables work I had done years ago strived to get us
there which allows us to get a simple explicit DAG through the linker.
Unfortunately that patch set fell through because folks were
more interested in questioning the alternative side benefits of
linker-tables, but the use-case for helping with init is still valid.
If we *do* want to resurrect this folks should let me know.
Since the kobject_uevent_env() interest here is for /sbin/hotplug and
that crap is deprecated, in practice the relevant calls we'd care about
are the request_module() calls.
> We could just remove the pr_warn_once(), but I think it's better to
> postpone enabling the usermodehelper framework until there is at least
> some chance of finding the executable. That is also a little more
> efficient in that a lot of work done in umh.c will be elided.
I *don't* think we were aware that such request_module() calls were
happening before the fs was even ready and failing silently with
-ENOENT. As such, although moving the usermodehelper_enable()
to right after scheduling populating the rootfs is the right thing,
we do loose on the opportunity to learn who were those odd callers
before. We could not care... but this is also a missed opportunity
in finding those. How important that is, is not clear to me as
this was silently failing before...
If we wanted to keep a print for the above purpose though, we'd likely
want the full stack trace to see who the hell made the call.
> However,
> it does change the error seen by those early callers from -ENOENT to
> -EBUSY, so there is a risk of a regression if any caller care about
> the exact error value.
I'd see this as a welcomed evolution as it tells us more: we're saying
"it's coming, try again" or whatever.
A debug option to allow us to get a full warning trace in the -EBUSY
case on early init would be nice to have.
Otherwise:
Reviewed-by: Luis Chamberlain <mcgrof@xxxxxxxxxx>
Luis
> Link: https://lkml.kernel.org/r/20210728134638.329060-1-linux@xxxxxxxxxxxxxxxxxx
> Fixes: e7cb072eb988 ("init/initramfs.c: do unpacking asynchronously")
> Signed-off-by: Rasmus Villemoes <linux@xxxxxxxxxxxxxxxxxx>
> Reported-by: Alexander Egorenkov <egorenar@xxxxxxxxxxxxx>
> Reported-by: Bruno Goncalves <bgoncalv@xxxxxxxxxx>
> Reported-by: Heiner Kallweit <hkallweit1@xxxxxxxxx>
> Cc: Luis Chamberlain <mcgrof@xxxxxxxxxx>
> Signed-off-by: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> ---
>
> init/initramfs.c | 2 ++
> init/main.c | 1 -
> init/noinitramfs.c | 2 ++
> 3 files changed, 4 insertions(+), 1 deletion(-)
>
> --- a/init/initramfs.c~init-move-usermodehelper_enable-to-populate_rootfs
> +++ a/init/initramfs.c
> @@ -15,6 +15,7 @@
> #include <linux/mm.h>
> #include <linux/namei.h>
> #include <linux/init_syscalls.h>
> +#include <linux/umh.h>
>
> static ssize_t __init xwrite(struct file *file, const char *p, size_t count,
> loff_t *pos)
> @@ -727,6 +728,7 @@ static int __init populate_rootfs(void)
> {
> initramfs_cookie = async_schedule_domain(do_populate_rootfs, NULL,
> &initramfs_domain);
> + usermodehelper_enable();
> if (!initramfs_async)
> wait_for_initramfs();
> return 0;
> --- a/init/main.c~init-move-usermodehelper_enable-to-populate_rootfs
> +++ a/init/main.c
> @@ -1392,7 +1392,6 @@ static void __init do_basic_setup(void)
> driver_init();
> init_irq_proc();
> do_ctors();
> - usermodehelper_enable();
> do_initcalls();
> }
>
> --- a/init/noinitramfs.c~init-move-usermodehelper_enable-to-populate_rootfs
> +++ a/init/noinitramfs.c
> @@ -10,6 +10,7 @@
> #include <linux/kdev_t.h>
> #include <linux/syscalls.h>
> #include <linux/init_syscalls.h>
> +#include <linux/umh.h>
>
> /*
> * Create a simple rootfs that is similar to the default initramfs
> @@ -18,6 +19,7 @@ static int __init default_rootfs(void)
> {
> int err;
>
> + usermodehelper_enable();
> err = init_mkdir("/dev", 0755);
> if (err < 0)
> goto out;
> _
