[PATCH] memcg: enable accounting for pids in nested pid namespaces

[PATCH] memcg: enable accounting for pids in nested pid namespaces

[Vasily Averin]

init_pid_ns.pid_cachep have enabled memcg accounting, though this
setting was disabled for nested pid namespaces.

Signed-off-by: Vasily Averin <vvs-5HdwGun5lf+gSpxsJD1C4w@public.gmane.org>
---
 kernel/pid_namespace.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/kernel/pid_namespace.c b/kernel/pid_namespace.c
index 6cd6715..a46a372 100644
--- a/kernel/pid_namespace.c
+++ b/kernel/pid_namespace.c
@@ -51,7 +51,8 @@ static struct kmem_cache *create_pid_cachep(unsigned int level)
 	mutex_lock(&pid_caches_mutex);
 	/* Name collision forces to do allocation under mutex. */
 	if (!*pkc)
-		*pkc = kmem_cache_create(name, len, 0, SLAB_HWCACHE_ALIGN, 0);
+		*pkc = kmem_cache_create(name, len, 0,
+					 SLAB_HWCACHE_ALIGN | SLAB_ACCOUNT, 0);
 	mutex_unlock(&pid_caches_mutex);
 	/* current can fail, but someone else can succeed. */
 	return READ_ONCE(*pkc);
-- 
1.8.3.1

Re: [PATCH] memcg: enable accounting for pids in nested pid namespaces

[Roman Gushchin]

On Thu, Apr 22, 2021 at 08:44:15AM +0300, Vasily Averin wrote:
> init_pid_ns.pid_cachep have enabled memcg accounting, though this
> setting was disabled for nested pid namespaces.
> 
> Signed-off-by: Vasily Averin <vvs-5HdwGun5lf+gSpxsJD1C4w@public.gmane.org>
> ---
>  kernel/pid_namespace.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/pid_namespace.c b/kernel/pid_namespace.c
> index 6cd6715..a46a372 100644
> --- a/kernel/pid_namespace.c
> +++ b/kernel/pid_namespace.c
> @@ -51,7 +51,8 @@ static struct kmem_cache *create_pid_cachep(unsigned int level)
>  	mutex_lock(&pid_caches_mutex);
>  	/* Name collision forces to do allocation under mutex. */
>  	if (!*pkc)
> -		*pkc = kmem_cache_create(name, len, 0, SLAB_HWCACHE_ALIGN, 0);
> +		*pkc = kmem_cache_create(name, len, 0,
> +					 SLAB_HWCACHE_ALIGN | SLAB_ACCOUNT, 0);
>  	mutex_unlock(&pid_caches_mutex);
>  	/* current can fail, but someone else can succeed. */
>  	return READ_ONCE(*pkc);
> -- 
> 1.8.3.1
> 

It looks good to me! It makes total sense to apply the same rules to the root
and non-root levels.

Acked-by: Roman Gushchin <guro-b10kYP2dOMg@public.gmane.org>

Btw, is there any reason why this patch is not included into the series?

Thanks!

Re: [PATCH] memcg: enable accounting for pids in nested pid namespaces

[Vasily Averin]

