[PATCH 0/6] mount-api: fixes and cleanups

[PATCH 0/6] mount-api: fixes and cleanups

[Miklos Szeredi]

These fix issues with handling subtype (fuse specific thing) and rename
some constants on the new API to make me happy.  I realize the last patch
could be viewed as bikeshedding, but I do think suggesting falsehood in a
constant name can lead to painful real life bugs, so now is the time to
think about these issues.

Miklos Szeredi (6):
  selinux: fold superblock_doinit() into only caller
  vfs_submount: use SB_SUBMOUNT instead of MS_SUBMOUNT
  mount: fix regression in setting "subtype" from legacy API
  fsconfig: parse "subtype" param for old internal API
  fsmount: do not use legacy MS_ flags
  fsconfig: rename FSCONFIG_CMD_CREATE to FSCONFIG_CMD_OBTAIN

 fs/fs_context.c            | 44 +++++++++------------------------------
 fs/fsopen.c                |  6 +++---
 fs/namespace.c             | 51 +++++++++++++++++++++++++++++++---------------
 include/uapi/linux/fs.h    |  2 +-
 include/uapi/linux/mount.h |  9 ++++++++
 samples/vfs/test-fsmount.c |  2 +-
 security/selinux/hooks.c   | 37 ++++++++-------------------------
 7 files changed, 68 insertions(+), 83 deletions(-)

-- 
2.14.3

[PATCH 1/6] selinux: fold superblock_doinit() into only caller

[Miklos Szeredi]

...and remove the unused option parsing part as well.

Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
---
 security/selinux/hooks.c | 37 +++++++++----------------------------
 1 file changed, 9 insertions(+), 28 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 99c2c40c5d7a..1069acea2682 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -1199,33 +1199,6 @@ static int selinux_parse_opts_str(char *options,
 	kfree(rootcontext);
 	return rc;
 }
-/*
- * string mount options parsing and call set the sbsec
- */
-static int superblock_doinit(struct super_block *sb, void *data)
-{
-	int rc = 0;
-	char *options = data;
-	struct security_mnt_opts opts;
-
-	security_init_mnt_opts(&opts);
-
-	if (!data)
-		goto out;
-
-	BUG_ON(sb->s_type->fs_flags & FS_BINARY_MOUNTDATA);
-
-	rc = selinux_parse_opts_str(options, &opts);
-	if (rc)
-		goto out_err;
-
-out:
-	rc = selinux_set_mnt_opts(sb, &opts, 0, NULL);
-
-out_err:
-	security_free_mnt_opts(&opts);
-	return rc;
-}
 
 static void selinux_write_opts(struct seq_file *m,
 			       struct security_mnt_opts *opts)
@@ -7370,7 +7343,15 @@ static __init int selinux_init(void)
 
 static void delayed_superblock_init(struct super_block *sb, void *unused)
 {
-	superblock_doinit(sb, NULL);
+{
+	int rc;
+	struct security_mnt_opts opts;
+
+	security_init_mnt_opts(&opts);
+	rc = selinux_set_mnt_opts(sb, &opts, 0, NULL);
+	security_free_mnt_opts(&opts);
+
+	return rc;
 }
 
 void selinux_complete_init(void)
-- 
2.14.3

Re: [PATCH 1/6] selinux: fold superblock_doinit() into only caller

[David Howells]

This will conflict with my patch to remove a lot of this stuff.

You can find it on this branch:

	https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/commit/?h=btrfs-mount-api

with the subject "security: Remove the set_mnt_opts and clone_mnt_opts ops".

Note that both nfs and btrfs need converting before this can be applied.

David

[PATCH 2/6] vfs_submount: use SB_SUBMOUNT instead of MS_SUBMOUNT

[Miklos Szeredi]

The latter is for the legacy userspace API only and should not be used
internally.

Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
---
 fs/namespace.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/namespace.c b/fs/namespace.c
index a28b713164bf..865b6f2c5e7d 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -3297,7 +3297,7 @@ vfs_submount(const struct dentry *mountpoint, struct file_system_type *type,
 	if (mountpoint->d_sb->s_user_ns != &init_user_ns)
 		return ERR_PTR(-EPERM);
 
-	return vfs_kern_mount(type, MS_SUBMOUNT, name, data, data_size);
+	return vfs_kern_mount(type, SB_SUBMOUNT, name, data, data_size);
 }
 EXPORT_SYMBOL_GPL(vfs_submount);
 
-- 
2.14.3

Re: [PATCH 2/6] vfs_submount: use SB_SUBMOUNT instead of MS_SUBMOUNT

[David Howells]

Miklos Szeredi <mszeredi@redhat.com> wrote:

> -	return vfs_kern_mount(type, MS_SUBMOUNT, name, data, data_size);
> +	return vfs_kern_mount(type, SB_SUBMOUNT, name, data, data_size);

Folded in.

David

[PATCH 3/6] mount: fix regression in setting "subtype" from legacy API

[Miklos Szeredi]

Subtype was initilized at the wrong place from the wrong source
(fstype->fs_type->name, which does not ever contain a subtype).

Set subtype from do_new_mount(), where it actually does something.
Verified with fuse.

Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
---
 fs/fs_context.c | 34 ----------------------------------
 fs/namespace.c  | 21 +++++++++++++++++++++
 2 files changed, 21 insertions(+), 34 deletions(-)

diff --git a/fs/fs_context.c b/fs/fs_context.c
index ceaf42559016..c0ecbb1ecdfe 100644
--- a/fs/fs_context.c
+++ b/fs/fs_context.c
@@ -684,27 +684,6 @@ static int legacy_validate(struct fs_context *fc)
 				     ctx->secdata);
 }
 
-/*
- * Determine the superblock subtype.
- */
-static int legacy_set_subtype(struct fs_context *fc)
-{
-	const char *subtype = strchr(fc->fs_type->name, '.');
-
-	if (subtype) {
-		subtype++;
-		if (!subtype[0])
-			return -EINVAL;
-	} else {
-		subtype = "";
-	}
-
-	fc->subtype = kstrdup(subtype, GFP_KERNEL);
-	if (!fc->subtype)
-		return -ENOMEM;
-	return 0;
-}
-
 /*
  * Get a mountable root with the legacy mount command.
  */
