[PATCH] memcg: account security cred as well to kmemcg

[PATCH] memcg: account security cred as well to kmemcg

[Shakeel Butt]

The cred_jar kmem_cache is already memcg accounted in the current
kernel but cred->security is not. Account cred->security to kmemcg.

Recently we saw high root slab usage on our production and on further
inspection, we found a buggy application leaking processes. Though that
buggy application was contained within its memcg but we observe much
more system memory overhead, couple of GiBs, during that period. This
overhead can adversely impact the isolation on the system. One of source
of high overhead, we found was cred->secuity objects.

Signed-off-by: Shakeel Butt <shakeelb@google.com>
---
 kernel/cred.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/kernel/cred.c b/kernel/cred.c
index c0a4c12d38b2..9ed51b70ed80 100644
--- a/kernel/cred.c
+++ b/kernel/cred.c
@@ -223,7 +223,7 @@ struct cred *cred_alloc_blank(void)
 	new->magic = CRED_MAGIC;
 #endif
 
-	if (security_cred_alloc_blank(new, GFP_KERNEL) < 0)
+	if (security_cred_alloc_blank(new, GFP_KERNEL_ACCOUNT) < 0)
 		goto error;
 
 	return new;
@@ -282,7 +282,7 @@ struct cred *prepare_creds(void)
 	new->security = NULL;
 #endif
 
-	if (security_prepare_creds(new, old, GFP_KERNEL) < 0)
+	if (security_prepare_creds(new, old, GFP_KERNEL_ACCOUNT) < 0)
 		goto error;
 	validate_creds(new);
 	return new;
@@ -715,7 +715,7 @@ struct cred *prepare_kernel_cred(struct task_struct *daemon)
 #ifdef CONFIG_SECURITY
 	new->security = NULL;
 #endif
-	if (security_prepare_creds(new, old, GFP_KERNEL) < 0)
+	if (security_prepare_creds(new, old, GFP_KERNEL_ACCOUNT) < 0)
 		goto error;
 
 	put_cred(old);
-- 
2.24.0.393.g34dc348eaf-goog

Re: [PATCH] memcg: account security cred as well to kmemcg

[Chris Down]

Shakeel Butt writes:
>The cred_jar kmem_cache is already memcg accounted in the current
>kernel but cred->security is not. Account cred->security to kmemcg.
>
>Recently we saw high root slab usage on our production and on further
>inspection, we found a buggy application leaking processes. Though that
>buggy application was contained within its memcg but we observe much
>more system memory overhead, couple of GiBs, during that period. This
>overhead can adversely impact the isolation on the system. One of source
>of high overhead, we found was cred->secuity objects.

Makes sense. I took a look through other cred-related allocations to see if any 
others stood out and this looks like it covers all the relevant cases.  
__alloc_file is the only other one that caught my eye, but SLAB_ACCOUNT is on 
the filp cache already.

Thanks :-)

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

Acked-by: Chris Down <chris@chrisdown.name>

Re: [PATCH] memcg: account security cred as well to kmemcg

[Roman Gushchin]

On Thu, Dec 05, 2019 at 02:37:21PM -0800, Shakeel Butt wrote:
> The cred_jar kmem_cache is already memcg accounted in the current
> kernel but cred->security is not. Account cred->security to kmemcg.
> 
> Recently we saw high root slab usage on our production and on further
> inspection, we found a buggy application leaking processes. Though that
> buggy application was contained within its memcg but we observe much
> more system memory overhead, couple of GiBs, during that period. This
> overhead can adversely impact the isolation on the system. One of source
> of high overhead, we found was cred->secuity objects.
> 
> Signed-off-by: Shakeel Butt <shakeelb@google.com>

Reviewed-by: Roman Gushchin <guro@fb.com>

Thanks!

Re: [PATCH] memcg: account security cred as well to kmemcg

[Michal Hocko]

On Thu 05-12-19 14:37:21, Shakeel Butt wrote:
> The cred_jar kmem_cache is already memcg accounted in the current
> kernel but cred->security is not. Account cred->security to kmemcg.
> 
> Recently we saw high root slab usage on our production and on further
> inspection, we found a buggy application leaking processes. Though that
> buggy application was contained within its memcg but we observe much
> more system memory overhead, couple of GiBs, during that period. This
> overhead can adversely impact the isolation on the system. One of source
> of high overhead, we found was cred->secuity objects.
 
I am not familiar with this area much. What is the timelife of these
objects? Do they go away with a task allocating them?

