Re: [PATCH] memcg: Check more strictly to avoid ULLONG overflow by PAGE_ALIGN

[Sha Zhengju <handai.szj@gmail.com>]

On Tue, Apr 16, 2013 at 4:58 AM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Fri, 12 Apr 2013 17:11:08 +0900 Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp> wrote:
>
>> > --- a/include/linux/res_counter.h
>> > +++ b/include/linux/res_counter.h
>> > @@ -54,7 +54,7 @@ struct res_counter {
>> >     struct res_counter *parent;
>> >  };
>> >
>> > -#define RESOURCE_MAX (unsigned long long)LLONG_MAX
>> > +#define RESOURCE_MAX (unsigned long long)ULLONG_MAX
>> >
>>
>> I don't think it's a good idea to change a user-visible value.
>
> The old value was a mistake, surely.
>
> RESOURCE_MAX shouldn't be in this header file - that is far too general
> a name.  I suggest the definition be moved to res_counter.c.  And the
> (unsigned long long) cast is surely unneeded if we're to use
> ULLONG_MAX.
>
>> >  /**
>> >   * Helpers to interact with userspace
>> > diff --git a/kernel/res_counter.c b/kernel/res_counter.c
>> > index ff55247..6c35310 100644
>> > --- a/kernel/res_counter.c
>> > +++ b/kernel/res_counter.c
>> > @@ -195,6 +195,12 @@ int res_counter_memparse_write_strategy(const char *buf,
>> >     if (*end != '\0')
>> >             return -EINVAL;
>> >
>> > -   *res = PAGE_ALIGN(*res);
>> > +   /* Since PAGE_ALIGN is aligning up(the next page boundary),
>> > +    * check the left space to avoid overflow to 0. */
>> > +   if (RESOURCE_MAX - *res < PAGE_SIZE - 1)
>> > +           *res = RESOURCE_MAX;
>> > +   else
>> > +           *res = PAGE_ALIGN(*res);
>> > +
>>
>> Current interface seems strange because we can set a bigger value than
>> the value which means "unlimited".
>
> I'm not sure what you mean by this?
>
>> So, how about some thing like:
>>
>>       if (*res > RESOURCE_MAX)
>>               return -EINVAL;
>>       if (*res > PAGE_ALIGN(RESOURCE_MAX) - PAGE_SIZE)
>>               *res = RESOURCE_MAX;
>>       else
>>               *res = PAGE_ALIGN(*res);
>>
>
> The first thing I'd do to res_counter_memparse_write_strategy() is to
> rename its second arg to `resp' then add a local called `res'.  Because
> that function dereferences res far too often.
>
> Then,
>
> -       *res = PAGE_ALIGN(*res);
>         if (PAGE_ALIGN(res) >= res)
>                 res = PAGE_ALIGN(res);
>         else
>                 res = RESOURCE_MAX;     /* PAGE_ALIGN wrapped to zero */
>
>         *resp = res;
>         return 0;
>

Okay, this one is better.


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>