On Tue, May 7, 2013 at 11:48 PM, Michal Hocko <mhocko@xxxxxxx> 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@xxxxxxxxxxxxxxx > 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@xxxxxxxxx. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>