@@ -713,7 +692,6 @@ static int legacy_get_tree(struct fs_context *fc)
 	struct legacy_fs_context *ctx = fc->fs_private;
 	struct super_block *sb;
 	struct dentry *root;
-	int ret;
 
 	root = fc->fs_type->mount(fc->fs_type, fc->sb_flags,
 				      fc->source, ctx->legacy_data,
@@ -724,20 +702,8 @@ static int legacy_get_tree(struct fs_context *fc)
 	sb = root->d_sb;
 	BUG_ON(!sb);
 
-	if ((fc->fs_type->fs_flags & FS_HAS_SUBTYPE) &&
-	    !fc->subtype) {
-		ret = legacy_set_subtype(fc);
-		if (ret < 0)
-			goto err_sb;
-	}
-
 	fc->root = root;
 	return 0;
-
-err_sb:
-	dput(root);
-	deactivate_locked_super(sb);
-	return ret;
 }
 
 /*
diff --git a/fs/namespace.c b/fs/namespace.c
index 865b6f2c5e7d..7671c1f6fc22 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -2617,6 +2617,7 @@ static int do_new_mount(struct path *mountpoint, const char *fstype,
 {
 	struct file_system_type *fs_type;
 	struct fs_context *fc;
+	const char *subtype = NULL;
 	int err;
 
 	if (!fstype)
@@ -2627,6 +2628,19 @@ static int do_new_mount(struct path *mountpoint, const char *fstype,
 	if (!fs_type)
 		goto out;
 
+	if (fs_type->fs_flags & FS_HAS_SUBTYPE) {
+		subtype = strchr(fstype, '.');
+		if (subtype) {
+			subtype++;
+			if (!subtype[0]) {
+				put_filesystem(fs_type);
+				return -EINVAL;
+			}
+		} else {
+			subtype = "";
+		}
+	}
+
 	fc = vfs_new_fs_context(fs_type, NULL, sb_flags, sb_flags,
 				FS_CONTEXT_FOR_USER_MOUNT);
 	put_filesystem(fs_type);
@@ -2635,6 +2649,13 @@ static int do_new_mount(struct path *mountpoint, const char *fstype,
 		goto out;
 	}
 
+	if (subtype) {
+		fc->subtype = kstrdup(subtype, GFP_KERNEL);
+		err = -ENOMEM;
+		if (!fc->subtype)
+			goto out;
+	}
+
 	if (name) {
 		err = vfs_parse_fs_string(fc, "source", name, strlen(name));
 		if (err < 0)
-- 
2.14.3

Re: [PATCH 3/6] mount: fix regression in setting "subtype" from legacy API

[David Howells]

Miklos Szeredi <mszeredi@redhat.com> wrote:

> Subtype was initilized at the wrong place from the wrong source
> (fstype->fs_type->name, which does not ever contain a subtype).
> 
> Set subtype from do_new_mount(), where it actually does something.
> Verified with fuse.

Folded.

[PATCH 4/6] fsconfig: parse "subtype" param for old internal API

[Miklos Szeredi]

This subtype thing is specific to "fuse" and "fuseblk" filesystems.  When these
are switched over to the new context API, the handling of this parameter can be
moved from legacy_parse_param() into fuse.

Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
---
 fs/fs_context.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/fs/fs_context.c b/fs/fs_context.c
index c0ecbb1ecdfe..0192456712d2 100644
--- a/fs/fs_context.c
+++ b/fs/fs_context.c
@@ -574,6 +574,16 @@ static int legacy_parse_param(struct fs_context *fc, struct fs_parameter *param)
 		param->string = NULL;
 		return 0;
 	}
+	if ((fc->fs_type->fs_flags & FS_HAS_SUBTYPE) &&
+	    strcmp(param->key, "subtype") == 0) {
+		if (param->type != fs_value_is_string)
+			return invalf(fc, "VFS: Legacy: Non-string subtype");
+		if (fc->subtype)
+			return invalf(fc, "VFS: Legacy: Multiple subtype");
+		fc->subtype = param->string;
+		param->string= NULL;
+		return 0;
+	}
 
 	if (ctx->param_type != LEGACY_FS_UNSET_PARAMS &&
 	    ctx->param_type != LEGACY_FS_INDIVIDUAL_PARAMS)
-- 
2.14.3

Re: [PATCH 4/6] fsconfig: parse "subtype" param for old internal API

[David Howells]

Miklos Szeredi <mszeredi@redhat.com> wrote:

> This subtype thing is specific to "fuse" and "fuseblk" filesystems.  When
> these are switched over to the new context API, the handling of this
> parameter can be moved from legacy_parse_param() into fuse.

I think do_new_mount() should do:

	vfs_parse_fs_string(fc, "subtype", subtype, strlen(subtype));

rather than setting fc->subtype itself.  I've made that change and folded this
patch also.

David

[PATCH 5/6] fsmount: do not use legacy MS_ flags

[Miklos Szeredi]

What happens if we introduce new flags for fsmount(2) and are already out
of flags for mount(2)?  I see a big mess that way.

So let's instead start a clean new set, to be used in the new API.

The MS_RELATIME flag was accepted but ignored.  Simply leave this out of
the new set, since "relatime" is the default.

The MNT_ prefix is already used by libmount, and we don't want any
confusion arising from that.  MOUNT_ feels a bit too long, so just go with
the short and sweet M_ prefix.

Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
---
 fs/namespace.c             | 28 +++++++++++++---------------
 include/uapi/linux/mount.h |  9 +++++++++
 2 files changed, 22 insertions(+), 15 deletions(-)

diff --git a/fs/namespace.c b/fs/namespace.c
index 7671c1f6fc22..027b99b993c0 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -3332,7 +3332,7 @@ EXPORT_SYMBOL_GPL(kern_mount);
  * Create a kernel mount representation for a new, prepared superblock
  * (specified by fs_fd) and attach to an open_tree-like file descriptor.
  */
