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

[Al Viro <viro@xxxxxxxxxxxxxxxxxx>]

On Tue, May 06, 2025 at 07:54:32PM +0200, Klara Modin wrote:

> > > Though now I hit another issue which I don't know if it's related or
> > > not. I'm using an overlay mount with squashfs as lower and btrfs as
> > > upper. The mount fails with invalid argument and I see this in the log:
> > > 
> > > overlayfs: failed to clone upperpath
> > 
> > Seeing that you already have a kernel with that thing reverted, could
> > you check if the problem exists there?
> 
> Yeah, it works fine with the revert instead.

Interesting...  That message means that you've got clone_private_mount()
returning an error; the thing is, mount passed to it has come from
pathname lookup - it is *not* the mount created by that fc_mount() of
vfs_create_mount() in the modified code.  That one gets passed to
mount_subvol() and consumed there (by mount_subtree()).  All that is returned
is root dentry; the mount passed to clone_private_mount() is created
from scratch using dentry left by btrfs_get_tree_subvol() in its fc->root -
see
        dentry = mount_subvol(ctx->subvol_name, ctx->subvol_objectid, mnt);
        ctx->subvol_name = NULL;
        if (IS_ERR(dentry))
                return PTR_ERR(dentry);

        fc->root = dentry;
        return 0;
in the end of btrfs_get_tree_subvol().

What's more, on the overlayfs side we managed to get to
        upper_mnt = clone_private_mount(upperpath);
        err = PTR_ERR(upper_mnt);
        if (IS_ERR(upper_mnt)) {
                pr_err("failed to clone upperpath\n");
                goto out;
so the upper path had been resolved...

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...

diff --git a/fs/namespace.c b/fs/namespace.c
index eb990e9a668a..f7ce53f437dd 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -2480,31 +2480,45 @@ struct vfsmount *clone_private_mount(const struct path *path)
 
 	guard(rwsem_read)(&namespace_sem);
 
-	if (IS_MNT_UNBINDABLE(old_mnt))
+	if (IS_MNT_UNBINDABLE(old_mnt)) {
+		pr_err("unbindable");
 		return ERR_PTR(-EINVAL);
+	}
 
 	if (mnt_has_parent(old_mnt)) {
-		if (!check_mnt(old_mnt))
+		if (!check_mnt(old_mnt)) {
+			pr_err("mounted, but not in our namespace");
 			return ERR_PTR(-EINVAL);
+		}
 	} else {
-		if (!is_mounted(&old_mnt->mnt))
+		if (!is_mounted(&old_mnt->mnt)) {
+			pr_err("not mounted");
 			return ERR_PTR(-EINVAL);
+		}
 
 		/* Make sure this isn't something purely kernel internal. */
-		if (!is_anon_ns(old_mnt->mnt_ns))
+		if (!is_anon_ns(old_mnt->mnt_ns)) {
+			pr_err("kern_mount?");
 			return ERR_PTR(-EINVAL);
+		}
 
 		/* Make sure we don't create mount namespace loops. */
-		if (!check_for_nsfs_mounts(old_mnt))
+		if (!check_for_nsfs_mounts(old_mnt)) {
+			pr_err("shite with nsfs");
 			return ERR_PTR(-EINVAL);
+		}
 	}
 
-	if (has_locked_children(old_mnt, path->dentry))
+	if (has_locked_children(old_mnt, path->dentry)) {
+		pr_err("has locked children");
 		return ERR_PTR(-EINVAL);
+	}
 
 	new_mnt = clone_mnt(old_mnt, path->dentry, CL_PRIVATE);
-	if (IS_ERR(new_mnt))
+	if (IS_ERR(new_mnt)) {
+		pr_err("clone_mnt failed (%ld)", PTR_ERR(new_mnt));
 		return ERR_PTR(-EINVAL);
+	}
 
 	/* Longterm mount to be removed by kern_unmount*() */
 	new_mnt->mnt_ns = MNT_NS_INTERNAL;