fix mount option parsing in remount

Remount currently happily accept any option thrown at it, although the only filesystem specific option it actually handles is barrier/nobarrier. And it actually doesn't handle these correctly either because it only uses the value it parsed when we're doing a ro->rw transition. In addition to that there's also a bad bug in xfs_parseargs which doesn't touch the actual option in the mount point except for a single one, XFS_MOUNT_SMALL_INUMS and thus forced any filesystem that's every remounted in some way to not support 64bit inodes with no way to recover unless unmounted.

This patch changes xfs_fs_remount to use it's own linux/parser.h based options parse instead of xfs_parseargs and reject all options except for barrier/nobarrier and to the right thing in general. Eventually I'd like to have a single big option table used for mount aswell but that can wait for a while.

Signed-off-by Christoph Hellwig <[email protected]>

Index: linux-2.6-xfs/fs/xfs/linux-2.6/xfs_super.c
===================================================================

fs/xfs/linux-2.6/xfs_super.c +49 -15

@@ -67,6 +67,7 @@
 #include <linux/writeback.h>
 #include <linux/kthread.h>
 #include <linux/freezer.h>
+#include <linux/parser.h>
 
 static struct quotactl_ops xfs_quotactl_operations;
 static struct super_operations xfs_super_operations;

@@ -1255,6 +1256,19 @@ xfs_fs_statfs(
 	return 0;
 }
 