-SYSCALL_DEFINE3(fsmount, int, fs_fd, unsigned int, flags, unsigned int, ms_flags)
+SYSCALL_DEFINE3(fsmount, int, fs_fd, unsigned int, flags, unsigned int, m_flags)
 {
 	struct fs_context *fc;
 	struct file *file;
@@ -3347,30 +3347,28 @@ SYSCALL_DEFINE3(fsmount, int, fs_fd, unsigned int, flags, unsigned int, ms_flags
 	if ((flags & ~(FSMOUNT_CLOEXEC)) != 0)
 		return -EINVAL;
 
-	if (ms_flags & ~(MS_RDONLY | MS_NOSUID | MS_NODEV | MS_NOEXEC |
-			 MS_NOATIME | MS_NODIRATIME | MS_RELATIME |
-			 MS_STRICTATIME))
+	if (m_flags & ~(M_RDONLY | M_NOSUID | M_NODEV | M_NOEXEC |
+			 M_NOATIME | M_STRICTATIME | M_NODIRATIME))
 		return -EINVAL;
 
-	if (ms_flags & MS_RDONLY)
+	if ((m_flags & M_STRICTATIME) && (m_flags & M_NOATIME))
+		return -EINVAL;
+
+	if (m_flags & M_RDONLY)
 		mnt_flags |= MNT_READONLY;
-	if (ms_flags & MS_NOSUID)
+	if (m_flags & M_NOSUID)
 		mnt_flags |= MNT_NOSUID;
-	if (ms_flags & MS_NODEV)
+	if (m_flags & M_NODEV)
 		mnt_flags |= MNT_NODEV;
-	if (ms_flags & MS_NOEXEC)
+	if (m_flags & M_NOEXEC)
 		mnt_flags |= MNT_NOEXEC;
-	if (ms_flags & MS_NODIRATIME)
+	if (m_flags & M_NODIRATIME)
 		mnt_flags |= MNT_NODIRATIME;
 
-	if (ms_flags & MS_STRICTATIME) {
-		if (ms_flags & MS_NOATIME)
-			return -EINVAL;
-	} else if (ms_flags & MS_NOATIME) {
+	if (m_flags & M_NOATIME)
 		mnt_flags |= MNT_NOATIME;
-	} else {
+	else if (!(m_flags & M_STRICTATIME))
 		mnt_flags |= MNT_RELATIME;
-	}
 
 	f = fdget(fs_fd);
 	if (!f.file)
diff --git a/include/uapi/linux/mount.h b/include/uapi/linux/mount.h
index 3634e065836c..fce3513fc242 100644
--- a/include/uapi/linux/mount.h
+++ b/include/uapi/linux/mount.h
@@ -1,6 +1,15 @@
 #ifndef _UAPI_LINUX_MOUNT_H
 #define _UAPI_LINUX_MOUNT_H
 
+/* Mount flags passed to fsmount(2) */
+#define M_RDONLY	0x01
+#define M_NOSUID	0x02
+#define M_NODEV		0x04
+#define M_NOEXEC	0x08
+#define M_NOATIME	0x10
+#define M_STRICTATIME	0x20
+#define M_NODIRATIME	0x40
+
 /*
  * These are the fs-independent mount-flags: up to 32 flags are supported
  *
-- 
2.14.3

Re: [PATCH 5/6] fsmount: do not use legacy MS_ flags

[David Howells]

Miklos Szeredi <mszeredi@redhat.com> wrote:

> What happens if we introduce new flags for fsmount(2) and are already out
> of flags for mount(2)?  I see a big mess that way.
> 
> So let's instead start a clean new set, to be used in the new API.

If we must.  But let's not call them just M_* please.  Let's call them
MOUNT_ATTR_* or something.

> The MS_RELATIME flag was accepted but ignored.  Simply leave this out of
> the new set, since "relatime" is the default.

Can we make RELATIME, STRICTATIME and NOATIME an enum rather than individual
flags?

	#define MOUNT_ATTR_RDONLY	0x01
	#define MOUNT_ATTR_NOSUID	0x02
	#define MOUNT_ATTR_NODEV	0x04
	#define MOUNT_ATTR_NOEXEC	0x08
	#define MOUNT_ATTR_RELATIME	0x00
	#define MOUNT_ATTR_NOATIME	0x10
	#define MOUNT_ATTR_STRICTATIME	0x20
	#define MOUNT_ATTR_ATIME_MASK	0x30
	#define MOUNT_ATTR_NODIRATIME	0x40

We can also use these for a mount_setattr() syscall:

	mount_setattr(int dfd, const char *path, unsigned int atflags,
		      unsigned int attr_values,
		      unsigned int attr_mask);

where atflags can potentially include AT_RECURSIVE.

David

Re: [PATCH 5/6] fsmount: do not use legacy MS_ flags

[Miklos Szeredi]

On Fri, Sep 21, 2018 at 5:07 PM, David Howells <dhowells@redhat.com> wrote:
> Miklos Szeredi <mszeredi@redhat.com> wrote:
>
>> What happens if we introduce new flags for fsmount(2) and are already out
>> of flags for mount(2)?  I see a big mess that way.
>>
>> So let's instead start a clean new set, to be used in the new API.
>
> If we must.  But let's not call them just M_* please.  Let's call them
> MOUNT_ATTR_* or something.

Oh well.

>> The MS_RELATIME flag was accepted but ignored.  Simply leave this out of
>> the new set, since "relatime" is the default.
>
> Can we make RELATIME, STRICTATIME and NOATIME an enum rather than individual
> flags?

Sure.

>
>         #define MOUNT_ATTR_RDONLY       0x01
>         #define MOUNT_ATTR_NOSUID       0x02
>         #define MOUNT_ATTR_NODEV        0x04
>         #define MOUNT_ATTR_NOEXEC       0x08
>         #define MOUNT_ATTR_RELATIME     0x00
>         #define MOUNT_ATTR_NOATIME      0x10
>         #define MOUNT_ATTR_STRICTATIME  0x20
>         #define MOUNT_ATTR_ATIME_MASK   0x30
>         #define MOUNT_ATTR_NODIRATIME   0x40
>
> We can also use these for a mount_setattr() syscall:
>
>         mount_setattr(int dfd, const char *path, unsigned int atflags,
>                       unsigned int attr_values,
>                       unsigned int attr_mask);
>
> where atflags can potentially include AT_RECURSIVE.

Indeed.  Also, shouldn't these include the propagation flags?

Thanks,
Miklos

Re: [PATCH 5/6] fsmount: do not use legacy MS_ flags

[David Howells]

Miklos Szeredi <miklos@szeredi.hu> wrote:

> Indeed.  Also, shouldn't these include the propagation flags?

I guess so, though I'd be tempted to put those in a separate set.  Also, I'm
not sure whether fsmount() should deal with prop flags or whether that should
be something move_mount() needs to deal with (ie. does propagation need
applying at the time of splicing, depending on the parent).

David

Re: [PATCH 5/6] fsmount: do not use legacy MS_ flags

[Christian Brauner]

On Fri, Sep 21, 2018 at 04:07:55PM +0100, David Howells wrote:
> Miklos Szeredi <mszeredi@redhat.com> wrote:
> 
> > What happens if we introduce new flags for fsmount(2) and are already out
> > of flags for mount(2)?  I see a big mess that way.
> > 
> > So let's instead start a clean new set, to be used in the new API.

If I may chime in. I think this is a really good idea.

> 
> If we must.  But let's not call them just M_* please.  Let's call them
> MOUNT_ATTR_* or something.
> 
> > The MS_RELATIME flag was accepted but ignored.  Simply leave this out of
> > the new set, since "relatime" is the default.
> 
> Can we make RELATIME, STRICTATIME and NOATIME an enum rather than individual
> flags?
> 
> 	#define MOUNT_ATTR_RDONLY	0x01
> 	#define MOUNT_ATTR_NOSUID	0x02
> 	#define MOUNT_ATTR_NODEV	0x04
> 	#define MOUNT_ATTR_NOEXEC	0x08
> 	#define MOUNT_ATTR_RELATIME	0x00
> 	#define MOUNT_ATTR_NOATIME	0x10
> 	#define MOUNT_ATTR_STRICTATIME	0x20
> 	#define MOUNT_ATTR_ATIME_MASK	0x30
> 	#define MOUNT_ATTR_NODIRATIME	0x40
> 
> We can also use these for a mount_setattr() syscall:

So a - maybe stupid - question I had when recently working on a patch
was how to add new mount properties in the new api.
What I would expect is that once the new mount API has landed the old
mount() syscall should not see any new features added since I take it
that we want userspace to very slowly over time have sufficient
incentive to only use the new mount API.

So from reading the patch I got the impression that superblock mount
options passed via fsconfig() are passed as strings like "ro" and are
translated into approriate objects (e.g. flags etc.) by the kernel. That
seems like a future proof mechanism to extend mount options for
userspace without having to worry about exceeding any integer types in
the future. Maybe this would make sense for non-superblock options as
well? I can see that this is less performant that checking flags and
string parsing is a thing that is not particularly nice but it would be
one option to solve the running out of flags problem.

> 
> 	mount_setattr(int dfd, const char *path, unsigned int atflags,
> 		      unsigned int attr_values,
> 		      unsigned int attr_mask);

If we go with the flag arguments wouldn't it make sense to use a larger
integer type?

> 
> where atflags can potentially include AT_RECURSIVE.

Very much in favor of having this operate recursively!

Christian

Re: [PATCH 5/6] fsmount: do not use legacy MS_ flags

[David Howells]

Christian Brauner <christian@brauner.io> wrote:

> So from reading the patch I got the impression that superblock mount
> options passed via fsconfig() are passed as strings like "ro" and are
> translated into approriate objects (e.g. flags etc.) by the kernel.

I'm having second throughts about that - at least for "ro": that one is
particularly problematic as it governs how source devices are opened.  I'm
kind of tempted to pass that as a flag to fsopen().

What I'd like to do in btrfs, ext4, etc. is open blockdevs as I get the
parameters that enumerate them - but I have to hold the looked-up paths till
the validate/get_tree stage so that I know the final ro/rw state before I can
do the opening.

> That seems like a future proof mechanism to extend mount options for
> userspace without having to worry about exceeding any integer types in the
> future. Maybe this would make sense for non-superblock options as well? I
> can see that this is less performant that checking flags and string parsing
> is a thing that is not particularly nice but it would be one option to solve
> the running out of flags problem.

Al disliked the idea of setting up a separate context to define the mount
options.

> > 	mount_setattr(int dfd, const char *path, unsigned int atflags,
> > 		      unsigned int attr_values,
> > 		      unsigned int attr_mask);
> 
> If we go with the flag arguments wouldn't it make sense to use a larger
> integer type?

You can't - at least not directly through syscall args.  They are 32-bit on a
32-bit system.

> > where atflags can potentially include AT_RECURSIVE.
> 
> Very much in favor of having this operate recursively!

It gets complicated with propagation.

David

Re: [PATCH 5/6] fsmount: do not use legacy MS_ flags

[Christian Brauner]

[-- Attachment #1: Type: text/plain, Size: 4568 bytes --]

On Fri, Sep 21, 2018 at 05:52:36PM +0100, David Howells wrote:
> Christian Brauner <christian@brauner.io> wrote:
> 
> > So from reading the patch I got the impression that superblock mount
> > options passed via fsconfig() are passed as strings like "ro" and are
> > translated into approriate objects (e.g. flags etc.) by the kernel.
> 
> I'm having second throughts about that - at least for "ro": that one is
> particularly problematic as it governs how source devices are opened.  I'm
> kind of tempted to pass that as a flag to fsopen().
> 
> What I'd like to do in btrfs, ext4, etc. is open blockdevs as I get the
> parameters that enumerate them - but I have to hold the looked-up paths till
> the validate/get_tree stage so that I know the final ro/rw state before I can
> do the opening.
> 
> > That seems like a future proof mechanism to extend mount options for
> > userspace without having to worry about exceeding any integer types in the
> > future. Maybe this would make sense for non-superblock options as well? I
> > can see that this is less performant that checking flags and string parsing
> > is a thing that is not particularly nice but it would be one option to solve
> > the running out of flags problem.
> 
> Al disliked the idea of setting up a separate context to define the mount
> options.

I hope thinking about mount flag exhaustion at this point is not too
hijacking this thread too much. But another option I was thinking about was to
split the mount flags into different sets where the sets can be
differentiated by a command.

enum {
        MOUNT_ATTR_PROPAGATION = 1
        MOUNT_ATTR_SECURITY
        MOUNT_ATTR_FSTIME
}

Let's say split mount propagation flags into a set identified by
MOUNT_ATTR_PROPAGATION:
#define MOUNT_ATTR_UNBINDABLE   (1<<0)  /* change to unbindable */
#define MOUNT_ATTR_PRIVATE      (1<<1)  /* change to private */
#define MOUNT_ATTR_SLAVE        (1<<2)  /* change to slave */
#define MOUNT_ATTR_SHARED       (1<<3)  /* change to shared */

and another set
MOUNT_ATTR_SECURITY:
#define MOUNT_ATTR_RDONLY        (1<<0) /* Mount read-only */
#define MOUNT_ATTR_NOSUID        (1<<1) /* Ignore suid and sgid bits */
#define MOUNT_ATTR_NODEV         (1<<2) /* Disallow access to device special files */
#define MOUNT_ATTR_NOEXEC        (1<<3) /* Disallow program execution */

and another set
MOUNT_ATTR_FSTIME:
#define MOUNT_ATTR_NOATIME      (1<<0)  /* Do not update access times. */
#define MOUNT_ATTR_NODIRATIME   (1<<1)  /* Do not update directory access times */

and so on...

So you could e.g. add another unsigned int cmd argument to
mount_setattr():

mount_setattr(int dfd, const char *path, unsigned int atflags,
              unsigned int attr_cmd,
	      unsigned int attr_values,
	      unsigned int attr_mask);

Then - similiar to fsconfig()'s FS_CONFIG_SET_{FLAG,STRING,...} - you
would pass:

/* set propagation */
mount_setattr(dfd, path, atflags,
              MOUNT_ATTR_PROPAGATION,
	      vals,
	      propagation_flagmask);

/* set security */
mount_setattr(dfd, path, atflags,
              MOUNT_ATTR_SECURITY,
	      vals,
	      security_flagmask);

/* set time */
mount_setattr(dfd, path, atflags,
              MOUNT_ATTR_FSTIME,
	      vals,
	      fstime_flagmask);

and then finally have an additional command like:

/* apply changes */
mount_setattr(dfd, path, atflags, MOUNT_ATTR_APPLY, vals, 0);

that applies all the properties.

In kernel you could then either have separate flags in the corresponding
structs of interest or bit arrays of arbitrary length or whatever.
Each of the flag setting commands before the MOUNT_ATTR_APPLY would then
perform verification whether the combination of flags makes sense and
immediately return back an error to userspace but only the
MOUNT_ATTR_APPLY call would actually commit the changes.

> 
> > > 	mount_setattr(int dfd, const char *path, unsigned int atflags,
> > > 		      unsigned int attr_values,
> > > 		      unsigned int attr_mask);
> > 
> > If we go with the flag arguments wouldn't it make sense to use a larger
> > integer type?
> 
> You can't - at least not directly through syscall args.  They are 32-bit on a
> 32-bit system.

Ah, 32bit systems. I tend to forget that they exist. :)

> 
> > > where atflags can potentially include AT_RECURSIVE.
> > 
> > Very much in favor of having this operate recursively!
> 
> It gets complicated with propagation.

Sure, I totally understand.

Christian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 5/6] fsmount: do not use legacy MS_ flags

