[PATCH] memcg: Fix NULL pointer deref in task_in_mem_cgroup()

[PATCH] memcg: Fix NULL pointer deref in task_in_mem_cgroup()

[Jan Kara]

'curr' pointer in task_in_mem_cgroup() can be NULL when we race with
somebody clearing task->mm. Check for it before dereferencing the
pointer.

Coverity-id: 1198369
Signed-off-by: Jan Kara <jack@suse.cz>
---
 mm/memcontrol.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 23976fd885fd..18ab127a0767 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -1469,7 +1469,8 @@ bool task_in_mem_cgroup(struct task_struct *task,
 	 * hierarchy(even if use_hierarchy is disabled in "memcg").
 	 */
 	ret = mem_cgroup_same_or_subtree(memcg, curr);
-	css_put(&curr->css);
+	if (curr)
+		css_put(&curr->css);
 	return ret;
 }
 
-- 
1.8.1.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: Fix NULL pointer deref in task_in_mem_cgroup()

[Johannes Weiner]

On Thu, Oct 23, 2014 at 06:47:45PM +0200, Jan Kara wrote:
> 'curr' pointer in task_in_mem_cgroup() can be NULL when we race with
> somebody clearing task->mm. Check for it before dereferencing the
> pointer.

If task->mm is already NULL, we fall back to mem_cgroup_from_task(),
which definitely returns a memcg unless you pass NULL in there.  So I
don't see how that could happen, and the NULL checks in the fallback
branch as well as in __mem_cgroup_same_or_subtree seem bogus to me.

--
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: Fix NULL pointer deref in task_in_mem_cgroup()

[Jan Kara]

On Thu 23-10-14 14:19:29, Johannes Weiner wrote:
> On Thu, Oct 23, 2014 at 06:47:45PM +0200, Jan Kara wrote:
> > 'curr' pointer in task_in_mem_cgroup() can be NULL when we race with
> > somebody clearing task->mm. Check for it before dereferencing the
> > pointer.
> 
> If task->mm is already NULL, we fall back to mem_cgroup_from_task(),
> which definitely returns a memcg unless you pass NULL in there.  So I
> don't see how that could happen, and the NULL checks in the fallback
> branch as well as in __mem_cgroup_same_or_subtree seem bogus to me.
  OK, I admittedly don't understand that code much. I was just wondering
that we check 'curr' for being NULL in all the places except for that one
which looked suspicious... If curr cannot be NULL, then we should just
remove those checks I assume.

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

--
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: Fix NULL pointer deref in task_in_mem_cgroup()

[Johannes Weiner]

On Thu, Oct 23, 2014 at 08:34:35PM +0200, Jan Kara wrote:
> On Thu 23-10-14 14:19:29, Johannes Weiner wrote:
> > On Thu, Oct 23, 2014 at 06:47:45PM +0200, Jan Kara wrote:
> > > 'curr' pointer in task_in_mem_cgroup() can be NULL when we race with
> > > somebody clearing task->mm. Check for it before dereferencing the
> > > pointer.
> > 
> > If task->mm is already NULL, we fall back to mem_cgroup_from_task(),
> > which definitely returns a memcg unless you pass NULL in there.  So I
> > don't see how that could happen, and the NULL checks in the fallback
> > branch as well as in __mem_cgroup_same_or_subtree seem bogus to me.
>   OK, I admittedly don't understand that code much. I was just wondering
> that we check 'curr' for being NULL in all the places except for that one
> which looked suspicious... If curr cannot be NULL, then we should just
> remove those checks I assume.

Agreed.  They make the code quite hard to understand and change
because all callchains need to be verified up the stack.

Thanks for the nudge, I'm going to remove the bogus ones.

--
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: Fix NULL pointer deref in task_in_mem_cgroup()

[Michal Hocko]

On Thu 23-10-14 14:19:29, Johannes Weiner wrote:
> On Thu, Oct 23, 2014 at 06:47:45PM +0200, Jan Kara wrote:
> > 'curr' pointer in task_in_mem_cgroup() can be NULL when we race with
> > somebody clearing task->mm. Check for it before dereferencing the
> > pointer.
> 
> If task->mm is already NULL, we fall back to mem_cgroup_from_task(),
> which definitely returns a memcg unless you pass NULL in there.  So I
> don't see how that could happen, and the NULL checks in the fallback
> branch as well as in __mem_cgroup_same_or_subtree seem bogus to me.

It came from 3a981f482cc2 (memcg: fix use_hierarchy css_is_ancestor oops
regression). I do not see mem_cgroup_same_or_subtree called from
page_referenced path so it is probably gone.
task_in_mem_cgroup is just confused because curr can never be NULL as
the task is never NULL.
---

Re: [PATCH] memcg: Fix NULL pointer deref in task_in_mem_cgroup()

[Johannes Weiner]

On Fri, Oct 24, 2014 at 10:58:07AM +0200, Michal Hocko wrote:
> On Thu 23-10-14 14:19:29, Johannes Weiner wrote:
> > On Thu, Oct 23, 2014 at 06:47:45PM +0200, Jan Kara wrote:
> > > 'curr' pointer in task_in_mem_cgroup() can be NULL when we race with
> > > somebody clearing task->mm. Check for it before dereferencing the
> > > pointer.
> > 
> > If task->mm is already NULL, we fall back to mem_cgroup_from_task(),
> > which definitely returns a memcg unless you pass NULL in there.  So I
> > don't see how that could happen, and the NULL checks in the fallback
> > branch as well as in __mem_cgroup_same_or_subtree seem bogus to me.
> 
> It came from 3a981f482cc2 (memcg: fix use_hierarchy css_is_ancestor oops
> regression). I do not see mem_cgroup_same_or_subtree called from
> page_referenced path so it is probably gone.

It's still there in invalid_page_referenced_vma().  And it can still
pass NULL.

> task_in_mem_cgroup is just confused because curr can never be NULL as
> the task is never NULL.

That's correct.

My patches to clean all this up have been stress-tested over night, I
will send them out in a jiffy.

Thanks

--
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: Fix NULL pointer deref in task_in_mem_cgroup()

[Michal Hocko]

On Fri 24-10-14 09:36:05, Johannes Weiner wrote:
> On Fri, Oct 24, 2014 at 10:58:07AM +0200, Michal Hocko wrote:
> > On Thu 23-10-14 14:19:29, Johannes Weiner wrote:
> > > On Thu, Oct 23, 2014 at 06:47:45PM +0200, Jan Kara wrote:
> > > > 'curr' pointer in task_in_mem_cgroup() can be NULL when we race with
> > > > somebody clearing task->mm. Check for it before dereferencing the
> > > > pointer.
> > > 
> > > If task->mm is already NULL, we fall back to mem_cgroup_from_task(),
> > > which definitely returns a memcg unless you pass NULL in there.  So I
> > > don't see how that could happen, and the NULL checks in the fallback
> > > branch as well as in __mem_cgroup_same_or_subtree seem bogus to me.
> > 
> > It came from 3a981f482cc2 (memcg: fix use_hierarchy css_is_ancestor oops
> > regression). I do not see mem_cgroup_same_or_subtree called from
> > page_referenced path so it is probably gone.
> 
> It's still there in invalid_page_referenced_vma().  And it can still
> pass NULL.

Ohh, my bad. I wasn't careful enough to check mm_match_cgroup.
 
> > task_in_mem_cgroup is just confused because curr can never be NULL as
> > the task is never NULL.
> 
> That's correct.
> 
> My patches to clean all this up have been stress-tested over night, I
> will send them out in a jiffy.

Will wait for your patch.
-- 
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>