handle optional-arg mount options better

PATCH Superseded Eric Sandeen 2 participants 6 messages Feb 3, 2010 – Feb 16, 2010 ext4 file system

Share & tools

Eric Sandeen

author

2010-02-03 21:13 UTC #0 lore raw

PATCH 1/1 handle optional-arg mount options better

We have 2 mount options, "barrier" and "auto_da_alloc" which may or may not take a 1/0 argument. This is confusing the parser, it seems, because if we pass it without an arg, it still tries to match_int for the arg, which is uninitialized, and match_number uses those uninit from/to values to do a kmalloc, resulting in potentially noisy failures.

I think just defining _arg variants of the tokens and handling them separately is the simplest fix.

Reported-by Michael S. Tsirkin <[email protected]>

Signed-off-by Eric Sandeen <[email protected]>

---

(I'll send one for ext3 as well if this looks good on review)

fs/ext4/super.c +20 ~~-13~~

index da184f4..f7d4f06 100644
@@ -1096,7 +1096,8 @@ enum {
 	Opt_resgid, Opt_resuid, Opt_sb, Opt_err_cont, Opt_err_panic, Opt_err_ro,
 	Opt_nouid32, Opt_debug, Opt_oldalloc, Opt_orlov,
 	Opt_user_xattr, Opt_nouser_xattr, Opt_acl, Opt_noacl,
-	Opt_auto_da_alloc, Opt_noauto_da_alloc, Opt_noload, Opt_nobh, Opt_bh,
+	Opt_auto_da_alloc_arg, Opt_auto_da_alloc, Opt_noauto_da_alloc,
+	Opt_noload, Opt_nobh, Opt_bh,
 	Opt_commit, Opt_min_batch_time, Opt_max_batch_time,
 	Opt_journal_update, Opt_journal_dev,
 	Opt_journal_checksum, Opt_journal_async_commit,

@@ -1104,8 +1105,8 @@ enum {
 	Opt_data_err_abort, Opt_data_err_ignore,
 	Opt_usrjquota, Opt_grpjquota, Opt_offusrjquota, Opt_offgrpjquota,
 	Opt_jqfmt_vfsold, Opt_jqfmt_vfsv0, Opt_jqfmt_vfsv1, Opt_quota,
-	Opt_noquota, Opt_ignore, Opt_barrier, Opt_nobarrier, Opt_err,
-	Opt_resize, Opt_usrquota, Opt_grpquota, Opt_i_version,
+	Opt_noquota, Opt_ignore, Opt_barrier_arg, Opt_barrier, Opt_nobarrier,
+	Opt_err, Opt_resize, Opt_usrquota, Opt_grpquota, Opt_i_version,
 	Opt_stripe, Opt_delalloc, Opt_nodelalloc,
 	Opt_block_validity, Opt_noblock_validity,
 	Opt_inode_readahead_blks, Opt_journal_ioprio,

@@ -1161,7 +1162,7 @@ static const match_table_t tokens = {
 	{Opt_noquota, "noquota"},
 	{Opt_quota, "quota"},
 	{Opt_usrquota, "usrquota"},
-	{Opt_barrier, "barrier=%u"},
+	{Opt_barrier_arg, "barrier=%u"},
 	{Opt_barrier, "barrier"},
 	{Opt_nobarrier, "nobarrier"},
 	{Opt_i_version, "i_version"},

@@ -1173,7 +1174,7 @@ static const match_table_t tokens = {
 	{Opt_noblock_validity, "noblock_validity"},
 	{Opt_inode_readahead_blks, "inode_readahead_blks=%u"},
 	{Opt_journal_ioprio, "journal_ioprio=%u"},
-	{Opt_auto_da_alloc, "auto_da_alloc=%u"},
+	{Opt_auto_da_alloc_arg, "auto_da_alloc=%u"},
 	{Opt_auto_da_alloc, "auto_da_alloc"},
 	{Opt_noauto_da_alloc, "noauto_da_alloc"},
 	{Opt_discard, "discard"},

@@ -1514,10 +1515,7 @@ set_qf_format:
 		case Opt_abort:
 			sbi->s_mount_flags |= EXT4_MF_FS_ABORTED;
 			break;
-		case Opt_nobarrier:
-			clear_opt(sbi->s_mount_opt, BARRIER);
-			break;
-		case Opt_barrier:
+		case Opt_barrier_arg:
 			if (match_int(&args[0], &option)) {
 				set_opt(sbi->s_mount_opt, BARRIER);
 				break;

@@ -1527,6 +1525,12 @@ set_qf_format:
 			else
 				clear_opt(sbi->s_mount_opt, BARRIER);
 			break;
+		case Opt_nobarrier:
+			clear_opt(sbi->s_mount_opt, BARRIER);
+			break;
+		case Opt_barrier:
+			set_opt(sbi->s_mount_opt, BARRIER);
+			break;
 		case Opt_ignore:
 			break;
 		case Opt_resize:

@@ -1590,10 +1594,7 @@ set_qf_format:
 			*journal_ioprio = IOPRIO_PRIO_VALUE(IOPRIO_CLASS_BE,
 							    option);
 			break;
-		case Opt_noauto_da_alloc:
-			set_opt(sbi->s_mount_opt,NO_AUTO_DA_ALLOC);
-			break;
-		case Opt_auto_da_alloc:
+		case Opt_auto_da_alloc_arg:
 			if (match_int(&args[0], &option)) {
 				clear_opt(sbi->s_mount_opt, NO_AUTO_DA_ALLOC);
 				break;

@@ -1603,6 +1604,12 @@ set_qf_format:
 			else
 				set_opt(sbi->s_mount_opt,NO_AUTO_DA_ALLOC);
 			break;
+		case Opt_noauto_da_alloc:
+			set_opt(sbi->s_mount_opt,NO_AUTO_DA_ALLOC);
+			break;
+		case Opt_auto_da_alloc:
+			clear_opt(sbi->s_mount_opt,NO_AUTO_DA_ALLOC);
+			break;
 		case Opt_discard:
 			set_opt(sbi->s_mount_opt, DISCARD);
 			break;

[email protected]

maintainer

2010-02-15 16:01 UTC #1 lore raw

Quoted 8 lines · Eric Sandeen

We have 2 mount options, "barrier" and "auto_da_alloc" which may or may not take a 1/0 argument. This is confusing the parser, it seems, because if we pass it without an arg, it still tries to match_int for the arg, which is uninitialized, and match_number uses those uninit from/to values to do a kmalloc, resulting in potentially noisy failures.

Quoted 6 lines

I think just defining _arg variants of the tokens and handling them separately is the simplest fix.
Reported-by Michael S. Tsirkin <[email protected]>
Signed-off-by Eric Sandeen <[email protected]>
---

This fix works just as well, and doesn't require _arg and _no_arg versions of the token tags.

As long as we initialize arg[0], things should work correctly. They work correctly even without !args[0].from check, but I added that just in case the implementation of the match_token library function changes in the future.

- Ted

commit 1e6276ea260bcd37284f4387c435239919fbc628

Author: Theodore Ts'o <[email protected]>
Date:   Mon Feb 15 11:00:12 2010 -0500

ext4: Fix optional-arg mount options

We have 2 mount options, "barrier" and "auto_da_alloc" which may or may not take a 1/0 argument. This causes the ext4 superblock mount code to subtract uninitialized pointers and pass the result to kmalloc, which results in very noisy failures.

Reported-by Michael S. Tsirkin <[email protected]>

Signed-off-by "Theodore Ts'o" <[email protected]>

fs/ext4/super.c +3 -2

index 735c20d..7e9a7ea 100644
@@ -1229,6 +1229,7 @@ static int parse_options(char *options, struct super_block *sb,
 		if (!*p)
 			continue;
 
+		args[0].to = args[0].from = 0;
 		token = match_token(p, tokens, args);
 		switch (token) {
 		case Opt_bsd_df:

@@ -1518,7 +1519,7 @@ set_qf_format:
 			clear_opt(sbi->s_mount_opt, BARRIER);
 			break;
 		case Opt_barrier:
-			if (match_int(&args[0], &option)) {
+			if (!args[0].from || match_int(&args[0], &option)) {
 				set_opt(sbi->s_mount_opt, BARRIER);
 				break;
 			}

@@ -1594,7 +1595,7 @@ set_qf_format:
 			set_opt(sbi->s_mount_opt,NO_AUTO_DA_ALLOC);
 			break;
 		case Opt_auto_da_alloc:
-			if (match_int(&args[0], &option)) {
+			if (!args[0].from || match_int(&args[0], &option)) {
 				clear_opt(sbi->s_mount_opt, NO_AUTO_DA_ALLOC);
 				break;
 			}

Eric Sandeen

author

2010-02-15 21:20 UTC #2 lore raw

PATCH v2 1/1 ext4: handle optional-arg mount options better

Quoted 18 lines · [email protected] wrote:

Quoted 8 lines · Eric Sandeen
We have 2 mount options, "barrier" and "auto_da_alloc" which may or may not take a 1/0 argument. This is confusing the parser, it seems, because if we pass it without an arg, it still tries to match_int for the arg, which is uninitialized, and match_number uses those uninit from/to values to do a kmalloc, resulting in potentially noisy failures.
Quoted 7 lines
I think just defining _arg variants of the tokens and handling them separately is the simplest fix.
Reported-by Michael S. Tsirkin <[email protected]>
Signed-off-by Eric Sandeen <[email protected]>
---

Quoted 8 lines

This fix works just as well, and doesn't require _arg and _no_arg versions of the token tags.
As long as we initialize arg[0], things should work correctly. They work correctly even without !args[0].from check, but I added that just in case the implementation of the match_token library function changes in the future.

Hm, I prefer the explicit to the clever. ;) But I guess it looks mostly right.

This new code:

	case Opt_barrier:
		if (!args[0].from || match_int(&args[0], &option)) {
			set_opt(sbi->s_mount_opt, BARRIER);
			break;
		}

takes a bit of thought for me to sort out what's going on.

I guess the first test is "if no argument was found" and the 2nd is "if argument was found but can't be scanned?"

Previously we were relying on match_int failing for no args, but now we test for that, explicitly so I think a match_int failure should return an error (well 0) like every other call in the function.

Maybe something like this? I think it's clearer and more correct.

ext4: handle optional-arg mount options better

We have 2 mount options, "barrier" and "auto_da_alloc" which may or may not take an argument. This is confusing the parser, it seems, because if we pass it in without an arg, it still tries to match_int for the arg, which is uninitialized, and match_number() uses those uninitialized from/to values to do a kmalloc, resulting in potentially noisy failures.

Per Ted's suggestion, initialize the args struct so that we know whether match_token() found an argument for the option, and skip match_int() if not.

Also, return error (0) from parse_options if we thought we found an argument, but match_int() Fails.

Reported-by Michael S. Tsirkin <[email protected]>

Signed-off-by Eric Sandeen <[email protected]>

---

fs/ext4/super.c +13 -2

index 735c20d..d107d29 100644
@@ -1229,6 +1229,11 @@ static int parse_options(char *options, struct super_block *sb,
 		if (!*p)
 			continue;
 
+		/*
+		 * Initialize args struct so we know whether arg was found;
+		 * Some options take optional arguments.
+		 */
+		args[0].to = args[0].from = 0;
 		token = match_token(p, tokens, args);
 		switch (token) {
 		case Opt_bsd_df:

@@ -1518,10 +1523,13 @@ set_qf_format:
 			clear_opt(sbi->s_mount_opt, BARRIER);
 			break;
 		case Opt_barrier:
-			if (match_int(&args[0], &option)) {
+			if (!args[0].from) {
+				/* No argument was found */
 				set_opt(sbi->s_mount_opt, BARRIER);
 				break;
 			}
+			if (match_int(&args[0], &option))
+				return 0;
 			if (option)
 				set_opt(sbi->s_mount_opt, BARRIER);
 			else

@@ -1594,10 +1602,13 @@ set_qf_format:
 			set_opt(sbi->s_mount_opt,NO_AUTO_DA_ALLOC);
 			break;
 		case Opt_auto_da_alloc:
-			if (match_int(&args[0], &option)) {
+			if (!args[0].from) {
+				/* No argument was found */
 				clear_opt(sbi->s_mount_opt, NO_AUTO_DA_ALLOC);
 				break;
 			}
+ 			if (match_int(&args[0], &option))
+				return 0;
 			if (option)
 				clear_opt(sbi->s_mount_opt, NO_AUTO_DA_ALLOC);
 			else

[email protected]

maintainer

2010-02-16 01:19 UTC #3 lore raw

Re: [PATCH V2] ext4: handle optional-arg mount options better

Ok, how about this, for even more explicit?

- Ted

commit 15121c18a22ae483279f76dc9e554334b800d0f7

Author: Eric Sandeen <[email protected]>
Date:   Mon Feb 15 20:17:55 2010 -0500

ext4: Fix optional-arg mount options

Per Ted's suggestion, initialize the args struct so that we know whether match_token() found an argument for the option, and skip match_int() if not.

Also, return error (0) from parse_options if we thought we found an argument, but match_int() Fails.

Reported-by Michael S. Tsirkin <[email protected]>

Signed-off-by Eric Sandeen <[email protected]>

Signed-off-by "Theodore Ts'o" <[email protected]>

fs/ext4/super.c +15 -8

index 735c20d..68a55df 100644
@@ -1229,6 +1229,11 @@ static int parse_options(char *options, struct super_block *sb,
 		if (!*p)
 			continue;
 
+		/*
+		 * Initialize args struct so we know whether arg was
+		 * found; some options take optional arguments.
+		 */
+		args[0].to = args[0].from = 0;
 		token = match_token(p, tokens, args);
 		switch (token) {
 		case Opt_bsd_df:

@@ -1518,10 +1523,11 @@ set_qf_format:
 			clear_opt(sbi->s_mount_opt, BARRIER);
 			break;
 		case Opt_barrier:
-			if (match_int(&args[0], &option)) {
-				set_opt(sbi->s_mount_opt, BARRIER);
-				break;
-			}
+			if (args[0].from) {
+				if (match_int(&args[0], &option))
+					return 0;
+			} else
+				option = 1;	/* No argument, default to 1 */
 			if (option)
 				set_opt(sbi->s_mount_opt, BARRIER);
 			else

@@ -1594,10 +1600,11 @@ set_qf_format:
 			set_opt(sbi->s_mount_opt,NO_AUTO_DA_ALLOC);
 			break;
 		case Opt_auto_da_alloc:
-			if (match_int(&args[0], &option)) {
-				clear_opt(sbi->s_mount_opt, NO_AUTO_DA_ALLOC);
-				break;
-			}
+			if (args[0].from) {
+				if (match_int(&args[0], &option))
+					return 0;
+			} else
+				option = 1;	/* No argument, default to 1 */
 			if (option)
 				clear_opt(sbi->s_mount_opt, NO_AUTO_DA_ALLOC);
 			else

Eric Sandeen

author

2010-02-16 03:23 UTC #4 lore raw

Re: [PATCH V2] ext4: handle optional-arg mount options better

Quoted 3 lines · [email protected] wrote:

Ok, how about this, for even more explicit?
- Ted

Sure, that looks even better. Who knew args parsing was so hard ;)

-Eric

Quoted 70 lines

commit 15121c18a22ae483279f76dc9e554334b800d0f7

Author: Eric Sandeen <[email protected]>
Date:   Mon Feb 15 20:17:55 2010 -0500

ext4: Fix optional-arg mount options

Per Ted's suggestion, initialize the args struct so that we know whether match_token() found an argument for the option, and skip match_int() if not.

Also, return error (0) from parse_options if we thought we found an argument, but match_int() Fails.

Reported-by Michael S. Tsirkin <[email protected]>

Signed-off-by Eric Sandeen <[email protected]>

Signed-off-by "Theodore Ts'o" <[email protected]>

fs/ext4/super.c +15 -8

index 735c20d..68a55df 100644
@@ -1229,6 +1229,11 @@ static int parse_options(char *options, struct super_block *sb,
 		if (!*p)
 			continue;
 
+		/*
+		 * Initialize args struct so we know whether arg was
+		 * found; some options take optional arguments.
+		 */
+		args[0].to = args[0].from = 0;
 		token = match_token(p, tokens, args);
 		switch (token) {
 		case Opt_bsd_df:

@@ -1518,10 +1523,11 @@ set_qf_format:
 			clear_opt(sbi->s_mount_opt, BARRIER);
 			break;
 		case Opt_barrier:
-			if (match_int(&args[0], &option)) {
-				set_opt(sbi->s_mount_opt, BARRIER);
-				break;
-			}
+			if (args[0].from) {
+				if (match_int(&args[0], &option))
+					return 0;
+			} else
+				option = 1;	/* No argument, default to 1 */
 			if (option)
 				set_opt(sbi->s_mount_opt, BARRIER);
 			else

@@ -1594,10 +1600,11 @@ set_qf_format:
 			set_opt(sbi->s_mount_opt,NO_AUTO_DA_ALLOC);
 			break;
 		case Opt_auto_da_alloc:
-			if (match_int(&args[0], &option)) {
-				clear_opt(sbi->s_mount_opt, NO_AUTO_DA_ALLOC);
-				break;
-			}
+			if (args[0].from) {
+				if (match_int(&args[0], &option))
+					return 0;
+			} else
+				option = 1;	/* No argument, default to 1 */
 			if (option)
 				clear_opt(sbi->s_mount_opt, NO_AUTO_DA_ALLOC);
 			else

Eric Sandeen

author

2010-02-16 17:28 UTC #5 lore raw

Re: [PATCH V2] ext4: handle optional-arg mount options better

Quoted 39 lines · Eric Sandeen

Quoted 3 lines · [email protected] wrote:
Ok, how about this, for even more explicit?
- Ted
Sure, that looks even better. Who knew args parsing was so hard ;)
-Eric
Quoted 30 lines
commit 15121c18a22ae483279f76dc9e554334b800d0f7
Author: Eric Sandeen <[email protected]>
Date:   Mon Feb 15 20:17:55 2010 -0500
ext4: Fix optional-arg mount options
We have 2 mount options, "barrier" and "auto_da_alloc" which may or may not take a 1/0 argument. This causes the ext4 superblock mount code to subtract uninitialized pointers and pass the result to kmalloc, which results in very noisy failures.
Per Ted's suggestion, initialize the args struct so that we know whether match_token() found an argument for the option, and skip match_int() if not.
Also, return error (0) from parse_options if we thought we found an argument, but match_int() Fails.
Reported-by Michael S. Tsirkin <[email protected]>
Signed-off-by Eric Sandeen <[email protected]>
Signed-off-by "Theodore Ts'o" <[email protected]>
fs/ext4/super.c +0 -0
index 735c20d..68a55df 100644
@@ -1229,6 +1229,11 @@ static int parse_options(char *options, struct super_block *sb,
 		if (!*p)
 			continue;
 

Quoted 5 lines

fs/ext4/super.c static int parse_options(char *options, struct super_block *sb, · line 1236
Quoted 5 lines
fs/ext4/super.c +5 -0
+		/*
+		 * Initialize args struct so we know whether arg was
+		 * found; some options take optional arguments.
+		 */
+		args[0].to = args[0].from = 0;

Unless it's already in, those should probably be "= NULL" since they're pointers.

-Eric