[PATCH 0/2] vfs: convert debugfs & tracefs to the new mount API

[PATCH 0/2] vfs: convert debugfs & tracefs to the new mount API

[Eric Sandeen]

Since debugfs and tracefs are cut & pasted one way or the other,
do these at the same time.

Both of these patches originated in dhowells' tree at
https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/log/?h=mount-api-viro

https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/commit/?h=mount-api-viro&id=ec14be9e2aa76f63458466bba86256e123ec4e51
and
https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/commit/?h=mount-api-viro&id=c4f2e60465859e02a6e36ed618dbaea16de8c8e0

I've forward-ported them to the mount API that landed, and
fixed up remounting; ->reconfigure() needed to copy the
parsed context options into the current superblock options
to effect any remount changes.

While these do use the invalf() functions for some errors, they
are new messages, not messages that used to go to dmesg that
would be lost if userspace isn't listening.

I've done minimal testing - booting with the patches, testing
some of the remount behavior for mode & uid.
Oh, and I built it too. </brown_paper_bag>

thanks,
-Eric

[PATCH 1/2] vfs: Convert debugfs to use the new mount API

[Eric Sandeen]

From: David Howells <dhowells@redhat.com>

Convert the debugfs filesystem to the new internal mount API as the old
one will be obsoleted and removed.  This allows greater flexibility in
communication of mount parameters between userspace, the VFS and the
filesystem.

See Documentation/filesystems/mount_api.txt for more information.

Signed-off-by: David Howells <dhowells@redhat.com>
Co-developed-by: Eric Sandeen <sandeen@redhat.com>
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
[sandeen: forward port to modern kernel, fix remounting]
cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
cc: "Rafael J. Wysocki" <rafael@kernel.org>
---
 fs/debugfs/inode.c | 198 +++++++++++++++++++++------------------------
 1 file changed, 93 insertions(+), 105 deletions(-)

diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
index 034a617cb1a5..c2adfb272da8 100644
--- a/fs/debugfs/inode.c
+++ b/fs/debugfs/inode.c
@@ -14,7 +14,8 @@
 
 #include <linux/module.h>
 #include <linux/fs.h>
-#include <linux/mount.h>
+#include <linux/fs_context.h>
+#include <linux/fs_parser.h>
 #include <linux/pagemap.h>
 #include <linux/init.h>
 #include <linux/kobject.h>
@@ -23,7 +24,6 @@
 #include <linux/fsnotify.h>
 #include <linux/string.h>
 #include <linux/seq_file.h>
-#include <linux/parser.h>
 #include <linux/magic.h>
 #include <linux/slab.h>
 #include <linux/security.h>
@@ -77,7 +77,7 @@ static struct inode *debugfs_get_inode(struct super_block *sb)
 	return inode;
 }
 
