Message ID | 20100216011944.GQ5337@thunk.org |
---|---|
State | Accepted, archived |
Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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