[PATCH] handle optional-arg mount options better

[PATCH] handle optional-arg mount options better

[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.

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

Reported-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

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

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index da184f4..f7d4f06 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -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;

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

[tytso]

On Wed, Feb 03, 2010 at 03:13:30PM -0600, Eric Sandeen wrote:
> 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 <mst@redhat.com>
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---

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 <tytso@mit.edu>
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 <mst@redhat.com>
    Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 735c20d..7e9a7ea 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -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;
 			}

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

[Eric Sandeen]

tytso@mit.edu wrote:
> On Wed, Feb 03, 2010 at 03:13:30PM -0600, Eric Sandeen wrote:
>   
>> 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 <mst@redhat.com>
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>> ---
>>     
>
> 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 <mst@redhat.com>
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 735c20d..d107d29 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -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

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

[tytso]

Ok, how about this, for even more explicit?

						- Ted

commit 15121c18a22ae483279f76dc9e554334b800d0f7
Author: Eric Sandeen <sandeen@redhat.com>
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 <mst@redhat.com>
    Signed-off-by: Eric Sandeen <sandeen@redhat.com>
    Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 735c20d..68a55df 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -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

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

[Eric Sandeen]

tytso@mit.edu wrote:
> Ok, how about this, for even more explicit?
> 
> 						- Ted

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

-Eric

> commit 15121c18a22ae483279f76dc9e554334b800d0f7
> Author: Eric Sandeen <sandeen@redhat.com>
> 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 <mst@redhat.com>
>     Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>     Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
> 
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 735c20d..68a55df 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -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

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

[Eric Sandeen]

Eric Sandeen wrote:
> tytso@mit.edu wrote:
>> Ok, how about this, for even more explicit?
>>
>> 						- Ted
> 
> Sure, that looks even better.  Who knew args parsing was so hard ;)
> 
> -Eric
> 
>> commit 15121c18a22ae483279f76dc9e554334b800d0f7
>> Author: Eric Sandeen <sandeen@redhat.com>
>> 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 <mst@redhat.com>
>>     Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>>     Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
>>
>> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
>> index 735c20d..68a55df 100644
>> --- a/fs/ext4/super.c
>> +++ b/fs/ext4/super.c
>> @@ -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;

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

-Eric