[PATCH RFC 0/3] fs: allow userspace to detect superblock reuse

[PATCH RFC 0/3] fs: allow userspace to detect superblock reuse

[Christian Brauner]

Summary
=======

This introduces FSCONFIG_CMD_CREATE_EXCL which will allows userspace to
implement something like mount -t ext4 --exclusive /dev/sda /B which
fails if a superblock for the requested filesystem does already exist:

Before this patch
-----------------

$ sudo ./move-mount -f xfs -o source=/dev/sda4 /A
Requesting filesystem type xfs
Mount options requested: source=/dev/sda4
Attaching mount at /A
Moving single attached mount
Setting key(source) with val(/dev/sda4)

$ sudo ./move-mount -f xfs -o source=/dev/sda4 /B
Requesting filesystem type xfs
Mount options requested: source=/dev/sda4
Attaching mount at /B
Moving single attached mount
Setting key(source) with val(/dev/sda4)

After this patch with --exclusive as a switch for FSCONFIG_CMD_CREATE_EXCL
--------------------------------------------------------------------------

$ sudo ./move-mount -f xfs --exclusive -o source=/dev/sda4 /A
Requesting filesystem type xfs
Request exclusive superblock creation
Mount options requested: source=/dev/sda4
Attaching mount at /A
Moving single attached mount
Setting key(source) with val(/dev/sda4)

$ sudo ./move-mount -f xfs --exclusive -o source=/dev/sda4 /B
Requesting filesystem type xfs
Request exclusive superblock creation
Mount options requested: source=/dev/sda4
Attaching mount at /B
Moving single attached mount
Setting key(source) with val(/dev/sda4)
Device or resource busy | move-mount.c: 300: do_fsconfig: i xfs: reusing existing superblock not allowed

Details
=======

As mentioned on the list (cf. [1]-[3]) mount requests like
mount -t ext4 /dev/sda /A are ambigous for userspace. Either a new
superblock has been created and mounted or an existing superblock has
been reused and a bind-mount has been created.

This becomes clear if we consider two processes creating the same mount
for the same block device:

P1                                                              P2
fd_fs = fsopen("ext4");                                         fd_fs = fsopen("ext4");
fsconfig(fd_fs, FSCONFIG_SET_STRING, "source", "/dev/sda");     fsconfig(fd_fs, FSCONFIG_SET_STRING, "source", "/dev/sda");
fsconfig(fd_fs, FSCONFIG_SET_STRING, "dax", "always");          fsconfig(fd_fs, FSCONFIG_SET_STRING, "resuid", "1000");

// wins and creates superblock
fsconfig(fd_fs, FSCONFIG_CMD_CREATE, ...)
                                                                // finds compatible superblock of P1
                                                                // spins until P1 sets SB_BORN and grabs a reference
                                                                fsconfig(fd_fs, FSCONFIG_CMD_CREATE, ...)

fd_mnt1 = fsmount(fd_fs);                                       fd_mnt2 = fsmount(fd_fs);
move_mount(fd_mnt1, "/A")                                       move_mount(fd_mnt2, "/B")

Not just does P2 get a bind-mount for nearly all filesystems the mount
options for P2 are usually completely ignored. The VFS itself doesn't
and shouldn't enforce filesystem specific mount option compatibility. It
only enforces incompatibility for read-only <-> read-write transitions:

mount -t ext4       /dev/sda /A
mount -t ext4 -o ro /dev/sda /B

The read-only request will fail with EBUSY as the VFS can't just
silently transition a superblock from read-write to read-only or vica
versa without risking security issues.

To userspace this silent superblock reuse can be security issue in
certain circumstances because there is currently no simple way for them
to know that they did indeed manage to create the superblock and didn't
just reuse an existing one.

This adds a new FSCONFIG_CMD_CREATE_EXCL command to fsconfig() that
returns EBUSY if an existing superblock is found. Userspace that needs
to be sure that they did create the superblock with the requested mount
options can request superblock creation using this command. If it
succeeds they can be sure that they did create the superblock with the
requested mount options.

This requires the new mount api. With the old mount api we would have to
plumb this through every legacy filesystem's file_system_type->mount()
method. If they want this feature they are most welcome to switch to the
new mount api.

Following is an analysis of the effect of FSCONFIG_CMD_CREATE_EXCL on
each high-level superblock creation helper:

(1) get_tree_nodev()

    Always allocate new superblock. Hence, FSCONFIG_CMD_CREATE and
    FSCONFIG_CMD_CREATE_EXCL are equivalent.

    The binderfs or overlayfs filesystems are examples.

(4) get_tree_keyed()

    Finds an existing superblock based on sb->s_fs_info. Hence,
    FSCONFIG_CMD_CREATE would reuse an existing superblock whereas
    FSCONFIG_CMD_CREATE_EXCL would reject it with EBUSY.

    The mqueue or nfsd filesystems are examples.

(2) get_tree_bdev()

    This effectively works like get_tree_keyed().

    The ext4 or xfs filesystems are examples.

(3) get_tree_single()

    Only one superblock of this filesystem type can ever exist.
    Hence, FSCONFIG_CMD_CREATE would reuse an existing superblock
    whereas FSCONFIG_CMD_CREATE_EXCL would reject it with EBUSY.

    The securityfs or configfs filesystems are examples.

    This has a further consequence. Some filesystems will never destroy
    the superblock once it has been created. For example, if securityfs
    is mounted the allocated superblock will never be destroyed again as
    long as there is still an LSM making use it. Consequently, even if
    securityfs is unmounted and seemingly destroyed it really isn't
    which means that FSCONFIG_CMD_CREATE_EXCL will continue rejecting
    reusing the existing superblock.

    This is unintuitive but not a problem. Such special purpose
    filesystems aren't mounted multiple times anyway.

Following is an analysis of the effect of FSCONFIG_CMD_CREATE_EXCL on
filesystems that make use of the low-level sget_fc() helper directly.
They're all effectively variants on get_tree_keyed() or get_tree_bdev():

(5) mtd_get_sb()

    Similar logic to get_tree_keyed().

(6) afs_get_tree()

    Similar logic to get_tree_keyed().

(7) ceph_get_tree()

    Similar logic to get_tree_keyed().

    Already explicitly allows forcing the allocation of a new superblock
    via CEPH_OPT_NOSHARE. This turns it into get_tree_nodev().

(8) fuse_get_tree_submount()

    Similar logic to get_tree_nodev().

(9) fuse_get_tree()

    Forces reuse of existing FUSE superblock.

    Forces reuse of existing superblock if passed in file refers to an
    existing FUSE connection.
    If FSCONFIG_CMD_CREATE_EXCL is specified together with an fd
    referring to an existing FUSE connections this would cause the
    superblock reusal to fail. If reusing is the intent then
    FSCONFIG_CMD_CREATE_EXCL shouldn't be specified.

(10) fuse_get_tree()
     -> get_tree_nodev()

    Same logic as in get_tree_nodev().

(11) fuse_get_tree()
     -> get_tree_bdev()

    Same logic as in get_tree_bdev().

(12) virtio_fs_get_tree()

     Same logic as get_tree_keyed().

(13) gfs2_meta_get_tree()

     Forces reuse of existing gfs2 superblock.

     Mounting gfs2meta enforces that a gf2s superblock must already
     exist. If not, it will error out. Consequently, mounting gfs2meta
     with FSCONFIG_CMD_CREATE_EXCL would always fail. If reusing is the
     intent then FSCONFIG_CMD_CREATE_EXCL shouldn't be specified.

(14) kernfs_get_tree()

     Similar logic to get_tree_keyed().

(15) nfs_get_tree_common()

    Similar logic to get_tree_keyed().

    Already explicitly allows forcing the allocation of a new superblock
    via NFS_MOUNT_UNSHARED. This effectively turns it into
    get_tree_nodev().

Link: [1] https://lore.kernel.org/linux-block/20230704-fasching-wertarbeit-7c6ffb01c83d@brauner
Link: [2] https://lore.kernel.org/linux-block/20230705-pumpwerk-vielversprechend-a4b1fd947b65@brauner
Link: [3] https://lore.kernel.org/linux-fsdevel/20230725-einnahmen-warnschilder-17779aec0a97@brauner

---



---
base-commit: 1dbd9ceb390c4c61d28cf2c9251dd2015946ce51
change-id: 20230801-vfs-super-exclusive-c96146d24b29

[PATCH RFC 1/3] super: remove get_tree_single_reconf()