[David Howells]

Christian Brauner <christian@brauner.io> wrote:

> mount_setattr(int dfd, const char *path, unsigned int atflags,
>               unsigned int attr_cmd,
> 	      unsigned int attr_values,
> 	      unsigned int attr_mask);

Whilst you can have up to six arguments on a syscall, I seem to remember that
6-arg syscalls require special handlers on some systems due to register count.

David

Re: [PATCH 5/6] fsmount: do not use legacy MS_ flags

[Christian Brauner]

[-- Attachment #1: Type: text/plain, Size: 808 bytes --]

On Sat, Sep 22, 2018 at 04:48:32PM +0100, David Howells wrote:
> Christian Brauner <christian@brauner.io> wrote:
> 
> > mount_setattr(int dfd, const char *path, unsigned int atflags,
> >               unsigned int attr_cmd,
> > 	      unsigned int attr_values,
> > 	      unsigned int attr_mask);
> 
> Whilst you can have up to six arguments on a syscall, I seem to remember that
> 6-arg syscalls require special handlers on some systems due to register count.

Sure, but if we wanted to we might just get the syscall down to five
arguments even with my suggestion. Of course, I'm not sure what the
reasons for all of the other arguments to this function are since it's
not yet implemented. Seems that attr_values and attr_mask could be
compacted to a single attr_mask maybe? :)