On 4/23/21 4:00 AM, Roman Gushchin wrote:
> On Thu, Apr 22, 2021 at 08:44:15AM +0300, Vasily Averin wrote:
>> init_pid_ns.pid_cachep have enabled memcg accounting, though this
>> setting was disabled for nested pid namespaces.
>>
>> Signed-off-by: Vasily Averin <vvs-5HdwGun5lf+gSpxsJD1C4w@public.gmane.org>
>> ---
>>  kernel/pid_namespace.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/kernel/pid_namespace.c b/kernel/pid_namespace.c
>> index 6cd6715..a46a372 100644
>> --- a/kernel/pid_namespace.c
>> +++ b/kernel/pid_namespace.c
>> @@ -51,7 +51,8 @@ static struct kmem_cache *create_pid_cachep(unsigned int level)
>>  	mutex_lock(&pid_caches_mutex);
>>  	/* Name collision forces to do allocation under mutex. */
>>  	if (!*pkc)
>> -		*pkc = kmem_cache_create(name, len, 0, SLAB_HWCACHE_ALIGN, 0);
>> +		*pkc = kmem_cache_create(name, len, 0,
>> +					 SLAB_HWCACHE_ALIGN | SLAB_ACCOUNT, 0);
>>  	mutex_unlock(&pid_caches_mutex);
>>  	/* current can fail, but someone else can succeed. */
>>  	return READ_ONCE(*pkc);
>> -- 
>> 1.8.3.1
>>
> 
> It looks good to me! It makes total sense to apply the same rules to the root
> and non-root levels.
> 
> Acked-by: Roman Gushchin <guro-b10kYP2dOMg@public.gmane.org>
> 
> Btw, is there any reason why this patch is not included into the series?

It is a bugfix and I think it should be added to upstream ASAP.
Another patches adds a new functionality, they can cause questions or objections
and anyway can wait.

Thank you,
	Vasily Averin

Re: [PATCH] memcg: enable accounting for pids in nested pid namespaces

[Roman Gushchin]

On Fri, Apr 23, 2021 at 05:09:01AM +0300, Vasily Averin wrote:
> On 4/23/21 4:00 AM, Roman Gushchin wrote:
> > On Thu, Apr 22, 2021 at 08:44:15AM +0300, Vasily Averin wrote:
> >> init_pid_ns.pid_cachep have enabled memcg accounting, though this
> >> setting was disabled for nested pid namespaces.
> >>
> >> Signed-off-by: Vasily Averin <vvs-5HdwGun5lf+gSpxsJD1C4w@public.gmane.org>
> >> ---
> >>  kernel/pid_namespace.c | 3 ++-
> >>  1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/kernel/pid_namespace.c b/kernel/pid_namespace.c
> >> index 6cd6715..a46a372 100644
> >> --- a/kernel/pid_namespace.c
> >> +++ b/kernel/pid_namespace.c
> >> @@ -51,7 +51,8 @@ static struct kmem_cache *create_pid_cachep(unsigned int level)
> >>  	mutex_lock(&pid_caches_mutex);
> >>  	/* Name collision forces to do allocation under mutex. */
> >>  	if (!*pkc)
> >> -		*pkc = kmem_cache_create(name, len, 0, SLAB_HWCACHE_ALIGN, 0);
> >> +		*pkc = kmem_cache_create(name, len, 0,
> >> +					 SLAB_HWCACHE_ALIGN | SLAB_ACCOUNT, 0);
> >>  	mutex_unlock(&pid_caches_mutex);
> >>  	/* current can fail, but someone else can succeed. */
> >>  	return READ_ONCE(*pkc);
> >> -- 
> >> 1.8.3.1
> >>
> > 
> > It looks good to me! It makes total sense to apply the same rules to the root
> > and non-root levels.
> > 
> > Acked-by: Roman Gushchin <guro-b10kYP2dOMg@public.gmane.org>
> > 
> > Btw, is there any reason why this patch is not included into the series?
> 
> It is a bugfix and I think it should be added to upstream ASAP.

Then it would be really useful to add some details on why it's a bug,
what kind of problems it causes, etc. If it has to be backported to
stable, please, add cc stable/fixes tag.

Thanks!

Re: [PATCH] memcg: enable accounting for pids in nested pid namespaces

[Vasily Averin]

