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

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

[Sha Zhengju]

From: Sha Zhengju <handai.szj@taobao.com>

While writing memory.limit_in_bytes, a confusing result may happen:

$ mkdir /memcg/test
$ cat /memcg/test/memory.limit_in_bytes
9223372036854775807
$ cat /memcg/test/memory.memsw.limit_in_bytes
9223372036854775807
$ echo 18446744073709551614 > /memcg/test/memory.limit_in_bytes
$ cat /memcg/test/memory.limit_in_bytes
0

Strangely, the write successed and reset the limit to 0.
The patch corrects RESOURCE_MAX and fixes this kind of overflow.


Signed-off-by: Sha Zhengju <handai.szj@taobao.com>
Reported-by: Li Wenpeng < xingke.lwp@taobao.com>
Cc: Jie Liu <jeff.liu@oracle.com>
---
 include/linux/res_counter.h |    2 +-
 kernel/res_counter.c        |    8 +++++++-
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/include/linux/res_counter.h b/include/linux/res_counter.h
index c230994..c2f01fc 100644
--- 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
 
 /**
  * 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);
+
 	return 0;
 }
-- 
1.7.9.5

--
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>

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

[Daisuke Nishimura]

Hi,

On Fri, 12 Apr 2013 14:39:23 +0800
Sha Zhengju <handai.szj@gmail.com> wrote:

> From: Sha Zhengju <handai.szj@taobao.com>
> 
> While writing memory.limit_in_bytes, a confusing result may happen:
> 
> $ mkdir /memcg/test
> $ cat /memcg/test/memory.limit_in_bytes
> 9223372036854775807
> $ cat /memcg/test/memory.memsw.limit_in_bytes
> 9223372036854775807
> $ echo 18446744073709551614 > /memcg/test/memory.limit_in_bytes
> $ cat /memcg/test/memory.limit_in_bytes
> 0
> 
> Strangely, the write successed and reset the limit to 0.
> The patch corrects RESOURCE_MAX and fixes this kind of overflow.
> 
> 
> Signed-off-by: Sha Zhengju <handai.szj@taobao.com>
> Reported-by: Li Wenpeng < xingke.lwp@taobao.com>
> Cc: Jie Liu <jeff.liu@oracle.com>
> ---
>  include/linux/res_counter.h |    2 +-
>  kernel/res_counter.c        |    8 +++++++-
>  2 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/res_counter.h b/include/linux/res_counter.h
> index c230994..c2f01fc 100644
> --- 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.

>  /**
>   * 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".
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);

?

>  	return 0;
>  }
> -- 
> 1.7.9.5
> 
> --
> 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>

--
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>

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

[Sha Zhengju]

Hi,

On Fri, Apr 12, 2013 at 4:11 PM, Daisuke Nishimura
<nishimura@mxp.nes.nec.co.jp> wrote:
> Hi,
>
> On Fri, 12 Apr 2013 14:39:23 +0800
> Sha Zhengju <handai.szj@gmail.com> wrote:
>
>> From: Sha Zhengju <handai.szj@taobao.com>
>>
>> While writing memory.limit_in_bytes, a confusing result may happen:
>>
>> $ mkdir /memcg/test
>> $ cat /memcg/test/memory.limit_in_bytes
>> 9223372036854775807
>> $ cat /memcg/test/memory.memsw.limit_in_bytes
>> 9223372036854775807
>> $ echo 18446744073709551614 > /memcg/test/memory.limit_in_bytes
>> $ cat /memcg/test/memory.limit_in_bytes
>> 0
>>
>> Strangely, the write successed and reset the limit to 0.
>> The patch corrects RESOURCE_MAX and fixes this kind of overflow.
>>
>>
>> Signed-off-by: Sha Zhengju <handai.szj@taobao.com>
>> Reported-by: Li Wenpeng < xingke.lwp@taobao.com>
>> Cc: Jie Liu <jeff.liu@oracle.com>
>> ---
>>  include/linux/res_counter.h |    2 +-
>>  kernel/res_counter.c        |    8 +++++++-
>>  2 files changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/linux/res_counter.h b/include/linux/res_counter.h
>> index c230994..c2f01fc 100644
>> --- 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.

I'm not sure why we set the max of ull to LLONG_MAX or there are other
considerations, but we indeed can write a larger limit into it.

>
>>  /**
>>   * 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".
> 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);
>

Actually, once a memcg is created even if the limits remain unchanged,
we still account the tasks' memory usages, but since the current
RESOURCE_MAX is already large enough(8589934591G...) we can consider
it as 'unlimited'.
So here if still using the current 'fake' MAX value and a little
hacking behavior above, we're just intended for making up for the
previous defect(keeping user-visible value unchanged)?

Except for those, I'm fine with both of the approaches. :)

>
>
>>       return 0;
>>  }
>> --
>> 1.7.9.5
>>
>> --
>> 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>



--
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>

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

[Andrew Morton]

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;
	
	

--
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>

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

[Daisuke Nishimura]

On Mon, 15 Apr 2013 13:58:05 -0700
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.
> 

I introduced 'RESOURCE_MAX' in commit:c5b947b2, but I just used the
default value of the res_counter.limit. I'm not sure why it had been
initialized to (unsigned long long)LLONG_MAX.

> 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.
> 

Hmm, RESOUCE_MAX is now used outside of res_counter.c(e.g. tcp_memcontrol.c).
Adding Glauber Costa to the cc list.
Just changing the name to RES_COUNTER_MAX might be the choice.

> > >  /**
> > >   * 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?
> 
9223372036854775807(LLONG_MAX) means "unlimited" now. But we can set a bigger
value than that.

# cat /cgroup/memory/A/memory.memsw.limit_in_bytes
9223372036854775807
# echo 9223372036854775808 >/cgroup/memory/A/memory.memsw.limit_in_bytes
# cat /cgroup/memory/A/memory.memsw.limit_in_bytes
9223372036854775808

I feel "bigger than unlimited" is strange.

> > 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;
> 	
> 	
Good idea :)


Thanks,
Daisuke Nishimura.

--
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>

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

[Sha Zhengju]

On Tue, Apr 16, 2013 at 9:29 AM, Daisuke Nishimura
<nishimura@mxp.nes.nec.co.jp> wrote:
> On Mon, 15 Apr 2013 13:58:05 -0700
> 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.
>>
>
> I introduced 'RESOURCE_MAX' in commit:c5b947b2, but I just used the
> default value of the res_counter.limit. I'm not sure why it had been
> initialized to (unsigned long long)LLONG_MAX.

Then let's correct it now.

>
>> 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.
>>
>
> Hmm, RESOUCE_MAX is now used outside of res_counter.c(e.g. tcp_memcontrol.c).

Yeah, and another mm/memcontrol.c used it too.

> Adding Glauber Costa to the cc list.
> Just changing the name to RES_COUNTER_MAX might be the choice.
>
>> > >  /**
>> > >   * 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?
>>
> 9223372036854775807(LLONG_MAX) means "unlimited" now. But we can set a bigger
> value than that.
>
> # cat /cgroup/memory/A/memory.memsw.limit_in_bytes
> 9223372036854775807
> # echo 9223372036854775808 >/cgroup/memory/A/memory.memsw.limit_in_bytes
> # cat /cgroup/memory/A/memory.memsw.limit_in_bytes
> 9223372036854775808
>
> I feel "bigger than unlimited" is strange.
>
>> > 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;
>>
>>
> Good idea :)
>
>
> Thanks,
> Daisuke Nishimura.



--
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>

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

[Andrew Morton]

On Tue, 16 Apr 2013 10:29:38 +0900 Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp> wrote:

> On Mon, 15 Apr 2013 13:58:05 -0700
> 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.
> > 
> 
> I introduced 'RESOURCE_MAX' in commit:c5b947b2, but I just used the
> default value of the res_counter.limit. I'm not sure why it had been
> initialized to (unsigned long long)LLONG_MAX.
> 
> > 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.
> > 
> 
> Hmm, RESOUCE_MAX is now used outside of res_counter.c(e.g. tcp_memcontrol.c).
> Adding Glauber Costa to the cc list.
> Just changing the name to RES_COUNTER_MAX might be the choice.

That sounds a lot better.

> > > >  /**
> > > >   * 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?
> > 
> 9223372036854775807(LLONG_MAX) means "unlimited" now. But we can set a bigger
> value than that.

Ah, hm, OK, that's pretty random.  I suggest we switch it to ~0ULL,
which is apparently what was intended.  I don't think that will alter
the API in terms of input - we still do "echo -1 > ...".

> # cat /cgroup/memory/A/memory.memsw.limit_in_bytes
> 9223372036854775807
> # echo 9223372036854775808 >/cgroup/memory/A/memory.memsw.limit_in_bytes
> # cat /cgroup/memory/A/memory.memsw.limit_in_bytes
> 9223372036854775808

But it affects the user *output*.

> I feel "bigger than unlimited" is strange.

Yes.  Let's just straighten it all out - I doubt if the small
compatibility changes will hurt anyone.  But please do spell out the
interface changes in full detail in the changelog so we can make the
correct decision here.

I suspect we're looking at two patches here: one to fix/change the
control interface, one to clean up random/misc things we noticed on
the way.

--
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>

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

[Sha Zhengju]

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>