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

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

[Sha Zhengju]

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.

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, 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>

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

[Michal Hocko]

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? The new implementation is inherently slower without a good
reason.

> 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

-- 
Michal Hocko
SUSE Labs

--
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>

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

[Jeff Liu]

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>

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

[Michal Hocko]

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. 
 
> > 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?

> 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>

-- 
Michal Hocko
SUSE Labs

--
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>

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

[Jeff Liu]

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@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>
> 

--
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>

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

[Michal Hocko]

On Tue 07-05-13 23:29:58, Jeff Liu wrote:
> 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.

I _really_ do not want to punish everybody just because of somthing that
is a configuration issue.

> 
> Thanks,
> -Jeff
-- 
Michal Hocko
SUSE Labs

--
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>

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

[Sha Zhengju]

On Tue, May 7, 2013 at 11:48 PM, Michal Hocko <mhocko@suse.cz> wrote:
> On Tue 07-05-13 23:29:58, Jeff Liu wrote:
>> 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.
>
> I _really_ do not want to punish everybody just because of somthing that
> is a configuration issue.

Okay, Let's lay it aside for the moment.  Thank you!

>
>>
>> Thanks,
>> -Jeff
> --
> Michal Hocko
> SUSE Labs
> --
> 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



--
Thanks,
Sha

--
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>