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

[Michal Hocko <mhocko@suse.cz>]

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>