Christian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 5/6] fsmount: do not use legacy MS_ flags

[David Howells]

Christian Brauner <christian@brauner.io> wrote:

> Of course, I'm not sure what the reasons for all of the other arguments to
> this function are since it's not yet implemented.

Well, dfd, path and atflags are pretty standard.  atflags conveys things like
AT_EMPTY_PATH or AT_SYMLINK_NOFOLLOW and dfd conveys a file descriptor
pointing to a vfs object or AT_FDCWD.

> Seems that attr_values and attr_mask could be compacted to a single
> attr_mask maybe?

If you don't have a mask, you can't really do recursion.  Without the mask,
you have to supply the entire set of options absolutely - and this would get
stamped on everything in the target range.

With a mask in combination with the set of desired values, you can turn on or
off a specific subset of the attributes without affecting the rest - without
needing to know the rest.

David

Re: [PATCH 5/6] fsmount: do not use legacy MS_ flags

[Christian Brauner]

[-- Attachment #1: Type: text/plain, Size: 1539 bytes --]

On Sun, Sep 23, 2018 at 11:45:17PM +0100, David Howells wrote:
> Christian Brauner <christian@brauner.io> wrote:
> 
> > Of course, I'm not sure what the reasons for all of the other arguments to
> > this function are since it's not yet implemented.
> 
> Well, dfd, path and atflags are pretty standard.  atflags conveys things like
> AT_EMPTY_PATH or AT_SYMLINK_NOFOLLOW and dfd conveys a file descriptor
> pointing to a vfs object or AT_FDCWD.
> 
> > Seems that attr_values and attr_mask could be compacted to a single
> > attr_mask maybe?
> 
> If you don't have a mask, you can't really do recursion.  Without the mask,
> you have to supply the entire set of options absolutely - and this would get
> stamped on everything in the target range.
> 
> With a mask in combination with the set of desired values, you can turn on or
> off a specific subset of the attributes without affecting the rest - without
> needing to know the rest.

Ok, understood. What about passing the different attrs as a struct?

struct mount_attr {
        unsigned int attr_cmd,
        unsigned int attr_values,
        unsigned int attr_mask,

};

mount_setattr(int dfd, const char *path, unsigned int atflags,
              struct mount_attr *attr);

I find that to be a little cleaner in all honesty.
One could also add a version argument similar to what we currently do
for vfs fcaps so that kernel and userspace can easily navigate
compabitility when a new member gets added or removed in later releases.

Christian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 5/6] fsmount: do not use legacy MS_ flags