On 4/23/21 5:30 AM, Roman Gushchin wrote:
> On Fri, Apr 23, 2021 at 05:09:01AM +0300, Vasily Averin wrote:
>> On 4/23/21 4:00 AM, Roman Gushchin wrote:
>>> On Thu, Apr 22, 2021 at 08:44:15AM +0300, Vasily Averin wrote:
>>>> init_pid_ns.pid_cachep have enabled memcg accounting, though this
>>>> setting was disabled for nested pid namespaces.
>>>>
>>>> Signed-off-by: Vasily Averin <vvs-5HdwGun5lf+gSpxsJD1C4w@public.gmane.org>
>>>> ---
>>>>  kernel/pid_namespace.c | 3 ++-
>>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/kernel/pid_namespace.c b/kernel/pid_namespace.c
>>>> index 6cd6715..a46a372 100644
>>>> --- a/kernel/pid_namespace.c
>>>> +++ b/kernel/pid_namespace.c
>>>> @@ -51,7 +51,8 @@ static struct kmem_cache *create_pid_cachep(unsigned int level)
>>>>  	mutex_lock(&pid_caches_mutex);
>>>>  	/* Name collision forces to do allocation under mutex. */
>>>>  	if (!*pkc)
>>>> -		*pkc = kmem_cache_create(name, len, 0, SLAB_HWCACHE_ALIGN, 0);
>>>> +		*pkc = kmem_cache_create(name, len, 0,
>>>> +					 SLAB_HWCACHE_ALIGN | SLAB_ACCOUNT, 0);
>>>>  	mutex_unlock(&pid_caches_mutex);
>>>>  	/* current can fail, but someone else can succeed. */
>>>>  	return READ_ONCE(*pkc);
>>>> -- 
>>>> 1.8.3.1
>>>>
>>>
>>> It looks good to me! It makes total sense to apply the same rules to the root
>>> and non-root levels.
>>>
>>> Acked-by: Roman Gushchin <guro-b10kYP2dOMg@public.gmane.org>
>>>
>>> Btw, is there any reason why this patch is not included into the series?
>>
>> It is a bugfix and I think it should be added to upstream ASAP.
> 
> Then it would be really useful to add some details on why it's a bug,
> what kind of problems it causes, etc. If it has to be backported to
> stable, please, add cc stable/fixes tag.

I mean, in this case we already decided to account pids, but forget to do it.
In another cases we did not have final decision about accounting.

I doubt we specially denied accounting for pids frem nested pid namespaces,
especially because they consumes more memory.
We can expect that all pids are accounted -- but it does not happen in fact.

Re: [PATCH] memcg: enable accounting for pids in nested pid namespaces

[Christian Brauner]

On Fri, Apr 23, 2021 at 05:53:43AM +0300, Vasily Averin wrote:
> On 4/23/21 5:30 AM, Roman Gushchin wrote:
> > On Fri, Apr 23, 2021 at 05:09:01AM +0300, Vasily Averin wrote:
> >> On 4/23/21 4:00 AM, Roman Gushchin wrote:
> >>> On Thu, Apr 22, 2021 at 08:44:15AM +0300, Vasily Averin wrote:
> >>>> init_pid_ns.pid_cachep have enabled memcg accounting, though this
> >>>> setting was disabled for nested pid namespaces.
> >>>>
> >>>> Signed-off-by: Vasily Averin <vvs-5HdwGun5lf+gSpxsJD1C4w@public.gmane.org>
> >>>> ---
> >>>>  kernel/pid_namespace.c | 3 ++-
> >>>>  1 file changed, 2 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/kernel/pid_namespace.c b/kernel/pid_namespace.c
> >>>> index 6cd6715..a46a372 100644
> >>>> --- a/kernel/pid_namespace.c
> >>>> +++ b/kernel/pid_namespace.c
> >>>> @@ -51,7 +51,8 @@ static struct kmem_cache *create_pid_cachep(unsigned int level)
> >>>>  	mutex_lock(&pid_caches_mutex);
> >>>>  	/* Name collision forces to do allocation under mutex. */
> >>>>  	if (!*pkc)
> >>>> -		*pkc = kmem_cache_create(name, len, 0, SLAB_HWCACHE_ALIGN, 0);
> >>>> +		*pkc = kmem_cache_create(name, len, 0,
> >>>> +					 SLAB_HWCACHE_ALIGN | SLAB_ACCOUNT, 0);
> >>>>  	mutex_unlock(&pid_caches_mutex);
> >>>>  	/* current can fail, but someone else can succeed. */
> >>>>  	return READ_ONCE(*pkc);
> >>>> -- 
> >>>> 1.8.3.1
> >>>>
> >>>
> >>> It looks good to me! It makes total sense to apply the same rules to the root
> >>> and non-root levels.
> >>>
> >>> Acked-by: Roman Gushchin <guro-b10kYP2dOMg@public.gmane.org>
> >>>
> >>> Btw, is there any reason why this patch is not included into the series?
> >>
> >> It is a bugfix and I think it should be added to upstream ASAP.
> > 
> > Then it would be really useful to add some details on why it's a bug,
> > what kind of problems it causes, etc. If it has to be backported to
> > stable, please, add cc stable/fixes tag.
> 
> I mean, in this case we already decided to account pids, but forget to do it.
> In another cases we did not have final decision about accounting.
> 
> I doubt we specially denied accounting for pids frem nested pid namespaces,
> especially because they consumes more memory.
> We can expect that all pids are accounted -- but it does not happen in fact.

