Re: [PATCH 3/3] memcg: replace memparse to avoid input overflow

[Jeff Liu <jeff.liu@oracle.com>]

On 05/07/2013 10:12 PM, Michal Hocko wrote:
> On Sun 05-05-13 23:44:41, Sha Zhengju wrote:
>> memparse() doesn't check if overflow has happens, and it even has no
>> args to inform user that the unexpected situation has occurred. Besides,
>> some of its callers make a little artful use of the current implementation
>> and it also seems to involve too much if changing memparse() interface.
>>
>> This patch rewrites memcg's internal res_counter_memparse_write_strategy().
>> It doesn't use memparse() any more and replaces simple_strtoull() with
>> kstrtoull() to avoid input overflow.
> 
> I do not like this to be honest. I do not think we should be really
> worried about overflows here. Or where this turned out to be a real
> issue? 
Yes. e.g.
Without this validation, user could specify a big value larger than ULLONG_MAX
which would result in 0 because of an overflow.  Even worse, all the processes
belonging to this group will be killed by OOM-Killer in this situation.
 
> The new implementation is inherently slower without a good
> reason.
In talking about this, I also concerned for the overhead as per an offline
discussion with Sha when she wrote this fix.  However, can we consider it to be
a tradeoff as this helper is not being used in any hot path?
That's why we didn't directly touch memparse(), but extracted those codes for
parsing memory string out of it to res_counter_memparse_write_strategy() instead.

Thanks,
-Jeff
> 
>> Signed-off-by: Sha Zhengju <handai.szj@taobao.com>
>> ---
>>  kernel/res_counter.c |   41 ++++++++++++++++++++++++++++++++++++-----
>>  1 file changed, 36 insertions(+), 5 deletions(-)
>>
>> diff --git a/kernel/res_counter.c b/kernel/res_counter.c
>> index be8ddda..a990e8e0 100644
>> --- a/kernel/res_counter.c
>> +++ b/kernel/res_counter.c
>> @@ -182,19 +182,50 @@ int res_counter_memparse_write_strategy(const char *buf,
>>  {
>>  	char *end;
>>  	unsigned long long res;
>> +	int ret, len, suffix = 0;
>> +	char *ptr;
>>  
>>  	/* return RES_COUNTER_MAX(unlimited) if "-1" is specified */
>>  	if (*buf == '-') {
>> -		res = simple_strtoull(buf + 1, &end, 10);
>> -		if (res != 1 || *end != '\0')
>> +		ret = kstrtoull(buf + 1, 10, &res);
>> +		if (res != 1 || ret)
>>  			return -EINVAL;
>>  		*resp = RES_COUNTER_MAX;
>>  		return 0;
>>  	}
>>  
>> -	res = memparse(buf, &end);
>> -	if (*end != '\0')
>> -		return -EINVAL;
>> +	len = strlen(buf);
>> +	end = buf + len - 1;
>> +	switch (*end) {
>> +	case 'G':
>> +	case 'g':
>> +		suffix ++;
>> +	case 'M':
>> +	case 'm':
>> +		suffix ++;
>> +	case 'K':
>> +	case 'k':
>> +		suffix ++;
>> +		len --;
>> +	default:
>> +		break;
>> +	}
>> +
>> +	ptr = kmalloc(len + 1, GFP_KERNEL);
>> +	if (!ptr) return -ENOMEM;
>> +
>> +	strlcpy(ptr, buf, len + 1);
>> +	ret = kstrtoull(ptr, 0, &res);
>> +	kfree(ptr);
>> +	if (ret) return -EINVAL;
>> +
>> +	while (suffix) {
>> +		/* check for overflow while multiplying suffix number */
>> +		if (unlikely(res & (~0ull << 54)))
>> +			return -EINVAL;
>> +		res <<= 10;
>> +		suffix --;
>> +	}
>>  
>>  	if (PAGE_ALIGN(res) >= res)
>>  		res = PAGE_ALIGN(res);
>> -- 
>> 1.7.9.5
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe cgroups" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>