memcg: account security cred as well to kmemcg
diff mbox series

Message ID 20191205223721.40034-1-shakeelb@google.com
State New
Headers show
Series
  • memcg: account security cred as well to kmemcg
Related show

Commit Message

Shakeel Butt Dec. 5, 2019, 10:37 p.m. UTC
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(-)

Comments

Chris Down Dec. 5, 2019, 11:16 p.m. UTC | #1
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>
Roman Gushchin Dec. 5, 2019, 11:23 p.m. UTC | #2
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!
Michal Hocko Dec. 6, 2019, 8:17 a.m. UTC | #3
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
>
Shakeel Butt Dec. 6, 2019, 4:51 p.m. UTC | #4
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
Andrew Morton Dec. 7, 2019, 12:13 a.m. UTC | #5
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?
Shakeel Butt Dec. 7, 2019, 5:06 a.m. UTC | #6
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.
Michal Hocko Dec. 9, 2019, 2:43 p.m. UTC | #7
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

Patch
diff mbox series

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