[David Howells]

Christian Brauner <christian@brauner.io> wrote:

> Ok, understood. What about passing the different attrs as a struct?
> 
> struct mount_attr {
>         unsigned int attr_cmd,
>         unsigned int attr_values,
>         unsigned int attr_mask,
> 
> };
> 
> mount_setattr(int dfd, const char *path, unsigned int atflags,
>               struct mount_attr *attr);
> 
> I find that to be a little cleaner in all honesty.
> One could also add a version argument similar to what we currently do
> for vfs fcaps so that kernel and userspace can easily navigate
> compabitility when a new member gets added or removed in later releases.

Yeah, we could do that - it's not like I expect mount_setattr() to have to be
particularly performant in the user interface.  I would put the attr_cmd in
the argument list, probably, so that you can use that to vary the struct in
future (say we run out of attribute bits).

David

Re: [PATCH 5/6] fsmount: do not use legacy MS_ flags

[Christian Brauner]

[-- Attachment #1: Type: text/plain, Size: 3595 bytes --]

On Mon, Sep 24, 2018 at 07:50:38AM +0100, David Howells wrote:
> Christian Brauner <christian@brauner.io> wrote:
> 
> > Ok, understood. What about passing the different attrs as a struct?
> > 
> > struct mount_attr {
> >         unsigned int attr_cmd,
> >         unsigned int attr_values,
> >         unsigned int attr_mask,
> > 
> > };
> > 
> > mount_setattr(int dfd, const char *path, unsigned int atflags,
> >               struct mount_attr *attr);
> > 
> > I find that to be a little cleaner in all honesty.
> > One could also add a version argument similar to what we currently do
> > for vfs fcaps so that kernel and userspace can easily navigate
> > compabitility when a new member gets added or removed in later releases.
> 
> Yeah, we could do that - it's not like I expect mount_setattr() to have to be
> particularly performant in the user interface.  I would put the attr_cmd in
> the argument list, probably, so that you can use that to vary the struct in
> future (say we run out of attribute bits).

Yes, that makes sense and mimicks standard ioctl() behavior. So

struct mount_attr {
        unsigned int attr_values,
        unsigned int attr_mask,
}

mount_setattr(int dfd, const char *path, unsigned int atflags,
              unsigned int attr_cmd, struct mount_attr *attr);

I have thought a little more about splitting up the mount flags into
sensible sets. I think the following four sets make sense:

enum {
        MOUNT_ATTR_PROPAGATION = 1,
        MOUNT_ATTR_SECURITY,
        MOUNT_ATTR_SYNC,
        MOUNT_ATTR_TIME,
};

MOUNT_ATTR_PROPAGATION:
#define MOUNT_ATTR_PRIVATE    (1<<0)
#define MOUNT_ATTR_SHARED     (1<<1)
#define MOUNT_ATTR_SLAVE      (1<<2)
#define MOUNT_ATTR_UNBINDABLE (1<<3)

MOUNT_ATTR_SECURITY:
#define MOUNT_ATTR_MANDLOCK     (1<<0)
#define MOUNT_ATTR_NODEV        (1<<1)   
#define MOUNT_ATTR_NOEXEC       (1<<2)  
#define MOUNT_ATTR_NOSUID       (1<<3)  
#define MOUNT_ATTR_NOREMOTELOCK (1<<4)
#define MOUNT_ATTR_RDONLY       (1<<5)  
#define MOUNT_ATTR_POSIXACL     (1<<6)
#define MOUNT_ATTR_SILENT       (1<<7)  

MOUNT_ATTR_SYNC
#define MOUNT_ATTR_DIRSYNC     (1<<0)
#define MOUNT_ATTR_SYNCHRONOUS (1<<1)

MOUNT_ATTR_TIME:
#define MOUNT_ATTR_LAZYTIME    (1<<0)
#define MOUNT_ATTR_NOATIME     (1<<1)
#define MOUNT_ATTR_NODIRATIME  (1<<2)
#define MOUNT_ATTR_RELATIME    (1<<3)
#define MOUNT_ATTR_STRICTATIME (1<<4)

If we ever run out of flags in a specific set I suggest to introduce a
new enum member of the same name with a version number appended and an
alias with a (obvs lower) version number for the old set. A concrete
example would be:

enum {
        MOUNT_ATTR_PROPAGATION = 1,
        MOUNT_ATTR_SECURITY,
        MOUNT_ATTR_SECURITY_1 = MOUNT_ATTR_SECURITY,
        MOUNT_ATTR_SYNC,
        MOUNT_ATTR_TIME,
        MOUNT_ATTR_SECURITY_2,
};

These flags will likely become AT_* flags or be tied to a syscall
afaict.

#define MS_REMOUNT      32
#define MS_BIND	        4096
#define MS_MOVE	        8192
#define MS_REC	        16384

Internal sb flags will not be part of the new mount attr sets. (They
should - imho - not be exposed to userspace at all.):

#define MS_KERNMOUNT    (1<<22)
#define MS_SUBMOUNT     (1<<26)
#define MS_NOREMOTELOCK (1<<27)
#define MS_NOSEC        (1<<28)
#define MS_BORN	        (1<<29)
#define MS_ACTIVE       (1<<30)
#define MS_NOUSER       (1<<31)

What remains is an odd duck that probably could be thrown into security
but also *shrug*

#define MS_I_VERSION    (1<<23)

Christian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 5/6] fsmount: do not use legacy MS_ flags

