On 05/07/2013 11:15 PM, Michal Hocko wrote:
> On Tue 07-05-13 23:04:54, Jeff Liu wrote:
>> 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.
>
> I would consider this to be a configuration problem.
It mostly should be a problem of configuration.
>
>>> 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?
>
> what is the positive part of the trade off? Fixing a potential overflow
> when somebody sets a limit to an unreasonable value?
I suppose it to be a defense for unreasonable value because this issue
is found on a production environment for an incorrect manipulation, but
it's up to you.
Thanks,
-Jeff
>
>> 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@xxxxxxxxxx>
>>>> ---
>>>> 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@xxxxxxxxxxxxxxx
>>>> 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@xxxxxxxxx. For more info on Linux MM,
>> see: http://www.linux-mm.org/ .
>> Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>
>
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>