[PATCH] memcg, oom: lock mem_cgroup_print_oom_info

[PATCH] memcg, oom: lock mem_cgroup_print_oom_info

[Michal Hocko]

mem_cgroup_print_oom_info uses a static buffer (memcg_name) to store the
name of the cgroup. This is not safe as pointed out by David Rientjes
because memcg oom is locked only for its hierarchy and nothing prevents
another parallel hierarchy to trigger oom as well and overwrite the
already in-use buffer.

This patch introduces oom_info_lock hidden inside mem_cgroup_print_oom_info
which is held throughout the function. It make access to memcg_name safe
and as a bonus it also prevents parallel memcg ooms to interleave their
statistics which would make the printed data hard to analyze otherwise.

Signed-off-by: Michal Hocko <mhocko@suse.cz>
---
 mm/memcontrol.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 28c9221b74ea..c72b03bf9679 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -1647,13 +1647,13 @@ static void move_unlock_mem_cgroup(struct mem_cgroup *memcg,
  */
 void mem_cgroup_print_oom_info(struct mem_cgroup *memcg, struct task_struct *p)
 {
-	struct cgroup *task_cgrp;
-	struct cgroup *mem_cgrp;
 	/*
-	 * Need a buffer in BSS, can't rely on allocations. The code relies
-	 * on the assumption that OOM is serialized for memory controller.
-	 * If this assumption is broken, revisit this code.
+	 * protects memcg_name and makes sure that parallel ooms do not
+	 * interleave
 	 */
+	static DEFINE_SPINLOCK(oom_info_lock);
+	struct cgroup *task_cgrp;
+	struct cgroup *mem_cgrp;
 	static char memcg_name[PATH_MAX];
 	int ret;
 	struct mem_cgroup *iter;
@@ -1662,6 +1662,7 @@ void mem_cgroup_print_oom_info(struct mem_cgroup *memcg, struct task_struct *p)
 	if (!p)
 		return;
 
+	spin_lock(&oom_info_lock);
 	rcu_read_lock();
 
 	mem_cgrp = memcg->css.cgroup;
@@ -1730,6 +1731,7 @@ done:
 
 		pr_cont("\n");
 	}
+	spin_unlock(&oom_info_lock);
 }
 
 /*
-- 
1.8.4.4

--
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, oom: lock mem_cgroup_print_oom_info

[David Rientjes]

On Wed, 11 Dec 2013, Michal Hocko wrote:

> mem_cgroup_print_oom_info uses a static buffer (memcg_name) to store the
> name of the cgroup. This is not safe as pointed out by David Rientjes
> because memcg oom is locked only for its hierarchy and nothing prevents
> another parallel hierarchy to trigger oom as well and overwrite the
> already in-use buffer.
> 
> This patch introduces oom_info_lock hidden inside mem_cgroup_print_oom_info
> which is held throughout the function. It make access to memcg_name safe
> and as a bonus it also prevents parallel memcg ooms to interleave their
> statistics which would make the printed data hard to analyze otherwise.
> 
> Signed-off-by: Michal Hocko <mhocko@suse.cz>

Acked-by: David Rientjes <rientjes@google.com>

> ---
>  mm/memcontrol.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 28c9221b74ea..c72b03bf9679 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -1647,13 +1647,13 @@ static void move_unlock_mem_cgroup(struct mem_cgroup *memcg,
>   */
>  void mem_cgroup_print_oom_info(struct mem_cgroup *memcg, struct task_struct *p)
>  {
> -	struct cgroup *task_cgrp;
> -	struct cgroup *mem_cgrp;
>  	/*
> -	 * Need a buffer in BSS, can't rely on allocations. The code relies
> -	 * on the assumption that OOM is serialized for memory controller.
> -	 * If this assumption is broken, revisit this code.
> +	 * protects memcg_name and makes sure that parallel ooms do not
> +	 * interleave

Parallel memcg oom kills can happen in disjoint memcg hierarchies, this 
just prevents the printing of the statistics from interleaving.  I'm not 
sure if that's clear from this comment.

--
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, oom: lock mem_cgroup_print_oom_info

[Michal Hocko]

On Wed 11-12-13 14:23:18, David Rientjes wrote:
> On Wed, 11 Dec 2013, Michal Hocko wrote:
> 
> > mem_cgroup_print_oom_info uses a static buffer (memcg_name) to store the
> > name of the cgroup. This is not safe as pointed out by David Rientjes
> > because memcg oom is locked only for its hierarchy and nothing prevents
> > another parallel hierarchy to trigger oom as well and overwrite the
> > already in-use buffer.
> > 
> > This patch introduces oom_info_lock hidden inside mem_cgroup_print_oom_info
> > which is held throughout the function. It make access to memcg_name safe
> > and as a bonus it also prevents parallel memcg ooms to interleave their
> > statistics which would make the printed data hard to analyze otherwise.
> > 
> > Signed-off-by: Michal Hocko <mhocko@suse.cz>
> 
> Acked-by: David Rientjes <rientjes@google.com>

Thanks

> 
> > ---
> >  mm/memcontrol.c | 12 +++++++-----
> >  1 file changed, 7 insertions(+), 5 deletions(-)
> > 
> > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > index 28c9221b74ea..c72b03bf9679 100644
> > --- a/mm/memcontrol.c
> > +++ b/mm/memcontrol.c
> > @@ -1647,13 +1647,13 @@ static void move_unlock_mem_cgroup(struct mem_cgroup *memcg,
> >   */
> >  void mem_cgroup_print_oom_info(struct mem_cgroup *memcg, struct task_struct *p)
> >  {
> > -	struct cgroup *task_cgrp;
> > -	struct cgroup *mem_cgrp;
> >  	/*
> > -	 * Need a buffer in BSS, can't rely on allocations. The code relies
> > -	 * on the assumption that OOM is serialized for memory controller.
> > -	 * If this assumption is broken, revisit this code.
> > +	 * protects memcg_name and makes sure that parallel ooms do not
> > +	 * interleave
> 
> Parallel memcg oom kills can happen in disjoint memcg hierarchies, this 
> just prevents the printing of the statistics from interleaving.  I'm not 
> sure if that's clear from this comment.

What about this instead:
	* Protects memcg_name and makes sure that ooms from parallel
	* hierarchies do not interleave.
?
-- 
Michal Hocko
SUSE Labs

--
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, oom: lock mem_cgroup_print_oom_info

[David Rientjes]

On Thu, 12 Dec 2013, Michal Hocko wrote:

> > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > > index 28c9221b74ea..c72b03bf9679 100644
> > > --- a/mm/memcontrol.c
> > > +++ b/mm/memcontrol.c
> > > @@ -1647,13 +1647,13 @@ static void move_unlock_mem_cgroup(struct mem_cgroup *memcg,
> > >   */
> > >  void mem_cgroup_print_oom_info(struct mem_cgroup *memcg, struct task_struct *p)
> > >  {
> > > -	struct cgroup *task_cgrp;
> > > -	struct cgroup *mem_cgrp;
> > >  	/*
> > > -	 * Need a buffer in BSS, can't rely on allocations. The code relies
> > > -	 * on the assumption that OOM is serialized for memory controller.
> > > -	 * If this assumption is broken, revisit this code.
> > > +	 * protects memcg_name and makes sure that parallel ooms do not
> > > +	 * interleave
> > 
> > Parallel memcg oom kills can happen in disjoint memcg hierarchies, this 
> > just prevents the printing of the statistics from interleaving.  I'm not 
> > sure if that's clear from this comment.
> 
> What about this instead:
> 	* Protects memcg_name and makes sure that ooms from parallel
> 	* hierarchies do not interleave.
> ?

I think it would be better to explicitly say that you're referring only to 
the printing here and that we're ensuring it does not interleave in the 
kernel log.

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