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

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

[Daisuke Nishimura]

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.


Signed-off-by: Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp>

---
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, 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][mmotm]memcg: handle null dereference of mm->owner

[KAMEZAWA Hiroyuki]

On Fri, 5 Sep 2008 16:50:17 +0900
Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp> 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@mxp.nes.nec.co.jp>
> 
> ---
> 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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

--
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][mmotm]memcg: handle null dereference of mm->owner

[Balbir Singh]

KAMEZAWA Hiroyuki wrote:
> On Fri, 5 Sep 2008 16:50:17 +0900
> Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp> 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.
> 

Thanks, Daisuke

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


For the following reasons, I had initially designed it to be that way because

1. There is no concept of a thread maintaining or managing its memory
independently of others
2. If we ever support full migration, it is easier to do so with the thread
group leader owning the memory, rather than figuring out what to do everytime a
task changed a cgroup.
3. A task with appropriate permissions can spread itself across cgroups and hog
memory


-- 
	Balbir

--
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][mmotm]memcg: handle null dereference of mm->owner

[Paul Menage]

On Fri, Sep 5, 2008 at 1:40 AM, KAMEZAWA Hiroyuki
<kamezawa.hiroyu@jp.fujitsu.com> wrote:
>
> 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 ?

Because lots of mm operations take place in a context where we don't
have a thread pointer, and hence no cgroup.

> - Why we can't disallow per-thread cgroup under memcg ?)

We can, but that's orthogonal - we still need to be able to get to
some thread (or a pointer directly in the mm to the cgroup, but with
multiple cgroup subsystems popping up that needed such a pointer, it
seems cleaner to have the owner pointer rather than adding multiple
separate cgroup subsystem pointers to mm.

Paul

--
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][mmotm]memcg: handle null dereference of mm->owner

[Balbir Singh]

Paul Menage wrote:
> On Fri, Sep 5, 2008 at 1:40 AM, KAMEZAWA Hiroyuki
> <kamezawa.hiroyu@jp.fujitsu.com> wrote:
>> 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 ?
> 
> Because lots of mm operations take place in a context where we don't
> have a thread pointer, and hence no cgroup.
> 

Right, Thanks! Allocating memory is not that big a problem (we usually know the
context), while freeing memory, we can't assume that current is freeing it

-- 
	Balbir

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