> Signed-off-by: Shakeel Butt <shakeelb@google.com>
>
> ---
>  kernel/cred.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/cred.c b/kernel/cred.c
> index c0a4c12d38b2..9ed51b70ed80 100644
> --- a/kernel/cred.c
> +++ b/kernel/cred.c
> @@ -223,7 +223,7 @@ struct cred *cred_alloc_blank(void)
>  	new->magic = CRED_MAGIC;
>  #endif
>  
> -	if (security_cred_alloc_blank(new, GFP_KERNEL) < 0)
> +	if (security_cred_alloc_blank(new, GFP_KERNEL_ACCOUNT) < 0)
>  		goto error;
>  
>  	return new;
> @@ -282,7 +282,7 @@ struct cred *prepare_creds(void)
>  	new->security = NULL;
>  #endif
>  
> -	if (security_prepare_creds(new, old, GFP_KERNEL) < 0)
> +	if (security_prepare_creds(new, old, GFP_KERNEL_ACCOUNT) < 0)
>  		goto error;
>  	validate_creds(new);
>  	return new;
> @@ -715,7 +715,7 @@ struct cred *prepare_kernel_cred(struct task_struct *daemon)
>  #ifdef CONFIG_SECURITY
>  	new->security = NULL;
>  #endif
> -	if (security_prepare_creds(new, old, GFP_KERNEL) < 0)
> +	if (security_prepare_creds(new, old, GFP_KERNEL_ACCOUNT) < 0)
>  		goto error;
>  
>  	put_cred(old);
> -- 
> 2.24.0.393.g34dc348eaf-goog
> 

-- 
Michal Hocko
SUSE Labs

Re: [PATCH] memcg: account security cred as well to kmemcg

[Shakeel Butt]

On Fri, Dec 6, 2019 at 12:17 AM Michal Hocko <mhocko@kernel.org> wrote:
>
> On Thu 05-12-19 14:37:21, Shakeel Butt wrote:
> > The cred_jar kmem_cache is already memcg accounted in the current
> > kernel but cred->security is not. Account cred->security to kmemcg.
> >
> > Recently we saw high root slab usage on our production and on further
> > inspection, we found a buggy application leaking processes. Though that
> > buggy application was contained within its memcg but we observe much
> > more system memory overhead, couple of GiBs, during that period. This
> > overhead can adversely impact the isolation on the system. One of source
> > of high overhead, we found was cred->secuity objects.
>
> I am not familiar with this area much. What is the timelife of these
> objects? Do they go away with a task allocating them?
>

Lifetime is at least the life of the process allocating them.

> > Signed-off-by: Shakeel Butt <shakeelb@google.com>
> >
> > ---
> >  kernel/cred.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/kernel/cred.c b/kernel/cred.c
> > index c0a4c12d38b2..9ed51b70ed80 100644
> > --- a/kernel/cred.c
> > +++ b/kernel/cred.c
> > @@ -223,7 +223,7 @@ struct cred *cred_alloc_blank(void)
> >       new->magic = CRED_MAGIC;
> >  #endif
> >
> > -     if (security_cred_alloc_blank(new, GFP_KERNEL) < 0)
> > +     if (security_cred_alloc_blank(new, GFP_KERNEL_ACCOUNT) < 0)
> >               goto error;
> >
> >       return new;
> > @@ -282,7 +282,7 @@ struct cred *prepare_creds(void)
> >       new->security = NULL;
> >  #endif
> >
> > -     if (security_prepare_creds(new, old, GFP_KERNEL) < 0)
> > +     if (security_prepare_creds(new, old, GFP_KERNEL_ACCOUNT) < 0)
> >               goto error;
> >       validate_creds(new);
> >       return new;
> > @@ -715,7 +715,7 @@ struct cred *prepare_kernel_cred(struct task_struct *daemon)
> >  #ifdef CONFIG_SECURITY
> >       new->security = NULL;
> >  #endif
> > -     if (security_prepare_creds(new, old, GFP_KERNEL) < 0)
> > +     if (security_prepare_creds(new, old, GFP_KERNEL_ACCOUNT) < 0)
> >               goto error;
> >
> >       put_cred(old);
> > --
> > 2.24.0.393.g34dc348eaf-goog
> >
>
> --
> Michal Hocko
> SUSE Labs

Re: [PATCH] memcg: account security cred as well to kmemcg

[Andrew Morton]

On Thu,  5 Dec 2019 14:37:21 -0800 Shakeel Butt <shakeelb@google.com> wrote:

> The cred_jar kmem_cache is already memcg accounted in the current
> kernel but cred->security is not. Account cred->security to kmemcg.
> 
> Recently we saw high root slab usage on our production and on further
> inspection, we found a buggy application leaking processes. Though that
> buggy application was contained within its memcg but we observe much
> more system memory overhead, couple of GiBs, during that period. This
> overhead can adversely impact the isolation on the system. One of source
> of high overhead, we found was cred->secuity objects.

A bit of an oversight and the fix is simple.  Is it worth a cc:stable?

Re: [PATCH] memcg: account security cred as well to kmemcg

[Shakeel Butt]

On Fri, Dec 6, 2019 at 4:13 PM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Thu,  5 Dec 2019 14:37:21 -0800 Shakeel Butt <shakeelb@google.com> wrote:
>
> > The cred_jar kmem_cache is already memcg accounted in the current
> > kernel but cred->security is not. Account cred->security to kmemcg.
> >
> > Recently we saw high root slab usage on our production and on further
> > inspection, we found a buggy application leaking processes. Though that
> > buggy application was contained within its memcg but we observe much
> > more system memory overhead, couple of GiBs, during that period. This
> > overhead can adversely impact the isolation on the system. One of source
> > of high overhead, we found was cred->secuity objects.
>
> A bit of an oversight and the fix is simple.  Is it worth a cc:stable?

Yes, I think it is simple and safe enough for stable.

Re: [PATCH] memcg: account security cred as well to kmemcg

[Michal Hocko]

On Fri 06-12-19 08:51:21, Shakeel Butt wrote:
> On Fri, Dec 6, 2019 at 12:17 AM Michal Hocko <mhocko@kernel.org> wrote:
> >
> > On Thu 05-12-19 14:37:21, Shakeel Butt wrote:
> > > The cred_jar kmem_cache is already memcg accounted in the current
> > > kernel but cred->security is not. Account cred->security to kmemcg.
> > >
> > > Recently we saw high root slab usage on our production and on further
> > > inspection, we found a buggy application leaking processes. Though that
> > > buggy application was contained within its memcg but we observe much
> > > more system memory overhead, couple of GiBs, during that period. This
> > > overhead can adversely impact the isolation on the system. One of source
> > > of high overhead, we found was cred->secuity objects.
> >
> > I am not familiar with this area much. What is the timelife of these
> > objects? Do they go away with a task allocating them?
> >
> 
> Lifetime is at least the life of the process allocating them.

Thanks for the clarification! It is better to be explicit about this in
the changelog I believe because it would make a review much easier.
Accounting for objects which are not bound to a user process context is
more complex and requires much more considerations.

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

With that clarification
Acked-by: Michal Hocko <mhocko@suse.com>

> > >
> > > ---
> > >  kernel/cred.c | 6 +++---
> > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/kernel/cred.c b/kernel/cred.c
> > > index c0a4c12d38b2..9ed51b70ed80 100644
> > > --- a/kernel/cred.c
> > > +++ b/kernel/cred.c
> > > @@ -223,7 +223,7 @@ struct cred *cred_alloc_blank(void)
> > >       new->magic = CRED_MAGIC;
> > >  #endif
> > >
> > > -     if (security_cred_alloc_blank(new, GFP_KERNEL) < 0)
> > > +     if (security_cred_alloc_blank(new, GFP_KERNEL_ACCOUNT) < 0)
> > >               goto error;
> > >
> > >       return new;
> > > @@ -282,7 +282,7 @@ struct cred *prepare_creds(void)
> > >       new->security = NULL;
> > >  #endif
> > >
> > > -     if (security_prepare_creds(new, old, GFP_KERNEL) < 0)
> > > +     if (security_prepare_creds(new, old, GFP_KERNEL_ACCOUNT) < 0)
> > >               goto error;
> > >       validate_creds(new);
> > >       return new;
> > > @@ -715,7 +715,7 @@ struct cred *prepare_kernel_cred(struct task_struct *daemon)
> > >  #ifdef CONFIG_SECURITY
> > >       new->security = NULL;
> > >  #endif
> > > -     if (security_prepare_creds(new, old, GFP_KERNEL) < 0)
> > > +     if (security_prepare_creds(new, old, GFP_KERNEL_ACCOUNT) < 0)
> > >               goto error;
> > >
> > >       put_cred(old);
> > > --
> > > 2.24.0.393.g34dc348eaf-goog
> > >
> >
> > --
> > Michal Hocko
> > SUSE Labs

-- 
Michal Hocko
SUSE Labs