[PATCH 0/4] ext4: Fix regression in mount option parsing and a small cleanup

[PATCH 0/4] ext4: Fix regression in mount option parsing and a small cleanup

[Jan Kara]

  Hello,

  this patch series aims at fixing a regression in parsing of journalled quota
mount options introduced by Ted's mount option parsing rewrite. Patches 1 and 2
cleanup the code a bit and patch 3 then fixes the regression. Patch 4 is a tiny
cleanup.

								Honza

[PATCH 1/4] ext4: Move several mount options to standard handling loop

[Jan Kara]

Several mount option (resuid, resgid, journal_dev, journal_ioprio) are
currently handled before we enter standard option handling loop. I don't
see a reason for this so move them to normal handling loop to make things
more regular.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext4/super.c |   47 ++++++++++++++++++++++++++---------------------
 1 files changed, 26 insertions(+), 21 deletions(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index ceebaf8..30e26f1 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -1406,6 +1406,10 @@ static const struct mount_opts {
 	{Opt_inode_readahead_blks, 0, MOPT_GTE0},
 	{Opt_init_itable, 0, MOPT_GTE0},
 	{Opt_stripe, 0, MOPT_GTE0},
+	{Opt_resuid, 0, MOPT_GTE0},
+	{Opt_resgid, 0, MOPT_GTE0},
+	{Opt_journal_dev, 0, MOPT_GTE0},
+	{Opt_journal_ioprio, 0, MOPT_GTE0},
 	{Opt_data_journal, EXT4_MOUNT_JOURNAL_DATA, MOPT_DATAJ},
 	{Opt_data_ordered, EXT4_MOUNT_ORDERED_DATA, MOPT_DATAJ},
 	{Opt_data_writeback, EXT4_MOUNT_WRITEBACK_DATA, MOPT_DATAJ},
@@ -1450,8 +1454,6 @@ static int handle_mount_opt(struct super_block *sb, char *opt, int token,
 	const struct mount_opts *m;
 	int arg = 0;
 
-	if (args->from && match_int(args, &arg))
-		return -1;
 	switch (token) {
 	case Opt_noacl:
 	case Opt_nouser_xattr:
@@ -1463,36 +1465,19 @@ static int handle_mount_opt(struct super_block *sb, char *opt, int token,
 		ext4_msg(sb, KERN_WARNING,
 			 "Ignoring removed %s option", opt);
 		return 1;
-	case Opt_resuid:
-		sbi->s_resuid = arg;
-		return 1;
-	case Opt_resgid:
-		sbi->s_resgid = arg;
-		return 1;
 	case Opt_abort:
 		sbi->s_mount_flags |= EXT4_MF_FS_ABORTED;
 		return 1;
 	case Opt_i_version:
 		sb->s_flags |= MS_I_VERSION;
 		return 1;
-	case Opt_journal_dev:
-		if (is_remount) {
-			ext4_msg(sb, KERN_ERR,
-				 "Cannot specify journal on remount");
-			return -1;
-		}
-		*journal_devnum = arg;
-		return 1;
-	case Opt_journal_ioprio:
-		if (arg < 0 || arg > 7)
-			return -1;
-		*journal_ioprio = IOPRIO_PRIO_VALUE(IOPRIO_CLASS_BE, arg);
-		return 1;
 	}
 
 	for (m = ext4_mount_opts; m->token != Opt_err; m++) {
 		if (token != m->token)
 			continue;
+		if (args->from && match_int(args, &arg))
+			return -1;
 		if (args->from && (m->flags & MOPT_GTE0) && (arg < 0))
 			return -1;
 		if (m->flags & MOPT_EXPLICIT)
@@ -1534,6 +1519,26 @@ static int handle_mount_opt(struct super_block *sb, char *opt, int token,
 			sbi->s_li_wait_mult = arg;
 		} else if (token == Opt_stripe) {
 			sbi->s_stripe = arg;
+		} else if (token == Opt_resuid) {
+			sbi->s_resuid = arg;
+		} else if (token == Opt_resgid) {
+			sbi->s_resgid = arg;
+		} else if (token == Opt_journal_dev) {
+			if (is_remount) {
+				ext4_msg(sb, KERN_ERR,
+					 "Cannot specify journal on remount");
+				return -1;
+			}
+			*journal_devnum = arg;
+		} else if (token == Opt_journal_ioprio) {
+			if (arg > 7) {
+				ext4_msg(sb, KERN_ERR,
+					 "Invalid journal IO priority"
+					 " (must be 0-7)");
+				return -1;
+			}
+			*journal_ioprio =
+				IOPRIO_PRIO_VALUE(IOPRIO_CLASS_BE, arg);
 		} else if (m->flags & MOPT_DATAJ) {
 			if (is_remount) {
 				if (!sbi->s_journal)
-- 
1.7.1

[PATCH 2/4] ext4: Make mount option parsing loop more logical

[Jan Kara]

The loop looking for correct mount option entry is more logical if it is
written rewritten as an empty loop looking for correct option entry and then
code handling the option. It also saves one level of indentation for a lot of
code so we can join a couple of split lines.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext4/super.c |  224 +++++++++++++++++++++++++++----------------------------
 1 files changed, 111 insertions(+), 113 deletions(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 30e26f1..167aaac 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -1473,130 +1473,128 @@ static int handle_mount_opt(struct super_block *sb, char *opt, int token,
 		return 1;
 	}
 
-	for (m = ext4_mount_opts; m->token != Opt_err; m++) {
-		if (token != m->token)
-			continue;
-		if (args->from && match_int(args, &arg))
+	for (m = ext4_mount_opts; m->token != Opt_err && token != m->token;
+	     m++);
+	if (m->token == Opt_err) {
+		ext4_msg(sb, KERN_ERR, "Unrecognized mount option \"%s\" "
+			 "or missing value", opt);
+		return -1;
+	}
+	
+	if (args->from && match_int(args, &arg))
+		return -1;
+	if (args->from && (m->flags & MOPT_GTE0) && (arg < 0))
+		return -1;
+	if (m->flags & MOPT_EXPLICIT)
+		set_opt2(sb, EXPLICIT_DELALLOC);
+	if (m->flags & MOPT_CLEAR_ERR)
+		clear_opt(sb, ERRORS_MASK);
+	if (token == Opt_noquota && sb_any_quota_loaded(sb)) {
+		ext4_msg(sb, KERN_ERR, "Cannot change quota "
+			 "options when quota turned on");
+		return -1;
+	}
+
+	if (m->flags & MOPT_NOSUPPORT) {
+		ext4_msg(sb, KERN_ERR, "%s option not supported", opt);
+	} else if (token == Opt_commit) {
+		if (arg == 0)
+			arg = JBD2_DEFAULT_MAX_COMMIT_AGE;
+		sbi->s_commit_interval = HZ * arg;
+	} else if (token == Opt_max_batch_time) {
+		if (arg == 0)
+			arg = EXT4_DEF_MAX_BATCH_TIME;
+		sbi->s_max_batch_time = arg;
+	} else if (token == Opt_min_batch_time) {
+		sbi->s_min_batch_time = arg;
+	} else if (token == Opt_inode_readahead_blks) {
+		if (arg > (1 << 30))
 			return -1;
-		if (args->from && (m->flags & MOPT_GTE0) && (arg < 0))
+		if (arg && !is_power_of_2(arg)) {
+			ext4_msg(sb, KERN_ERR,
+				 "EXT4-fs: inode_readahead_blks"
+				 " must be a power of 2");
 			return -1;
-		if (m->flags & MOPT_EXPLICIT)
-			set_opt2(sb, EXPLICIT_DELALLOC);
-		if (m->flags & MOPT_CLEAR_ERR)
-			clear_opt(sb, ERRORS_MASK);
-		if (token == Opt_noquota && sb_any_quota_loaded(sb)) {
-			ext4_msg(sb, KERN_ERR, "Cannot change quota "
-				 "options when quota turned on");
+		}
+		sbi->s_inode_readahead_blks = arg;
+	} else if (token == Opt_init_itable) {
+		set_opt(sb, INIT_INODE_TABLE);
+		if (!args->from)
+			arg = EXT4_DEF_LI_WAIT_MULT;
+		sbi->s_li_wait_mult = arg;
+	} else if (token == Opt_stripe) {
+		sbi->s_stripe = arg;
+	} else if (token == Opt_resuid) {
+		sbi->s_resuid = arg;
+	} else if (token == Opt_resgid) {
+		sbi->s_resgid = arg;
+	} else if (token == Opt_journal_dev) {
+		if (is_remount) {
+			ext4_msg(sb, KERN_ERR,
+				 "Cannot specify journal on remount");
 			return -1;
 		}
-
-		if (m->flags & MOPT_NOSUPPORT) {
-			ext4_msg(sb, KERN_ERR, "%s option not supported", opt);
-		} else if (token == Opt_commit) {
-			if (arg == 0)
-				arg = JBD2_DEFAULT_MAX_COMMIT_AGE;
-			sbi->s_commit_interval = HZ * arg;
-		} else if (token == Opt_max_batch_time) {
-			if (arg == 0)
-				arg = EXT4_DEF_MAX_BATCH_TIME;
-			sbi->s_max_batch_time = arg;
-		} else if (token == Opt_min_batch_time) {
-			sbi->s_min_batch_time = arg;
-		} else if (token == Opt_inode_readahead_blks) {
-			if (arg > (1 << 30))
-				return -1;
-			if (arg && !is_power_of_2(arg)) {
-				ext4_msg(sb, KERN_ERR,
-					 "EXT4-fs: inode_readahead_blks"
-					 " must be a power of 2");
-				return -1;
-			}
-			sbi->s_inode_readahead_blks = arg;
-		} else if (token == Opt_init_itable) {
-			set_opt(sb, INIT_INODE_TABLE);
-			if (!args->from)
-				arg = EXT4_DEF_LI_WAIT_MULT;
-			sbi->s_li_wait_mult = arg;
-		} else if (token == Opt_stripe) {
-			sbi->s_stripe = arg;
-		} else if (token == Opt_resuid) {
-			sbi->s_resuid = arg;
-		} else if (token == Opt_resgid) {
-			sbi->s_resgid = arg;
-		} else if (token == Opt_journal_dev) {
-			if (is_remount) {
-				ext4_msg(sb, KERN_ERR,
-					 "Cannot specify journal on remount");
-				return -1;
-			}
-			*journal_devnum = arg;
-		} else if (token == Opt_journal_ioprio) {
-			if (arg > 7) {
+		*journal_devnum = arg;
+	} else if (token == Opt_journal_ioprio) {
+		if (arg > 7) {
+			ext4_msg(sb, KERN_ERR,
+				 "Invalid journal IO priority (must be 0-7)");
+			return -1;
+		}
+		*journal_ioprio = IOPRIO_PRIO_VALUE(IOPRIO_CLASS_BE, arg);
+	} else if (m->flags & MOPT_DATAJ) {
+		if (is_remount) {
+			if (!sbi->s_journal)
+				ext4_msg(sb, KERN_WARNING, "Remounting file system with no journal so ignoring journalled data option");
+			else if (test_opt(sb, DATA_FLAGS) != m->mount_opt) {
 				ext4_msg(sb, KERN_ERR,
-					 "Invalid journal IO priority"
-					 " (must be 0-7)");
+				 "Cannot change data mode on remount");
 				return -1;
 			}
-			*journal_ioprio =
-				IOPRIO_PRIO_VALUE(IOPRIO_CLASS_BE, arg);
-		} else if (m->flags & MOPT_DATAJ) {
-			if (is_remount) {
-				if (!sbi->s_journal)
-					ext4_msg(sb, KERN_WARNING, "Remounting file system with no journal so ignoring journalled data option");
-				else if (test_opt(sb, DATA_FLAGS) !=
-					 m->mount_opt) {
-					ext4_msg(sb, KERN_ERR,
-					 "Cannot change data mode on remount");
-					return -1;
-				}
-			} else {
-				clear_opt(sb, DATA_FLAGS);
-				sbi->s_mount_opt |= m->mount_opt;
-			}
+		} else {
+			clear_opt(sb, DATA_FLAGS);
+			sbi->s_mount_opt |= m->mount_opt;
+		}
 #ifdef CONFIG_QUOTA
-		} else if (token == Opt_usrjquota) {
-			if (!set_qf_name(sb, USRQUOTA, &args[0]))
-				return -1;
-		} else if (token == Opt_grpjquota) {
-			if (!set_qf_name(sb, GRPQUOTA, &args[0]))
-				return -1;
-		} else if (token == Opt_offusrjquota) {
-			if (!clear_qf_name(sb, USRQUOTA))
-				return -1;
-		} else if (token == Opt_offgrpjquota) {
-			if (!clear_qf_name(sb, GRPQUOTA))
-				return -1;
-		} else if (m->flags & MOPT_QFMT) {
-			if (sb_any_quota_loaded(sb) &&
-			    sbi->s_jquota_fmt != m->mount_opt) {
-				ext4_msg(sb, KERN_ERR, "Cannot "
-					 "change journaled quota options "
-					 "when quota turned on");
-				return -1;
-			}
-			sbi->s_jquota_fmt = m->mount_opt;
+	} else if (token == Opt_usrjquota) {
+		if (!set_qf_name(sb, USRQUOTA, &args[0]))
+			return -1;
+	} else if (token == Opt_grpjquota) {
+		if (!set_qf_name(sb, GRPQUOTA, &args[0]))
+			return -1;
+	} else if (token == Opt_offusrjquota) {
+		if (!clear_qf_name(sb, USRQUOTA))
+			return -1;
+	} else if (token == Opt_offgrpjquota) {
+		if (!clear_qf_name(sb, GRPQUOTA))
+			return -1;
+	} else if (m->flags & MOPT_QFMT) {
+		if (sb_any_quota_loaded(sb) &&
+		    sbi->s_jquota_fmt != m->mount_opt) {
+			ext4_msg(sb, KERN_ERR, "Cannot "
+				 "change journaled quota options "
+				 "when quota turned on");
+			return -1;
+		}
+		sbi->s_jquota_fmt = m->mount_opt;
 #endif
-		} else {
-			if (!args->from)
-				arg = 1;
-			if (m->flags & MOPT_CLEAR)
-				arg = !arg;
-			else if (unlikely(!(m->flags & MOPT_SET))) {
-				ext4_msg(sb, KERN_WARNING,
-					 "buggy handling of option %s", opt);
-				WARN_ON(1);
-				return -1;
-			}
-			if (arg != 0)
-				sbi->s_mount_opt |= m->mount_opt;
-			else
-				sbi->s_mount_opt &= ~m->mount_opt;
+	} else {
+		if (!args->from)
+			arg = 1;
+		if (m->flags & MOPT_CLEAR)
+			arg = !arg;
+		else if (unlikely(!(m->flags & MOPT_SET))) {
+			ext4_msg(sb, KERN_WARNING,
+				 "buggy handling of option %s", opt);
+			WARN_ON(1);
+			return -1;
 		}
-		return 1;
+		if (arg != 0)
+			sbi->s_mount_opt |= m->mount_opt;
+		else
+			sbi->s_mount_opt &= ~m->mount_opt;
 	}
-	ext4_msg(sb, KERN_ERR, "Unrecognized mount option \"%s\" "
-		 "or missing value", opt);
-	return -1;
+	return 1;
 }
 
 static int parse_options(char *options, struct super_block *sb,
-- 
1.7.1

[PATCH 3/4] ext4: Fix handling of journalled quota options

[Jan Kara]

Commit 26092bf5 broke handling of journalled quota mount options by trying
to parse argument of every mount option as a number. Add a mount option flag
indicating whether an argument should be a number and use it where appropriate.
We also print appropriate error message when argument parsing fails (as
otherwise it's hard to find out why the mount failed).

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext4/super.c |   40 +++++++++++++++++++++++++---------------
 1 files changed, 25 insertions(+), 15 deletions(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 167aaac..f1acaa6 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -1357,7 +1357,8 @@ static int clear_qf_name(struct super_block *sb, int qtype)
 #define MOPT_NOSUPPORT	0x0004
 #define MOPT_EXPLICIT	0x0008
 #define MOPT_CLEAR_ERR	0x0010
-#define MOPT_GTE0	0x0020
+#define MOPT_NUM_ARG	0x0020	/* Option may have numeric argument */
+#define MOPT_GTE0	0x0040	/* Numeric argument must be >= 0 */
 #ifdef CONFIG_QUOTA
 #define MOPT_Q		0
 #define MOPT_QFMT	0x0040
@@ -1400,16 +1401,16 @@ static const struct mount_opts {
 	{Opt_noauto_da_alloc, EXT4_MOUNT_NO_AUTO_DA_ALLOC, MOPT_SET},
 	{Opt_auto_da_alloc, EXT4_MOUNT_NO_AUTO_DA_ALLOC, MOPT_CLEAR},
 	{Opt_noinit_itable, EXT4_MOUNT_INIT_INODE_TABLE, MOPT_CLEAR},
-	{Opt_commit, 0, MOPT_GTE0},
-	{Opt_max_batch_time, 0, MOPT_GTE0},
-	{Opt_min_batch_time, 0, MOPT_GTE0},
-	{Opt_inode_readahead_blks, 0, MOPT_GTE0},
-	{Opt_init_itable, 0, MOPT_GTE0},
-	{Opt_stripe, 0, MOPT_GTE0},
-	{Opt_resuid, 0, MOPT_GTE0},
-	{Opt_resgid, 0, MOPT_GTE0},
-	{Opt_journal_dev, 0, MOPT_GTE0},
-	{Opt_journal_ioprio, 0, MOPT_GTE0},
+	{Opt_commit, 0, MOPT_NUM_ARG | MOPT_GTE0},
+	{Opt_max_batch_time, 0, MOPT_NUM_ARG | MOPT_GTE0},
+	{Opt_min_batch_time, 0, MOPT_NUM_ARG | MOPT_GTE0},
+	{Opt_inode_readahead_blks, 0, MOPT_NUM_ARG | MOPT_GTE0},
+	{Opt_init_itable, 0, MOPT_NUM_ARG | MOPT_GTE0},
+	{Opt_stripe, 0, MOPT_NUM_ARG | MOPT_GTE0},
+	{Opt_resuid, 0, MOPT_NUM_ARG | MOPT_GTE0},
+	{Opt_resgid, 0, MOPT_NUM_ARG | MOPT_GTE0},
+	{Opt_journal_dev, 0, MOPT_NUM_ARG | MOPT_GTE0},
+	{Opt_journal_ioprio, 0, MOPT_NUM_ARG | MOPT_GTE0},
 	{Opt_data_journal, EXT4_MOUNT_JOURNAL_DATA, MOPT_DATAJ},
 	{Opt_data_ordered, EXT4_MOUNT_ORDERED_DATA, MOPT_DATAJ},
 	{Opt_data_writeback, EXT4_MOUNT_WRITEBACK_DATA, MOPT_DATAJ},
@@ -1481,10 +1482,19 @@ static int handle_mount_opt(struct super_block *sb, char *opt, int token,
 		return -1;
 	}
 	
-	if (args->from && match_int(args, &arg))
-		return -1;
-	if (args->from && (m->flags & MOPT_GTE0) && (arg < 0))
-		return -1;
+	if (args->from && (m->flags & MOPT_NUM_ARG)) {
+		if (match_int(args, &arg)) {
+			ext4_msg(sb, KERN_ERR, "Cannot parse argument for mount"
+				 "option \"%s\"", opt);
+			return -1;
+		}
+		if ((m->flags & MOPT_GTE0) && (arg < 0)) {
+			ext4_msg(sb, KERN_ERR, "Argument for mount option "
+				 "\"%s\" must be >= 0", opt);
+			return -1;
+		}
+	}
+	
 	if (m->flags & MOPT_EXPLICIT)
 		set_opt2(sb, EXPLICIT_DELALLOC);
 	if (m->flags & MOPT_CLEAR_ERR)
-- 
1.7.1

[PATCH 4/4] ext4: Print error when argument of inode_readahead_blk is invalid

[Jan Kara]

If argument of inode_readahead_blk is too big, we just bail out without
printing any error. Fix this since it could confuse users.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext4/super.c |    8 +++-----
 1 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index f1acaa6..ced6671 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -1518,12 +1518,10 @@ static int handle_mount_opt(struct super_block *sb, char *opt, int token,
 	} else if (token == Opt_min_batch_time) {
 		sbi->s_min_batch_time = arg;
 	} else if (token == Opt_inode_readahead_blks) {
-		if (arg > (1 << 30))
-			return -1;
-		if (arg && !is_power_of_2(arg)) {
+		if (arg && (arg > (1 << 30) || !is_power_of_2(arg))) {
 			ext4_msg(sb, KERN_ERR,
-				 "EXT4-fs: inode_readahead_blks"
-				 " must be a power of 2");
+				 "EXT4-fs: inode_readahead_blks must be "
+				 "0 or a power of 2 smaller than 2^31");
 			return -1;
 		}
 		sbi->s_inode_readahead_blks = arg;
-- 
1.7.1

Re: [PATCH 0/4] ext4: Fix regression in mount option parsing and a small cleanup

[Ted Ts'o]

On Mon, Apr 16, 2012 at 02:25:30PM +0200, Jan Kara wrote:
> 
> 
>   this patch series aims at fixing a regression in parsing of
> journalled quota mount options introduced by Ted's mount option
> parsing rewrite. Patches 1 and 2 cleanup the code a bit and patch 3
> then fixes the regression. Patch 4 is a tiny cleanup.

Hi Jan,

Apologies for breaking the journalled mount options, and thank you for
this patch series.  I agree it's an improvement on what we currently
have.  My main concern with though, is I'd like to fix this for 3.4,
and the four patches together is a bit larger than what I feel
comfortable pushing to Linus at this point.  What do you think of this
patch now, and I'll we'll add your changes for the next merge window?

      	       	    	      	   	   - Ted

>From 816c710aa92cdf442e6fc722b74bb99bf8575a30 Mon Sep 17 00:00:00 2001
From: Theodore Ts'o <tytso@mit.edu>
Date: Mon, 16 Apr 2012 12:54:07 -0400
Subject: [PATCH] ext4: fix handling of journalled quota options

Commit 26092bf5 broke handling of journalled quota mount options by
trying to parse argument of every mount option as a number.  Fix this
by dealing with the quota options before we call match_int().

Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
Cc: Jan Kara <jack@suse.cz>
---
 fs/ext4/super.c |   32 +++++++++++++++-----------------
 1 file changed, 15 insertions(+), 17 deletions(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index ef7db50..6da1935 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -1305,20 +1305,20 @@ static int set_qf_name(struct super_block *sb, int qtype, substring_t *args)
 		ext4_msg(sb, KERN_ERR,
 			"Cannot change journaled "
 			"quota options when quota turned on");
-		return 0;
+		return -1;
 	}
 	qname = match_strdup(args);
 	if (!qname) {
 		ext4_msg(sb, KERN_ERR,
 			"Not enough memory for storing quotafile name");
-		return 0;
+		return -1;
 	}
 	if (sbi->s_qf_names[qtype] &&
 		strcmp(sbi->s_qf_names[qtype], qname)) {
 		ext4_msg(sb, KERN_ERR,
 			"%s quota file already specified", QTYPE2NAME(qtype));
 		kfree(qname);
-		return 0;
+		return -1;
 	}
 	sbi->s_qf_names[qtype] = qname;
 	if (strchr(sbi->s_qf_names[qtype], '/')) {
@@ -1326,7 +1326,7 @@ static int set_qf_name(struct super_block *sb, int qtype, substring_t *args)
 			"quotafile must be on filesystem root");
 		kfree(sbi->s_qf_names[qtype]);
 		sbi->s_qf_names[qtype] = NULL;
-		return 0;
+		return -1;
 	}
 	set_opt(sb, QUOTA);
 	return 1;
@@ -1341,7 +1341,7 @@ static int clear_qf_name(struct super_block *sb, int qtype)
 		sbi->s_qf_names[qtype]) {
 		ext4_msg(sb, KERN_ERR, "Cannot change journaled quota options"
 			" when quota turned on");
-		return 0;
+		return -1;
 	}
 	/*
 	 * The space will be released later when all options are confirmed
@@ -1450,6 +1450,16 @@ static int handle_mount_opt(struct super_block *sb, char *opt, int token,
 	const struct mount_opts *m;
 	int arg = 0;
 
+#ifdef CONFIG_QUOTA
+	if (token == Opt_usrjquota)
+		return set_qf_name(sb, USRQUOTA, &args[0]);
+	else if (token == Opt_grpjquota)
+		return set_qf_name(sb, GRPQUOTA, &args[0]);
+	else if (token == Opt_offusrjquota)
+		return clear_qf_name(sb, USRQUOTA);
+	else if (token == Opt_offgrpjquota)
+		return clear_qf_name(sb, GRPQUOTA);
+#endif
 	if (args->from && match_int(args, &arg))
 		return -1;
 	switch (token) {
@@ -1549,18 +1559,6 @@ static int handle_mount_opt(struct super_block *sb, char *opt, int token,
 				sbi->s_mount_opt |= m->mount_opt;
 			}
 #ifdef CONFIG_QUOTA
-		} else if (token == Opt_usrjquota) {
-			if (!set_qf_name(sb, USRQUOTA, &args[0]))
-				return -1;
-		} else if (token == Opt_grpjquota) {
-			if (!set_qf_name(sb, GRPQUOTA, &args[0]))
-				return -1;
-		} else if (token == Opt_offusrjquota) {
-			if (!clear_qf_name(sb, USRQUOTA))
-				return -1;
-		} else if (token == Opt_offgrpjquota) {
-			if (!clear_qf_name(sb, GRPQUOTA))
-				return -1;
 		} else if (m->flags & MOPT_QFMT) {
 			if (sb_any_quota_loaded(sb) &&
 			    sbi->s_jquota_fmt != m->mount_opt) {
-- 
1.7.10.rc3

Re: [PATCH 0/4] ext4: Fix regression in mount option parsing and a small cleanup

[Jan Kara]

On Mon 16-04-12 13:06:07, Ted Tso wrote:
> On Mon, Apr 16, 2012 at 02:25:30PM +0200, Jan Kara wrote:
> > 
> > 
> >   this patch series aims at fixing a regression in parsing of
> > journalled quota mount options introduced by Ted's mount option
> > parsing rewrite. Patches 1 and 2 cleanup the code a bit and patch 3
> > then fixes the regression. Patch 4 is a tiny cleanup.
> 
> Hi Jan,
> 
> Apologies for breaking the journalled mount options, and thank you for
> this patch series.  I agree it's an improvement on what we currently
> have.  My main concern with though, is I'd like to fix this for 3.4,
> and the four patches together is a bit larger than what I feel
> comfortable pushing to Linus at this point.  What do you think of this
> patch now, and I'll we'll add your changes for the next merge window?
  Yeah, I figured it might be a bit too big. I didn't think of the simple
trick which makes the quick fix small :) The patch looks good (and I've
also tested it) so you can add:
  Reviewed-by: Jan Kara <jack@suse.cz>

								Honza


> From 816c710aa92cdf442e6fc722b74bb99bf8575a30 Mon Sep 17 00:00:00 2001
> From: Theodore Ts'o <tytso@mit.edu>
> Date: Mon, 16 Apr 2012 12:54:07 -0400
> Subject: [PATCH] ext4: fix handling of journalled quota options
> 
> Commit 26092bf5 broke handling of journalled quota mount options by
> trying to parse argument of every mount option as a number.  Fix this
> by dealing with the quota options before we call match_int().
> 
> Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
> Cc: Jan Kara <jack@suse.cz>
> ---
>  fs/ext4/super.c |   32 +++++++++++++++-----------------
>  1 file changed, 15 insertions(+), 17 deletions(-)
> 
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index ef7db50..6da1935 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -1305,20 +1305,20 @@ static int set_qf_name(struct super_block *sb, int qtype, substring_t *args)
>  		ext4_msg(sb, KERN_ERR,
>  			"Cannot change journaled "
>  			"quota options when quota turned on");
> -		return 0;
> +		return -1;
>  	}
>  	qname = match_strdup(args);
>  	if (!qname) {
>  		ext4_msg(sb, KERN_ERR,
>  			"Not enough memory for storing quotafile name");
> -		return 0;
> +		return -1;
>  	}
>  	if (sbi->s_qf_names[qtype] &&
>  		strcmp(sbi->s_qf_names[qtype], qname)) {
>  		ext4_msg(sb, KERN_ERR,
>  			"%s quota file already specified", QTYPE2NAME(qtype));
>  		kfree(qname);
> -		return 0;
> +		return -1;
>  	}
>  	sbi->s_qf_names[qtype] = qname;
>  	if (strchr(sbi->s_qf_names[qtype], '/')) {
> @@ -1326,7 +1326,7 @@ static int set_qf_name(struct super_block *sb, int qtype, substring_t *args)
>  			"quotafile must be on filesystem root");
>  		kfree(sbi->s_qf_names[qtype]);
>  		sbi->s_qf_names[qtype] = NULL;
> -		return 0;
> +		return -1;
>  	}
>  	set_opt(sb, QUOTA);
>  	return 1;
> @@ -1341,7 +1341,7 @@ static int clear_qf_name(struct super_block *sb, int qtype)
>  		sbi->s_qf_names[qtype]) {
>  		ext4_msg(sb, KERN_ERR, "Cannot change journaled quota options"
>  			" when quota turned on");
> -		return 0;
> +		return -1;
>  	}
>  	/*
>  	 * The space will be released later when all options are confirmed
> @@ -1450,6 +1450,16 @@ static int handle_mount_opt(struct super_block *sb, char *opt, int token,
>  	const struct mount_opts *m;
>  	int arg = 0;
>  
> +#ifdef CONFIG_QUOTA
> +	if (token == Opt_usrjquota)
> +		return set_qf_name(sb, USRQUOTA, &args[0]);
> +	else if (token == Opt_grpjquota)
> +		return set_qf_name(sb, GRPQUOTA, &args[0]);
> +	else if (token == Opt_offusrjquota)
> +		return clear_qf_name(sb, USRQUOTA);
> +	else if (token == Opt_offgrpjquota)
> +		return clear_qf_name(sb, GRPQUOTA);
> +#endif
>  	if (args->from && match_int(args, &arg))
>  		return -1;
>  	switch (token) {
> @@ -1549,18 +1559,6 @@ static int handle_mount_opt(struct super_block *sb, char *opt, int token,
>  				sbi->s_mount_opt |= m->mount_opt;
>  			}
>  #ifdef CONFIG_QUOTA
> -		} else if (token == Opt_usrjquota) {
> -			if (!set_qf_name(sb, USRQUOTA, &args[0]))
> -				return -1;
> -		} else if (token == Opt_grpjquota) {
> -			if (!set_qf_name(sb, GRPQUOTA, &args[0]))
> -				return -1;
> -		} else if (token == Opt_offusrjquota) {
> -			if (!clear_qf_name(sb, USRQUOTA))
> -				return -1;
> -		} else if (token == Opt_offgrpjquota) {
> -			if (!clear_qf_name(sb, GRPQUOTA))
> -				return -1;
>  		} else if (m->flags & MOPT_QFMT) {
>  			if (sb_any_quota_loaded(sb) &&
>  			    sbi->s_jquota_fmt != m->mount_opt) {
> -- 
> 1.7.10.rc3
> 
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

[PATCH 0/4] Clean up ext4's mount option parsing code

[Theodore Ts'o]

The first three patches are from a patch series which Jan Kara submitted
back in April 2012, forward ported to modern kernels.  The last patch
addresses the feature which Carlos suggested, but I've implemented it as
a flag in the mount options parsing, instead of using the mount option
flags.  This allows us to disallow features which don't involve setting
a mount option flags.

						- Ted

Jan Kara (3):
  ext4: move several mount options to standard handling loop
  ext4: make mount option parsing loop more logical
  ext4: print error when argument of inode_readahead_blk is invalid

Theodore Ts'o (1):
  ext4: check incompatible mount options while mounting ext2/3

 fs/ext4/super.c | 261 +++++++++++++++++++++++++++++++-------------------------
 1 file changed, 143 insertions(+), 118 deletions(-)

-- 
1.7.12.rc0.22.gcdd159b

[PATCH 1/4] ext4: move several mount options to standard handling loop

[Theodore Ts'o]

From: Jan Kara <jack@suse.cz>

Several mount option (resuid, resgid, journal_dev, journal_ioprio) are
currently handled before we enter standard option handling loop. I don't
see a reason for this so move them to normal handling loop to make things
more regular.

Signed-off-by: Jan Kara <jack@suse.cz>
Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
---
 fs/ext4/super.c | 69 +++++++++++++++++++++++++++++++--------------------------
 1 file changed, 38 insertions(+), 31 deletions(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index dc20e4d..78c9336 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -1447,6 +1447,10 @@ static const struct mount_opts {
 	{Opt_inode_readahead_blks, 0, MOPT_GTE0},
 	{Opt_init_itable, 0, MOPT_GTE0},
 	{Opt_stripe, 0, MOPT_GTE0},
+	{Opt_resuid, 0, MOPT_GTE0},
+	{Opt_resgid, 0, MOPT_GTE0},
+	{Opt_journal_dev, 0, MOPT_GTE0},
+	{Opt_journal_ioprio, 0, MOPT_GTE0},
 	{Opt_data_journal, EXT4_MOUNT_JOURNAL_DATA, MOPT_DATAJ},
 	{Opt_data_ordered, EXT4_MOUNT_ORDERED_DATA, MOPT_DATAJ},
 	{Opt_data_writeback, EXT4_MOUNT_WRITEBACK_DATA, MOPT_DATAJ},
@@ -1499,8 +1503,6 @@ static int handle_mount_opt(struct super_block *sb, char *opt, int token,
 	else if (token == Opt_offgrpjquota)
 		return clear_qf_name(sb, GRPQUOTA);
 #endif
-	if (args->from && match_int(args, &arg))
-		return -1;
 	switch (token) {
 	case Opt_noacl:
 	case Opt_nouser_xattr:
@@ -1512,46 +1514,19 @@ static int handle_mount_opt(struct super_block *sb, char *opt, int token,
 		ext4_msg(sb, KERN_WARNING,
 			 "Ignoring removed %s option", opt);
 		return 1;
-	case Opt_resuid:
-		uid = make_kuid(current_user_ns(), arg);
-		if (!uid_valid(uid)) {
-			ext4_msg(sb, KERN_ERR, "Invalid uid value %d", arg);
-			return -1;
-		}
-		sbi->s_resuid = uid;
-		return 1;
-	case Opt_resgid:
-		gid = make_kgid(current_user_ns(), arg);
-		if (!gid_valid(gid)) {
-			ext4_msg(sb, KERN_ERR, "Invalid gid value %d", arg);
-			return -1;
-		}
-		sbi->s_resgid = gid;
-		return 1;
 	case Opt_abort:
 		sbi->s_mount_flags |= EXT4_MF_FS_ABORTED;
 		return 1;
 	case Opt_i_version:
 		sb->s_flags |= MS_I_VERSION;
 		return 1;
-	case Opt_journal_dev:
-		if (is_remount) {
-			ext4_msg(sb, KERN_ERR,
-				 "Cannot specify journal on remount");
-			return -1;
-		}
-		*journal_devnum = arg;
-		return 1;
-	case Opt_journal_ioprio:
-		if (arg < 0 || arg > 7)
-			return -1;
-		*journal_ioprio = IOPRIO_PRIO_VALUE(IOPRIO_CLASS_BE, arg);
-		return 1;
 	}
 
 	for (m = ext4_mount_opts; m->token != Opt_err; m++) {
 		if (token != m->token)
 			continue;
+		if (args->from && match_int(args, &arg))
+			return -1;
 		if (args->from && (m->flags & MOPT_GTE0) && (arg < 0))
 			return -1;
 		if (m->flags & MOPT_EXPLICIT)
@@ -1595,6 +1570,38 @@ static int handle_mount_opt(struct super_block *sb, char *opt, int token,
 			sbi->s_max_dir_size_kb = arg;
 		} else if (token == Opt_stripe) {
 			sbi->s_stripe = arg;
+		} else if (token == Opt_resuid) {
+			uid = make_kuid(current_user_ns(), arg);
+			if (!uid_valid(uid)) {
+				ext4_msg(sb, KERN_ERR,
+					 "Invalid uid value %d", arg);
+				return -1;
+			}
+			sbi->s_resuid = uid;
+		} else if (token == Opt_resgid) {
+			gid = make_kgid(current_user_ns(), arg);
+			if (!gid_valid(gid)) {
+				ext4_msg(sb, KERN_ERR,
+					 "Invalid gid value %d", arg);
+				return -1;
+			}
+			sbi->s_resgid = gid;
+		} else if (token == Opt_journal_dev) {
+			if (is_remount) {
+				ext4_msg(sb, KERN_ERR,
+					 "Cannot specify journal on remount");
+				return -1;
+			}
+			*journal_devnum = arg;
+		} else if (token == Opt_journal_ioprio) {
+			if (arg > 7) {
+				ext4_msg(sb, KERN_ERR,
+					 "Invalid journal IO priority"
+					 " (must be 0-7)");
+				return -1;
+			}
+			*journal_ioprio =
+				IOPRIO_PRIO_VALUE(IOPRIO_CLASS_BE, arg);
 		} else if (m->flags & MOPT_DATAJ) {
 			if (is_remount) {
 				if (!sbi->s_journal)
-- 
1.7.12.rc0.22.gcdd159b