[PATCH] udf: convert to new mount API

[PATCH] udf: convert to new mount API

[Eric Sandeen]

Convert the UDF filesystem to the new mount API.

UDF is slightly unique in that it always preserves prior mount
options across a remount, so that's handled by udf_init_options().

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

Tested by running through xfstests, as well as some targeted testing of
remount behavior.

NB: I did not convert i.e any udf_err() to errorf(fc, ) because the former
does some nice formatting, not sure how or if you'd like to handle that, Jan?

 fs/udf/super.c | 495 +++++++++++++++++++++++++------------------------
 1 file changed, 255 insertions(+), 240 deletions(-)

diff --git a/fs/udf/super.c b/fs/udf/super.c
index 928a04d9d9e0..03fa98fe4e1c 100644
--- a/fs/udf/super.c
+++ b/fs/udf/super.c
@@ -40,20 +40,20 @@
 #include <linux/slab.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
-#include <linux/parser.h>
 #include <linux/stat.h>
 #include <linux/cdrom.h>
 #include <linux/nls.h>
 #include <linux/vfs.h>
 #include <linux/vmalloc.h>
 #include <linux/errno.h>
-#include <linux/mount.h>
 #include <linux/seq_file.h>
 #include <linux/bitmap.h>
 #include <linux/crc-itu-t.h>
 #include <linux/log2.h>
 #include <asm/byteorder.h>
 #include <linux/iversion.h>
+#include <linux/fs_context.h>
+#include <linux/fs_parser.h>
 
 #include "udf_sb.h"
 #include "udf_i.h"
@@ -91,16 +91,20 @@ enum { UDF_MAX_LINKS = 0xffff };
 #define UDF_MAX_FILESIZE (1ULL << 42)
 
 /* These are the "meat" - everything else is stuffing */
-static int udf_fill_super(struct super_block *, void *, int);
+static int udf_fill_super(struct super_block *sb, struct fs_context *fc);
 static void udf_put_super(struct super_block *);
 static int udf_sync_fs(struct super_block *, int);
-static int udf_remount_fs(struct super_block *, int *, char *);
 static void udf_load_logicalvolint(struct super_block *, struct kernel_extent_ad);
 static void udf_open_lvid(struct super_block *);
 static void udf_close_lvid(struct super_block *);
 static unsigned int udf_count_free(struct super_block *);
 static int udf_statfs(struct dentry *, struct kstatfs *);
 static int udf_show_options(struct seq_file *, struct dentry *);
+static int udf_init_fs_context(struct fs_context *fc);
+static int udf_parse_param(struct fs_context *fc, struct fs_parameter *param);
+static int udf_reconfigure(struct fs_context *fc);
+static void udf_free_fc(struct fs_context *fc);
+static const struct fs_parameter_spec udf_param_spec[];
 
 struct logicalVolIntegrityDescImpUse *udf_sb_lvidiu(struct super_block *sb)
 {
@@ -119,18 +123,25 @@ struct logicalVolIntegrityDescImpUse *udf_sb_lvidiu(struct super_block *sb)
 }
 
 /* UDF filesystem type */
-static struct dentry *udf_mount(struct file_system_type *fs_type,
-		      int flags, const char *dev_name, void *data)
+static int udf_get_tree(struct fs_context *fc)
 {
-	return mount_bdev(fs_type, flags, dev_name, data, udf_fill_super);
+	return get_tree_bdev(fc, udf_fill_super);
 }
 
+static const struct fs_context_operations udf_context_ops = {
+	.parse_param	= udf_parse_param,
+	.get_tree	= udf_get_tree,
+	.reconfigure	= udf_reconfigure,
+	.free		= udf_free_fc,
+};
+
 static struct file_system_type udf_fstype = {
 	.owner		= THIS_MODULE,
 	.name		= "udf",
-	.mount		= udf_mount,
 	.kill_sb	= kill_block_super,
 	.fs_flags	= FS_REQUIRES_DEV,
+	.init_fs_context = udf_init_fs_context,
+	.parameters	= udf_param_spec,
 };
 MODULE_ALIAS_FS("udf");
 
@@ -204,7 +215,6 @@ static const struct super_operations udf_sb_ops = {
 	.put_super	= udf_put_super,
 	.sync_fs	= udf_sync_fs,
 	.statfs		= udf_statfs,
-	.remount_fs	= udf_remount_fs,
 	.show_options	= udf_show_options,
 };
 
@@ -223,6 +233,58 @@ struct udf_options {
 	struct nls_table *nls_map;
 };
 
