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)
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;
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 <[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]>
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;
}
[email protected] 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 <[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.
>
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]>
---
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
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
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]>
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
[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
> 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]>
>
> 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
Eric Sandeen wrote:
> [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
>
>> 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]>
>>
>> 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