As Roman said you should probably explain this in the cover letter and
essentially justify why this should be backported. The thing with
changes such as this is that it's easy for someone to reply and go
"noone noticed for <n> years so why do we care now?". Otherwise:

Acked-by: Christian Brauner <christian.brauner-GeWIH/nMZzLQT0dZR+AlfA@public.gmane.org>

Christian

Re: [PATCH] memcg: enable accounting for pids in nested pid namespaces

[Michal Koutný]

[-- Attachment #1: Type: text/plain, Size: 421 bytes --]

On Thu, Apr 22, 2021 at 08:44:15AM +0300, Vasily Averin <vvs-5HdwGun5lf+gSpxsJD1C4w@public.gmane.org> wrote:
> init_pid_ns.pid_cachep have enabled memcg accounting, though this
> setting was disabled for nested pid namespaces.
Good catch.
Cursory grep of user_namespace.c and nsproxy.c suggests it's the only
case of namespace-induced new cache.

Reviewed-by: Michal Koutný <mkoutny-IBi9RG/b67k@public.gmane.org>

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 0/1] memcg: enable accounting for pids in nested pid namespaces

[Vasily Averin]

Pid was one the first kernel objects enabled for memcg accounting, see
5d097056c9a0 ("kmemcg: account certain kmem allocations to memcg")

init_pid_ns.pid_cachep marked by SLAB_ACCOUNT and we can expect that any new pids 
in the system are memcg-accounted.

Though recently I've noticed that it is wrong. nested pid namespaces creates 
own slab caches for pid objects, nested pids have increased size because contain 
id both for all parent and for own pid namespaces. The problem is that these slab
caches are _NOT_ marked by SLAB_ACCOUNT,as a result any pids allocated in 
nested pid namespaces are not memcg-accounted.

Pid struct in nested pid namespace consumes up to 500 bytes memory,
100000 such objects gives us up to ~50Mb unaccounted memory.
This allow container to exceed assigned memcg limits.

For me this issue lookslike bug and I would like to ask to push this fix 
both to upstream and to stable

Vasily Averin (1):
  memcg: enable accounting for pids in nested pid namespaces

 kernel/pid_namespace.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

-- 
1.8.3.1

[PATCH v2 1/1] memcg: enable accounting for pids in nested pid namespaces

[Vasily Averin]

Commit 5d097056c9a0 ("kmemcg: account certain kmem allocations to memcg")
enabled memcg accounting for pids allocated from init_pid_ns.pid_cachep,
but forgot to adjust the setting for nested pid namespaces.
As a result, pid memory is not accounted exactly where it is really needed,
inside memcg-limited containers with their own pid namespaces.