+/*
+ * UDF has historically preserved prior mount options across
+ * a remount, so copy those here if remounting, otherwise set
+ * initial mount defaults.
+ */
+static void udf_init_options(struct fs_context *fc, struct udf_options *uopt)
+{
+	if (fc->purpose == FS_CONTEXT_FOR_RECONFIGURE) {
+		struct super_block *sb = fc->root->d_sb;
+		struct udf_sb_info *sbi = UDF_SB(sb);
+
+		uopt->flags = sbi->s_flags;
+		uopt->uid   = sbi->s_uid;
+		uopt->gid   = sbi->s_gid;
+		uopt->umask = sbi->s_umask;
+		uopt->fmode = sbi->s_fmode;
+		uopt->dmode = sbi->s_dmode;
+		uopt->nls_map = NULL;
+	} else {
+		uopt->flags = (1 << UDF_FLAG_USE_AD_IN_ICB) | (1 << UDF_FLAG_STRICT);
+		/* By default we'll use overflow[ug]id when UDF inode [ug]id == -1 */
+		uopt->uid = make_kuid(current_user_ns(), overflowuid);
+		uopt->gid = make_kgid(current_user_ns(), overflowgid);
+		uopt->umask = 0;
+		uopt->fmode = UDF_INVALID_MODE;
+		uopt->dmode = UDF_INVALID_MODE;
+		uopt->nls_map = NULL;
+		uopt->session = 0xFFFFFFFF;
+	}
+}
+
+static int udf_init_fs_context(struct fs_context *fc)
+{
+	struct udf_options *uopt;
+
+	uopt = kzalloc(sizeof(*uopt), GFP_KERNEL);
+	if (!uopt)
+		return -ENOMEM;
+
+	udf_init_options(fc, uopt);
+
+	fc->fs_private = uopt;
+	fc->ops = &udf_context_ops;
+
+	return 0;
+}
+
+static void udf_free_fc(struct fs_context *fc)
+{
+	kfree(fc->fs_private);
+}
+
 static int __init init_udf_fs(void)
 {
 	int err;
@@ -358,7 +420,7 @@ static int udf_show_options(struct seq_file *seq, struct dentry *root)
 }
 
 /*
- * udf_parse_options
+ * udf_parse_param
  *
  * PURPOSE
  *	Parse mount options.
@@ -401,12 +463,12 @@ static int udf_show_options(struct seq_file *seq, struct dentry *root)
  *		yield highly unpredictable results.
  *
  * PRE-CONDITIONS
- *	options		Pointer to mount options string.
- *	uopts		Pointer to mount options variable.
+ *	fc		fs_context with pointer to mount options variable.
+ *	param		Pointer to fs_parameter being parsed.
  *
  * POST-CONDITIONS
- *	<return>	1	Mount options parsed okay.
- *	<return>	0	Error parsing mount options.
+ *	<return>	0	Mount options parsed okay.
+ *	<return>	errno	Error parsing mount options.
  *
  * HISTORY
  *	July 1, 1997 - Andrew E. Mileski
@@ -418,229 +480,194 @@ enum {
 	Opt_noadinicb, Opt_adinicb, Opt_shortad, Opt_longad,
 	Opt_gid, Opt_uid, Opt_umask, Opt_session, Opt_lastblock,
 	Opt_anchor, Opt_volume, Opt_partition, Opt_fileset,
-	Opt_rootdir, Opt_utf8, Opt_iocharset,
-	Opt_err, Opt_uforget, Opt_uignore, Opt_gforget, Opt_gignore,
-	Opt_fmode, Opt_dmode
+	Opt_rootdir, Opt_utf8, Opt_iocharset, Opt_err, Opt_fmode, Opt_dmode
 };
 
-static const match_table_t tokens = {
-	{Opt_novrs,	"novrs"},
-	{Opt_nostrict,	"nostrict"},
-	{Opt_bs,	"bs=%u"},
-	{Opt_unhide,	"unhide"},
-	{Opt_undelete,	"undelete"},
-	{Opt_noadinicb,	"noadinicb"},
-	{Opt_adinicb,	"adinicb"},
-	{Opt_shortad,	"shortad"},
-	{Opt_longad,	"longad"},
-	{Opt_uforget,	"uid=forget"},
-	{Opt_uignore,	"uid=ignore"},
-	{Opt_gforget,	"gid=forget"},
-	{Opt_gignore,	"gid=ignore"},
-	{Opt_gid,	"gid=%u"},
-	{Opt_uid,	"uid=%u"},
-	{Opt_umask,	"umask=%o"},
-	{Opt_session,	"session=%u"},
-	{Opt_lastblock,	"lastblock=%u"},
-	{Opt_anchor,	"anchor=%u"},
-	{Opt_volume,	"volume=%u"},
-	{Opt_partition,	"partition=%u"},
-	{Opt_fileset,	"fileset=%u"},
-	{Opt_rootdir,	"rootdir=%u"},
-	{Opt_utf8,	"utf8"},
-	{Opt_iocharset,	"iocharset=%s"},
-	{Opt_fmode,     "mode=%o"},
-	{Opt_dmode,     "dmode=%o"},
-	{Opt_err,	NULL}
-};
-
-static int udf_parse_options(char *options, struct udf_options *uopt,
-			     bool remount)
+static const struct fs_parameter_spec udf_param_spec[] = {
+	fsparam_flag	("novrs",		Opt_novrs),
+	fsparam_flag	("nostrict",		Opt_nostrict),
+	fsparam_u32	("bs",			Opt_bs),
+	fsparam_flag	("unhide",		Opt_unhide),
+	fsparam_flag	("undelete",		Opt_undelete),
+	fsparam_flag	("noadinicb",		Opt_noadinicb),
+	fsparam_flag	("adinicb",		Opt_adinicb),
+	fsparam_flag	("shortad",		Opt_shortad),
+	fsparam_flag	("longad",		Opt_longad),
+	fsparam_string	("gid",			Opt_gid),
+	fsparam_string	("uid",			Opt_uid),
+	fsparam_u32	("umask",		Opt_umask),
+	fsparam_u32	("session",		Opt_session),
+	fsparam_u32	("lastblock",		Opt_lastblock),
+	fsparam_u32	("anchor",		Opt_anchor),
+	fsparam_u32	("volume",		Opt_volume),
+	fsparam_u32	("partition",		Opt_partition),
+	fsparam_u32	("fileset",		Opt_fileset),
+	fsparam_u32	("rootdir",		Opt_rootdir),
+	fsparam_flag	("utf8",		Opt_utf8),
+	fsparam_string	("iocharset",		Opt_iocharset),
+	fsparam_u32	("mode",		Opt_fmode),
+	fsparam_u32	("dmode",		Opt_dmode),
+	{}
+ };
+ 
+static int udf_parse_param(struct fs_context *fc, struct fs_parameter *param)
 {
 	char *p;
-	int option;
 	unsigned int uv;
-
-	uopt->novrs = 0;
-	uopt->session = 0xFFFFFFFF;
-	uopt->lastblock = 0;
-	uopt->anchor = 0;
-
-	if (!options)
-		return 1;
-
-	while ((p = strsep(&options, ",")) != NULL) {
-		substring_t args[MAX_OPT_ARGS];
-		int token;
-		unsigned n;
-		if (!*p)
-			continue;
-
-		token = match_token(p, tokens, args);
-		switch (token) {
-		case Opt_novrs:
-			uopt->novrs = 1;
-			break;
-		case Opt_bs:
-			if (match_int(&args[0], &option))
-				return 0;
-			n = option;
-			if (n != 512 && n != 1024 && n != 2048 && n != 4096)
-				return 0;
-			uopt->blocksize = n;
-			uopt->flags |= (1 << UDF_FLAG_BLOCKSIZE_SET);
-			break;
-		case Opt_unhide:
-			uopt->flags |= (1 << UDF_FLAG_UNHIDE);
-			break;
-		case Opt_undelete:
-			uopt->flags |= (1 << UDF_FLAG_UNDELETE);
-			break;
-		case Opt_noadinicb:
-			uopt->flags &= ~(1 << UDF_FLAG_USE_AD_IN_ICB);
-			break;
-		case Opt_adinicb:
-			uopt->flags |= (1 << UDF_FLAG_USE_AD_IN_ICB);
-			break;
-		case Opt_shortad:
-			uopt->flags |= (1 << UDF_FLAG_USE_SHORT_AD);
-			break;
-		case Opt_longad:
-			uopt->flags &= ~(1 << UDF_FLAG_USE_SHORT_AD);
-			break;
-		case Opt_gid:
-			if (match_uint(args, &uv))
-				return 0;
+	unsigned int n;
+	struct udf_options *uopt = fc->fs_private;
+	struct fs_parse_result result;
+	int token;
+	bool remount = (fc->purpose & FS_CONTEXT_FOR_RECONFIGURE);
+
+	token = fs_parse(fc, udf_param_spec, param, &result);
+	if (token < 0)
+		return token;
+
+	switch (token) {
+	case Opt_novrs:
+		uopt->novrs = 1;
+		break;
+	case Opt_bs:
+		n = result.uint_32;;
+		if (n != 512 && n != 1024 && n != 2048 && n != 4096)
+			return -EINVAL;
+		uopt->blocksize = n;
+		uopt->flags |= (1 << UDF_FLAG_BLOCKSIZE_SET);
+		break;
+	case Opt_unhide:
+		uopt->flags |= (1 << UDF_FLAG_UNHIDE);
+		break;
+	case Opt_undelete:
+		uopt->flags |= (1 << UDF_FLAG_UNDELETE);
+		break;
+	case Opt_noadinicb:
+		uopt->flags &= ~(1 << UDF_FLAG_USE_AD_IN_ICB);
+		break;
+	case Opt_adinicb:
+		uopt->flags |= (1 << UDF_FLAG_USE_AD_IN_ICB);
+		break;
+	case Opt_shortad:
+		uopt->flags |= (1 << UDF_FLAG_USE_SHORT_AD);
+		break;
+	case Opt_longad:
+		uopt->flags &= ~(1 << UDF_FLAG_USE_SHORT_AD);
+		break;
+	case Opt_gid:
+		if (0 == kstrtoint(param->string, 10, &uv)) {
 			uopt->gid = make_kgid(current_user_ns(), uv);
 			if (!gid_valid(uopt->gid))
-				return 0;
+				return -EINVAL;
 			uopt->flags |= (1 << UDF_FLAG_GID_SET);
-			break;
-		case Opt_uid:
-			if (match_uint(args, &uv))
-				return 0;
+		} else if (!strcmp(param->string, "forget")) {
+			uopt->flags |= (1 << UDF_FLAG_GID_FORGET);
+		} else if (!strcmp(param->string, "ignore")) {
+			/* this option is superseded by gid=<number> */
+			;
+		} else {
+			return -EINVAL;
+		}
+		break;
+	case Opt_uid:
+		if (0 == kstrtoint(param->string, 10, &uv)) {
 			uopt->uid = make_kuid(current_user_ns(), uv);
 			if (!uid_valid(uopt->uid))
-				return 0;
+				return -EINVAL;
 			uopt->flags |= (1 << UDF_FLAG_UID_SET);
-			break;
-		case Opt_umask:
-			if (match_octal(args, &option))
-				return 0;
-			uopt->umask = option;
-			break;
-		case Opt_nostrict:
-			uopt->flags &= ~(1 << UDF_FLAG_STRICT);
-			break;
-		case Opt_session:
-			if (match_int(args, &option))
-				return 0;
-			uopt->session = option;
-			if (!remount)
-				uopt->flags |= (1 << UDF_FLAG_SESSION_SET);
-			break;
-		case Opt_lastblock:
-			if (match_int(args, &option))
-				return 0;
-			uopt->lastblock = option;
-			if (!remount)
-				uopt->flags |= (1 << UDF_FLAG_LASTBLOCK_SET);
-			break;
-		case Opt_anchor:
-			if (match_int(args, &option))
-				return 0;
-			uopt->anchor = option;
-			break;
-		case Opt_volume:
-		case Opt_partition:
-		case Opt_fileset:
-		case Opt_rootdir:
-			/* Ignored (never implemented properly) */
-			break;
-		case Opt_utf8:
-			if (!remount) {
-				unload_nls(uopt->nls_map);
-				uopt->nls_map = NULL;
-			}
-			break;
-		case Opt_iocharset:
-			if (!remount) {
-				unload_nls(uopt->nls_map);
-				uopt->nls_map = NULL;
-			}
-			/* When nls_map is not loaded then UTF-8 is used */
-			if (!remount && strcmp(args[0].from, "utf8") != 0) {
-				uopt->nls_map = load_nls(args[0].from);
-				if (!uopt->nls_map) {
-					pr_err("iocharset %s not found\n",
-						args[0].from);
-					return 0;
-				}
-			}
-			break;
-		case Opt_uforget:
+		} else if (!strcmp(param->string, "forget")) {
 			uopt->flags |= (1 << UDF_FLAG_UID_FORGET);
-			break;
-		case Opt_uignore:
-		case Opt_gignore:
-			/* These options are superseeded by uid=<number> */
-			break;
-		case Opt_gforget:
-			uopt->flags |= (1 << UDF_FLAG_GID_FORGET);
-			break;
-		case Opt_fmode:
-			if (match_octal(args, &option))
-				return 0;
-			uopt->fmode = option & 0777;
-			break;
-		case Opt_dmode:
-			if (match_octal(args, &option))
-				return 0;
-			uopt->dmode = option & 0777;
-			break;
-		default:
-			pr_err("bad mount option \"%s\" or missing value\n", p);
-			return 0;
+		} else if (!strcmp(param->string, "ignore")) {
+			/* this option is superseded by uid=<number> */
+			;
+		} else {
+			return -EINVAL;
+		}
+		break;
+	case Opt_umask:
+		uopt->umask = result.uint_32;
+		break;
+	case Opt_nostrict:
+		uopt->flags &= ~(1 << UDF_FLAG_STRICT);
+		break;
+	case Opt_session:
+		uopt->session = result.uint_32;
+		if (!remount)
+			uopt->flags |= (1 << UDF_FLAG_SESSION_SET);
+		break;
+	case Opt_lastblock:
+		uopt->lastblock = result.uint_32;
+		if (!remount)
+			uopt->flags |= (1 << UDF_FLAG_LASTBLOCK_SET);
+		break;
+	case Opt_anchor:
+		uopt->anchor = result.uint_32;
+		break;
+	case Opt_volume:
+	case Opt_partition:
+	case Opt_fileset:
+	case Opt_rootdir:
+		/* Ignored (never implemented properly) */
+		break;
+	case Opt_utf8:
+		if (!remount) {
+			unload_nls(uopt->nls_map);
+			uopt->nls_map = NULL;
+		}
+		break;
+	case Opt_iocharset:
+		if (!remount) {
+			unload_nls(uopt->nls_map);
+			uopt->nls_map = NULL;
 		}