+/*
+ * Eventually we should extend this table and use it for mount, too.
+ */
+enum {
+	Opt_barrier, Opt_nobarrier, Opt_err
+};
+
+static match_table_t tokens = {
+	{Opt_barrier, "barrier"},
+	{Opt_nobarrier, "nobarrier"},
+	{Opt_err, NULL}
+};
+
 STATIC int
 xfs_fs_remount(
 	struct super_block	*sb,

@@ -1262,36 +1276,54 @@ xfs_fs_remount(
 	char			*options)
 {
 	struct xfs_mount	*mp = XFS_M(sb);
-	struct xfs_mount_args	*args;
-	int			error;
+	substring_t		args[MAX_OPT_ARGS];
+	char			*p;
 
-	args = xfs_args_allocate(sb, 0);
-	if (!args)
-		return -ENOMEM;
+	while ((p = strsep(&options, ",")) != NULL) {
+		int token;
 
-	error = xfs_parseargs(mp, options, args, 1);
-	if (error)
-		goto out_free_args;
+		if (!*p)
+			continue;
 
-	if (!(*flags & MS_RDONLY)) {			/* rw/ro -> rw */
-		if (mp->m_flags & XFS_MOUNT_RDONLY)
-		mp->m_flags &= ~XFS_MOUNT_RDONLY;
-		if (args->flags & XFSMNT_BARRIER) {
+		token = match_token(p, tokens, args);
+		switch (token) {
+		case Opt_barrier:
 			mp->m_flags |= XFS_MOUNT_BARRIER;
-			xfs_mountfs_check_barriers(mp);
-		} else {
+
+			/*
+			 * Test if barriers are actually working if we can,
+			 * else delay this check until the filesystem is
+			 * marked writeable.
+			 */
+			if (!(mp->m_flags & XFS_MOUNT_RDONLY))
+				xfs_mountfs_check_barriers(mp);
+			break;
+		case Opt_nobarrier:
 			mp->m_flags &= ~XFS_MOUNT_BARRIER;
+			break;
+		default:
+			printk(KERN_INFO
+	"XFS: mount option \"%s\" not support for remount\n", p);
+			return -EINVAL;
 		}
-	} else if (!(mp->m_flags & XFS_MOUNT_RDONLY)) {	/* rw -> ro */
+	}
+
+	/* rw/ro -> rw */
+	if ((mp->m_flags & XFS_MOUNT_RDONLY) && !(*flags & MS_RDONLY)) {
+		mp->m_flags &= ~XFS_MOUNT_RDONLY;
+		if (mp->m_flags & XFS_MOUNT_BARRIER)
+			xfs_mountfs_check_barriers(mp);
+	}
+
+	/* rw -> ro */
+	if (!(mp->m_flags & XFS_MOUNT_RDONLY) && (*flags & MS_RDONLY)) {
 		xfs_filestream_flush(mp);
 		xfs_sync(mp, SYNC_DATA_QUIESCE);
 		xfs_attr_quiesce(mp);

ping? silently ignoring wrong options causes a lot of confusion for users, and the patch sould simple enough. Anyone take 10 minutes to review it and check it in?

Quoted 117 lines · Christoph Hellwig

Remount currently happily accept any option thrown at it, although the only filesystem specific option it actually handles is barrier/nobarrier. And it actually doesn't handle these correctly either because it only uses the value it parsed when we're doing a ro->rw transition. In addition to that there's also a bad bug in xfs_parseargs which doesn't touch the actual option in the mount point except for a single one, XFS_MOUNT_SMALL_INUMS and thus forced any filesystem that's every remounted in some way to not support 64bit inodes with no way to recover unless unmounted.

This patch changes xfs_fs_remount to use it's own linux/parser.h based options parse instead of xfs_parseargs and reject all options except for barrier/nobarrier and to the right thing in general. Eventually I'd like to have a single big option table used for mount aswell but that can wait for a while.

Signed-off-by Christoph Hellwig <[email protected]>

Index: linux-2.6-xfs/fs/xfs/linux-2.6/xfs_super.c
===================================================================

fs/xfs/linux-2.6/xfs_super.c +49 -15

@@ -67,6 +67,7 @@
 #include <linux/writeback.h>
 #include <linux/kthread.h>
 #include <linux/freezer.h>
+#include <linux/parser.h>
 
 static struct quotactl_ops xfs_quotactl_operations;
 static struct super_operations xfs_super_operations;

@@ -1255,6 +1256,19 @@ xfs_fs_statfs(
 	return 0;
 }
 
+/*
+ * Eventually we should extend this table and use it for mount, too.
+ */
+enum {
+	Opt_barrier, Opt_nobarrier, Opt_err
+};
+
+static match_table_t tokens = {
+	{Opt_barrier, "barrier"},
+	{Opt_nobarrier, "nobarrier"},
+	{Opt_err, NULL}
+};
+
 STATIC int
 xfs_fs_remount(
 	struct super_block	*sb,

@@ -1262,36 +1276,54 @@ xfs_fs_remount(
 	char			*options)
 {
 	struct xfs_mount	*mp = XFS_M(sb);
-	struct xfs_mount_args	*args;
-	int			error;
+	substring_t		args[MAX_OPT_ARGS];
+	char			*p;
 
-	args = xfs_args_allocate(sb, 0);
-	if (!args)
-		return -ENOMEM;
+	while ((p = strsep(&options, ",")) != NULL) {
+		int token;
 
-	error = xfs_parseargs(mp, options, args, 1);
-	if (error)
-		goto out_free_args;
+		if (!*p)
+			continue;
 
-	if (!(*flags & MS_RDONLY)) {			/* rw/ro -> rw */
-		if (mp->m_flags & XFS_MOUNT_RDONLY)
-		mp->m_flags &= ~XFS_MOUNT_RDONLY;
-		if (args->flags & XFSMNT_BARRIER) {
+		token = match_token(p, tokens, args);
+		switch (token) {
+		case Opt_barrier:
 			mp->m_flags |= XFS_MOUNT_BARRIER;
-			xfs_mountfs_check_barriers(mp);
-		} else {
+
+			/*
+			 * Test if barriers are actually working if we can,
+			 * else delay this check until the filesystem is
+			 * marked writeable.
+			 */
+			if (!(mp->m_flags & XFS_MOUNT_RDONLY))
+				xfs_mountfs_check_barriers(mp);
+			break;
+		case Opt_nobarrier:
 			mp->m_flags &= ~XFS_MOUNT_BARRIER;
+			break;
+		default:
+			printk(KERN_INFO
+	"XFS: mount option \"%s\" not support for remount\n", p);
+			return -EINVAL;
 		}
-	} else if (!(mp->m_flags & XFS_MOUNT_RDONLY)) {	/* rw -> ro */
+	}
+
+	/* rw/ro -> rw */
+	if ((mp->m_flags & XFS_MOUNT_RDONLY) && !(*flags & MS_RDONLY)) {
+		mp->m_flags &= ~XFS_MOUNT_RDONLY;
+		if (mp->m_flags & XFS_MOUNT_BARRIER)
+			xfs_mountfs_check_barriers(mp);
+	}
+
+	/* rw -> ro */
+	if (!(mp->m_flags & XFS_MOUNT_RDONLY) && (*flags & MS_RDONLY)) {
 		xfs_filestream_flush(mp);
 		xfs_sync(mp, SYNC_DATA_QUIESCE);
 		xfs_attr_quiesce(mp);
 		mp->m_flags |= XFS_MOUNT_RDONLY;
 	}
 

fs/xfs/linux-2.6/xfs_super.c xfs_fs_remount( · line 1329

Quoted 7 lines

fs/xfs/linux-2.6/xfs_super.c +1 -3

- out_free_args:
-	kfree(args);
-	return -error;
+	return 0;
 }
 
 /*

---end quoted text---

ping^2

Quoted 3 lines · Christoph Hellwig

ping? silently ignoring wrong options causes a lot of confusion for users, and the patch sould simple enough. Anyone take 10 minutes to review it and check it in?

Remount currently happily accept any option thrown at it, although the only filesystem specific option it actually handles is barrier/nobarrier. And it actually doesn't handle these correctly either because it only uses the value it parsed when we're doing a ro->rw transition. In addition to that there's also a bad bug in xfs_parseargs which doesn't touch the actual option in the mount point except for a single one, XFS_MOUNT_SMALL_INUMS and thus forced any filesystem that's every remounted in some way to not support 64bit inodes with no way to recover unless unmounted.

This patch changes xfs_fs_remount to use it's own linux/parser.h based options parse instead of xfs_parseargs and reject all options except for barrier/nobarrier and to the right thing in general. Eventually I'd like to have a single big option table used for mount aswell but that can wait for a while.

Signed-off-by Christoph Hellwig <[email protected]>

Index: linux-2.6-xfs/fs/xfs/linux-2.6/xfs_super.c
===================================================================

fs/xfs/linux-2.6/xfs_super.c +1 -0

@@ -67,6 +67,7 @@
 #include <linux/writeback.h>
 #include <linux/kthread.h>
 #include <linux/freezer.h>
+#include <linux/parser.h>
 
 static struct quotactl_ops xfs_quotactl_operations;
 static struct super_operations xfs_super_operations;

fs/xfs/linux-2.6/xfs_super.c +47 -18

+ */
+enum {
+	Opt_barrier, Opt_nobarrier, Opt_err
+};
+
+static match_table_t tokens = {
+	{Opt_barrier, "barrier"},
+	{Opt_nobarrier, "nobarrier"},
+	{Opt_err, NULL}
+};
+
 STATIC int
 xfs_fs_remount(
 	struct super_block	*sb,

@@ -1262,36 +1276,54 @@ xfs_fs_remount(
 	char			*options)
 {
 	struct xfs_mount	*mp = XFS_M(sb);
-	struct xfs_mount_args	*args;
-	int			error;
+	substring_t		args[MAX_OPT_ARGS];
+	char			*p;
 
-	args = xfs_args_allocate(sb, 0);
-	if (!args)
-		return -ENOMEM;
+	while ((p = strsep(&options, ",")) != NULL) {
+		int token;
 
-	error = xfs_parseargs(mp, options, args, 1);
-	if (error)
-		goto out_free_args;
+		if (!*p)
+			continue;
 
-	if (!(*flags & MS_RDONLY)) {			/* rw/ro -> rw */
-		if (mp->m_flags & XFS_MOUNT_RDONLY)
-		mp->m_flags &= ~XFS_MOUNT_RDONLY;
-		if (args->flags & XFSMNT_BARRIER) {
+		token = match_token(p, tokens, args);
+		switch (token) {
+		case Opt_barrier:
 			mp->m_flags |= XFS_MOUNT_BARRIER;
-			xfs_mountfs_check_barriers(mp);
-		} else {
+
+			/*
+			 * Test if barriers are actually working if we can,
+			 * else delay this check until the filesystem is
+			 * marked writeable.
+			 */
+			if (!(mp->m_flags & XFS_MOUNT_RDONLY))
+				xfs_mountfs_check_barriers(mp);
+			break;
+		case Opt_nobarrier:
 			mp->m_flags &= ~XFS_MOUNT_BARRIER;
+			break;
+		default:
+			printk(KERN_INFO
+	"XFS: mount option \"%s\" not support for remount\n", p);
+			return -EINVAL;
 		}
-	} else if (!(mp->m_flags & XFS_MOUNT_RDONLY)) {	/* rw -> ro */
+	}
+
+	/* rw/ro -> rw */
+	if ((mp->m_flags & XFS_MOUNT_RDONLY) && !(*flags & MS_RDONLY)) {
+		mp->m_flags &= ~XFS_MOUNT_RDONLY;
+		if (mp->m_flags & XFS_MOUNT_BARRIER)
+			xfs_mountfs_check_barriers(mp);
+	}
+
+	/* rw -> ro */
+	if (!(mp->m_flags & XFS_MOUNT_RDONLY) && (*flags & MS_RDONLY)) {
 		xfs_filestream_flush(mp);
 		xfs_sync(mp, SYNC_DATA_QUIESCE);
 		xfs_attr_quiesce(mp);
 		mp->m_flags |= XFS_MOUNT_RDONLY;
 	}
 
- out_free_args:
-	kfree(args);
-	return -error;
+	return 0;
 }
 
 /*

Maybe expand this a little:

/*
* Today only the barrier/nobarrier xfs mount options are handled
* on remount. This table should be expanded to cover all options,
* and used for the initial mount path, too.
*/

... I'd also like this for the mount path because for example:

mount -t xfs -o barrier=0 /dev/blah /mnt/foo

works... but does not turn off barriers.

So as a first step/example this looks ok but I'd sure like to see it extended soon. Maybe when I'm bored... :)

Remount currently happily accept any option thrown at it, although the only filesystem specific option it actually handles is barrier/nobarrier. And it actually doesn't handle these correctly either because it only uses the value it parsed when we're doing a ro->rw transition. In addition to that there's also a bad bug in xfs_parseargs which doesn't touch the actual option in the mount point except for a single one, XFS_MOUNT_SMALL_INUMS and thus forced any filesystem that's every remounted in some way to not support 64bit inodes with no way to recover unless unmounted.

This patch changes xfs_fs_remount to use it's own linux/parser.h based options parse instead of xfs_parseargs and reject all options except for barrier/nobarrier and to the right thing in general. Eventually I'd like to have a single big option table used for mount aswell but that can wait for a while.

Signed-off-by Christoph Hellwig <[email protected]>

Index: linux-2.6-xfs/fs/xfs/linux-2.6/xfs_super.c
===================================================================

fs/xfs/linux-2.6/xfs_super.c +28 -14

@@ -67,6 +67,7 @@
 #include <linux/writeback.h>
 #include <linux/kthread.h>
 #include <linux/freezer.h>
+#include <linux/parser.h>
 
 static struct quotactl_ops xfs_quotactl_operations;
 static struct super_operations xfs_super_operations;

@@ -1255,6 +1256,19 @@ xfs_fs_statfs(
 	return 0;
 }
 
+/*
+ * Eventually we should extend this table and use it for mount, too.
+ */
+enum {
+	Opt_barrier, Opt_nobarrier, Opt_err
+};
+
+static match_table_t tokens = {
+	{Opt_barrier, "barrier"},
+	{Opt_nobarrier, "nobarrier"},
+	{Opt_err, NULL}
+};
+
 STATIC int
 xfs_fs_remount(
 	struct super_block	*sb,

@@ -1262,36 +1276,54 @@ xfs_fs_remount(
 	char			*options)
 {
 	struct xfs_mount	*mp = XFS_M(sb);
-	struct xfs_mount_args	*args;
-	int			error;
+	substring_t		args[MAX_OPT_ARGS];
+	char			*p;
 
-	args = xfs_args_allocate(sb, 0);
-	if (!args)
-		return -ENOMEM;
+	while ((p = strsep(&options, ",")) != NULL) {
+		int token;
 
-	error = xfs_parseargs(mp, options, args, 1);
-	if (error)
-		goto out_free_args;
+		if (!*p)
+			continue;
 
-	if (!(*flags & MS_RDONLY)) {			/* rw/ro -> rw */
-		if (mp->m_flags & XFS_MOUNT_RDONLY)
-		mp->m_flags &= ~XFS_MOUNT_RDONLY;
-		if (args->flags & XFSMNT_BARRIER) {
+		token = match_token(p, tokens, args);
+		switch (token) {
+		case Opt_barrier:
 			mp->m_flags |= XFS_MOUNT_BARRIER;
-			xfs_mountfs_check_barriers(mp);
-		} else {
+
+			/*
+			 * Test if barriers are actually working if we can,
+			 * else delay this check until the filesystem is
+			 * marked writeable.

fs/xfs/linux-2.6/xfs_super.c +8 -1

+			return -EINVAL;
 		}
-	} else if (!(mp->m_flags & XFS_MOUNT_RDONLY)) {	/* rw -> ro */
+	}
+
+	/* rw/ro -> rw */
+	if ((mp->m_flags & XFS_MOUNT_RDONLY) && !(*flags & MS_RDONLY)) {
+		mp->m_flags &= ~XFS_MOUNT_RDONLY;
+		if (mp->m_flags & XFS_MOUNT_BARRIER)
+			xfs_mountfs_check_barriers(mp);

typo: s/support/supported/

Looking at ext3 and other XFS out of curiosity:
"XFS: unknown mount option [%s].", this_char "EXT3-fs: Unrecognized mount option \"%s\" "
./smbfs/getopt.c: printk("%s: Unrecognized mount option %s\n", caller, token);
Though I see for remount it is more a question of support versus recognising it or not. Ok.

I'm a little confused why we call xfs_mountfs_check_barriers(mp) in 2 places. Oh okay,
so you want it tested if we have barrier option and
currently not-readonly or after we transition to not-readonly. And you have it around the parsing code because you don't care about what it might transition to in the current not-readonly case.

I think we ideally should have a qa test for this. We have 017 but it doesn't do any barrier or illegal options.