tytso@xxxxxxx 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@xxxxxxxxxx>
>> Signed-off-by: Eric Sandeen <sandeen@xxxxxxxxxx>
>> ---
>>
>
> 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@xxxxxxxxxx>
Signed-off-by: Eric Sandeen <sandeen@xxxxxxxxxx>
---
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
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