[David Howells]

Christian Brauner <christian@brauner.io> wrote:

> I have thought a little more about splitting up the mount flags into
> sensible sets. I think the following four sets make sense:
>
> enum {
>         MOUNT_ATTR_PROPAGATION = 1,
>         MOUNT_ATTR_SECURITY,
>         MOUNT_ATTR_SYNC,
>         MOUNT_ATTR_TIME,
> };

Al (I think it was) has been against splitting them up before (I've previously
proposed splitting the topology propagation flags from the mount attributes).

> #define MOUNT_ATTR_NOATIME     (1<<1)
> #define MOUNT_ATTR_RELATIME    (1<<3)
> #define MOUNT_ATTR_STRICTATIME (1<<4)

These aren't independent, but are actually settings on the same dial, so I
would suggest that they shouldn't be separate flags.  I'm not sure about
LAZYTIME though.

> enum {
>         MOUNT_ATTR_PROPAGATION = 1,
>         MOUNT_ATTR_SECURITY,
>         MOUNT_ATTR_SECURITY_1 = MOUNT_ATTR_SECURITY,
>         MOUNT_ATTR_SYNC,
>         MOUNT_ATTR_TIME,
>         MOUNT_ATTR_SECURITY_2,
> };

In UAPI headers, always explicitly number your symbols, even in an enum, just
to make sure that the numbers don't get transparently changed by accident.

> These flags will likely become AT_* flags or be tied to a syscall
> afaict.
>
> #define MS_REMOUNT      32
> #define MS_BIND	        4096
> #define MS_MOVE	        8192
> #define MS_REC	        16384

MS_REMOUNT: fd = fspick(); fscommand(fd, FSCONFIG_CMD_RECONFIGURE);

MS_REMOUNT|MS_BIND: mount_setattr().

MS_BIND: fd = open_tree(..., OPEN_TREE_CLONE); move_mount(fd, "", ...);

MS_MOVE: fd = open_tree(..., 0); move_mount(fd, "", ...);

MS_REC: AT_RECURSIVE

> Internal sb flags will not be part of the new mount attr sets. (They
> should - imho - not be exposed to userspace at all.):

Agreed.

> What remains is an odd duck that probably could be thrown into security
> but also *shrug*
>
> #define MS_I_VERSION    (1<<23)

Um.  I think it would probably belong with atime settings.

David

Re: [PATCH 5/6] fsmount: do not use legacy MS_ flags

[Christian Brauner]

[-- Attachment #1: Type: text/plain, Size: 3128 bytes --]

On Mon, Sep 24, 2018 at 01:37:45PM +0100, David Howells wrote:
> Christian Brauner <christian@brauner.io> wrote:
> 
> > I have thought a little more about splitting up the mount flags into
> > sensible sets. I think the following four sets make sense:
> >
> > enum {
> >         MOUNT_ATTR_PROPAGATION = 1,
> >         MOUNT_ATTR_SECURITY,
> >         MOUNT_ATTR_SYNC,
> >         MOUNT_ATTR_TIME,
> > };
> 
> Al (I think it was) has been against splitting them up before (I've previously
> proposed splitting the topology propagation flags from the mount attributes).

Right, that request could probably be fulfilled by the first draft for
this idea that I had but didn't send out.
Basically, having a sequential enum that only ever gets bumped when we
run out of flags in a set, i.e.

enum {
        MOUNT_ATTR_SET_1 = 1,
        MOUNT_ATTR_SET_2 = 2,
        MOUNT_ATTR_SET_3 = 3,
        .
        .
        .
};

Then we would currently only define a single set
enum {
        MOUNT_ATTR_SET_1 = 1,
};

dump all the current mount flags we would like to support in there and
call it a day until we run out of flags at which point we introduce
MOUNT_ATTR_SET_2.
But honestly, I find defining cuts by forming sets of logically related
flags to be more intuitive and transparent for kernel- and userspace.
But I'm just another muppet with an opinion. :)

> 
> > #define MOUNT_ATTR_NOATIME     (1<<1)
> > #define MOUNT_ATTR_RELATIME    (1<<3)
> > #define MOUNT_ATTR_STRICTATIME (1<<4)
> 
> These aren't independent, but are actually settings on the same dial, so I
> would suggest that they shouldn't be separate flags.  I'm not sure about
> LAZYTIME though.

So what you or Miklos suggested before, namely making them an enum too?

> 
> > enum {
> >         MOUNT_ATTR_PROPAGATION = 1,
> >         MOUNT_ATTR_SECURITY,
> >         MOUNT_ATTR_SECURITY_1 = MOUNT_ATTR_SECURITY,
> >         MOUNT_ATTR_SYNC,
> >         MOUNT_ATTR_TIME,
> >         MOUNT_ATTR_SECURITY_2,
> > };
> 
> In UAPI headers, always explicitly number your symbols, even in an enum, just
> to make sure that the numbers don't get transparently changed by accident.

+1 and thanks for the tip!

> 
> > These flags will likely become AT_* flags or be tied to a syscall
> > afaict.
> >
> > #define MS_REMOUNT      32
> > #define MS_BIND	        4096
> > #define MS_MOVE	        8192
> > #define MS_REC	        16384
> 
> MS_REMOUNT: fd = fspick(); fscommand(fd, FSCONFIG_CMD_RECONFIGURE);
> 
> MS_REMOUNT|MS_BIND: mount_setattr().
> 
> MS_BIND: fd = open_tree(..., OPEN_TREE_CLONE); move_mount(fd, "", ...);
> 
> MS_MOVE: fd = open_tree(..., 0); move_mount(fd, "", ...);
> 
> MS_REC: AT_RECURSIVE
> 
> > Internal sb flags will not be part of the new mount attr sets. (They
> > should - imho - not be exposed to userspace at all.):
> 
> Agreed.
> 
> > What remains is an odd duck that probably could be thrown into security
> > but also *shrug*
> >
> > #define MS_I_VERSION    (1<<23)
> 
> Um.  I think it would probably belong with atime settings.
> 
> David

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 6/6] fsconfig: rename FSCONFIG_CMD_CREATE to FSCONFIG_CMD_OBTAIN

