On 2025-05-06 20:33:05 +0100, Al Viro wrote:
> On Tue, May 06, 2025 at 08:58:59PM +0200, Klara Modin wrote:
> > > OK, let's try to see what clone_private_mount() is unhappy about...
> > > Could you try the following on top of -next + braino fix and see
> > > what shows up? Another interesting thing, assuming you can get
> > > to shell after overlayfs mount failure, would be /proc/self/mountinfo
> > > contents and stat(1) output for upper path of your overlayfs mount...
> >
> > ret = vfs_get_tree(dup_fc);
> > if (!ret) {
> > ret = btrfs_reconfigure_for_mount(dup_fc);
> > up_write(&dup_fc->root->d_sb->s_umount);
> > }
> > if (!ret)
> > mnt = vfs_create_mount(fc);
> > else
> > mnt = ERR_PTR(ret);
> > put_fs_context(dup_fc);
> >
> >
> > I tried replacing fc with dup_fc in vfs_create_mount and it seems to
> > work.
>
> *blink*
>
> OK, I'm a blind idiot - blind for not seeing the braino duplication,
> idiot for not thinking to check if the same thing has happened
> more than once.
No worries, thanks for taking the time to debug.
>
> Kudos for catching that. I still wonder what the hell got passed
> to overlayfs, though - vfs_create_mount() should've hit
> if (!fc->root)
> return ERR_PTR(-EINVAL);
> since fc->root definitely was NULL there. So we should've gotten
> a failing btrfs mount; fine, but that does not explain the form
> of breakage you are seeing on the overlayfs side... Actually...
> is that mount attempted while still on initramfs? Because if
> it is, we are running into a separate clone_private_mount()
> bug.
>
> There's nothing to prohibit an overlayfs mount with writable layer
> being a subtree of initramfs; odd, but nothing inherently wrong
> with that setup. And prior to that clone_private_mount() change
> it used to be fine; we would get a private mount with root
> pointing to subtree of initramfs and went on to use that.
>
Yep, this is still within the initramfs. I later switch_root into the
overlay.
> We used to require the original mount to be in our namespace;
> Christian's change added "... or root of anon namespace".
> The order of checks went wrong, though. We check for "is
> it an absolute root" *first* and treat that as a discriminator
> between the new and old cases. It should be the other way
> round - "is it in our namespace" should take precedence.
>
> IOW,
> if (!check_mount(...)) { // if it's not ours...
> // ... it should be root...
> if (mnt_has_parent(...))
> fail
> // ... of anon namespace...
> if (!is_mounted(...) || !is_anon_ns(...))
> fail
> // ... and create no namespace loops -
> // or no hidden references to namespaces, period
> if (mnt_ns_loop(...)) // or, perhaps, if (mnt_ns_from_dentry(...))
> fail
> }
> Anyway, that's a separate issue.