Pid was one the first kernel objects enabled for memcg accounting.
init_pid_ns.pid_cachep marked by SLAB_ACCOUNT and we can expect that
any new pids in the system are memcg-accounted.

Though recently I've noticed that it is wrong. nested pid namespaces creates 
own slab caches for pid objects, nested pids have increased size because contain 
id both for all parent and for own pid namespaces. The problem is that these slab
caches are _NOT_ marked by SLAB_ACCOUNT, as a result any pids allocated in 
nested pid namespaces are not memcg-accounted.

Pid struct in nested pid namespace consumes up to 500 bytes memory, 
100000 such objects gives us up to ~50Mb unaccounted memory,
this allow container to exceed assigned memcg limits.

Fixes: 5d097056c9a0 ("kmemcg: account certain kmem allocations to memcg")
Cc: stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Signed-off-by: Vasily Averin <vvs-5HdwGun5lf+gSpxsJD1C4w@public.gmane.org>
Reviewed-by: Michal Koutn√Ω <mkoutny-IBi9RG/b67k@public.gmane.org>
Acked-by: Christian Brauner <christian.brauner-GeWIH/nMZzLQT0dZR+AlfA@public.gmane.org>
Acked-by: Roman Gushchin <guro-b10kYP2dOMg@public.gmane.org>
---
 kernel/pid_namespace.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/kernel/pid_namespace.c b/kernel/pid_namespace.c
index 6cd6715..a46a372 100644
--- a/kernel/pid_namespace.c
+++ b/kernel/pid_namespace.c
@@ -51,7 +51,8 @@ static struct kmem_cache *create_pid_cachep(unsigned int level)
 	mutex_lock(&pid_caches_mutex);
 	/* Name collision forces to do allocation under mutex. */
 	if (!*pkc)
-		*pkc = kmem_cache_create(name, len, 0, SLAB_HWCACHE_ALIGN, 0);
+		*pkc = kmem_cache_create(name, len, 0,
+					 SLAB_HWCACHE_ALIGN | SLAB_ACCOUNT, 0);
 	mutex_unlock(&pid_caches_mutex);
 	/* current can fail, but someone else can succeed. */
 	return READ_ONCE(*pkc);
-- 
1.8.3.1

Re: [PATCH v2 1/1] memcg: enable accounting for pids in nested pid namespaces

[Shakeel Butt]

On Sat, Apr 24, 2021 at 4:54 AM Vasily Averin <vvs@virtuozzo.com> wrote:
>
> Commit 5d097056c9a0 ("kmemcg: account certain kmem allocations to memcg")
> enabled memcg accounting for pids allocated from init_pid_ns.pid_cachep,
> but forgot to adjust the setting for nested pid namespaces.
> As a result, pid memory is not accounted exactly where it is really needed,
> inside memcg-limited containers with their own pid namespaces.
>
> Pid was one the first kernel objects enabled for memcg accounting.
> init_pid_ns.pid_cachep marked by SLAB_ACCOUNT and we can expect that
> any new pids in the system are memcg-accounted.
>
> Though recently I've noticed that it is wrong. nested pid namespaces creates
> own slab caches for pid objects, nested pids have increased size because contain
> id both for all parent and for own pid namespaces. The problem is that these slab
> caches are _NOT_ marked by SLAB_ACCOUNT, as a result any pids allocated in
> nested pid namespaces are not memcg-accounted.
>
> Pid struct in nested pid namespace consumes up to 500 bytes memory,
> 100000 such objects gives us up to ~50Mb unaccounted memory,
> this allow container to exceed assigned memcg limits.
>
> Fixes: 5d097056c9a0 ("kmemcg: account certain kmem allocations to memcg")
> Cc: stable@vger.kernel.org
> Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
> Reviewed-by: Michal Koutn√Ω <mkoutny@suse.com>
> Acked-by: Christian Brauner <christian.brauner@ubuntu.com>
> Acked-by: Roman Gushchin <guro@fb.com>