[Miklos Szeredi]

The old name strongly implies that a new superblock will be created from
the fs_context.  This is not true: filesystems are free to retuse an
existing superblock and return that (for good reason).

To avoid confusion, rename this command to something more appropriate.

Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
---
 fs/fsopen.c                | 6 +++---
 include/uapi/linux/fs.h    | 2 +-
 samples/vfs/test-fsmount.c | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/fs/fsopen.c b/fs/fsopen.c
index 9ead9220e2cb..bf1ff70b1fe2 100644
--- a/fs/fsopen.c
+++ b/fs/fsopen.c
@@ -311,7 +311,7 @@ static int vfs_fsconfig_action(struct fs_context *fc, enum fsconfig_command cmd)
 	int ret = -EINVAL;
 
 	switch (cmd) {
-	case FSCONFIG_CMD_CREATE:
+	case FSCONFIG_CMD_OBTAIN:
 		if (fc->phase != FS_CONTEXT_CREATE_PARAMS)
 			return -EBUSY;
 		fc->phase = FS_CONTEXT_CREATING;
@@ -434,7 +434,7 @@ SYSCALL_DEFINE5(fsconfig,
 		if (!_key || _value || aux < 0)
 			return -EINVAL;
 		break;
-	case FSCONFIG_CMD_CREATE:
+	case FSCONFIG_CMD_OBTAIN:
 	case FSCONFIG_CMD_RECONFIGURE:
 		if (_key || _value || aux)
 			return -EINVAL;
@@ -523,7 +523,7 @@ SYSCALL_DEFINE5(fsconfig,
 	ret = mutex_lock_interruptible(&fc->uapi_mutex);
 	if (ret == 0) {
 		switch (cmd) {
-		case FSCONFIG_CMD_CREATE:
+		case FSCONFIG_CMD_OBTAIN:
 		case FSCONFIG_CMD_RECONFIGURE:
 			ret = vfs_fsconfig_action(fc, cmd);
 			break;
diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h
index 7f01503a9e9b..169642bb2656 100644
--- a/include/uapi/linux/fs.h
+++ b/include/uapi/linux/fs.h
@@ -366,7 +366,7 @@ 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_OBTAIN	= 6,	/* Obtain new or existing superblock */
 	FSCONFIG_CMD_RECONFIGURE = 7,	/* Invoke superblock reconfiguration */
 };
 
diff --git a/samples/vfs/test-fsmount.c b/samples/vfs/test-fsmount.c
index 74124025ade0..a161dc530a91 100644
--- a/samples/vfs/test-fsmount.c
+++ b/samples/vfs/test-fsmount.c
@@ -101,7 +101,7 @@ int main(int argc, char *argv[])
 	}
 
 	E_fsconfig(fsfd, FSCONFIG_SET_STRING, "source", "#grand.central.org:root.cell.", 0);
-	E_fsconfig(fsfd, FSCONFIG_CMD_CREATE, NULL, NULL, 0);
+	E_fsconfig(fsfd, FSCONFIG_CMD_OBTAIN, NULL, NULL, 0);
 
 	mfd = fsmount(fsfd, 0, MS_RDONLY);
 	if (mfd < 0)
-- 
2.14.3

Re: [PATCH 6/6] fsconfig: rename FSCONFIG_CMD_CREATE to FSCONFIG_CMD_OBTAIN

[David Howells]

Miklos Szeredi <mszeredi@redhat.com> wrote:

> The old name strongly implies that a new superblock will be created from
> the fs_context.  This is not true: filesystems are free to retuse an
> existing superblock and return that (for good reason).

Kind of like open(O_CREAT) only ever creates files, right;-)

Actually, FSCONFIG_CMD_OPEN might be a better name.

David

Re: [PATCH 6/6] fsconfig: rename FSCONFIG_CMD_CREATE to FSCONFIG_CMD_OBTAIN

[Miklos Szeredi]

On Fri, Sep 21, 2018 at 5:11 PM, David Howells <dhowells@redhat.com> wrote:
> Miklos Szeredi <mszeredi@redhat.com> wrote:
>
>> The old name strongly implies that a new superblock will be created from
>> the fs_context.  This is not true: filesystems are free to retuse an
>> existing superblock and return that (for good reason).
>
> Kind of like open(O_CREAT) only ever creates files, right;-)
>
> Actually, FSCONFIG_CMD_OPEN might be a better name.

We've already opened the context with fsopen() and about to open a
file referring to a subtree with fsmount().  And this one doesn't
actually involve opening any files, so IMO it should not be called
_OPEN.

Thanks,
Miklos

Re: [PATCH 0/6] mount-api: fixes and cleanups

[David Howells]

Hi Miklós,

I've reposted my patches with some of your changes folded in.  I will consider
the other things, but I don't have time to do that before Monday.  I think I
should also have a go at porting fuse so that we can get the subtype bits
done.

Note that I have a perl script that does some of the initial work.  It needs a
bit of fixing up, but I'll post that in a mail at some point.

David