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

[Michal Hocko <mhocko@suse.cz>]

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>