[Christian Brauner]

The get_tree_single_reconf() helper isn't used anywhere. Remote it.

Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 fs/super.c                 | 34 ++++++++--------------------------
 include/linux/fs_context.h |  3 ---
 2 files changed, 8 insertions(+), 29 deletions(-)

diff --git a/fs/super.c b/fs/super.c
index 3ef39df5bec5..c086ce4929cd 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -1136,10 +1136,10 @@ static int test_single_super(struct super_block *s, struct fs_context *fc)
 	return 1;
 }
 
-static int vfs_get_super(struct fs_context *fc, bool reconf,
-		int (*test)(struct super_block *, struct fs_context *),
-		int (*fill_super)(struct super_block *sb,
-				  struct fs_context *fc))
+static int vfs_get_super(struct fs_context *fc,
+			 int (*test)(struct super_block *, struct fs_context *),
+			 int (*fill_super)(struct super_block *sb,
+					   struct fs_context *fc))
 {
 	struct super_block *sb;
 	int err;
@@ -1154,19 +1154,9 @@ static int vfs_get_super(struct fs_context *fc, bool reconf,
 			goto error;
 
 		sb->s_flags |= SB_ACTIVE;
-		fc->root = dget(sb->s_root);
-	} else {
-		fc->root = dget(sb->s_root);
-		if (reconf) {
-			err = reconfigure_super(fc);
-			if (err < 0) {
-				dput(fc->root);
-				fc->root = NULL;
-				goto error;
-			}
-		}
 	}
 
+	fc->root = dget(sb->s_root);
 	return 0;
 
 error:
@@ -1178,7 +1168,7 @@ int get_tree_nodev(struct fs_context *fc,
 		  int (*fill_super)(struct super_block *sb,
 				    struct fs_context *fc))
 {
-	return vfs_get_super(fc, false, NULL, fill_super);
+	return vfs_get_super(fc, NULL, fill_super);
 }
 EXPORT_SYMBOL(get_tree_nodev);
 
@@ -1186,25 +1176,17 @@ int get_tree_single(struct fs_context *fc,
 		  int (*fill_super)(struct super_block *sb,
 				    struct fs_context *fc))
 {
-	return vfs_get_super(fc, false, test_single_super, fill_super);
+	return vfs_get_super(fc, test_single_super, fill_super);
 }
 EXPORT_SYMBOL(get_tree_single);
 
-int get_tree_single_reconf(struct fs_context *fc,
-		  int (*fill_super)(struct super_block *sb,
-				    struct fs_context *fc))
-{
-	return vfs_get_super(fc, true, test_single_super, fill_super);
-}
-EXPORT_SYMBOL(get_tree_single_reconf);
-
 int get_tree_keyed(struct fs_context *fc,
 		  int (*fill_super)(struct super_block *sb,
 				    struct fs_context *fc),
 		void *key)
 {
 	fc->s_fs_info = key;
-	return vfs_get_super(fc, false, test_keyed_super, fill_super);
+	return vfs_get_super(fc, test_keyed_super, fill_super);
 }
 EXPORT_SYMBOL(get_tree_keyed);
 
diff --git a/include/linux/fs_context.h b/include/linux/fs_context.h
index ff6341e09925..851b3fe2549c 100644
--- a/include/linux/fs_context.h
+++ b/include/linux/fs_context.h
@@ -150,9 +150,6 @@ extern int get_tree_nodev(struct fs_context *fc,
 extern int get_tree_single(struct fs_context *fc,
 			 int (*fill_super)(struct super_block *sb,
 					   struct fs_context *fc));
-extern int get_tree_single_reconf(struct fs_context *fc,
-			 int (*fill_super)(struct super_block *sb,
-					   struct fs_context *fc));
 extern int get_tree_keyed(struct fs_context *fc,
 			 int (*fill_super)(struct super_block *sb,
 					   struct fs_context *fc),

-- 
2.34.1

[PATCH RFC 2/3] fs: add vfs_cmd_create()

[Christian Brauner]

Split the steps to create a superblock into a tiny helper. This will
make the next patch easier to follow.

Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 fs/fsopen.c | 45 +++++++++++++++++++++++++++++++--------------
 1 file changed, 31 insertions(+), 14 deletions(-)

diff --git a/fs/fsopen.c b/fs/fsopen.c
index fc9d2d9fd234..af2ff05dcee5 100644
--- a/fs/fsopen.c
+++ b/fs/fsopen.c
@@ -209,6 +209,36 @@ SYSCALL_DEFINE3(fspick, int, dfd, const char __user *, path, unsigned int, flags
 	return ret;
 }
 
+static int vfs_cmd_create(struct fs_context *fc)
+{
+	struct super_block *sb;
+	int ret;
+
+	if (fc->phase != FS_CONTEXT_CREATE_PARAMS)
+		return -EBUSY;
+
+	if (!mount_capable(fc))
+		return -EPERM;
+
+	fc->phase = FS_CONTEXT_CREATING;
+
+	ret = vfs_get_tree(fc);
+	if (ret)
+		return ret;
+
+	sb = fc->root->d_sb;
+	ret = security_sb_kern_mount(sb);
+	if (unlikely(ret)) {
+		fc_drop_locked(fc);
+		return ret;
+	}
+
+	/* vfs_get_tree() callchains will have grabbed @s_umount */
+	up_write(&sb->s_umount);
+	fc->phase = FS_CONTEXT_AWAITING_MOUNT;
+	return 0;
+}
+
 /*
  * Check the state and apply the configuration.  Note that this function is
  * allowed to 'steal' the value by setting param->xxx to NULL before returning.
@@ -224,22 +254,9 @@ static int vfs_fsconfig_locked(struct fs_context *fc, int cmd,
 		return ret;
 	switch (cmd) {
 	case FSCONFIG_CMD_CREATE:
-		if (fc->phase != FS_CONTEXT_CREATE_PARAMS)
-			return -EBUSY;
-		if (!mount_capable(fc))
-			return -EPERM;
-		fc->phase = FS_CONTEXT_CREATING;
-		ret = vfs_get_tree(fc);
+		ret = vfs_cmd_create(fc);
 		if (ret)
 			break;
-		sb = fc->root->d_sb;
-		ret = security_sb_kern_mount(sb);
-		if (unlikely(ret)) {
-			fc_drop_locked(fc);
-			break;
-		}
-		up_write(&sb->s_umount);
-		fc->phase = FS_CONTEXT_AWAITING_MOUNT;
 		return 0;
 	case FSCONFIG_CMD_RECONFIGURE:
 		if (fc->phase != FS_CONTEXT_RECONF_PARAMS)

-- 
2.34.1

[PATCH RFC 3/3] fs: add FSCONFIG_CMD_CREATE_EXCL

[Christian Brauner]

Summary
=======

This introduces FSCONFIG_CMD_CREATE_EXCL which will allows userspace to
implement something like mount -t ext4 --exclusive /dev/sda /B which
fails if a superblock for the requested filesystem does already exist:

Before this patch
-----------------

$ sudo ./move-mount -f xfs -o source=/dev/sda4 /A
Requesting filesystem type xfs
Mount options requested: source=/dev/sda4
Attaching mount at /A
Moving single attached mount
Setting key(source) with val(/dev/sda4)

$ sudo ./move-mount -f xfs -o source=/dev/sda4 /B
Requesting filesystem type xfs
Mount options requested: source=/dev/sda4
Attaching mount at /B
Moving single attached mount
Setting key(source) with val(/dev/sda4)

After this patch with --exclusive as a switch for FSCONFIG_CMD_CREATE_EXCL
--------------------------------------------------------------------------

$ sudo ./move-mount -f xfs --exclusive -o source=/dev/sda4 /A
Requesting filesystem type xfs
Request exclusive superblock creation
Mount options requested: source=/dev/sda4
Attaching mount at /A
Moving single attached mount
Setting key(source) with val(/dev/sda4)

$ sudo ./move-mount -f xfs --exclusive -o source=/dev/sda4 /B
Requesting filesystem type xfs
Request exclusive superblock creation
Mount options requested: source=/dev/sda4
Attaching mount at /B
Moving single attached mount
Setting key(source) with val(/dev/sda4)
Device or resource busy | move-mount.c: 300: do_fsconfig: i xfs: reusing existing superblock not allowed

Details
=======

As mentioned on the list (cf. [1]-[3]) mount requests like
mount -t ext4 /dev/sda /A are ambigous for userspace. Either a new
superblock has been created and mounted or an existing superblock has
been reused and a bind-mount has been created.

This becomes clear if we consider two processes creating the same mount
for the same block device:

P1                                                              P2
fd_fs = fsopen("ext4");                                         fd_fs = fsopen("ext4");
fsconfig(fd_fs, FSCONFIG_SET_STRING, "source", "/dev/sda");     fsconfig(fd_fs, FSCONFIG_SET_STRING, "source", "/dev/sda");
fsconfig(fd_fs, FSCONFIG_SET_STRING, "dax", "always");          fsconfig(fd_fs, FSCONFIG_SET_STRING, "resuid", "1000");

// wins and creates superblock
fsconfig(fd_fs, FSCONFIG_CMD_CREATE, ...)
                                                                // finds compatible superblock of P1
                                                                // spins until P1 sets SB_BORN and grabs a reference
                                                                fsconfig(fd_fs, FSCONFIG_CMD_CREATE, ...)

fd_mnt1 = fsmount(fd_fs);                                       fd_mnt2 = fsmount(fd_fs);
move_mount(fd_mnt1, "/A")                                       move_mount(fd_mnt2, "/B")

Not just does P2 get a bind-mount for nearly all filesystems the mount
options for P2 are usually completely ignored. The VFS itself doesn't
and shouldn't enforce filesystem specific mount option compatibility. It
only enforces incompatibility for read-only <-> read-write transitions:

mount -t ext4       /dev/sda /A
mount -t ext4 -o ro /dev/sda /B

The read-only request will fail with EBUSY as the VFS can't just
silently transition a superblock from read-write to read-only or vica
versa without risking security issues.

To userspace this silent superblock reuse can be security issue in
certain circumstances because there is currently no simple way for them
to know that they did indeed manage to create the superblock and didn't
just reuse an existing one.

This adds a new FSCONFIG_CMD_CREATE_EXCL command to fsconfig() that
returns EBUSY if an existing superblock is found. Userspace that needs
to be sure that they did create the superblock with the requested mount
options can request superblock creation using this command. If it
succeeds they can be sure that they did create the superblock with the
requested mount options.

This requires the new mount api. With the old mount api we would have to
plumb this through every legacy filesystem's file_system_type->mount()
method. If they want this feature they are most welcome to switch to the
new mount api.

Following is an analysis of the effect of FSCONFIG_CMD_CREATE_EXCL on
each high-level superblock creation helper:

(1) get_tree_nodev()

    Always allocate new superblock. Hence, FSCONFIG_CMD_CREATE and
    FSCONFIG_CMD_CREATE_EXCL are equivalent.

    The binderfs or overlayfs filesystems are examples.

(4) get_tree_keyed()

    Finds an existing superblock based on sb->s_fs_info. Hence,
    FSCONFIG_CMD_CREATE would reuse an existing superblock whereas
    FSCONFIG_CMD_CREATE_EXCL would reject it with EBUSY.

    The mqueue or nfsd filesystems are examples.

(2) get_tree_bdev()

    This effectively works like get_tree_keyed().

    The ext4 or xfs filesystems are examples.

(3) get_tree_single()

    Only one superblock of this filesystem type can ever exist.
    Hence, FSCONFIG_CMD_CREATE would reuse an existing superblock
    whereas FSCONFIG_CMD_CREATE_EXCL would reject it with EBUSY.

    The securityfs or configfs filesystems are examples.

    This has a further consequence. Some filesystems will never destroy
    the superblock once it has been created. For example, if securityfs
    is mounted the allocated superblock will never be destroyed again as
    long as there is still an LSM making use it. Consequently, even if
    securityfs is unmounted and seemingly destroyed it really isn't
    which means that FSCONFIG_CMD_CREATE_EXCL will continue rejecting
    reusing the existing superblock.

    This is unintuitive but not a problem. Such special purpose
    filesystems aren't mounted multiple times anyway.

Following is an analysis of the effect of FSCONFIG_CMD_CREATE_EXCL on
filesystems that make use of the low-level sget_fc() helper directly.
They're all effectively variants on get_tree_keyed() or get_tree_bdev():

(5) mtd_get_sb()

    Similar logic to get_tree_keyed().

(6) afs_get_tree()

    Similar logic to get_tree_keyed().

(7) ceph_get_tree()

    Similar logic to get_tree_keyed().

    Already explicitly allows forcing the allocation of a new superblock
    via CEPH_OPT_NOSHARE. This turns it into get_tree_nodev().

(8) fuse_get_tree_submount()

    Similar logic to get_tree_nodev().

(9) fuse_get_tree()

    Forces reuse of existing FUSE superblock.

    Forces reuse of existing superblock if passed in file refers to an
    existing FUSE connection.
    If FSCONFIG_CMD_CREATE_EXCL is specified together with an fd
    referring to an existing FUSE connections this would cause the
    superblock reusal to fail. If reusing is the intent then
    FSCONFIG_CMD_CREATE_EXCL shouldn't be specified.

(10) fuse_get_tree()
     -> get_tree_nodev()

    Same logic as in get_tree_nodev().

(11) fuse_get_tree()
     -> get_tree_bdev()

    Same logic as in get_tree_bdev().

(12) virtio_fs_get_tree()

     Same logic as get_tree_keyed().

(13) gfs2_meta_get_tree()

     Forces reuse of existing gfs2 superblock.

     Mounting gfs2meta enforces that a gf2s superblock must already
     exist. If not, it will error out. Consequently, mounting gfs2meta
     with FSCONFIG_CMD_CREATE_EXCL would always fail. If reusing is the
     intent then FSCONFIG_CMD_CREATE_EXCL shouldn't be specified.

(14) kernfs_get_tree()

     Similar logic to get_tree_keyed().

(15) nfs_get_tree_common()

    Similar logic to get_tree_keyed().

    Already explicitly allows forcing the allocation of a new superblock
    via NFS_MOUNT_UNSHARED. This effectively turns it into
    get_tree_nodev().

Link: [1] https://lore.kernel.org/linux-block/20230704-fasching-wertarbeit-7c6ffb01c83d@brauner
Link: [2] https://lore.kernel.org/linux-block/20230705-pumpwerk-vielversprechend-a4b1fd947b65@brauner
Link: [3] https://lore.kernel.org/linux-fsdevel/20230725-einnahmen-warnschilder-17779aec0a97@brauner
Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 fs/fs_context.c            |  1 +
 fs/fsopen.c                | 18 +++++++++++++++---
 fs/super.c                 |  4 +++-
 include/linux/fs_context.h |  1 +
 include/uapi/linux/mount.h |  3 ++-
 5 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/fs/fs_context.c b/fs/fs_context.c
index 851214d1d013..30d82d2979af 100644
--- a/fs/fs_context.c
+++ b/fs/fs_context.c
@@ -692,6 +692,7 @@ void vfs_clean_context(struct fs_context *fc)
 	security_free_mnt_opts(&fc->security);
 	kfree(fc->source);
 	fc->source = NULL;
+	fc->exclusive = false;
 
 	fc->purpose = FS_CONTEXT_FOR_RECONFIGURE;
 	fc->phase = FS_CONTEXT_AWAITING_RECONF;
diff --git a/fs/fsopen.c b/fs/fsopen.c
index af2ff05dcee5..e510bc7983e8 100644
--- a/fs/fsopen.c
+++ b/fs/fsopen.c
@@ -209,7 +209,7 @@ SYSCALL_DEFINE3(fspick, int, dfd, const char __user *, path, unsigned int, flags
 	return ret;
 }
 
-static int vfs_cmd_create(struct fs_context *fc)
+static int vfs_cmd_create(struct fs_context *fc, bool exclusive)
 {
 	struct super_block *sb;
 	int ret;
@@ -220,16 +220,24 @@ static int vfs_cmd_create(struct fs_context *fc)
 	if (!mount_capable(fc))
 		return -EPERM;
 
+	/* require the new mount api */
+	if (exclusive && (fc->ops == &legacy_fs_context_ops))
+		return -EOPNOTSUPP;
+
 	fc->phase = FS_CONTEXT_CREATING;
+	fc->exclusive = exclusive;
 
 	ret = vfs_get_tree(fc);
-	if (ret)
+	if (ret) {
+		fc->exclusive = false;
 		return ret;
+	}
 
 	sb = fc->root->d_sb;
 	ret = security_sb_kern_mount(sb);
 	if (unlikely(ret)) {
 		fc_drop_locked(fc);
+		fc->exclusive = false;
 		return ret;
 	}
 
@@ -253,8 +261,10 @@ static int vfs_fsconfig_locked(struct fs_context *fc, int cmd,
 	if (ret)
 		return ret;
 	switch (cmd) {
+	case FSCONFIG_CMD_CREATE_EXCL:
+		fallthrough;
 	case FSCONFIG_CMD_CREATE:
-		ret = vfs_cmd_create(fc);
+		ret = vfs_cmd_create(fc, cmd == FSCONFIG_CMD_CREATE_EXCL);
 		if (ret)
 			break;
 		return 0;
@@ -369,6 +379,8 @@ SYSCALL_DEFINE5(fsconfig,
 		if (!_key || _value || aux < 0)
 			return -EINVAL;
 		break;
+	case FSCONFIG_CMD_CREATE_EXCL:
+		fallthrough;
 	case FSCONFIG_CMD_CREATE:
 	case FSCONFIG_CMD_RECONFIGURE:
 		if (_key || _value || aux)
diff --git a/fs/super.c b/fs/super.c
index c086ce4929cd..61dcd7922a10 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -603,9 +603,11 @@ struct super_block *sget_fc(struct fs_context *fc,
 	return s;
 
 share_extant_sb:
-	if (user_ns != old->s_user_ns) {
+	if (user_ns != old->s_user_ns || fc->exclusive) {
 		spin_unlock(&sb_lock);
 		destroy_unused_super(s);
+		if (fc->exclusive)
+			infofc(fc, "reusing existing superblock not allowed");
 		return ERR_PTR(-EBUSY);
 	}
 	if (!grab_super(old))
diff --git a/include/linux/fs_context.h b/include/linux/fs_context.h
index 851b3fe2549c..a33a3b1d9016 100644
--- a/include/linux/fs_context.h
+++ b/include/linux/fs_context.h
@@ -109,6 +109,7 @@ struct fs_context {
 	bool			need_free:1;	/* Need to call ops->free() */
 	bool			global:1;	/* Goes into &init_user_ns */
 	bool			oldapi:1;	/* Coming from mount(2) */
+	bool			exclusive:1;    /* create new superblock, reject existing one */
 };
 
 struct fs_context_operations {
diff --git a/include/uapi/linux/mount.h b/include/uapi/linux/mount.h
index 8eb0d7b758d2..bb242fdcfe6b 100644
--- a/include/uapi/linux/mount.h
+++ b/include/uapi/linux/mount.h
@@ -100,8 +100,9 @@ enum fsconfig_command {
 	FSCONFIG_SET_PATH	= 3,	/* Set parameter, supplying an object by path */
 	FSCONFIG_SET_PATH_EMPTY	= 4,	/* Set parameter, supplying an object by (empty) path */
 	FSCONFIG_SET_FD		= 5,	/* Set parameter, supplying an object by fd */
-	FSCONFIG_CMD_CREATE	= 6,	/* Invoke superblock creation */
+	FSCONFIG_CMD_CREATE	= 6,	/* Create new or reuse existing superblock */
 	FSCONFIG_CMD_RECONFIGURE = 7,	/* Invoke superblock reconfiguration */
+	FSCONFIG_CMD_CREATE_EXCL = 8,	/* Create new superblock, fail if reusing existing superblock */
 };
 
 /*

-- 
2.34.1

Re: [PATCH RFC 0/3] fs: allow userspace to detect superblock reuse

[Josef Bacik]

On Tue, Aug 01, 2023 at 03:08:59PM +0200, Christian Brauner wrote:
> Summary
> =======
> 
> This introduces FSCONFIG_CMD_CREATE_EXCL which will allows userspace to
> implement something like mount -t ext4 --exclusive /dev/sda /B which
> fails if a superblock for the requested filesystem does already exist:
> 
> Before this patch
> -----------------
> 
> $ sudo ./move-mount -f xfs -o source=/dev/sda4 /A
> Requesting filesystem type xfs
> Mount options requested: source=/dev/sda4
> Attaching mount at /A
> Moving single attached mount
> Setting key(source) with val(/dev/sda4)
> 
> $ sudo ./move-mount -f xfs -o source=/dev/sda4 /B
> Requesting filesystem type xfs
> Mount options requested: source=/dev/sda4
> Attaching mount at /B
> Moving single attached mount
> Setting key(source) with val(/dev/sda4)
> 
> After this patch with --exclusive as a switch for FSCONFIG_CMD_CREATE_EXCL
> --------------------------------------------------------------------------
> 
> $ sudo ./move-mount -f xfs --exclusive -o source=/dev/sda4 /A
> Requesting filesystem type xfs
> Request exclusive superblock creation
> Mount options requested: source=/dev/sda4
> Attaching mount at /A
> Moving single attached mount
> Setting key(source) with val(/dev/sda4)
> 
> $ sudo ./move-mount -f xfs --exclusive -o source=/dev/sda4 /B
> Requesting filesystem type xfs
> Request exclusive superblock creation
> Mount options requested: source=/dev/sda4
> Attaching mount at /B
> Moving single attached mount
> Setting key(source) with val(/dev/sda4)
> Device or resource busy | move-mount.c: 300: do_fsconfig: i xfs: reusing existing superblock not allowed
> 
> Details
> =======
> 
> As mentioned on the list (cf. [1]-[3]) mount requests like
> mount -t ext4 /dev/sda /A are ambigous for userspace. Either a new
> superblock has been created and mounted or an existing superblock has
> been reused and a bind-mount has been created.
> 
> This becomes clear if we consider two processes creating the same mount
> for the same block device:
> 
> P1                                                              P2
> fd_fs = fsopen("ext4");                                         fd_fs = fsopen("ext4");
> fsconfig(fd_fs, FSCONFIG_SET_STRING, "source", "/dev/sda");     fsconfig(fd_fs, FSCONFIG_SET_STRING, "source", "/dev/sda");
> fsconfig(fd_fs, FSCONFIG_SET_STRING, "dax", "always");          fsconfig(fd_fs, FSCONFIG_SET_STRING, "resuid", "1000");
> 
> // wins and creates superblock
> fsconfig(fd_fs, FSCONFIG_CMD_CREATE, ...)
>                                                                 // finds compatible superblock of P1
>                                                                 // spins until P1 sets SB_BORN and grabs a reference
>                                                                 fsconfig(fd_fs, FSCONFIG_CMD_CREATE, ...)
> 
> fd_mnt1 = fsmount(fd_fs);                                       fd_mnt2 = fsmount(fd_fs);
> move_mount(fd_mnt1, "/A")                                       move_mount(fd_mnt2, "/B")
> 
> Not just does P2 get a bind-mount for nearly all filesystems the mount
> options for P2 are usually completely ignored. The VFS itself doesn't
> and shouldn't enforce filesystem specific mount option compatibility. It
> only enforces incompatibility for read-only <-> read-write transitions:
> 
> mount -t ext4       /dev/sda /A
> mount -t ext4 -o ro /dev/sda /B
> 
> The read-only request will fail with EBUSY as the VFS can't just
> silently transition a superblock from read-write to read-only or vica
> versa without risking security issues.
> 
> To userspace this silent superblock reuse can be security issue in
> certain circumstances because there is currently no simple way for them
> to know that they did indeed manage to create the superblock and didn't
> just reuse an existing one.
> 
> This adds a new FSCONFIG_CMD_CREATE_EXCL command to fsconfig() that
> returns EBUSY if an existing superblock is found. Userspace that needs
> to be sure that they did create the superblock with the requested mount
> options can request superblock creation using this command. If it
> succeeds they can be sure that they did create the superblock with the
> requested mount options.
> 
> This requires the new mount api. With the old mount api we would have to
> plumb this through every legacy filesystem's file_system_type->mount()
> method. If they want this feature they are most welcome to switch to the
> new mount api.
> 
> Following is an analysis of the effect of FSCONFIG_CMD_CREATE_EXCL on
> each high-level superblock creation helper:
> 
> (1) get_tree_nodev()
> 
>     Always allocate new superblock. Hence, FSCONFIG_CMD_CREATE and
>     FSCONFIG_CMD_CREATE_EXCL are equivalent.
> 
>     The binderfs or overlayfs filesystems are examples.
> 
> (4) get_tree_keyed()
> 
>     Finds an existing superblock based on sb->s_fs_info. Hence,
>     FSCONFIG_CMD_CREATE would reuse an existing superblock whereas
>     FSCONFIG_CMD_CREATE_EXCL would reject it with EBUSY.
> 
>     The mqueue or nfsd filesystems are examples.
> 
> (2) get_tree_bdev()
> 
>     This effectively works like get_tree_keyed().
> 
>     The ext4 or xfs filesystems are examples.
> 
> (3) get_tree_single()
> 
>     Only one superblock of this filesystem type can ever exist.
>     Hence, FSCONFIG_CMD_CREATE would reuse an existing superblock
>     whereas FSCONFIG_CMD_CREATE_EXCL would reject it with EBUSY.
> 
>     The securityfs or configfs filesystems are examples.
> 
>     This has a further consequence. Some filesystems will never destroy
>     the superblock once it has been created. For example, if securityfs
>     is mounted the allocated superblock will never be destroyed again as
>     long as there is still an LSM making use it. Consequently, even if
>     securityfs is unmounted and seemingly destroyed it really isn't
>     which means that FSCONFIG_CMD_CREATE_EXCL will continue rejecting
>     reusing the existing superblock.
> 
>     This is unintuitive but not a problem. Such special purpose
>     filesystems aren't mounted multiple times anyway.
> 
> Following is an analysis of the effect of FSCONFIG_CMD_CREATE_EXCL on
> filesystems that make use of the low-level sget_fc() helper directly.
> They're all effectively variants on get_tree_keyed() or get_tree_bdev():
> 
> (5) mtd_get_sb()
> 
>     Similar logic to get_tree_keyed().
> 
> (6) afs_get_tree()
> 
>     Similar logic to get_tree_keyed().
> 
> (7) ceph_get_tree()
> 
>     Similar logic to get_tree_keyed().
> 
>     Already explicitly allows forcing the allocation of a new superblock
>     via CEPH_OPT_NOSHARE. This turns it into get_tree_nodev().
> 
> (8) fuse_get_tree_submount()
> 
>     Similar logic to get_tree_nodev().
> 
> (9) fuse_get_tree()
> 
>     Forces reuse of existing FUSE superblock.
> 
>     Forces reuse of existing superblock if passed in file refers to an
>     existing FUSE connection.
>     If FSCONFIG_CMD_CREATE_EXCL is specified together with an fd
>     referring to an existing FUSE connections this would cause the
>     superblock reusal to fail. If reusing is the intent then
>     FSCONFIG_CMD_CREATE_EXCL shouldn't be specified.
> 
> (10) fuse_get_tree()
>      -> get_tree_nodev()
> 
>     Same logic as in get_tree_nodev().
> 
> (11) fuse_get_tree()
>      -> get_tree_bdev()
> 
>     Same logic as in get_tree_bdev().
> 
> (12) virtio_fs_get_tree()
> 
>      Same logic as get_tree_keyed().
> 
> (13) gfs2_meta_get_tree()
> 
>      Forces reuse of existing gfs2 superblock.
> 
>      Mounting gfs2meta enforces that a gf2s superblock must already
>      exist. If not, it will error out. Consequently, mounting gfs2meta
>      with FSCONFIG_CMD_CREATE_EXCL would always fail. If reusing is the
>      intent then FSCONFIG_CMD_CREATE_EXCL shouldn't be specified.
> 
> (14) kernfs_get_tree()
> 
>      Similar logic to get_tree_keyed().
> 
> (15) nfs_get_tree_common()
> 
>     Similar logic to get_tree_keyed().
> 
>     Already explicitly allows forcing the allocation of a new superblock
>     via NFS_MOUNT_UNSHARED. This effectively turns it into
>     get_tree_nodev().
> 
> Link: [1] https://lore.kernel.org/linux-block/20230704-fasching-wertarbeit-7c6ffb01c83d@brauner
> Link: [2] https://lore.kernel.org/linux-block/20230705-pumpwerk-vielversprechend-a4b1fd947b65@brauner
> Link: [3] https://lore.kernel.org/linux-fsdevel/20230725-einnahmen-warnschilder-17779aec0a97@brauner
> 

The description and use case seem reasonable to me, the code is straightforward,
I assume you've added RFC only because you want comments on wether we want the
functionality/your approach to fixing the problem is acceptable?  For me this
makes sense and is simple enough.  I have a nit where I think we should say
something like "can't reuse existing file system" instead of "superblock", just
because we know what superblock means doesn't mean the user will necessarily
understand that out the gate.

You can add

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

to the series.  Thanks,

Josef

Re: [PATCH RFC 1/3] super: remove get_tree_single_reconf()

[Christoph Hellwig]

On Tue, Aug 01, 2023 at 03:09:00PM +0200, Christian Brauner wrote:
> The get_tree_single_reconf() helper isn't used anywhere. Remote it.

Yeah, I've got pretty much the same patch hiding somewhere in one of
me trees..

> -static int vfs_get_super(struct fs_context *fc, bool reconf,
> -		int (*test)(struct super_block *, struct fs_context *),
> -		int (*fill_super)(struct super_block *sb,
> -				  struct fs_context *fc))
> +static int vfs_get_super(struct fs_context *fc,
> +			 int (*test)(struct super_block *, struct fs_context *),
> +			 int (*fill_super)(struct super_block *sb,
> +					   struct fs_context *fc))

.a althought keeping the existing formatting here seems much more readable
to me.  No idea why the odd align to brace formatting has picked up so
many fans recently given that it is horrible to read and causes tons
of churn when touching the protoptype or function name.

Re: [PATCH RFC 2/3] fs: add vfs_cmd_create()

[Christoph Hellwig]

On Tue, Aug 01, 2023 at 03:09:01PM +0200, Christian Brauner wrote:
> Split the steps to create a superblock into a tiny helper. This will
> make the next patch easier to follow.
> 
> Signed-off-by: Christian Brauner <brauner@kernel.org>
> ---
>  fs/fsopen.c | 45 +++++++++++++++++++++++++++++++--------------
>  1 file changed, 31 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/fsopen.c b/fs/fsopen.c
> index fc9d2d9fd234..af2ff05dcee5 100644
> --- a/fs/fsopen.c
> +++ b/fs/fsopen.c
> @@ -209,6 +209,36 @@ SYSCALL_DEFINE3(fspick, int, dfd, const char __user *, path, unsigned int, flags
>  	return ret;
>  }
>  
> +static int vfs_cmd_create(struct fs_context *fc)
> +{
> +	struct super_block *sb;
> +	int ret;
> +
> +	if (fc->phase != FS_CONTEXT_CREATE_PARAMS)
> +		return -EBUSY;
> +
> +	if (!mount_capable(fc))
> +		return -EPERM;
> +
> +	fc->phase = FS_CONTEXT_CREATING;
> +
> +	ret = vfs_get_tree(fc);
> +	if (ret)
> +		return ret;

The error handling here now fails to set FS_CONTEXT_FAILED.

Also at a very minimum I'd also want a helper for the reconfigure
case to mirror this one.  But I think the whole sys_fsconfig and
vfs_fsconfig_locked combination is a complete and utter mess and
should be split into one top-level handler by cmd with a bunch of
shared helper instead of this spaghetti code.

Re: [PATCH RFC 3/3] fs: add FSCONFIG_CMD_CREATE_EXCL

[Christoph Hellwig]

> +	/* require the new mount api */
> +	if (exclusive && (fc->ops == &legacy_fs_context_ops))

No need for the inner braces.

> +		return -EOPNOTSUPP;
> +
>  	fc->phase = FS_CONTEXT_CREATING;
> +	fc->exclusive = exclusive;
>  
>  	ret = vfs_get_tree(fc);
> -	if (ret)
> +	if (ret) {
> +		fc->exclusive = false;

What's the point in clearing the flag on error?

> +	case FSCONFIG_CMD_CREATE_EXCL:
> +		fallthrough;
>  	case FSCONFIG_CMD_CREATE:
> -		ret = vfs_cmd_create(fc);
> +		ret = vfs_cmd_create(fc, cmd == FSCONFIG_CMD_CREATE_EXCL);
>  		if (ret)
>  			break;
>  		return 0;

Nitpick, but I always find it cleaner to do something like:

	case FSCONFIG_CMD_CREATE_EXCL:
		ret = vfs_cmd_create(fc, true)
		break;
 	case FSCONFIG_CMD_CREATE:
		ret = vfs_cmd_create(fc, false);

but that might just be preference.

Re: [PATCH RFC 2/3] fs: add vfs_cmd_create()

[Christoph Hellwig]

On Tue, Aug 01, 2023 at 05:43:33PM +0200, Christoph Hellwig wrote:
> > +	ret = vfs_get_tree(fc);
> > +	if (ret)
> > +		return ret;
> 
> The error handling here now fails to set FS_CONTEXT_FAILED.

Actually that's still done in the caller.  But that just reconfirms
my opinion on the shape of the pre-existing code..

Re: [PATCH RFC 1/3] super: remove get_tree_single_reconf()

[Christian Brauner]

On Tue, Aug 01, 2023 at 05:31:00PM +0200, Christoph Hellwig wrote:
> On Tue, Aug 01, 2023 at 03:09:00PM +0200, Christian Brauner wrote:
> > The get_tree_single_reconf() helper isn't used anywhere. Remote it.
> 
> Yeah, I've got pretty much the same patch hiding somewhere in one of
> me trees..
> 
> > -static int vfs_get_super(struct fs_context *fc, bool reconf,
> > -		int (*test)(struct super_block *, struct fs_context *),
> > -		int (*fill_super)(struct super_block *sb,
> > -				  struct fs_context *fc))
> > +static int vfs_get_super(struct fs_context *fc,
> > +			 int (*test)(struct super_block *, struct fs_context *),
> > +			 int (*fill_super)(struct super_block *sb,
> > +					   struct fs_context *fc))
> 
> .a althought keeping the existing formatting here seems much more readable
> to me.  No idea why the odd align to brace formatting has picked up so
> many fans recently given that it is horrible to read and causes tons
> of churn when touching the protoptype or function name.

I'm not doing that manually. I'm using clang-format for this so I don't
have to care about stuff like this.

Tbh, the fact that we don't simply have an authoritative code formatting
tool in-kernel over 30 years later that we settle on - at least on a per
subsystem basis - is funny and sad.

I'm fine not touching the header.

Re: [PATCH RFC 3/3] fs: add FSCONFIG_CMD_CREATE_EXCL

[Christian Brauner]

On Tue, Aug 01, 2023 at 05:46:07PM +0200, Christoph Hellwig wrote:
> > +	/* require the new mount api */
> > +	if (exclusive && (fc->ops == &legacy_fs_context_ops))
> 
> No need for the inner braces.

My love for operator precendence isn't very strong and having also
studied math has left me with an obsessive relationsip with brackets...

> 
> > +		return -EOPNOTSUPP;
> > +
> >  	fc->phase = FS_CONTEXT_CREATING;
> > +	fc->exclusive = exclusive;
> >  
> >  	ret = vfs_get_tree(fc);
> > -	if (ret)
> > +	if (ret) {
> > +		fc->exclusive = false;
> 
> What's the point in clearing the flag on error?

I originally thought that when the caller did:

fsconfig(fd_fs, FSCONFIG_CMD_CREATE_EXCL)

and this failed but the caller immediately followed this up with another
call to:

fsconfig(fd_fs, FSCONFIG_CMD_CREATE)

then leaving fc->exclusive set might turn FSCONFIG_CMD_CREATE into an
exclusive create on accident leaving the caller confused. But then I
remembered that this code explicitly fails the whole fs_context forever
after a failed superblock creation because after vfs_get_tree() it is
unclear what state the fs_context is in. And I apparently forgot to
remove that after I remembered that.

> 
> > +	case FSCONFIG_CMD_CREATE_EXCL:
> > +		fallthrough;
> >  	case FSCONFIG_CMD_CREATE:
> > -		ret = vfs_cmd_create(fc);
> > +		ret = vfs_cmd_create(fc, cmd == FSCONFIG_CMD_CREATE_EXCL);
> >  		if (ret)
> >  			break;
> >  		return 0;
> 
> Nitpick, but I always find it cleaner to do something like:
> 
> 	case FSCONFIG_CMD_CREATE_EXCL:
> 		ret = vfs_cmd_create(fc, true)
> 		break;
>  	case FSCONFIG_CMD_CREATE:
> 		ret = vfs_cmd_create(fc, false);
> 
> but that might just be preference.

Fine by me.

Re: [PATCH RFC 0/3] fs: allow userspace to detect superblock reuse

[Christian Brauner]

On Tue, Aug 01, 2023 at 10:25:21AM -0400, Josef Bacik wrote:
> On Tue, Aug 01, 2023 at 03:08:59PM +0200, Christian Brauner wrote:
> > Summary
> > =======
> > 
> > This introduces FSCONFIG_CMD_CREATE_EXCL which will allows userspace to
> > implement something like mount -t ext4 --exclusive /dev/sda /B which
> > fails if a superblock for the requested filesystem does already exist:
> > 
> > Before this patch
> > -----------------
> > 
> > $ sudo ./move-mount -f xfs -o source=/dev/sda4 /A
> > Requesting filesystem type xfs
> > Mount options requested: source=/dev/sda4
> > Attaching mount at /A
> > Moving single attached mount
> > Setting key(source) with val(/dev/sda4)
> > 
> > $ sudo ./move-mount -f xfs -o source=/dev/sda4 /B
> > Requesting filesystem type xfs
> > Mount options requested: source=/dev/sda4
> > Attaching mount at /B
> > Moving single attached mount
> > Setting key(source) with val(/dev/sda4)
> > 
> > After this patch with --exclusive as a switch for FSCONFIG_CMD_CREATE_EXCL
> > --------------------------------------------------------------------------
> > 
> > $ sudo ./move-mount -f xfs --exclusive -o source=/dev/sda4 /A
> > Requesting filesystem type xfs
> > Request exclusive superblock creation
> > Mount options requested: source=/dev/sda4
> > Attaching mount at /A
> > Moving single attached mount
> > Setting key(source) with val(/dev/sda4)
> > 
> > $ sudo ./move-mount -f xfs --exclusive -o source=/dev/sda4 /B
> > Requesting filesystem type xfs
> > Request exclusive superblock creation
> > Mount options requested: source=/dev/sda4
> > Attaching mount at /B
> > Moving single attached mount
> > Setting key(source) with val(/dev/sda4)
> > Device or resource busy | move-mount.c: 300: do_fsconfig: i xfs: reusing existing superblock not allowed
> > 
> > Details
> > =======
> > 
> > As mentioned on the list (cf. [1]-[3]) mount requests like
> > mount -t ext4 /dev/sda /A are ambigous for userspace. Either a new
> > superblock has been created and mounted or an existing superblock has
> > been reused and a bind-mount has been created.
> > 
> > This becomes clear if we consider two processes creating the same mount
> > for the same block device:
> > 
> > P1                                                              P2
> > fd_fs = fsopen("ext4");                                         fd_fs = fsopen("ext4");
> > fsconfig(fd_fs, FSCONFIG_SET_STRING, "source", "/dev/sda");     fsconfig(fd_fs, FSCONFIG_SET_STRING, "source", "/dev/sda");
> > fsconfig(fd_fs, FSCONFIG_SET_STRING, "dax", "always");          fsconfig(fd_fs, FSCONFIG_SET_STRING, "resuid", "1000");
> > 
> > // wins and creates superblock
> > fsconfig(fd_fs, FSCONFIG_CMD_CREATE, ...)
> >                                                                 // finds compatible superblock of P1
> >                                                                 // spins until P1 sets SB_BORN and grabs a reference
> >                                                                 fsconfig(fd_fs, FSCONFIG_CMD_CREATE, ...)
> > 
> > fd_mnt1 = fsmount(fd_fs);                                       fd_mnt2 = fsmount(fd_fs);
> > move_mount(fd_mnt1, "/A")                                       move_mount(fd_mnt2, "/B")
> > 
> > Not just does P2 get a bind-mount for nearly all filesystems the mount
> > options for P2 are usually completely ignored. The VFS itself doesn't
> > and shouldn't enforce filesystem specific mount option compatibility. It
> > only enforces incompatibility for read-only <-> read-write transitions:
> > 
> > mount -t ext4       /dev/sda /A
> > mount -t ext4 -o ro /dev/sda /B
> > 
> > The read-only request will fail with EBUSY as the VFS can't just
> > silently transition a superblock from read-write to read-only or vica
> > versa without risking security issues.
> > 
> > To userspace this silent superblock reuse can be security issue in
> > certain circumstances because there is currently no simple way for them
> > to know that they did indeed manage to create the superblock and didn't
> > just reuse an existing one.
> > 
> > This adds a new FSCONFIG_CMD_CREATE_EXCL command to fsconfig() that
> > returns EBUSY if an existing superblock is found. Userspace that needs
> > to be sure that they did create the superblock with the requested mount
> > options can request superblock creation using this command. If it
> > succeeds they can be sure that they did create the superblock with the
> > requested mount options.
> > 
> > This requires the new mount api. With the old mount api we would have to
> > plumb this through every legacy filesystem's file_system_type->mount()
> > method. If they want this feature they are most welcome to switch to the
> > new mount api.
> > 
> > Following is an analysis of the effect of FSCONFIG_CMD_CREATE_EXCL on
> > each high-level superblock creation helper:
> > 
> > (1) get_tree_nodev()
> > 
> >     Always allocate new superblock. Hence, FSCONFIG_CMD_CREATE and
> >     FSCONFIG_CMD_CREATE_EXCL are equivalent.
> > 
> >     The binderfs or overlayfs filesystems are examples.
> > 
> > (4) get_tree_keyed()
> > 
> >     Finds an existing superblock based on sb->s_fs_info. Hence,
> >     FSCONFIG_CMD_CREATE would reuse an existing superblock whereas
> >     FSCONFIG_CMD_CREATE_EXCL would reject it with EBUSY.
> > 
> >     The mqueue or nfsd filesystems are examples.
> > 
> > (2) get_tree_bdev()
> > 
> >     This effectively works like get_tree_keyed().
> > 
> >     The ext4 or xfs filesystems are examples.
> > 
> > (3) get_tree_single()
> > 
> >     Only one superblock of this filesystem type can ever exist.
> >     Hence, FSCONFIG_CMD_CREATE would reuse an existing superblock
> >     whereas FSCONFIG_CMD_CREATE_EXCL would reject it with EBUSY.
> > 
> >     The securityfs or configfs filesystems are examples.
> > 
> >     This has a further consequence. Some filesystems will never destroy
> >     the superblock once it has been created. For example, if securityfs
> >     is mounted the allocated superblock will never be destroyed again as
> >     long as there is still an LSM making use it. Consequently, even if
> >     securityfs is unmounted and seemingly destroyed it really isn't
> >     which means that FSCONFIG_CMD_CREATE_EXCL will continue rejecting
> >     reusing the existing superblock.
> > 
> >     This is unintuitive but not a problem. Such special purpose
> >     filesystems aren't mounted multiple times anyway.
> > 
> > Following is an analysis of the effect of FSCONFIG_CMD_CREATE_EXCL on
> > filesystems that make use of the low-level sget_fc() helper directly.
> > They're all effectively variants on get_tree_keyed() or get_tree_bdev():
> > 
> > (5) mtd_get_sb()
> > 
> >     Similar logic to get_tree_keyed().
> > 
> > (6) afs_get_tree()
> > 
> >     Similar logic to get_tree_keyed().
> > 
> > (7) ceph_get_tree()
> > 
> >     Similar logic to get_tree_keyed().
> > 
> >     Already explicitly allows forcing the allocation of a new superblock
> >     via CEPH_OPT_NOSHARE. This turns it into get_tree_nodev().
> > 
> > (8) fuse_get_tree_submount()
> > 
> >     Similar logic to get_tree_nodev().
> > 
> > (9) fuse_get_tree()
> > 
> >     Forces reuse of existing FUSE superblock.
> > 
> >     Forces reuse of existing superblock if passed in file refers to an
> >     existing FUSE connection.
> >     If FSCONFIG_CMD_CREATE_EXCL is specified together with an fd
> >     referring to an existing FUSE connections this would cause the
> >     superblock reusal to fail. If reusing is the intent then
> >     FSCONFIG_CMD_CREATE_EXCL shouldn't be specified.
> > 
> > (10) fuse_get_tree()
> >      -> get_tree_nodev()
> > 
> >     Same logic as in get_tree_nodev().
> > 
> > (11) fuse_get_tree()
> >      -> get_tree_bdev()
> > 
> >     Same logic as in get_tree_bdev().
> > 
> > (12) virtio_fs_get_tree()
> > 
> >      Same logic as get_tree_keyed().
> > 
> > (13) gfs2_meta_get_tree()
> > 
> >      Forces reuse of existing gfs2 superblock.
> > 
> >      Mounting gfs2meta enforces that a gf2s superblock must already
> >      exist. If not, it will error out. Consequently, mounting gfs2meta
> >      with FSCONFIG_CMD_CREATE_EXCL would always fail. If reusing is the
> >      intent then FSCONFIG_CMD_CREATE_EXCL shouldn't be specified.
> > 
> > (14) kernfs_get_tree()
> > 
> >      Similar logic to get_tree_keyed().
> > 
> > (15) nfs_get_tree_common()
> > 
> >     Similar logic to get_tree_keyed().
> > 
> >     Already explicitly allows forcing the allocation of a new superblock
> >     via NFS_MOUNT_UNSHARED. This effectively turns it into
> >     get_tree_nodev().
> > 
> > Link: [1] https://lore.kernel.org/linux-block/20230704-fasching-wertarbeit-7c6ffb01c83d@brauner
> > Link: [2] https://lore.kernel.org/linux-block/20230705-pumpwerk-vielversprechend-a4b1fd947b65@brauner
> > Link: [3] https://lore.kernel.org/linux-fsdevel/20230725-einnahmen-warnschilder-17779aec0a97@brauner
> > 
> 
> The description and use case seem reasonable to me, the code is straightforward,
> I assume you've added RFC only because you want comments on wether we want the
> functionality/your approach to fixing the problem is acceptable?  For me this

Yes, exactly.

> makes sense and is simple enough.  I have a nit where I think we should say

Ok, great.

> something like "can't reuse existing file system" instead of "superblock", just

Ok.

> because we know what superblock means doesn't mean the user will necessarily
> understand that out the gate.
> 
> You can add
> 
> Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Thanks!

Re: [PATCH RFC 2/3] fs: add vfs_cmd_create()

[Christian Brauner]

On Tue, Aug 01, 2023 at 05:43:33PM +0200, Christoph Hellwig wrote:
> On Tue, Aug 01, 2023 at 03:09:01PM +0200, Christian Brauner wrote:
> > Split the steps to create a superblock into a tiny helper. This will
> > make the next patch easier to follow.
> > 
> > Signed-off-by: Christian Brauner <brauner@kernel.org>
> > ---
> >  fs/fsopen.c | 45 +++++++++++++++++++++++++++++++--------------
> >  1 file changed, 31 insertions(+), 14 deletions(-)
> > 
> > diff --git a/fs/fsopen.c b/fs/fsopen.c
> > index fc9d2d9fd234..af2ff05dcee5 100644
> > --- a/fs/fsopen.c
> > +++ b/fs/fsopen.c
> > @@ -209,6 +209,36 @@ SYSCALL_DEFINE3(fspick, int, dfd, const char __user *, path, unsigned int, flags
> >  	return ret;
> >  }
> >  
> > +static int vfs_cmd_create(struct fs_context *fc)
> > +{
> > +	struct super_block *sb;
> > +	int ret;
> > +
> > +	if (fc->phase != FS_CONTEXT_CREATE_PARAMS)
> > +		return -EBUSY;
> > +
> > +	if (!mount_capable(fc))
> > +		return -EPERM;
> > +
> > +	fc->phase = FS_CONTEXT_CREATING;
> > +
> > +	ret = vfs_get_tree(fc);
> > +	if (ret)
> > +		return ret;
> 
> The error handling here now fails to set FS_CONTEXT_FAILED.
> 
> Also at a very minimum I'd also want a helper for the reconfigure
> case to mirror this one.  But I think the whole sys_fsconfig and

That's acceptable.

Re: [PATCH RFC 1/3] super: remove get_tree_single_reconf()

[Jan Kara]

On Tue 01-08-23 15:09:00, Christian Brauner wrote:
> The get_tree_single_reconf() helper isn't used anywhere. Remote it.
							   ^^ Remove
> 
> Signed-off-by: Christian Brauner <brauner@kernel.org>

Otherwise looks good to me. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/super.c                 | 34 ++++++++--------------------------
>  include/linux/fs_context.h |  3 ---
>  2 files changed, 8 insertions(+), 29 deletions(-)
> 
> diff --git a/fs/super.c b/fs/super.c
> index 3ef39df5bec5..c086ce4929cd 100644
> --- a/fs/super.c
> +++ b/fs/super.c
> @@ -1136,10 +1136,10 @@ static int test_single_super(struct super_block *s, struct fs_context *fc)
>  	return 1;
>  }
>  
> -static int vfs_get_super(struct fs_context *fc, bool reconf,
> -		int (*test)(struct super_block *, struct fs_context *),
> -		int (*fill_super)(struct super_block *sb,
> -				  struct fs_context *fc))
> +static int vfs_get_super(struct fs_context *fc,
> +			 int (*test)(struct super_block *, struct fs_context *),
> +			 int (*fill_super)(struct super_block *sb,
> +					   struct fs_context *fc))
>  {
>  	struct super_block *sb;
>  	int err;
> @@ -1154,19 +1154,9 @@ static int vfs_get_super(struct fs_context *fc, bool reconf,
>  			goto error;
>  
>  		sb->s_flags |= SB_ACTIVE;
> -		fc->root = dget(sb->s_root);
> -	} else {
> -		fc->root = dget(sb->s_root);
> -		if (reconf) {
> -			err = reconfigure_super(fc);
> -			if (err < 0) {
> -				dput(fc->root);
> -				fc->root = NULL;
> -				goto error;
> -			}
> -		}
>  	}
>  
> +	fc->root = dget(sb->s_root);
>  	return 0;
>  
>  error:
> @@ -1178,7 +1168,7 @@ int get_tree_nodev(struct fs_context *fc,
>  		  int (*fill_super)(struct super_block *sb,
>  				    struct fs_context *fc))
>  {
> -	return vfs_get_super(fc, false, NULL, fill_super);
> +	return vfs_get_super(fc, NULL, fill_super);
>  }
>  EXPORT_SYMBOL(get_tree_nodev);
>  
> @@ -1186,25 +1176,17 @@ int get_tree_single(struct fs_context *fc,
>  		  int (*fill_super)(struct super_block *sb,
>  				    struct fs_context *fc))
>  {
> -	return vfs_get_super(fc, false, test_single_super, fill_super);
> +	return vfs_get_super(fc, test_single_super, fill_super);
>  }
>  EXPORT_SYMBOL(get_tree_single);
>  
> -int get_tree_single_reconf(struct fs_context *fc,
> -		  int (*fill_super)(struct super_block *sb,
> -				    struct fs_context *fc))
> -{
> -	return vfs_get_super(fc, true, test_single_super, fill_super);
> -}
> -EXPORT_SYMBOL(get_tree_single_reconf);
> -
>  int get_tree_keyed(struct fs_context *fc,
>  		  int (*fill_super)(struct super_block *sb,
>  				    struct fs_context *fc),
>  		void *key)
>  {
>  	fc->s_fs_info = key;
> -	return vfs_get_super(fc, false, test_keyed_super, fill_super);
> +	return vfs_get_super(fc, test_keyed_super, fill_super);
>  }
>  EXPORT_SYMBOL(get_tree_keyed);
>  
> diff --git a/include/linux/fs_context.h b/include/linux/fs_context.h
> index ff6341e09925..851b3fe2549c 100644
> --- a/include/linux/fs_context.h
> +++ b/include/linux/fs_context.h
> @@ -150,9 +150,6 @@ extern int get_tree_nodev(struct fs_context *fc,
>  extern int get_tree_single(struct fs_context *fc,
>  			 int (*fill_super)(struct super_block *sb,
>  					   struct fs_context *fc));
> -extern int get_tree_single_reconf(struct fs_context *fc,
> -			 int (*fill_super)(struct super_block *sb,
> -					   struct fs_context *fc));
>  extern int get_tree_keyed(struct fs_context *fc,
>  			 int (*fill_super)(struct super_block *sb,
>  					   struct fs_context *fc),
> 
> -- 
> 2.34.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH RFC 2/3] fs: add vfs_cmd_create()

[Jan Kara]

On Tue 01-08-23 15:09:01, Christian Brauner wrote:
> Split the steps to create a superblock into a tiny helper. This will
> make the next patch easier to follow.
> 
> Signed-off-by: Christian Brauner <brauner@kernel.org>

I agree with Christoph that the error handling in vfs_fsconfig_locked() is
confusing - in particular the fact that if you 'break' out of the switch
statement it causes the fs context to be marked as failed is probably handy
but too subtle to my taste.

Also I think this patch does cause a behavioral change because before if we
bailed e.g. due to:

if (fc->phase != FS_CONTEXT_CREATE_PARAMS)

we returned -EBUSY but didn't set fc->phase = FS_CONTEXT_FAILED. After your
patch we 'break' on any error and thus fc->phase is set on any error...

								Honza

> ---
>  fs/fsopen.c | 45 +++++++++++++++++++++++++++++++--------------
>  1 file changed, 31 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/fsopen.c b/fs/fsopen.c
> index fc9d2d9fd234..af2ff05dcee5 100644
> --- a/fs/fsopen.c
> +++ b/fs/fsopen.c
> @@ -209,6 +209,36 @@ SYSCALL_DEFINE3(fspick, int, dfd, const char __user *, path, unsigned int, flags
>  	return ret;
>  }
>  
> +static int vfs_cmd_create(struct fs_context *fc)
> +{
> +	struct super_block *sb;
> +	int ret;
> +
> +	if (fc->phase != FS_CONTEXT_CREATE_PARAMS)
> +		return -EBUSY;
> +
> +	if (!mount_capable(fc))
> +		return -EPERM;
> +
> +	fc->phase = FS_CONTEXT_CREATING;
> +
> +	ret = vfs_get_tree(fc);
> +	if (ret)
> +		return ret;
> +
> +	sb = fc->root->d_sb;
> +	ret = security_sb_kern_mount(sb);
> +	if (unlikely(ret)) {
> +		fc_drop_locked(fc);
> +		return ret;
> +	}
> +
> +	/* vfs_get_tree() callchains will have grabbed @s_umount */
> +	up_write(&sb->s_umount);
> +	fc->phase = FS_CONTEXT_AWAITING_MOUNT;
> +	return 0;
> +}
> +
>  /*
>   * Check the state and apply the configuration.  Note that this function is
>   * allowed to 'steal' the value by setting param->xxx to NULL before returning.
> @@ -224,22 +254,9 @@ static int vfs_fsconfig_locked(struct fs_context *fc, int cmd,
>  		return ret;
>  	switch (cmd) {
>  	case FSCONFIG_CMD_CREATE:
> -		if (fc->phase != FS_CONTEXT_CREATE_PARAMS)
> -			return -EBUSY;
> -		if (!mount_capable(fc))
> -			return -EPERM;
> -		fc->phase = FS_CONTEXT_CREATING;
> -		ret = vfs_get_tree(fc);
> +		ret = vfs_cmd_create(fc);
>  		if (ret)
>  			break;
> -		sb = fc->root->d_sb;
> -		ret = security_sb_kern_mount(sb);
> -		if (unlikely(ret)) {
> -			fc_drop_locked(fc);
> -			break;
> -		}
> -		up_write(&sb->s_umount);
> -		fc->phase = FS_CONTEXT_AWAITING_MOUNT;
>  		return 0;
>  	case FSCONFIG_CMD_RECONFIGURE:
>  		if (fc->phase != FS_CONTEXT_RECONF_PARAMS)
> 
> -- 
> 2.34.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH RFC 2/3] fs: add vfs_cmd_create()

[Jan Kara]

On Wed 02-08-23 19:01:55, Jan Kara wrote:
> On Tue 01-08-23 15:09:01, Christian Brauner wrote:
> > Split the steps to create a superblock into a tiny helper. This will
> > make the next patch easier to follow.
> > 
> > Signed-off-by: Christian Brauner <brauner@kernel.org>
> 
> I agree with Christoph that the error handling in vfs_fsconfig_locked() is
> confusing - in particular the fact that if you 'break' out of the switch
> statement it causes the fs context to be marked as failed is probably handy
> but too subtle to my taste.
> 
> Also I think this patch does cause a behavioral change because before if we
> bailed e.g. due to:
> 
> if (fc->phase != FS_CONTEXT_CREATE_PARAMS)
> 
> we returned -EBUSY but didn't set fc->phase = FS_CONTEXT_FAILED. After your
> patch we 'break' on any error and thus fc->phase is set on any error...

Ah, I can see you've already posted v2 where you addressed this problem.
Sorry for the noise.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR