Re: [PATCH][mmotm]memcg: handle null dereference of mm->owner

[KAMEZAWA Hiroyuki]

On Fri, 5 Sep 2008 16:50:17 +0900
Daisuke Nishimura <nishimura@xxxxxxxxxxxxxxxxx> wrote:

> Hi.
> 
> mm_update_next_owner() may clear mm->owner to NULL
> if it races with swapoff, page migration, etc.
> (This behavior was introduced by mm-owner-fix-race-between-swap-and-exit.patch.)
> 
> But memcg doesn't take account of this situation, and causes:
> 
>   BUG: unable to handle kernel NULL pointer dereference at 0000000000000630
> 
> This fixes it.
> 
Thank you for catching this.

BTW, I have a question to Balbir and Paul. (I'm sorry I missed the discussion.)
Recently I wonder why we need MM_OWNER.

- What's bad with thread's cgroup ?
- Why we can't disallow per-thread cgroup under memcg ?)

Thanks,
-Kame


> 
> Signed-off-by: Daisuke Nishimura <nishimura@xxxxxxxxxxxxxxxxx>
> 
> ---
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 2979d22..ec2c16b 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -244,6 +244,14 @@ static struct mem_cgroup *mem_cgroup_from_cont(struct cgroup *cont)
>  
>  struct mem_cgroup *mem_cgroup_from_task(struct task_struct *p)
>  {
> +	/*
> +	 * mm_update_next_owner() may clear mm->owner to NULL
> +	 * if it races with swapoff, page migration, etc.
> +	 * So this can be called with p == NULL.
> +	 */
> +	if (unlikely(!p))
> +		return NULL;
> +
>  	return container_of(task_subsys_state(p, mem_cgroup_subsys_id),
>  				struct mem_cgroup, css);
>  }
> @@ -534,6 +542,11 @@ static int mem_cgroup_charge_common(struct page *page, struct mm_struct *mm,
>  	if (likely(!memcg)) {
>  		rcu_read_lock();
>  		mem = mem_cgroup_from_task(rcu_dereference(mm->owner));
> +		if (unlikely(!mem)) {
> +			rcu_read_unlock();
> +			kmem_cache_free(page_cgroup_cache, pc);
> +			return 0;
> +		}
>  		/*
>  		 * For every charge from the cgroup, increment reference count
>  		 */
> @@ -790,6 +803,10 @@ int mem_cgroup_shrink_usage(struct mm_struct *mm, gfp_t gfp_mask)
>  
>  	rcu_read_lock();
>  	mem = mem_cgroup_from_task(rcu_dereference(mm->owner));
> +	if (unlikely(!mem)) {
> +		rcu_read_unlock();
> +		return 0;
> +	}
>  	css_get(&mem->css);
>  	rcu_read_unlock();
>  
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/