Reviewed-by: Shakeel Butt <shakeelb@google.com>

Re: [PATCH v2 1/1] memcg: enable accounting for pids in nested pid namespaces

[Vasily Averin]

Dear Andrew,
could you please pick up this patch and add
 Reviewed-by: Shakeel Butt <shakeelb-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>

Thank you,
	Vasily Averin

On 4/24/21 2:54 PM, Vasily Averin wrote:
> Commit 5d097056c9a0 ("kmemcg: account certain kmem allocations to memcg")
> enabled memcg accounting for pids allocated from init_pid_ns.pid_cachep,
> but forgot to adjust the setting for nested pid namespaces.
> As a result, pid memory is not accounted exactly where it is really needed,
> inside memcg-limited containers with their own pid namespaces.
> 
> Pid was one the first kernel objects enabled for memcg accounting.
> init_pid_ns.pid_cachep marked by SLAB_ACCOUNT and we can expect that
> any new pids in the system are memcg-accounted.
> 
> Though recently I've noticed that it is wrong. nested pid namespaces creates 
> own slab caches for pid objects, nested pids have increased size because contain 
> id both for all parent and for own pid namespaces. The problem is that these slab
> caches are _NOT_ marked by SLAB_ACCOUNT, as a result any pids allocated in 
> nested pid namespaces are not memcg-accounted.
> 
> Pid struct in nested pid namespace consumes up to 500 bytes memory, 
> 100000 such objects gives us up to ~50Mb unaccounted memory,
> this allow container to exceed assigned memcg limits.
> 
> Fixes: 5d097056c9a0 ("kmemcg: account certain kmem allocations to memcg")
> Cc: stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Signed-off-by: Vasily Averin <vvs-5HdwGun5lf+gSpxsJD1C4w@public.gmane.org>
> Reviewed-by: Michal Koutn√Ω <mkoutny-IBi9RG/b67k@public.gmane.org>
> Acked-by: Christian Brauner <christian.brauner-GeWIH/nMZzLQT0dZR+AlfA@public.gmane.org>
> Acked-by: Roman Gushchin <guro-b10kYP2dOMg@public.gmane.org>
> ---
>  kernel/pid_namespace.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/pid_namespace.c b/kernel/pid_namespace.c
> index 6cd6715..a46a372 100644
> --- a/kernel/pid_namespace.c
> +++ b/kernel/pid_namespace.c
> @@ -51,7 +51,8 @@ static struct kmem_cache *create_pid_cachep(unsigned int level)
>  	mutex_lock(&pid_caches_mutex);
>  	/* Name collision forces to do allocation under mutex. */
>  	if (!*pkc)
> -		*pkc = kmem_cache_create(name, len, 0, SLAB_HWCACHE_ALIGN, 0);
> +		*pkc = kmem_cache_create(name, len, 0,
> +					 SLAB_HWCACHE_ALIGN | SLAB_ACCOUNT, 0);
>  	mutex_unlock(&pid_caches_mutex);
>  	/* current can fail, but someone else can succeed. */
>  	return READ_ONCE(*pkc);
> 

Re: [PATCH] memcg: enable accounting for pids in nested pid namespaces

[Christian Brauner]

On Thu, Apr 22, 2021 at 08:44:15AM +0300, Vasily Averin wrote:
> init_pid_ns.pid_cachep have enabled memcg accounting, though this
> setting was disabled for nested pid namespaces.
> 
> Signed-off-by: Vasily Averin <vvs-5HdwGun5lf+gSpxsJD1C4w@public.gmane.org>
> ---

Not sure I already acked this but looks good,
Acked-by: Christian Brauner <christian.brauner-GeWIH/nMZzLQT0dZR+AlfA@public.gmane.org>