On 07/24/2012 07:28 AM, Dave Chinner wrote:
> On Wed, Jul 11, 2012 at 02:29:05PM +0800, Wanlong Gao wrote:
>> - return simple_strtoul((const char *)s, endp, base) << shift_left_factor;
>> + string = match_strdup(s);
>
> GFP_KERNEL allocation....
>
>> + if (!string)
>> + return ENOMEM;
>> +
>> + *result = simple_strtoul((const char *)string, NULL, 0) <<
>> + shift_left_factor;
>> +
>> + kfree(string);
>> + return 0;
>> +}
>> +
>> +STATIC int
>> +match_name_strdup(substring_t *s, char *name)
>> +{
>> + char *string;
>> + string = match_strdup(s);
>
> GFP_KERNEL allocation....
>
>> + if (!string)
>> + return ENOMEM;
>> +
>> + name = kstrndup(string, MAXNAMELEN, GFP_KERNEL);
>
> GFP_KERNEL allocation....
>
>> + if (!name)
>> + goto free;
>> + return 0;
>
> Leaks string - it should always be freed.
Sorry, will update.
>
>> +free:
>> + kfree(string);
>> + return ENOMEM;
>> }
>>
>> /*
>> @@ -166,11 +246,15 @@ xfs_parseargs(
>> char *options)
>> {
>> struct super_block *sb = mp->m_super;
>> - char *this_char, *value, *eov;
>> + char *p;
>> int dsunit = 0;
>> int dswidth = 0;
>> - int iosize = 0;
>> __uint8_t iosizelog = 0;
>> + int intarg;
>> + unsigned long ularg;
>> + substring_t args[MAX_OPT_ARGS];
>> + char *orig = NULL;
>> + int ret = 0;
>>
>> /*
>> * set up the mount name first so all the errors will refer to the
>> @@ -208,175 +292,192 @@ xfs_parseargs(
>> if (!options)
>> goto done;
>>
>> - while ((this_char = strsep(&options, ",")) != NULL) {
>> - if (!*this_char)
>> + options = kstrdup(options, GFP_NOFS);
>
> GFP_NOFS allocation. Why is this GFP_NOFS, and all the other
> allocations GFP_KERNEL? If it is not safe to use GFP_KERNEL
> allocations here, then all of the above allocations need to be
> GFP_NOFS, too.
Since strsep() will change the options, so we should make GFP_NOFS
safely to dup the orig options, but the parse functions can safely
use GFP_KERNEL.
>
>
>> + if (!options) {
>> + ret = ENOMEM;
>> + goto done;
>> + }
>> +
>> + orig = options;
>
> Also, no need to set up orig like this. Just a simple:
>
> orig = kstrdup(options, GFP_NOFS);
> if (!orig) {
> ret = ENOMEM;
> goto done;
> }
>
> will work fine.
Yeah, thank you.
>
> ....
>
>> if (mp->m_logbufs > 0)
>> - seq_printf(m, "," MNTOPT_LOGBUFS "=%d", mp->m_logbufs);
>> + seq_printf(seq, "," MNTOPT_LOGBUFS "=%d", mp->m_logbufs);
>> if (mp->m_logbsize > 0)
>> - seq_printf(m, "," MNTOPT_LOGBSIZE "=%dk", mp->m_logbsize >> 10);
>> + seq_printf(seq, "," MNTOPT_LOGBSIZE "=%dk",
>> + mp->m_logbsize >> 10);
>
> Change of user visible output format here. That will break any
> scripts that are parsing the output and expecting numbers. Just
> leave it as a raw number.
OK, defer to you.
Thanks,
Wanlong Gao
>
> Cheers,
>
> Dave.
>
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
