Re: [RFC][PATCH] btrfs_get_tree_subvol(): switch from fc_mount() to vfs_create_mount()

[Al Viro <viro@zeniv.linux.org.uk>]

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.

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.

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.