-struct debugfs_mount_opts {
+struct debugfs_fs_info {
 	kuid_t uid;
 	kgid_t gid;
 	umode_t mode;
@@ -89,68 +89,51 @@ enum {
 	Opt_uid,
 	Opt_gid,
 	Opt_mode,
-	Opt_err
 };
 
-static const match_table_t tokens = {
-	{Opt_uid, "uid=%u"},
-	{Opt_gid, "gid=%u"},
-	{Opt_mode, "mode=%o"},
-	{Opt_err, NULL}
+static const struct fs_parameter_spec debugfs_param_specs[] = {
+	fsparam_u32	("gid",		Opt_gid),
+	fsparam_u32oct	("mode",	Opt_mode),
+	fsparam_u32	("uid",		Opt_uid),
+	{}
 };
 
-struct debugfs_fs_info {
-	struct debugfs_mount_opts mount_opts;
-};
-
-static int debugfs_parse_options(char *data, struct debugfs_mount_opts *opts)
+static int debugfs_parse_param(struct fs_context *fc, struct fs_parameter *param)
 {
-	substring_t args[MAX_OPT_ARGS];
-	int option;
-	int token;
+	struct debugfs_fs_info *opts = fc->s_fs_info;
+	struct fs_parse_result result;
 	kuid_t uid;
 	kgid_t gid;
-	char *p;
-
-	opts->opts = 0;
-	opts->mode = DEBUGFS_DEFAULT_MODE;
-
-	while ((p = strsep(&data, ",")) != NULL) {
-		if (!*p)
-			continue;
-
-		token = match_token(p, tokens, args);
-		switch (token) {
-		case Opt_uid:
-			if (match_int(&args[0], &option))
-				return -EINVAL;
-			uid = make_kuid(current_user_ns(), option);
-			if (!uid_valid(uid))
-				return -EINVAL;
-			opts->uid = uid;
-			break;
-		case Opt_gid:
-			if (match_int(&args[0], &option))
-				return -EINVAL;
-			gid = make_kgid(current_user_ns(), option);
-			if (!gid_valid(gid))
-				return -EINVAL;
-			opts->gid = gid;
-			break;
-		case Opt_mode:
-			if (match_octal(&args[0], &option))
-				return -EINVAL;
-			opts->mode = option & S_IALLUGO;
-			break;
-		/*
-		 * We might like to report bad mount options here;
-		 * but traditionally debugfs has ignored all mount options
-		 */
-		}
-
-		opts->opts |= BIT(token);
+	int opt;
+
+	opt = fs_parse(fc, debugfs_param_specs, param, &result);
+	if (opt < 0)
+		return opt;
+
+	switch (opt) {
+	case Opt_uid:
+		uid = make_kuid(current_user_ns(), result.uint_32);
+		if (!uid_valid(uid))
+			return invalf(fc, "Unknown uid");
+		opts->uid = uid;
+		break;
+	case Opt_gid:
+		gid = make_kgid(current_user_ns(), result.uint_32);
+		if (!gid_valid(gid))
+			return invalf(fc, "Unknown gid");
+		opts->gid = gid;
+		break;
+	case Opt_mode:
+		opts->mode = result.uint_32 & S_IALLUGO;
+		break;
+	/*
+	 * We might like to report bad mount options here;
+	 * but traditionally debugfs has ignored all mount options
+	 */
 	}
 
+	opts->opts |= BIT(opt);
+
 	return 0;
 }
 
@@ -158,23 +141,22 @@ static void _debugfs_apply_options(struct super_block *sb, bool remount)
 {
 	struct debugfs_fs_info *fsi = sb->s_fs_info;
 	struct inode *inode = d_inode(sb->s_root);
-	struct debugfs_mount_opts *opts = &fsi->mount_opts;
 
 	/*
 	 * On remount, only reset mode/uid/gid if they were provided as mount
 	 * options.
 	 */
 
-	if (!remount || opts->opts & BIT(Opt_mode)) {
+	if (!remount || fsi->opts & BIT(Opt_mode)) {
 		inode->i_mode &= ~S_IALLUGO;
-		inode->i_mode |= opts->mode;
+		inode->i_mode |= fsi->mode;
 	}
 
-	if (!remount || opts->opts & BIT(Opt_uid))
-		inode->i_uid = opts->uid;
+	if (!remount || fsi->opts & BIT(Opt_uid))
+		inode->i_uid = fsi->uid;
 
-	if (!remount || opts->opts & BIT(Opt_gid))
-		inode->i_gid = opts->gid;
+	if (!remount || fsi->opts & BIT(Opt_gid))
+		inode->i_gid = fsi->gid;
 }
 
 static void debugfs_apply_options(struct super_block *sb)
@@ -187,35 +169,33 @@ static void debugfs_apply_options_remount(struct super_block *sb)
 	_debugfs_apply_options(sb, true);
 }
 
-static int debugfs_remount(struct super_block *sb, int *flags, char *data)
+static int debugfs_reconfigure(struct fs_context *fc)
 {
-	int err;
-	struct debugfs_fs_info *fsi = sb->s_fs_info;
+	struct super_block *sb = fc->root->d_sb;
+	struct debugfs_fs_info *sb_opts = sb->s_fs_info;
+	struct debugfs_fs_info *new_opts = fc->s_fs_info;
 
 	sync_filesystem(sb);
-	err = debugfs_parse_options(data, &fsi->mount_opts);
-	if (err)
-		goto fail;
 
+	/* structure copy of new mount options to sb */
+	*sb_opts = *new_opts;
 	debugfs_apply_options_remount(sb);
 
-fail:
-	return err;
+	return 0;
 }
 
 static int debugfs_show_options(struct seq_file *m, struct dentry *root)
 {
 	struct debugfs_fs_info *fsi = root->d_sb->s_fs_info;
-	struct debugfs_mount_opts *opts = &fsi->mount_opts;
 
-	if (!uid_eq(opts->uid, GLOBAL_ROOT_UID))
+	if (!uid_eq(fsi->uid, GLOBAL_ROOT_UID))
 		seq_printf(m, ",uid=%u",
-			   from_kuid_munged(&init_user_ns, opts->uid));
-	if (!gid_eq(opts->gid, GLOBAL_ROOT_GID))
+			   from_kuid_munged(&init_user_ns, fsi->uid));
+	if (!gid_eq(fsi->gid, GLOBAL_ROOT_GID))
 		seq_printf(m, ",gid=%u",
-			   from_kgid_munged(&init_user_ns, opts->gid));
-	if (opts->mode != DEBUGFS_DEFAULT_MODE)
-		seq_printf(m, ",mode=%o", opts->mode);
+			   from_kgid_munged(&init_user_ns, fsi->gid));
+	if (fsi->mode != DEBUGFS_DEFAULT_MODE)
+		seq_printf(m, ",mode=%o", fsi->mode);
 
 	return 0;
 }
@@ -229,7 +209,6 @@ static void debugfs_free_inode(struct inode *inode)
 
 static const struct super_operations debugfs_super_operations = {
 	.statfs		= simple_statfs,
-	.remount_fs	= debugfs_remount,
 	.show_options	= debugfs_show_options,
 	.free_inode	= debugfs_free_inode,
 };
@@ -263,26 +242,14 @@ static const struct dentry_operations debugfs_dops = {
 	.d_automount = debugfs_automount,
 };
 
-static int debug_fill_super(struct super_block *sb, void *data, int silent)
+static int debugfs_fill_super(struct super_block *sb, struct fs_context *fc)
 {
 	static const struct tree_descr debug_files[] = {{""}};
-	struct debugfs_fs_info *fsi;
 	int err;
 
-	fsi = kzalloc(sizeof(struct debugfs_fs_info), GFP_KERNEL);
-	sb->s_fs_info = fsi;
-	if (!fsi) {
-		err = -ENOMEM;
-		goto fail;
-	}
-
-	err = debugfs_parse_options(data, &fsi->mount_opts);
+	err = simple_fill_super(sb, DEBUGFS_MAGIC, debug_files);
 	if (err)
-		goto fail;
-
-	err  =  simple_fill_super(sb, DEBUGFS_MAGIC, debug_files);
-	if (err)
-		goto fail;
+		return err;
 
 	sb->s_op = &debugfs_super_operations;
 	sb->s_d_op = &debugfs_dops;
@@ -290,27 +257,48 @@ static int debug_fill_super(struct super_block *sb, void *data, int silent)
 	debugfs_apply_options(sb);
 
 	return 0;
-
-fail:
-	kfree(fsi);
-	sb->s_fs_info = NULL;
-	return err;
 }
 
-static struct dentry *debug_mount(struct file_system_type *fs_type,
-			int flags, const char *dev_name,
-			void *data)
+static int debugfs_get_tree(struct fs_context *fc)
 {
 	if (!(debugfs_allow & DEBUGFS_ALLOW_API))
-		return ERR_PTR(-EPERM);
+		return -EPERM;
+
+	return get_tree_single(fc, debugfs_fill_super);
+}
+
+static void debugfs_free_fc(struct fs_context *fc)
+{
+	kfree(fc->s_fs_info);
+}
 
-	return mount_single(fs_type, flags, data, debug_fill_super);
+static const struct fs_context_operations debugfs_context_ops = {
+	.free		= debugfs_free_fc,
+	.parse_param	= debugfs_parse_param,
+	.get_tree	= debugfs_get_tree,
+	.reconfigure	= debugfs_reconfigure,
+};
+
+static int debugfs_init_fs_context(struct fs_context *fc)
+{
+	struct debugfs_fs_info *fsi;
+
+	fsi = kzalloc(sizeof(struct debugfs_fs_info), GFP_KERNEL);
+	if (!fsi)
+		return -ENOMEM;
+
+	fsi->mode = DEBUGFS_DEFAULT_MODE;
+
+	fc->s_fs_info = fsi;
+	fc->ops = &debugfs_context_ops;
+	return 0;
 }
 
 static struct file_system_type debug_fs_type = {
 	.owner =	THIS_MODULE,
 	.name =		"debugfs",
-	.mount =	debug_mount,
+	.init_fs_context = debugfs_init_fs_context,
+	.parameters =	debugfs_param_specs,
 	.kill_sb =	kill_litter_super,
 };
 MODULE_ALIAS_FS("debugfs");
-- 
2.43.0

[PATCH 2/2] vfs: Convert tracefs to use the new mount API

[Eric Sandeen]

From: David Howells <dhowells@redhat.com>

Convert the tracefs filesystem to the new internal mount API as the old
one will be obsoleted and removed.  This allows greater flexibility in
communication of mount parameters between userspace, the VFS and the
filesystem.

See Documentation/filesystems/mount_api.txt for more information.

Signed-off-by: David Howells <dhowells@redhat.com>
Co-developed-by: Eric Sandeen <sandeen@redhat.com>
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
[sandeen: forward port to modern kernel, fix remounting]
cc: Steven Rostedt <rostedt@goodmis.org>
cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 fs/tracefs/inode.c | 196 +++++++++++++++++++++------------------------
 1 file changed, 91 insertions(+), 105 deletions(-)

diff --git a/fs/tracefs/inode.c b/fs/tracefs/inode.c
index d65ffad4c327..69d0fb2e03a2 100644
--- a/fs/tracefs/inode.c
+++ b/fs/tracefs/inode.c
@@ -11,14 +11,14 @@
 
 #include <linux/module.h>
 #include <linux/fs.h>
-#include <linux/mount.h>
+#include <linux/fs_context.h>
+#include <linux/fs_parser.h>
 #include <linux/kobject.h>
 #include <linux/namei.h>
 #include <linux/tracefs.h>
 #include <linux/fsnotify.h>
 #include <linux/security.h>
 #include <linux/seq_file.h>
-#include <linux/parser.h>
 #include <linux/magic.h>
 #include <linux/slab.h>
 #include "internal.h"
@@ -231,7 +231,7 @@ struct inode *tracefs_get_inode(struct super_block *sb)
 	return inode;
 }
 
-struct tracefs_mount_opts {
+struct tracefs_fs_info {
 	kuid_t uid;
 	kgid_t gid;
 	umode_t mode;
@@ -243,68 +243,51 @@ enum {
 	Opt_uid,
 	Opt_gid,
 	Opt_mode,
-	Opt_err
 };
 
-static const match_table_t tokens = {
-	{Opt_uid, "uid=%u"},
-	{Opt_gid, "gid=%u"},
-	{Opt_mode, "mode=%o"},
-	{Opt_err, NULL}
+static const struct fs_parameter_spec tracefs_param_specs[] = {
+	fsparam_u32	("gid",		Opt_gid),
+	fsparam_u32oct	("mode",	Opt_mode),
+	fsparam_u32	("uid",		Opt_uid),
+	{}
 };
 
-struct tracefs_fs_info {
-	struct tracefs_mount_opts mount_opts;
-};
-
-static int tracefs_parse_options(char *data, struct tracefs_mount_opts *opts)
+static int tracefs_parse_param(struct fs_context *fc, struct fs_parameter *param)
 {
-	substring_t args[MAX_OPT_ARGS];
-	int option;
-	int token;
+	struct tracefs_fs_info *opts = fc->s_fs_info;
+	struct fs_parse_result result;
 	kuid_t uid;
 	kgid_t gid;
-	char *p;
-
-	opts->opts = 0;
-	opts->mode = TRACEFS_DEFAULT_MODE;
-
-	while ((p = strsep(&data, ",")) != NULL) {
-		if (!*p)
-			continue;
-
-		token = match_token(p, tokens, args);
-		switch (token) {
-		case Opt_uid:
-			if (match_int(&args[0], &option))
-				return -EINVAL;
-			uid = make_kuid(current_user_ns(), option);
-			if (!uid_valid(uid))
-				return -EINVAL;
-			opts->uid = uid;
-			break;
-		case Opt_gid:
-			if (match_int(&args[0], &option))
-				return -EINVAL;
-			gid = make_kgid(current_user_ns(), option);
-			if (!gid_valid(gid))
-				return -EINVAL;
-			opts->gid = gid;
-			break;
-		case Opt_mode:
-			if (match_octal(&args[0], &option))
-				return -EINVAL;
-			opts->mode = option & S_IALLUGO;
-			break;
-		/*
-		 * We might like to report bad mount options here;
-		 * but traditionally tracefs has ignored all mount options
-		 */
-		}
-
-		opts->opts |= BIT(token);
+	int opt;
+
+	opt = fs_parse(fc, tracefs_param_specs, param, &result);
+	if (opt < 0)
+		return opt;
+
+	switch (opt) {
+	case Opt_uid:
+		uid = make_kuid(current_user_ns(), result.uint_32);
+		if (!uid_valid(uid))
+			return invalf(fc, "Unknown uid");
+		opts->uid = uid;
+		break;
+	case Opt_gid:
+		gid = make_kgid(current_user_ns(), result.uint_32);
+		if (!gid_valid(gid))
+			return invalf(fc, "Unknown gid");
+		opts->gid = gid;
+		break;
+	case Opt_mode:
+		opts->mode = result.uint_32 & S_IALLUGO;
+		break;
+	/*
+	 * We might like to report bad mount options here;
+	 * but traditionally tracefs has ignored all mount options
+	 */
 	}
 
+	opts->opts |= BIT(opt);
+
 	return 0;
 }
 
@@ -312,7 +295,6 @@ static int tracefs_apply_options(struct super_block *sb, bool remount)
 {
 	struct tracefs_fs_info *fsi = sb->s_fs_info;
 	struct inode *inode = d_inode(sb->s_root);
-	struct tracefs_mount_opts *opts = &fsi->mount_opts;
 	umode_t tmp_mode;
 
 	/*
@@ -320,50 +302,46 @@ static int tracefs_apply_options(struct super_block *sb, bool remount)
 	 * options.
 	 */
 
-	if (!remount || opts->opts & BIT(Opt_mode)) {
+	if (!remount || fsi->opts & BIT(Opt_mode)) {
 		tmp_mode = READ_ONCE(inode->i_mode) & ~S_IALLUGO;
-		tmp_mode |= opts->mode;
+		tmp_mode |= fsi->mode;
 		WRITE_ONCE(inode->i_mode, tmp_mode);
 	}
 
-	if (!remount || opts->opts & BIT(Opt_uid))
-		inode->i_uid = opts->uid;
+	if (!remount || fsi->opts & BIT(Opt_uid))
+		inode->i_uid = fsi->uid;
 
-	if (!remount || opts->opts & BIT(Opt_gid))
-		inode->i_gid = opts->gid;
+	if (!remount || fsi->opts & BIT(Opt_gid))
+		inode->i_gid = fsi->gid;
 
 	return 0;
 }
 
-static int tracefs_remount(struct super_block *sb, int *flags, char *data)
+static int tracefs_reconfigure(struct fs_context *fc)
 {
-	int err;
-	struct tracefs_fs_info *fsi = sb->s_fs_info;
+	struct super_block *sb = fc->root->d_sb;
+	struct tracefs_fs_info *sb_opts = sb->s_fs_info;
+	struct tracefs_fs_info *new_opts = fc->s_fs_info;
 
 	sync_filesystem(sb);
-	err = tracefs_parse_options(data, &fsi->mount_opts);
-	if (err)
-		goto fail;
+	/* structure copy of new mount options to sb */
+	*sb_opts = *new_opts;
 
-	tracefs_apply_options(sb, true);
-
-fail:
-	return err;
+	return tracefs_apply_options(sb, true);
 }
 
 static int tracefs_show_options(struct seq_file *m, struct dentry *root)
 {
 	struct tracefs_fs_info *fsi = root->d_sb->s_fs_info;
-	struct tracefs_mount_opts *opts = &fsi->mount_opts;
 
-	if (!uid_eq(opts->uid, GLOBAL_ROOT_UID))
+	if (!uid_eq(fsi->uid, GLOBAL_ROOT_UID))
 		seq_printf(m, ",uid=%u",
-			   from_kuid_munged(&init_user_ns, opts->uid));
-	if (!gid_eq(opts->gid, GLOBAL_ROOT_GID))
+			   from_kuid_munged(&init_user_ns, fsi->uid));
+	if (!gid_eq(fsi->gid, GLOBAL_ROOT_GID))
 		seq_printf(m, ",gid=%u",
-			   from_kgid_munged(&init_user_ns, opts->gid));
-	if (opts->mode != TRACEFS_DEFAULT_MODE)
-		seq_printf(m, ",mode=%o", opts->mode);
+			   from_kgid_munged(&init_user_ns, fsi->gid));
+	if (fsi->mode != TRACEFS_DEFAULT_MODE)
+		seq_printf(m, ",mode=%o", fsi->mode);
 
 	return 0;
 }
@@ -373,7 +351,6 @@ static const struct super_operations tracefs_super_operations = {
 	.free_inode     = tracefs_free_inode,
 	.drop_inode     = generic_delete_inode,
 	.statfs		= simple_statfs,
-	.remount_fs	= tracefs_remount,
 	.show_options	= tracefs_show_options,
 };
 
@@ -403,26 +380,14 @@ static const struct dentry_operations tracefs_dentry_operations = {
 	.d_release = tracefs_d_release,
 };
 
-static int trace_fill_super(struct super_block *sb, void *data, int silent)
+static int tracefs_fill_super(struct super_block *sb, struct fs_context *fc)
 {
 	static const struct tree_descr trace_files[] = {{""}};
-	struct tracefs_fs_info *fsi;
 	int err;
 
-	fsi = kzalloc(sizeof(struct tracefs_fs_info), GFP_KERNEL);
-	sb->s_fs_info = fsi;
-	if (!fsi) {
-		err = -ENOMEM;
-		goto fail;
-	}
-
-	err = tracefs_parse_options(data, &fsi->mount_opts);
-	if (err)
-		goto fail;
-
-	err  =  simple_fill_super(sb, TRACEFS_MAGIC, trace_files);
+	err = simple_fill_super(sb, TRACEFS_MAGIC, trace_files);
 	if (err)
-		goto fail;
+		return err;
 
 	sb->s_op = &tracefs_super_operations;
 	sb->s_d_op = &tracefs_dentry_operations;
@@ -430,24 +395,45 @@ static int trace_fill_super(struct super_block *sb, void *data, int silent)
 	tracefs_apply_options(sb, false);
 
 	return 0;
+}
 
-fail:
-	kfree(fsi);
-	sb->s_fs_info = NULL;
-	return err;
+static int tracefs_get_tree(struct fs_context *fc)
+{
+	return get_tree_single(fc, tracefs_fill_super);
 }
 
-static struct dentry *trace_mount(struct file_system_type *fs_type,
-			int flags, const char *dev_name,
-			void *data)
+static void tracefs_free_fc(struct fs_context *fc)
 {
-	return mount_single(fs_type, flags, data, trace_fill_super);
+	kfree(fc->s_fs_info);
+}
+
+static const struct fs_context_operations tracefs_context_ops = {
+	.free		= tracefs_free_fc,
+	.parse_param	= tracefs_parse_param,
+	.get_tree	= tracefs_get_tree,
+	.reconfigure	= tracefs_reconfigure,
+};
+
+static int tracefs_init_fs_context(struct fs_context *fc)
+{
+	struct tracefs_fs_info *fsi;
+
+	fsi = kzalloc(sizeof(struct tracefs_fs_info), GFP_KERNEL);
+	if (!fsi)
+		return -ENOMEM;
+
+	fsi->mode = TRACEFS_DEFAULT_MODE;
+
+	fc->s_fs_info = fsi;
+	fc->ops = &tracefs_context_ops;
+	return 0;
 }
 
 static struct file_system_type trace_fs_type = {
 	.owner =	THIS_MODULE,
 	.name =		"tracefs",
-	.mount =	trace_mount,
+	.init_fs_context = tracefs_init_fs_context,
+	.parameters	= tracefs_param_specs,
 	.kill_sb =	kill_litter_super,
 };
 MODULE_ALIAS_FS("tracefs");
-- 
2.43.0

Re: [PATCH 1/2] vfs: Convert debugfs to use the new mount API

[Christian Brauner]

On Tue, Mar 05, 2024 at 05:08:39PM -0600, Eric Sandeen wrote:
> From: David Howells <dhowells@redhat.com>
> 
> Convert the debugfs filesystem to the new internal mount API as the old
> one will be obsoleted and removed.  This allows greater flexibility in
> communication of mount parameters between userspace, the VFS and the
> filesystem.
> 
> See Documentation/filesystems/mount_api.txt for more information.
> 
> Signed-off-by: David Howells <dhowells@redhat.com>
> Co-developed-by: Eric Sandeen <sandeen@redhat.com>
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> [sandeen: forward port to modern kernel, fix remounting]
> cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> cc: "Rafael J. Wysocki" <rafael@kernel.org>
> ---
>  fs/debugfs/inode.c | 198 +++++++++++++++++++++------------------------
>  1 file changed, 93 insertions(+), 105 deletions(-)
> 
> diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
> index 034a617cb1a5..c2adfb272da8 100644
> --- a/fs/debugfs/inode.c
> +++ b/fs/debugfs/inode.c
> @@ -14,7 +14,8 @@
>  
>  #include <linux/module.h>
>  #include <linux/fs.h>
> -#include <linux/mount.h>
> +#include <linux/fs_context.h>
> +#include <linux/fs_parser.h>
>  #include <linux/pagemap.h>
>  #include <linux/init.h>
>  #include <linux/kobject.h>
> @@ -23,7 +24,6 @@
>  #include <linux/fsnotify.h>
>  #include <linux/string.h>
>  #include <linux/seq_file.h>
> -#include <linux/parser.h>
>  #include <linux/magic.h>
>  #include <linux/slab.h>
>  #include <linux/security.h>
> @@ -77,7 +77,7 @@ static struct inode *debugfs_get_inode(struct super_block *sb)
>  	return inode;
>  }
>  
> -struct debugfs_mount_opts {
> +struct debugfs_fs_info {
>  	kuid_t uid;
>  	kgid_t gid;
>  	umode_t mode;
> @@ -89,68 +89,51 @@ enum {
>  	Opt_uid,
>  	Opt_gid,
>  	Opt_mode,
> -	Opt_err
>  };
>  
> -static const match_table_t tokens = {
> -	{Opt_uid, "uid=%u"},
> -	{Opt_gid, "gid=%u"},
> -	{Opt_mode, "mode=%o"},
> -	{Opt_err, NULL}
> +static const struct fs_parameter_spec debugfs_param_specs[] = {
> +	fsparam_u32	("gid",		Opt_gid),
> +	fsparam_u32oct	("mode",	Opt_mode),
> +	fsparam_u32	("uid",		Opt_uid),
> +	{}
>  };
>  
> -struct debugfs_fs_info {
> -	struct debugfs_mount_opts mount_opts;
> -};
> -
> -static int debugfs_parse_options(char *data, struct debugfs_mount_opts *opts)
> +static int debugfs_parse_param(struct fs_context *fc, struct fs_parameter *param)
>  {
> -	substring_t args[MAX_OPT_ARGS];
> -	int option;
> -	int token;
> +	struct debugfs_fs_info *opts = fc->s_fs_info;
> +	struct fs_parse_result result;
>  	kuid_t uid;
>  	kgid_t gid;
> -	char *p;
> -
> -	opts->opts = 0;
> -	opts->mode = DEBUGFS_DEFAULT_MODE;
> -
> -	while ((p = strsep(&data, ",")) != NULL) {
> -		if (!*p)
> -			continue;
> -
> -		token = match_token(p, tokens, args);
> -		switch (token) {
> -		case Opt_uid:
> -			if (match_int(&args[0], &option))
> -				return -EINVAL;
> -			uid = make_kuid(current_user_ns(), option);
> -			if (!uid_valid(uid))
> -				return -EINVAL;
> -			opts->uid = uid;
> -			break;
> -		case Opt_gid:
> -			if (match_int(&args[0], &option))
> -				return -EINVAL;
> -			gid = make_kgid(current_user_ns(), option);
> -			if (!gid_valid(gid))
> -				return -EINVAL;
> -			opts->gid = gid;
> -			break;
> -		case Opt_mode:
> -			if (match_octal(&args[0], &option))
> -				return -EINVAL;
> -			opts->mode = option & S_IALLUGO;
> -			break;
> -		/*
> -		 * We might like to report bad mount options here;
> -		 * but traditionally debugfs has ignored all mount options
> -		 */
> -		}
> -
> -		opts->opts |= BIT(token);
> +	int opt;
> +
> +	opt = fs_parse(fc, debugfs_param_specs, param, &result);
> +	if (opt < 0)
> +		return opt;
> +
> +	switch (opt) {
> +	case Opt_uid:
> +		uid = make_kuid(current_user_ns(), result.uint_32);
> +		if (!uid_valid(uid))
> +			return invalf(fc, "Unknown uid");

Fwiw, Opt_{g,u}d I would like to see that either moved completely to the
VFS or we need to provide standardized helpers.

The issue is that for a userns mountable filesytems the validation done
here isn't enough and that's easy to miss (Obviously, debugfs isn't
relevant as it's not userns mountable but still.). For example, for
in tmpfs I recently fixed a bug where validation was wrong:

        case Opt_uid:
                kuid = make_kuid(current_user_ns(), result.uint_32);
                if (!uid_valid(kuid))
                        goto bad_value;

                /*
                 * The requested uid must be representable in the
                 * filesystem's idmapping.
                 */
                if (!kuid_has_mapping(fc->user_ns, kuid))
                        goto bad_value;

                ctx->uid = kuid;
                break;

The crucial step where the {g,u}id must also be representable in the
superblock's namespace not just in the caller's was missing. So really
we should have a generic helper that we can reycle for all Opt_{g,u}id
mount options or move that Opt_{g,u}id to the VFS itself. There was some
nastiness involved in this when I last looked at this though. And all
that invalfc() reporting should then also be identical across
filesystems.

So that's a ToDo for the future.

> +		opts->uid = uid;
> +		break;
> +	case Opt_gid:
> +		gid = make_kgid(current_user_ns(), result.uint_32);
> +		if (!gid_valid(gid))
> +			return invalf(fc, "Unknown gid");
> +		opts->gid = gid;
> +		break;
> +	case Opt_mode:
> +		opts->mode = result.uint_32 & S_IALLUGO;
> +		break;
> +	/*
> +	 * We might like to report bad mount options here;
> +	 * but traditionally debugfs has ignored all mount options
> +	 */
>  	}

We can actually differentiate this. During superblock creation and
remount we're now setting fc->oldapi e.g., what I've done for btrfs in
fs/btrfs/super.c:btrfs_reconfigure_for_mount() or what I did for
fs/overlayfs/params.c:ovl_parse_param().

There's a tiny wrinkle though. We currently have no way of letting
userspace know whether a filesystem supports the new mount API or not
(see that mount option probing systemd does we recently discussed). So
if say mount(8) remounts debugfs with mount options that were ignored in
the old mount api that are now rejected in the new mount api users now
see failures they didn't see before.

For the user it's completely intransparent why that failure happens. For
them nothing changed from util-linux's perspective. So really, we should
probably continue to ignore old mount options for backward compatibility.

>  
> +	opts->opts |= BIT(opt);
> +
>  	return 0;
>  }
>  
> @@ -158,23 +141,22 @@ static void _debugfs_apply_options(struct super_block *sb, bool remount)
>  {
>  	struct debugfs_fs_info *fsi = sb->s_fs_info;
>  	struct inode *inode = d_inode(sb->s_root);
> -	struct debugfs_mount_opts *opts = &fsi->mount_opts;
>  
>  	/*
>  	 * On remount, only reset mode/uid/gid if they were provided as mount
>  	 * options.
>  	 */
>  
> -	if (!remount || opts->opts & BIT(Opt_mode)) {
> +	if (!remount || fsi->opts & BIT(Opt_mode)) {
>  		inode->i_mode &= ~S_IALLUGO;
> -		inode->i_mode |= opts->mode;
> +		inode->i_mode |= fsi->mode;
>  	}
>  
> -	if (!remount || opts->opts & BIT(Opt_uid))
> -		inode->i_uid = opts->uid;
> +	if (!remount || fsi->opts & BIT(Opt_uid))
> +		inode->i_uid = fsi->uid;
>  
> -	if (!remount || opts->opts & BIT(Opt_gid))
> -		inode->i_gid = opts->gid;
> +	if (!remount || fsi->opts & BIT(Opt_gid))
> +		inode->i_gid = fsi->gid;
>  }
>  
>  static void debugfs_apply_options(struct super_block *sb)
> @@ -187,35 +169,33 @@ static void debugfs_apply_options_remount(struct super_block *sb)
>  	_debugfs_apply_options(sb, true);
>  }
>  
> -static int debugfs_remount(struct super_block *sb, int *flags, char *data)
> +static int debugfs_reconfigure(struct fs_context *fc)
>  {
> -	int err;
> -	struct debugfs_fs_info *fsi = sb->s_fs_info;
> +	struct super_block *sb = fc->root->d_sb;
> +	struct debugfs_fs_info *sb_opts = sb->s_fs_info;
> +	struct debugfs_fs_info *new_opts = fc->s_fs_info;
>  
>  	sync_filesystem(sb);
> -	err = debugfs_parse_options(data, &fsi->mount_opts);
> -	if (err)
> -		goto fail;
>  
> +	/* structure copy of new mount options to sb */
> +	*sb_opts = *new_opts;
>  	debugfs_apply_options_remount(sb);
>  
> -fail:
> -	return err;
> +	return 0;
>  }
>  
>  static int debugfs_show_options(struct seq_file *m, struct dentry *root)
>  {
>  	struct debugfs_fs_info *fsi = root->d_sb->s_fs_info;
> -	struct debugfs_mount_opts *opts = &fsi->mount_opts;
>  
> -	if (!uid_eq(opts->uid, GLOBAL_ROOT_UID))
> +	if (!uid_eq(fsi->uid, GLOBAL_ROOT_UID))
>  		seq_printf(m, ",uid=%u",
> -			   from_kuid_munged(&init_user_ns, opts->uid));
> -	if (!gid_eq(opts->gid, GLOBAL_ROOT_GID))
> +			   from_kuid_munged(&init_user_ns, fsi->uid));
> +	if (!gid_eq(fsi->gid, GLOBAL_ROOT_GID))
>  		seq_printf(m, ",gid=%u",
> -			   from_kgid_munged(&init_user_ns, opts->gid));
> -	if (opts->mode != DEBUGFS_DEFAULT_MODE)
> -		seq_printf(m, ",mode=%o", opts->mode);
> +			   from_kgid_munged(&init_user_ns, fsi->gid));
> +	if (fsi->mode != DEBUGFS_DEFAULT_MODE)
> +		seq_printf(m, ",mode=%o", fsi->mode);
>  
>  	return 0;
>  }
> @@ -229,7 +209,6 @@ static void debugfs_free_inode(struct inode *inode)
>  
>  static const struct super_operations debugfs_super_operations = {
>  	.statfs		= simple_statfs,
> -	.remount_fs	= debugfs_remount,
>  	.show_options	= debugfs_show_options,
>  	.free_inode	= debugfs_free_inode,
>  };
> @@ -263,26 +242,14 @@ static const struct dentry_operations debugfs_dops = {
>  	.d_automount = debugfs_automount,
>  };
>  
> -static int debug_fill_super(struct super_block *sb, void *data, int silent)
> +static int debugfs_fill_super(struct super_block *sb, struct fs_context *fc)
>  {
>  	static const struct tree_descr debug_files[] = {{""}};
> -	struct debugfs_fs_info *fsi;
>  	int err;
>  
> -	fsi = kzalloc(sizeof(struct debugfs_fs_info), GFP_KERNEL);
> -	sb->s_fs_info = fsi;
> -	if (!fsi) {
> -		err = -ENOMEM;
> -		goto fail;
> -	}
> -
> -	err = debugfs_parse_options(data, &fsi->mount_opts);
> +	err = simple_fill_super(sb, DEBUGFS_MAGIC, debug_files);
>  	if (err)
> -		goto fail;
> -
> -	err  =  simple_fill_super(sb, DEBUGFS_MAGIC, debug_files);
> -	if (err)
> -		goto fail;
> +		return err;
>  
>  	sb->s_op = &debugfs_super_operations;
>  	sb->s_d_op = &debugfs_dops;
> @@ -290,27 +257,48 @@ static int debug_fill_super(struct super_block *sb, void *data, int silent)
>  	debugfs_apply_options(sb);
>  
>  	return 0;
> -
> -fail:
> -	kfree(fsi);
> -	sb->s_fs_info = NULL;
> -	return err;
>  }
>  
> -static struct dentry *debug_mount(struct file_system_type *fs_type,
> -			int flags, const char *dev_name,
> -			void *data)
> +static int debugfs_get_tree(struct fs_context *fc)
>  {
>  	if (!(debugfs_allow & DEBUGFS_ALLOW_API))
> -		return ERR_PTR(-EPERM);
> +		return -EPERM;
> +
> +	return get_tree_single(fc, debugfs_fill_super);
> +}
> +
> +static void debugfs_free_fc(struct fs_context *fc)
> +{
> +	kfree(fc->s_fs_info);
> +}
>  
> -	return mount_single(fs_type, flags, data, debug_fill_super);
> +static const struct fs_context_operations debugfs_context_ops = {
> +	.free		= debugfs_free_fc,
> +	.parse_param	= debugfs_parse_param,
> +	.get_tree	= debugfs_get_tree,
> +	.reconfigure	= debugfs_reconfigure,
> +};
> +
> +static int debugfs_init_fs_context(struct fs_context *fc)
> +{
> +	struct debugfs_fs_info *fsi;
> +
> +	fsi = kzalloc(sizeof(struct debugfs_fs_info), GFP_KERNEL);
> +	if (!fsi)
> +		return -ENOMEM;
> +
> +	fsi->mode = DEBUGFS_DEFAULT_MODE;
> +
> +	fc->s_fs_info = fsi;
> +	fc->ops = &debugfs_context_ops;
> +	return 0;
>  }
>  
>  static struct file_system_type debug_fs_type = {
>  	.owner =	THIS_MODULE,
>  	.name =		"debugfs",
> -	.mount =	debug_mount,
> +	.init_fs_context = debugfs_init_fs_context,
> +	.parameters =	debugfs_param_specs,
>  	.kill_sb =	kill_litter_super,
>  };
>  MODULE_ALIAS_FS("debugfs");
> -- 
> 2.43.0
> 
> 

Re: [PATCH 0/2] vfs: convert debugfs & tracefs to the new mount API

[Christian Brauner]

On Tue, Mar 05, 2024 at 05:07:29PM -0600, Eric Sandeen wrote:
> Since debugfs and tracefs are cut & pasted one way or the other,
> do these at the same time.
> 
> Both of these patches originated in dhowells' tree at
> https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/log/?h=mount-api-viro
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/commit/?h=mount-api-viro&id=ec14be9e2aa76f63458466bba86256e123ec4e51
> and
> https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/commit/?h=mount-api-viro&id=c4f2e60465859e02a6e36ed618dbaea16de8c8e0
> 
> I've forward-ported them to the mount API that landed, and
> fixed up remounting; ->reconfigure() needed to copy the
> parsed context options into the current superblock options
> to effect any remount changes.
> 
> While these do use the invalf() functions for some errors, they
> are new messages, not messages that used to go to dmesg that
> would be lost if userspace isn't listening.
> 
> I've done minimal testing - booting with the patches, testing
> some of the remount behavior for mode & uid.
> Oh, and I built it too. </brown_paper_bag>

Looks ok to me.

Re: [PATCH 1/2] vfs: Convert debugfs to use the new mount API

[Miklos Szeredi]

On Wed, 6 Mar 2024 at 11:57, Christian Brauner <brauner@kernel.org> wrote:

> There's a tiny wrinkle though. We currently have no way of letting
> userspace know whether a filesystem supports the new mount API or not
> (see that mount option probing systemd does we recently discussed). So
> if say mount(8) remounts debugfs with mount options that were ignored in
> the old mount api that are now rejected in the new mount api users now
> see failures they didn't see before.
>
> For the user it's completely intransparent why that failure happens. For
> them nothing changed from util-linux's perspective. So really, we should
> probably continue to ignore old mount options for backward compatibility.

The reject behavior could be made conditional on e.g. an fsopen() flag.

I.e. FSOPEN_REJECT_UNKNOWN would make unknown options be always
rejected.  Without this flag fsconfig(2) would behave identically
before/after the conversion.

Thanks,
Miklos

Re: [PATCH 1/2] vfs: Convert debugfs to use the new mount API

[Christian Brauner]

On Wed, Mar 06, 2024 at 01:13:05PM +0100, Miklos Szeredi wrote:
> On Wed, 6 Mar 2024 at 11:57, Christian Brauner <brauner@kernel.org> wrote:
> 
> > There's a tiny wrinkle though. We currently have no way of letting
> > userspace know whether a filesystem supports the new mount API or not
> > (see that mount option probing systemd does we recently discussed). So
> > if say mount(8) remounts debugfs with mount options that were ignored in
> > the old mount api that are now rejected in the new mount api users now
> > see failures they didn't see before.
> >
> > For the user it's completely intransparent why that failure happens. For
> > them nothing changed from util-linux's perspective. So really, we should
> > probably continue to ignore old mount options for backward compatibility.
> 
> The reject behavior could be made conditional on e.g. an fsopen() flag.

and fspick() which I think is more relevant.

> 
> I.e. FSOPEN_REJECT_UNKNOWN would make unknown options be always
> rejected.  Without this flag fsconfig(2) would behave identically
> before/after the conversion.

Yeah, that would work. That would only make sense if we make all
filesystems reject unknown mount options by default when they're
switched to the new mount api imho. When we recognize the request comes
from the old mount api fc->oldapi we continue ignoring as we did before.
If it comes from the new mount api we reject unless
FSOPEN/FSPICK_REJECT_UKNOWN was specified.

Re: [PATCH 1/2] vfs: Convert debugfs to use the new mount API

[Eric Sandeen]

On 3/6/24 6:17 AM, Christian Brauner wrote:
> On Wed, Mar 06, 2024 at 01:13:05PM +0100, Miklos Szeredi wrote:
>> On Wed, 6 Mar 2024 at 11:57, Christian Brauner <brauner@kernel.org> wrote:
>>
>>> There's a tiny wrinkle though. We currently have no way of letting
>>> userspace know whether a filesystem supports the new mount API or not
>>> (see that mount option probing systemd does we recently discussed). So
>>> if say mount(8) remounts debugfs with mount options that were ignored in
>>> the old mount api that are now rejected in the new mount api users now
>>> see failures they didn't see before.

Oh, right - the problem is the new mount API rejects unknown options
internally, right?

>>> For the user it's completely intransparent why that failure happens. For
>>> them nothing changed from util-linux's perspective. So really, we should
>>> probably continue to ignore old mount options for backward compatibility.
>>
>> The reject behavior could be made conditional on e.g. an fsopen() flag.
> 
> and fspick() which I think is more relevant.
> 
>>
>> I.e. FSOPEN_REJECT_UNKNOWN would make unknown options be always
>> rejected.  Without this flag fsconfig(2) would behave identically
>> before/after the conversion.
> 
> Yeah, that would work. That would only make sense if we make all
> filesystems reject unknown mount options by default when they're
> switched to the new mount api imho. When we recognize the request comes
> from the old mount api fc->oldapi we continue ignoring as we did before.
> If it comes from the new mount api we reject unless
> FSOPEN/FSPICK_REJECT_UKNOWN was specified.

Ok, good point. Just thinking out loud, I guess an fsopen/fspick flag does
make more sense than i.e. each filesystem deciding whether it should reject
unknown options in its ->init_fs_context(), for consistency?

Right now it looks like the majority of filesystems do reject unknown
options internally, already.

(To muddy the waters more, other inconsistencies I've thought about are
re: how the fileystem handles remount. For example, which options are
remountable and which are not, and should non-remountable options fail?
Also whether the filesystem internally preserves the original set of
options and applies the new set as a delta, or whether it treats the
new set as the exact set of options requested post-remount, but that's
probably a topic for another day.)

-Eric

Re: [PATCH 2/2] vfs: Convert tracefs to use the new mount API

[Steven Rostedt]

On Tue, 5 Mar 2024 17:09:30 -0600
Eric Sandeen <sandeen@redhat.com> (by way of Steven Rostedt <rostedt@goodmis.org>) wrote:

> From: David Howells <dhowells@redhat.com>
> 
> Convert the tracefs filesystem to the new internal mount API as the old
> one will be obsoleted and removed.  This allows greater flexibility in
> communication of mount parameters between userspace, the VFS and the
> filesystem.
> 
> See Documentation/filesystems/mount_api.txt for more information.
> 
> Signed-off-by: David Howells <dhowells@redhat.com>
> Co-developed-by: Eric Sandeen <sandeen@redhat.com>
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> [sandeen: forward port to modern kernel, fix remounting]
> cc: Steven Rostedt <rostedt@goodmis.org>
> cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>

I applied this and ran it through all my tests and it passed.

Acked-by: Steven Rostedt (Google) <rostedt@goodmis.org>
Tested-by: Steven Rostedt (Google) <rostedt@goodmis.org>

-- Steve

Re: [PATCH 1/2] vfs: Convert debugfs to use the new mount API

[Christian Brauner]

On Wed, Mar 06, 2024 at 10:35:02AM -0600, Eric Sandeen wrote:
> On 3/6/24 6:17 AM, Christian Brauner wrote:
> > On Wed, Mar 06, 2024 at 01:13:05PM +0100, Miklos Szeredi wrote:
> >> On Wed, 6 Mar 2024 at 11:57, Christian Brauner <brauner@kernel.org> wrote:
> >>
> >>> There's a tiny wrinkle though. We currently have no way of letting
> >>> userspace know whether a filesystem supports the new mount API or not
> >>> (see that mount option probing systemd does we recently discussed). So
> >>> if say mount(8) remounts debugfs with mount options that were ignored in
> >>> the old mount api that are now rejected in the new mount api users now
> >>> see failures they didn't see before.
> 
> Oh, right - the problem is the new mount API rejects unknown options
> internally, right?
> 
> >>> For the user it's completely intransparent why that failure happens. For
> >>> them nothing changed from util-linux's perspective. So really, we should
> >>> probably continue to ignore old mount options for backward compatibility.
> >>
> >> The reject behavior could be made conditional on e.g. an fsopen() flag.
> > 
> > and fspick() which I think is more relevant.
> > 
> >>
> >> I.e. FSOPEN_REJECT_UNKNOWN would make unknown options be always
> >> rejected.  Without this flag fsconfig(2) would behave identically
> >> before/after the conversion.
> > 
> > Yeah, that would work. That would only make sense if we make all
> > filesystems reject unknown mount options by default when they're
> > switched to the new mount api imho. When we recognize the request comes
> > from the old mount api fc->oldapi we continue ignoring as we did before.
> > If it comes from the new mount api we reject unless
> > FSOPEN/FSPICK_REJECT_UKNOWN was specified.

I actually did misparse that I now realize. I read that as "ignore
unknown" instead of "reject unknown".

> 
> Ok, good point. Just thinking out loud, I guess an fsopen/fspick flag does
> make more sense than i.e. each filesystem deciding whether it should reject
> unknown options in its ->init_fs_context(), for consistency?

Yes, I think so. The interesting case for util-linux according to Karel
was remounting where mount(8) wants to gather all options from fstab and
mountinfo, add new options from the command line and send it to the
kernel without having to care about filesystems specific options that
cannot be changed on remount.

However, other users that do use the api programatically do care about
this. They want to get an error when changing a mount property doesn't
work.

I think doing this on a per-fs basis just leads to more inconsistency.
I'd rather have this be something we enforce on a higher level if we do
it at all.

> 
> Right now it looks like the majority of filesystems do reject unknown
> options internally, already.

Yeah, it's mostly pseudo fses that don't, I reckon.

> 
> (To muddy the waters more, other inconsistencies I've thought about are
> re: how the fileystem handles remount. For example, which options are
> remountable and which are not, and should non-remountable options fail?

Yes, they should but similar to fsopen() we should have an fspick()
flag. This was what I mentioned earlier in my response to Miklos.

But I'm not yet clear whether FSOPEN/FSPICK_IGNORE_UNKNOWN wouldn't make
more sense than FSOPEN/FSPICK_REJECT_UNKNOWN. IOW, invert the logic.

Because as you said most filesystems do already reject unknown mount
options and it's a few that don't. So I think we should focus on the
remount case and for that I think we want FSOPEN_IGNORE_UNKNOWN
otherwise default to rejecting unknown options if coming from the new
mount api? I'm not sure.

> Also whether the filesystem internally preserves the original set of
> options and applies the new set as a delta, or whether it treats the
> new set as the exact set of options requested post-remount, but that's
> probably a topic for another day.)

For vfs mount properties it's a delta in the new mount api. The old
mount api didn't have a concept of add or subtract. If you had a
read-only mount and you wanted to also make it noexec then you'd have to
specify "ro" again otherwise the mount would be made rw. mount(8) hides
that behavior by retrieving the current mountflags from mountinfo and
adding it to the remount call if the old mount api is used.
mount_setattr() does that directly in the kernel and always does a
delta.

For filesystem specific properties it's probably irrelevant because
remount already is effectively a delta for most filesystems. IOW, you
don't suddenly get "usrquota" unset because you've changed the "dax"
property on your xfs filesystem which would be worrisome. :)

The thing is though that changing one mount option might implicitly
change other mount options. But that's something that only the
filesystem should decide. So I don't think this is something we need to
worry about?

The way I see mount(8) currently doing it is to change:

(1) filesystem specific mount properties via fspick()+fsconfig()
(2) generic mount properties via mount_setattr()

during a remount call.

Re: [PATCH 1/2] vfs: Convert debugfs to use the new mount API

[Greg Kroah-Hartman]

On Tue, Mar 05, 2024 at 05:08:39PM -0600, Eric Sandeen wrote:
> From: David Howells <dhowells@redhat.com>
> 
> Convert the debugfs filesystem to the new internal mount API as the old
> one will be obsoleted and removed.  This allows greater flexibility in
> communication of mount parameters between userspace, the VFS and the
> filesystem.
> 
> See Documentation/filesystems/mount_api.txt for more information.
> 
> Signed-off-by: David Howells <dhowells@redhat.com>
> Co-developed-by: Eric Sandeen <sandeen@redhat.com>
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> [sandeen: forward port to modern kernel, fix remounting]
> cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> cc: "Rafael J. Wysocki" <rafael@kernel.org>
> ---
>  fs/debugfs/inode.c | 198 +++++++++++++++++++++------------------------
>  1 file changed, 93 insertions(+), 105 deletions(-)

If the vfs maintainers are ok with the conversion, it's fine with me:

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [PATCH 1/2] vfs: Convert debugfs to use the new mount API

[Miklos Szeredi]

On Thu, 7 Mar 2024 at 13:04, Christian Brauner <brauner@kernel.org> wrote:

> But I'm not yet clear whether FSOPEN/FSPICK_IGNORE_UNKNOWN wouldn't make
> more sense than FSOPEN/FSPICK_REJECT_UNKNOWN. IOW, invert the logic.

I think there needs to be a mode for fsopen/fspick/fconfig API that
allows implementing full backward compatibility with the old behavior
of mount(8), both in case of new mount and remount.  By old I mean
before any of the API conversions started.  If some filesystems
rejected unknown options and some ignored them, then that is what this
mode should continue to do.  This is what we currently have, so
without additional flags this is what the API should continue to
support.

And I think there needs to be a new "strict" mode for fsopen/fspick
that has clear rules for how filesystems should handle options, which
as you say most filesystem already do.   Since this is a new mode, I
think it needs a new flag, that is rejected if the API or the fs
doesn't support this mode.  Filesystems which already have sane
behavior need not care, they would work the same in both modes.
Filesystems that are currently inconsistent would have to implement
both modes.

Thanks,
Miklos

Re: [PATCH 0/2] vfs: convert debugfs & tracefs to the new mount API

[Christian Brauner]

On Tue, 05 Mar 2024 17:07:29 -0600, Eric Sandeen wrote:
> Since debugfs and tracefs are cut & pasted one way or the other,
> do these at the same time.
> 
> Both of these patches originated in dhowells' tree at
> https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/log/?h=mount-api-viro
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/commit/?h=mount-api-viro&id=ec14be9e2aa76f63458466bba86256e123ec4e51
> and
> https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/commit/?h=mount-api-viro&id=c4f2e60465859e02a6e36ed618dbaea16de8c8e0
> 
> [...]

I'm assuming that your Acks meant I'm good to pick those up.
Please yell, if not!

---

Applied to the vfs.misc branch of the vfs/vfs.git tree.
Patches in the vfs.misc branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs.misc

[1/2] vfs: Convert debugfs to use the new mount API
      https://git.kernel.org/vfs/vfs/c/393b0b4c609e
[2/2] vfs: Convert tracefs to use the new mount API
      https://git.kernel.org/vfs/vfs/c/140a2056102a