+		/* When nls_map is not loaded then UTF-8 is used */
+		if (!remount && strcmp(param->string, "utf8") != 0) {
+			uopt->nls_map = load_nls(param->string);
+			if (!uopt->nls_map) {
+				errorf(fc, "iocharset %s not found",
+					param->string);
+				return -EINVAL;;
+			}
+		}
+		break;
+	case Opt_fmode:
+		uopt->fmode = result.uint_32 & 0777;
+		break;
+	case Opt_dmode:
+		uopt->dmode = result.uint_32 & 0777;
+		break;
+	default:
+		errorf(fc, "bad mount option \"%s\" or missing value", p);
+		return -EINVAL;
 	}
-	return 1;
+	return 0;
 }
 
-static int udf_remount_fs(struct super_block *sb, int *flags, char *options)
+static int udf_reconfigure(struct fs_context *fc)
 {
-	struct udf_options uopt;
+	struct udf_options *uopt = fc->fs_private;
+	struct super_block *sb = fc->root->d_sb;
 	struct udf_sb_info *sbi = UDF_SB(sb);
+	int readonly = fc->sb_flags & SB_RDONLY;
 	int error = 0;
 
-	if (!(*flags & SB_RDONLY) && UDF_QUERY_FLAG(sb, UDF_FLAG_RW_INCOMPAT))
+	if (!readonly && UDF_QUERY_FLAG(sb, UDF_FLAG_RW_INCOMPAT))
 		return -EACCES;
 
 	sync_filesystem(sb);
 
-	uopt.flags = sbi->s_flags;
-	uopt.uid   = sbi->s_uid;
-	uopt.gid   = sbi->s_gid;
-	uopt.umask = sbi->s_umask;
-	uopt.fmode = sbi->s_fmode;
-	uopt.dmode = sbi->s_dmode;
-	uopt.nls_map = NULL;
-
-	if (!udf_parse_options(options, &uopt, true))
-		return -EINVAL;
-
 	write_lock(&sbi->s_cred_lock);
-	sbi->s_flags = uopt.flags;
-	sbi->s_uid   = uopt.uid;
-	sbi->s_gid   = uopt.gid;
-	sbi->s_umask = uopt.umask;
-	sbi->s_fmode = uopt.fmode;
-	sbi->s_dmode = uopt.dmode;
+	sbi->s_flags = uopt->flags;
+	sbi->s_uid   = uopt->uid;
+	sbi->s_gid   = uopt->gid;
+	sbi->s_umask = uopt->umask;
+	sbi->s_fmode = uopt->fmode;
+	sbi->s_dmode = uopt->dmode;
 	write_unlock(&sbi->s_cred_lock);
 
-	if ((bool)(*flags & SB_RDONLY) == sb_rdonly(sb))
+	if (readonly == sb_rdonly(sb))
 		goto out_unlock;
 
-	if (*flags & SB_RDONLY)
+	if (readonly)
 		udf_close_lvid(sb);
 	else
 		udf_open_lvid(sb);
@@ -2084,23 +2111,15 @@ u64 lvid_get_unique_id(struct super_block *sb)
 	return ret;
 }
 
