Re: [PATCH 1/1] mm_owner: fix cgroup null dereference

[KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>]

On Wed, 20 Aug 2008 13:20:40 +0800
Li Zefan <lizf@cn.fujitsu.com> wrote:

> Balbir Singh wote:
> > * Jiri Slaby <jirislaby@gmail.com> [2008-08-14 22:16:53]:
> > 
> >> Hi,
> >>
> >> found this in mmotm, a fix for
> >> mm-owner-fix-race-between-swap-and-exit.patch
> >>
> > 
> > Does the patch below fix your problem, it's against mmotm 19th August
> > 2008.
> > 
> 
> I just triggered this bug. I think you also need the following change:
> 
> make memrlimit_cgroup_mm_owner_changed() aware that old_cgrp can be NULL,
> and note we can't call memrlimit_cgroup_from_cgrp() with NULL argument.
> 
> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
> 

Thank you, it seems you fixed a problem which I met in my test.

(*) When I migrate task under cpuset repeatedly, I can see Oops easily.

Tested-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>





> diff --git a/mm/memrlimitcgroup.c.orig b/mm/memrlimitcgroup.c
> index f7536dc..6559470 100644
> --- a/mm/memrlimitcgroup.c.orig
> +++ b/mm/memrlimitcgroup.c
> @@ -249,17 +249,23 @@ static void memrlimit_cgroup_mm_owner_changed(struct cgroup_subsys *ss,
>  	struct mm_struct *mm = get_task_mm(p);
>  
>  	BUG_ON(!mm);
> -	memrcg = memrlimit_cgroup_from_cgrp(cgrp);
> -	old_memrcg = memrlimit_cgroup_from_cgrp(old_cgrp);
>  
>  	/*
>  	 * If we don't have a new cgroup, we just uncharge from the old one.
>  	 * It means that the task is going away
>  	 */
> -	if (memrcg &&
> -	    res_counter_charge(&memrcg->as_res, (mm->total_vm << PAGE_SHIFT)))
> -		goto out;
> -	res_counter_uncharge(&old_memrcg->as_res, (mm->total_vm << PAGE_SHIFT));
> +	if (cgrp) {
> +		memrcg = memrlimit_cgroup_from_cgrp(cgrp);
> +		if (res_counter_charge(&memrcg->as_res,
> +				       mm->total_vm << PAGE_SHIFT))
> +			goto out;
> +	}
> +
> +	if (old_cgrp) {
> +		old_memrcg = memrlimit_cgroup_from_cgrp(old_cgrp);
> +		res_counter_uncharge(&old_memrcg->as_res,
> +				     mm->total_vm <<PAGE_SHIFT);
> +	}
>  out:
>  	mmput(mm);
>  }
> 
> 
> >  
> > Reported-by: jirislaby@gmail.com
> > 
> > Jiri reported a problem and saw an oops when the memrlimit-fix-race-with-swap
> > patch is applied. He sent his patch on top to fix the problem, but ran into
> > another issue. The root cause of the problem is that we are not suppose
> > to call task_cgroup on NULL tasks. This patch reverts Jiri's patch and
> > does not call task_cgroup if the passed task_struct (old) is NULL.
> > 
> > 
> > Signed-off-by: Balbir Singh <balbir@linux.vnet.ibm.com>
> > ---
> > 
> >  kernel/cgroup.c |    5 +++--
> >  kernel/exit.c   |    2 +-
> >  2 files changed, 4 insertions(+), 3 deletions(-)
> > 
> > diff -puN kernel/exit.c~memrlimit-fix-race-with-swap-oops kernel/exit.c
> > --- linux-2.6.27-rc3/kernel/exit.c~memrlimit-fix-race-with-swap-oops	2008-08-19 18:50:39.000000000 +0530
> > +++ linux-2.6.27-rc3-balbir/kernel/exit.c	2008-08-19 18:51:05.000000000 +0530
> > @@ -641,8 +641,8 @@ retry:
> >  	 * the callback and take action
> >  	 */
> >  	down_write(&mm->mmap_sem);
> > -	cgroup_mm_owner_callbacks(mm->owner, NULL);
> >  	mm->owner = NULL;
> > +	cgroup_mm_owner_callbacks(mm->owner, NULL);
> >  	up_write(&mm->mmap_sem);
> >  	return;
> >  
> > diff -puN kernel/cgroup.c~memrlimit-fix-race-with-swap-oops kernel/cgroup.c
> > --- linux-2.6.27-rc3/kernel/cgroup.c~memrlimit-fix-race-with-swap-oops	2008-08-19 18:50:39.000000000 +0530
> > +++ linux-2.6.27-rc3-balbir/kernel/cgroup.c	2008-08-19 18:55:38.000000000 +0530
> > @@ -2743,13 +2743,14 @@ void cgroup_fork_callbacks(struct task_s
> >   */
> >  void cgroup_mm_owner_callbacks(struct task_struct *old, struct task_struct *new)
> >  {
> > -	struct cgroup *oldcgrp, *newcgrp = NULL;
> > +	struct cgroup *oldcgrp = NULL, *newcgrp = NULL;
> >  
> >  	if (need_mm_owner_callback) {
> >  		int i;
> >  		for (i = 0; i < CGROUP_SUBSYS_COUNT; i++) {
> >  			struct cgroup_subsys *ss = subsys[i];
> > -			oldcgrp = task_cgroup(old, ss->subsys_id);
> > +			if (old)
> > +				oldcgrp = task_cgroup(old, ss->subsys_id);
> >  			if (new)
> >  				newcgrp = task_cgroup(new, ss->subsys_id);
> >  			if (oldcgrp == newcgrp)
> > diff -puN mm/memrlimitcgroup.c~memrlimit-fix-race-with-swap-oops mm/memrlimitcgroup.c
> > _
> > 
> 
> --
> 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>