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@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>
--
Michal Hocko
SUSE Labs
--
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>