-static int udf_fill_super(struct super_block *sb, void *options, int silent)
+static int udf_fill_super(struct super_block *sb, struct fs_context *fc)
 {
 	int ret = -EINVAL;
 	struct inode *inode = NULL;
-	struct udf_options uopt;
+	struct udf_options *uopt = fc->fs_private;
 	struct kernel_lb_addr rootdir, fileset;
 	struct udf_sb_info *sbi;
 	bool lvid_open = false;
-
-	uopt.flags = (1 << UDF_FLAG_USE_AD_IN_ICB) | (1 << UDF_FLAG_STRICT);
-	/* By default we'll use overflow[ug]id when UDF inode [ug]id == -1 */
-	uopt.uid = make_kuid(current_user_ns(), overflowuid);
-	uopt.gid = make_kgid(current_user_ns(), overflowgid);
-	uopt.umask = 0;
-	uopt.fmode = UDF_INVALID_MODE;
-	uopt.dmode = UDF_INVALID_MODE;
-	uopt.nls_map = NULL;
+	int silent = fc->sb_flags & SB_SILENT;
 
 	sbi = kzalloc(sizeof(*sbi), GFP_KERNEL);
 	if (!sbi)
@@ -2110,25 +2129,22 @@ static int udf_fill_super(struct super_block *sb, void *options, int silent)
 
 	mutex_init(&sbi->s_alloc_mutex);
 
-	if (!udf_parse_options((char *)options, &uopt, false))
-		goto parse_options_failure;
-
 	fileset.logicalBlockNum = 0xFFFFFFFF;
 	fileset.partitionReferenceNum = 0xFFFF;
 
-	sbi->s_flags = uopt.flags;
-	sbi->s_uid = uopt.uid;
-	sbi->s_gid = uopt.gid;
-	sbi->s_umask = uopt.umask;
-	sbi->s_fmode = uopt.fmode;
-	sbi->s_dmode = uopt.dmode;
-	sbi->s_nls_map = uopt.nls_map;
+	sbi->s_flags = uopt->flags;
+	sbi->s_uid = uopt->uid;
+	sbi->s_gid = uopt->gid;
+	sbi->s_umask = uopt->umask;
+	sbi->s_fmode = uopt->fmode;
+	sbi->s_dmode = uopt->dmode;
+	sbi->s_nls_map = uopt->nls_map;
 	rwlock_init(&sbi->s_cred_lock);
 
-	if (uopt.session == 0xFFFFFFFF)
+	if (uopt->session == 0xFFFFFFFF)
 		sbi->s_session = udf_get_last_session(sb);
 	else
-		sbi->s_session = uopt.session;
+		sbi->s_session = uopt->session;
 
 	udf_debug("Multi-session=%d\n", sbi->s_session);
 
@@ -2139,16 +2155,16 @@ static int udf_fill_super(struct super_block *sb, void *options, int silent)
 	sb->s_magic = UDF_SUPER_MAGIC;
 	sb->s_time_gran = 1000;
 
-	if (uopt.flags & (1 << UDF_FLAG_BLOCKSIZE_SET)) {
-		ret = udf_load_vrs(sb, &uopt, silent, &fileset);
+	if (uopt->flags & (1 << UDF_FLAG_BLOCKSIZE_SET)) {
+		ret = udf_load_vrs(sb, uopt, silent, &fileset);
 	} else {
-		uopt.blocksize = bdev_logical_block_size(sb->s_bdev);
-		while (uopt.blocksize <= 4096) {
-			ret = udf_load_vrs(sb, &uopt, silent, &fileset);
+		uopt->blocksize = bdev_logical_block_size(sb->s_bdev);
+		while (uopt->blocksize <= 4096) {
+			ret = udf_load_vrs(sb, uopt, silent, &fileset);
 			if (ret < 0) {
 				if (!silent && ret != -EACCES) {
 					pr_notice("Scanning with blocksize %u failed\n",
-						  uopt.blocksize);
+						  uopt->blocksize);
 				}
 				brelse(sbi->s_lvid_bh);
 				sbi->s_lvid_bh = NULL;
@@ -2161,7 +2177,7 @@ static int udf_fill_super(struct super_block *sb, void *options, int silent)
 			} else
 				break;
 
-			uopt.blocksize <<= 1;
+			uopt->blocksize <<= 1;
 		}
 	}
 	if (ret < 0) {
@@ -2266,8 +2282,7 @@ static int udf_fill_super(struct super_block *sb, void *options, int silent)
 
 error_out:
 	iput(sbi->s_vat_inode);
-parse_options_failure:
-	unload_nls(uopt.nls_map);
+	unload_nls(uopt->nls_map);
 	if (lvid_open)
 		udf_close_lvid(sb);
 	brelse(sbi->s_lvid_bh);
-- 
2.43.0

Re: [PATCH] udf: convert to new mount API

[Eric Sandeen]

On 2/9/24 1:43 PM, Eric Sandeen wrote:
> Convert the UDF filesystem to the new mount API.
> 
> UDF is slightly unique in that it always preserves prior mount
> options across a remount, so that's handled by udf_init_options().
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> 
> Tested by running through xfstests, as well as some targeted testing of
> remount behavior.
> 
> NB: I did not convert i.e any udf_err() to errorf(fc, ) because the former
> does some nice formatting, not sure how or if you'd like to handle that, Jan?
> 
>  fs/udf/super.c | 495 +++++++++++++++++++++++++------------------------
>  1 file changed, 255 insertions(+), 240 deletions(-)
> 
> diff --git a/fs/udf/super.c b/fs/udf/super.c
> index 928a04d9d9e0..03fa98fe4e1c 100644
> --- a/fs/udf/super.c
> +++ b/fs/udf/super.c

...

> +	case Opt_gid:
> +		if (0 == kstrtoint(param->string, 10, &uv)) {
>  			uopt->gid = make_kgid(current_user_ns(), uv);
>  			if (!gid_valid(uopt->gid))
> -				return 0;
> +				return -EINVAL;
>  			uopt->flags |= (1 <<  );
> -			break;
> -		case Opt_uid:
> -			if (match_uint(args, &uv))
> -				return 0;
> +		} else if (!strcmp(param->string, "forget")) {
> +			uopt->flags |= (1 << UDF_FLAG_GID_FORGET);
> +		} else if (!strcmp(param->string, "ignore")) {
> +			/* this option is superseded by gid=<number> */
> +			;
> +		} else {
> +			return -EINVAL;
> +		}
> +		break;

I wonder if I need to redo this and not directly set the make_kgid option
into uopt->gid. We do test that uopt->gid is valid, and return an error, and
skip setting UDF_FLAG_GID_SET, but ...

...

> -static int udf_fill_super(struct super_block *sb, void *options, int silent)
> +static int udf_fill_super(struct super_block *sb, struct fs_context *fc)
>  {
>  	int ret = -EINVAL;
>  	struct inode *inode = NULL;
> -	struct udf_options uopt;
> +	struct udf_options *uopt = fc->fs_private;
>  	struct kernel_lb_addr rootdir, fileset;
>  	struct udf_sb_info *sbi;
>  	bool lvid_open = false;
> -
> -	uopt.flags = (1 << UDF_FLAG_USE_AD_IN_ICB) | (1 << UDF_FLAG_STRICT);
> -	/* By default we'll use overflow[ug]id when UDF inode [ug]id == -1 */
> -	uopt.uid = make_kuid(current_user_ns(), overflowuid);
> -	uopt.gid = make_kgid(current_user_ns(), overflowgid);

this initialization (now moved to udf_init_options) gets overwritten
even if the [gu]id was invalid during parsing ...

> +	sbi->s_flags = uopt->flags;
> +	sbi->s_uid = uopt->uid;
> +	sbi->s_gid = uopt->gid;

... and gets set into sbi here.

In the past (I think) the whole mount would fail with an invalid UID/GID but w/
fsconfig, we could just fail that one config and continue with the rest.

It looks like sbi->s_[gu]id is not accessed unless UDF_FLAG_[GU]ID_SET is
set, but maybe it's best to never set something invalid into the uopt.

-Eric

Re: [PATCH] udf: convert to new mount API

[Jan Kara]

On Fri 09-02-24 13:43:09, Eric Sandeen wrote:
> Convert the UDF filesystem to the new mount API.
> 
> UDF is slightly unique in that it always preserves prior mount
> options across a remount, so that's handled by udf_init_options().

Well, ext4 does this as well AFAICT. See e.g. ext4_apply_options() and how
it does:

        sbi->s_mount_opt &= ~ctx->mask_s_mount_opt;
        sbi->s_mount_opt |= ctx->vals_s_mount_opt;
        sbi->s_mount_opt2 &= ~ctx->mask_s_mount_opt2;
        sbi->s_mount_opt2 |= ctx->vals_s_mount_opt2;
        sb->s_flags &= ~ctx->mask_s_flags;
        sb->s_flags |= ctx->vals_s_flags;

so it really modifies the current superblock state, not overwrites it.

> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> 
> Tested by running through xfstests, as well as some targeted testing of
> remount behavior.
> 
> NB: I did not convert i.e any udf_err() to errorf(fc, ) because the former
> does some nice formatting, not sure how or if you'd like to handle that, Jan?

Which one would you like to convert? I didn't find any obvious
candidates... Or do you mean the messages in udf_fill_super() when we find
on-disk inconsistencies or similar? I guess we can leave that for later
because propagating 'fc' into all the validation functions will be a lot of
churn.

> +static void udf_init_options(struct fs_context *fc, struct udf_options *uopt)
> +{
> +	if (fc->purpose == FS_CONTEXT_FOR_RECONFIGURE) {
> +		struct super_block *sb = fc->root->d_sb;
> +		struct udf_sb_info *sbi = UDF_SB(sb);
> +
> +		uopt->flags = sbi->s_flags;
> +		uopt->uid   = sbi->s_uid;
> +		uopt->gid   = sbi->s_gid;
> +		uopt->umask = sbi->s_umask;
> +		uopt->fmode = sbi->s_fmode;
> +		uopt->dmode = sbi->s_dmode;
> +		uopt->nls_map = NULL;
> +	} else {
> +		uopt->flags = (1 << UDF_FLAG_USE_AD_IN_ICB) | (1 << UDF_FLAG_STRICT);
> +		/* By default we'll use overflow[ug]id when UDF inode [ug]id == -1 */

Nit: Please wrap these two lines.

> +		uopt->uid = make_kuid(current_user_ns(), overflowuid);
> +		uopt->gid = make_kgid(current_user_ns(), overflowgid);
> +		uopt->umask = 0;
> +		uopt->fmode = UDF_INVALID_MODE;
> +		uopt->dmode = UDF_INVALID_MODE;
> +		uopt->nls_map = NULL;
> +		uopt->session = 0xFFFFFFFF;
> +	}
> +}
> +
> +static int udf_init_fs_context(struct fs_context *fc)
> +{
> +	struct udf_options *uopt;
> +
> +	uopt = kzalloc(sizeof(*uopt), GFP_KERNEL);
> +	if (!uopt)
> +		return -ENOMEM;
> +
> +	udf_init_options(fc, uopt);
> +
> +	fc->fs_private = uopt;
> +	fc->ops = &udf_context_ops;
> +
> +	return 0;
> +}
> +
> +static void udf_free_fc(struct fs_context *fc)
> +{
> +	kfree(fc->fs_private);
> +}

So I think we need to unload uopt->nls_map in case we eventually failed the
mount (which means we also need to zero uopt->nls_map if we copy it to the
sbi).

> +static const struct fs_parameter_spec udf_param_spec[] = {
> +	fsparam_flag	("novrs",		Opt_novrs),
> +	fsparam_flag	("nostrict",		Opt_nostrict),
> +	fsparam_u32	("bs",			Opt_bs),
> +	fsparam_flag	("unhide",		Opt_unhide),
> +	fsparam_flag	("undelete",		Opt_undelete),
> +	fsparam_flag	("noadinicb",		Opt_noadinicb),
> +	fsparam_flag	("adinicb",		Opt_adinicb),

We could actually use the fs_param_neg_with_no for the above. I don't
insist but it's an interesting exercise :)

> +	fsparam_flag	("shortad",		Opt_shortad),
> +	fsparam_flag	("longad",		Opt_longad),
> +	fsparam_string	("gid",			Opt_gid),
> +	fsparam_string	("uid",			Opt_uid),
> +	fsparam_u32	("umask",		Opt_umask),
> +	fsparam_u32	("session",		Opt_session),
> +	fsparam_u32	("lastblock",		Opt_lastblock),
> +	fsparam_u32	("anchor",		Opt_anchor),
> +	fsparam_u32	("volume",		Opt_volume),
> +	fsparam_u32	("partition",		Opt_partition),
> +	fsparam_u32	("fileset",		Opt_fileset),
> +	fsparam_u32	("rootdir",		Opt_rootdir),
> +	fsparam_flag	("utf8",		Opt_utf8),
> +	fsparam_string	("iocharset",		Opt_iocharset),
> +	fsparam_u32	("mode",		Opt_fmode),
> +	fsparam_u32	("dmode",		Opt_dmode),
> +	{}
> + };
> + 
> +static int udf_parse_param(struct fs_context *fc, struct fs_parameter *param)

...

> +	unsigned int n;
> +	struct udf_options *uopt = fc->fs_private;
> +	struct fs_parse_result result;
> +	int token;
> +	bool remount = (fc->purpose & FS_CONTEXT_FOR_RECONFIGURE);
> +
> +	token = fs_parse(fc, udf_param_spec, param, &result);
> +	if (token < 0)
> +		return token;
> +
> +	switch (token) {
> +	case Opt_novrs:
> +		uopt->novrs = 1;

I guess we can make this just an ordinary flag as a prep patch?

> +		break;
> +	case Opt_bs:
> +		n = result.uint_32;;
				  ^^ one is enough ;)

> +		if (n != 512 && n != 1024 && n != 2048 && n != 4096)
> +			return -EINVAL;
> +		uopt->blocksize = n;
> +		uopt->flags |= (1 << UDF_FLAG_BLOCKSIZE_SET);
> +		break;
> +	case Opt_unhide:
> +		uopt->flags |= (1 << UDF_FLAG_UNHIDE);
> +		break;
> +	case Opt_undelete:
> +		uopt->flags |= (1 << UDF_FLAG_UNDELETE);
> +		break;

These two are nops so we should deprecate them and completely ignore them.
I'm writing it here mostly as a reminder to myself as a work item after the
conversion is done :)

> +	case Opt_noadinicb:
> +		uopt->flags &= ~(1 << UDF_FLAG_USE_AD_IN_ICB);
> +		break;
> +	case Opt_adinicb:
> +		uopt->flags |= (1 << UDF_FLAG_USE_AD_IN_ICB);
> +		break;
> +	case Opt_shortad:
> +		uopt->flags |= (1 << UDF_FLAG_USE_SHORT_AD);
> +		break;
> +	case Opt_longad:
> +		uopt->flags &= ~(1 << UDF_FLAG_USE_SHORT_AD);
> +		break;
> +	case Opt_gid:
> +		if (0 == kstrtoint(param->string, 10, &uv)) {
Nit:
		    ^^ I prefer "kstrtoint() == 0"

Otherwise looks good.

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

Re: [PATCH] udf: convert to new mount API

[Jan Kara]

On Mon 12-02-24 14:31:10, Eric Sandeen wrote:
> On 2/9/24 1:43 PM, Eric Sandeen wrote:
> > Convert the UDF filesystem to the new mount API.
> > 
> > UDF is slightly unique in that it always preserves prior mount
> > options across a remount, so that's handled by udf_init_options().
> > 
> > Signed-off-by: Eric Sandeen <sandeen@redhat.com>

...

> > +	case Opt_gid:
> > +		if (0 == kstrtoint(param->string, 10, &uv)) {
> >  			uopt->gid = make_kgid(current_user_ns(), uv);
> >  			if (!gid_valid(uopt->gid))
> > -				return 0;
> > +				return -EINVAL;
> >  			uopt->flags |= (1 <<  );
> > -			break;
> > -		case Opt_uid:
> > -			if (match_uint(args, &uv))
> > -				return 0;
> > +		} else if (!strcmp(param->string, "forget")) {
> > +			uopt->flags |= (1 << UDF_FLAG_GID_FORGET);
> > +		} else if (!strcmp(param->string, "ignore")) {
> > +			/* this option is superseded by gid=<number> */
> > +			;
> > +		} else {
> > +			return -EINVAL;
> > +		}
> > +		break;
> 
> I wonder if I need to redo this and not directly set the make_kgid option
> into uopt->gid. We do test that uopt->gid is valid, and return an error, and
> skip setting UDF_FLAG_GID_SET, but ...
> 
> ...
> 
> > -static int udf_fill_super(struct super_block *sb, void *options, int silent)
> > +static int udf_fill_super(struct super_block *sb, struct fs_context *fc)
> >  {
> >  	int ret = -EINVAL;
> >  	struct inode *inode = NULL;
> > -	struct udf_options uopt;
> > +	struct udf_options *uopt = fc->fs_private;
> >  	struct kernel_lb_addr rootdir, fileset;
> >  	struct udf_sb_info *sbi;
> >  	bool lvid_open = false;
> > -
> > -	uopt.flags = (1 << UDF_FLAG_USE_AD_IN_ICB) | (1 << UDF_FLAG_STRICT);
> > -	/* By default we'll use overflow[ug]id when UDF inode [ug]id == -1 */
> > -	uopt.uid = make_kuid(current_user_ns(), overflowuid);
> > -	uopt.gid = make_kgid(current_user_ns(), overflowgid);
> 
> this initialization (now moved to udf_init_options) gets overwritten
> even if the [gu]id was invalid during parsing ...
> 
> > +	sbi->s_flags = uopt->flags;
> > +	sbi->s_uid = uopt->uid;
> > +	sbi->s_gid = uopt->gid;
> 
> ... and gets set into sbi here.
> 
> In the past (I think) the whole mount would fail with an invalid UID/GID but w/
> fsconfig, we could just fail that one config and continue with the rest.

I see. Yes, I guess it cannot happen with normal mount but let's be
defensive and make sure only valid uid/gid gets into uopt and sbi.

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

Re: [PATCH] udf: convert to new mount API

[Eric Sandeen]

On 2/13/24 6:49 AM, Jan Kara wrote:
> On Fri 09-02-24 13:43:09, Eric Sandeen wrote:
>> Convert the UDF filesystem to the new mount API.
>>
>> UDF is slightly unique in that it always preserves prior mount
>> options across a remount, so that's handled by udf_init_options().
> Well, ext4 does this as well AFAICT. See e.g. ext4_apply_options() and how
> it does:
> 
>         sbi->s_mount_opt &= ~ctx->mask_s_mount_opt;
>         sbi->s_mount_opt |= ctx->vals_s_mount_opt;
>         sbi->s_mount_opt2 &= ~ctx->mask_s_mount_opt2;
>         sbi->s_mount_opt2 |= ctx->vals_s_mount_opt2;
>         sb->s_flags &= ~ctx->mask_s_flags;
>         sb->s_flags |= ctx->vals_s_flags;
> 
> so it really modifies the current superblock state, not overwrites it.
> 
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>> ---
>>
>> Tested by running through xfstests, as well as some targeted testing of
>> remount behavior.
>>
>> NB: I did not convert i.e any udf_err() to errorf(fc, ) because the former
>> does some nice formatting, not sure how or if you'd like to handle that, Jan?

> Which one would you like to convert? I didn't find any obvious
> candidates... Or do you mean the messages in udf_fill_super() when we find
> on-disk inconsistencies or similar? I guess we can leave that for later
> because propagating 'fc' into all the validation functions will be a lot of
> churn.

Yup I was thinking about messages in fill_super. I can check w/ dhowells later
to see what the expectation is, but I had the hunch that generally any errors
encountered during mount should route through those functions now.

Happy to leave it for later ;)

-Eric

Re: [PATCH] udf: convert to new mount API

[Eric Sandeen]

On 2/13/24 6:49 AM, Jan Kara wrote:
> On Fri 09-02-24 13:43:09, Eric Sandeen wrote:
>> Convert the UDF filesystem to the new mount API.

...

>> +static void udf_init_options(struct fs_context *fc, struct udf_options *uopt)
>> +{
>> +	if (fc->purpose == FS_CONTEXT_FOR_RECONFIGURE) {
>> +		struct super_block *sb = fc->root->d_sb;
>> +		struct udf_sb_info *sbi = UDF_SB(sb);
>> +
>> +		uopt->flags = sbi->s_flags;
>> +		uopt->uid   = sbi->s_uid;
>> +		uopt->gid   = sbi->s_gid;
>> +		uopt->umask = sbi->s_umask;
>> +		uopt->fmode = sbi->s_fmode;
>> +		uopt->dmode = sbi->s_dmode;
>> +		uopt->nls_map = NULL;
>> +	} else {
>> +		uopt->flags = (1 << UDF_FLAG_USE_AD_IN_ICB) | (1 << UDF_FLAG_STRICT);
>> +		/* By default we'll use overflow[ug]id when UDF inode [ug]id == -1 */
> 
> Nit: Please wrap these two lines.

Done, noticed that after sending, sorry!

>> +static void udf_free_fc(struct fs_context *fc)
>> +{
>> +	kfree(fc->fs_private);
>> +}
> 
> So I think we need to unload uopt->nls_map in case we eventually failed the
> mount (which means we also need to zero uopt->nls_map if we copy it to the
> sbi).

Hmm let me look into that - I guess there are ways for i.e. udf_fill_super
can fail that wouldn't free it before we got here.

>> +static const struct fs_parameter_spec udf_param_spec[] = {
>> +	fsparam_flag	("novrs",		Opt_novrs),
>> +	fsparam_flag	("nostrict",		Opt_nostrict),
>> +	fsparam_u32	("bs",			Opt_bs),
>> +	fsparam_flag	("unhide",		Opt_unhide),
>> +	fsparam_flag	("undelete",		Opt_undelete),
>> +	fsparam_flag	("noadinicb",		Opt_noadinicb),
>> +	fsparam_flag	("adinicb",		Opt_adinicb),
> 
> We could actually use the fs_param_neg_with_no for the above. I don't
> insist but it's an interesting exercise :)

good idea, done.

...

>> +	switch (token) {
>> +	case Opt_novrs:
>> +		uopt->novrs = 1;
> 
> I guess we can make this just an ordinary flag as a prep patch?

Sorry, not sure what you mean by this?

Oh, a uopt->flag. Ok, I can do that.

>> +		break;
>> +	case Opt_bs:
>> +		n = result.uint_32;;
> 				  ^^ one is enough ;)
> 
>> +		if (n != 512 && n != 1024 && n != 2048 && n != 4096)
>> +			return -EINVAL;
>> +		uopt->blocksize = n;
>> +		uopt->flags |= (1 << UDF_FLAG_BLOCKSIZE_SET);
>> +		break;
>> +	case Opt_unhide:
>> +		uopt->flags |= (1 << UDF_FLAG_UNHIDE);
>> +		break;
>> +	case Opt_undelete:
>> +		uopt->flags |= (1 << UDF_FLAG_UNDELETE);
>> +		break;
> 
> These two are nops so we should deprecate them and completely ignore them.
> I'm writing it here mostly as a reminder to myself as a work item after the
> conversion is done :)

ok ;)
 
>> +	case Opt_noadinicb:
>> +		uopt->flags &= ~(1 << UDF_FLAG_USE_AD_IN_ICB);
>> +		break;
>> +	case Opt_adinicb:
>> +		uopt->flags |= (1 << UDF_FLAG_USE_AD_IN_ICB);
>> +		break;
>> +	case Opt_shortad:
>> +		uopt->flags |= (1 << UDF_FLAG_USE_SHORT_AD);
>> +		break;
>> +	case Opt_longad:
>> +		uopt->flags &= ~(1 << UDF_FLAG_USE_SHORT_AD);
>> +		break;
>> +	case Opt_gid:
>> +		if (0 == kstrtoint(param->string, 10, &uv)) {
> Nit:
> 		    ^^ I prefer "kstrtoint() == 0"

No problem.

-Eric

> Otherwise looks good.
> 
> 								Honza

Re: [PATCH] udf: convert to new mount API

[Jan Kara]

On Tue 13-02-24 17:11:27, Eric Sandeen wrote:
> On 2/13/24 6:49 AM, Jan Kara wrote:
> > On Fri 09-02-24 13:43:09, Eric Sandeen wrote:
> >> +	switch (token) {
> >> +	case Opt_novrs:
> >> +		uopt->novrs = 1;
> > 
> > I guess we can make this just an ordinary flag as a prep patch?
> 
> Sorry, not sure what you mean by this?
> 
> Oh, a uopt->flag. Ok, I can do that.

Yes, just a flag in uopt->flags. Sorry for being unclear.

								Honza

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