linux-kernel - Re: [PATCH RESEND] init/initramfs.c: make initramfs support pivot

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20210525014304.GH4332@42.do-not-panic.com>
Date:   Tue, 25 May 2021 01:43:04 +0000
From:   Luis Chamberlain <mcgrof@...nel.org>
To:     Menglong Dong <menglong8.dong@...il.com>
Cc:     Jan Kara <jack@...e.cz>, Jens Axboe <axboe@...nel.dk>,
        hare@...e.de, gregkh@...uxfoundation.org, tj@...nel.org,
        Menglong Dong <dong.menglong@....com.cn>, song@...nel.org,
        NeilBrown <neilb@...e.de>,
        Andrew Morton <akpm@...ux-foundation.org>,
        wangkefeng.wang@...wei.com, f.fainelli@...il.com, arnd@...db.de,
        Barret Rhoden <brho@...gle.com>,
        Rasmus Villemoes <linux@...musvillemoes.dk>,
        mhiramat@...nel.org, Steven Rostedt <rostedt@...dmis.org>,
        Kees Cook <keescook@...omium.org>, vbabka@...e.cz,
        Alexander Potapenko <glider@...gle.com>, pmladek@...e.com,
        Chris Down <chris@...isdown.name>, ebiederm@...ssion.com,
        jojing64@...il.com, LKML <linux-kernel@...r.kernel.org>,
        palmerdabbelt@...gle.com, linux-fsdevel@...r.kernel.org,
        Alexander Viro <viro@...iv.linux.org.uk>
Subject: Re: [PATCH RESEND] init/initramfs.c: make initramfs support
 pivot_root

On Tue, May 25, 2021 at 08:55:48AM +0800, Menglong Dong wrote:
> On Tue, May 25, 2021 at 6:58 AM Luis Chamberlain <mcgrof@...nel.org> wrote:
> >
> > However, if you introduce it as a kconfig option so that users
> > who want to use this new feature can enable it, and then use it,
> > the its sold as a new feature.
> >
> > Should this always be enabled, or done this way? Should we never have
> > the option to revert back to the old behaviour? If not, why not?
> >
> 
> This change seems transparent to users, which don't change the behavior
> of initramfs. 

Are we sure there nothing in the kernel that can regress with this
change? Are you sure? How sure?

> However, it seems more reasonable to make it a kconfig option.
> I'll do it in the v2 of the three patches I sended.

I'm actually quite convinced now this is a desirable default *other*
than the concern if this could regress. I recently saw some piece of
code fetching for the top most mount, I think it was on the
copy_user_ns() path or something like that, which made me just
consider possible regressions for heuristics we might have forgotten
about.

I however have't yet had time to review the path I was concerned for
yet.

> > What do you mean? init_mount_tree() is always called, and it has
> > statically:
> >
> > static void __init init_mount_tree(void)
> > {
> >         struct vfsmount *mnt;
> >         ...
> >         mnt = vfs_kern_mount(&rootfs_fs_type, 0, "rootfs", NULL);
> >         ...
> > }
> >
> > And as I noted, this is *always* called earlier than
> > do_populate_rootfs(). Your changes did not remove the init_mount_tree()
> > or modify it, and so why would the context of the above call always
> > be OK to be used now with a ramfs context now?
> >
> > > So it makes no sense to make the file system of the first mount selectable.
> >
> > Why? I don't see why, nor is it explained, we're always caling
> > vfs_kern_mount(&rootfs_fs_type, ...) and you have not changed that
> > either.
> >
> > > To simplify the code here, I make it ramfs_init_fs_context directly. In fact,
> > > it's fine to make it shmen_init_fs_context here too.
> >
> > So indeed you're suggesting its arbitrary now.... I don't see why.
> >
> 
> So the biggest problem now seems to be the first mount I changed, maybe I didn't
> make it clear before.
> 
> Let's call the first mount which is created in init_mount_tree() the
> 'init_mount'.
> If the 'root' is a block fs, initrd or nfs, the 'init_mount' will be a
> ramfs, that seems
> clear, it can be seen from the enable of tmpfs:
> 
> void __init init_rootfs(void)
> {
>     if (IS_ENABLED(CONFIG_TMPFS) && !saved_root_name[0] &&
>        (!root_fs_names || strstr(root_fs_names, "tmpfs")))
>        is_tmpfs = true;
> }

Ah yes, I see now... Thanks!
 
  Luis