+ cgroups-more-safe-tasklist-locking-in-cgroup_attach_proc.patch added to -mm tree

+ cgroups-more-safe-tasklist-locking-in-cgroup_attach_proc.patch added to -mm tree

[akpm]

The patch titled
     cgroups: more safe tasklist locking in cgroup_attach_proc
has been added to the -mm tree.  Its filename is
     cgroups-more-safe-tasklist-locking-in-cgroup_attach_proc.patch

Before you just go and hit "reply", please:
   a) Consider who else should be cc'ed
   b) Prefer to cc a suitable mailing list as well
   c) Ideally: find the original patch on the mailing list and do a
      reply-to-all to that, adding suitable additional cc's

*** Remember to use Documentation/SubmitChecklist when testing your code ***

See http://userweb.kernel.org/~akpm/stuff/added-to-mm.txt to find
out what to do about this

The current -mm tree may be found at http://userweb.kernel.org/~akpm/mmotm/

------------------------------------------------------
Subject: cgroups: more safe tasklist locking in cgroup_attach_proc
From: Ben Blum <bblum@andrew.cmu.edu>

Fix unstable tasklist locking in cgroup_attach_proc.

According to this thread - https://lkml.org/lkml/2011/7/27/243 - RCU is
not sufficient to guarantee the tasklist is stable w.r.t.  de_thread and
exit.  Taking tasklist_lock for reading, instead of rcu_read_lock, ensures
proper exclusion.

Signed-off-by: Ben Blum <bblum@andrew.cmu.edu>
Acked-by: Paul Menage <paul@paulmenage.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Cc: Neil Brown <neilb@suse.de>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 kernel/cgroup.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff -puN kernel/cgroup.c~cgroups-more-safe-tasklist-locking-in-cgroup_attach_proc kernel/cgroup.c
--- a/kernel/cgroup.c~cgroups-more-safe-tasklist-locking-in-cgroup_attach_proc
+++ a/kernel/cgroup.c
@@ -2027,7 +2027,7 @@ int cgroup_attach_proc(struct cgroup *cg
 		goto out_free_group_list;
 
 	/* prevent changes to the threadgroup list while we take a snapshot. */
-	rcu_read_lock();
+	read_lock(&tasklist_lock);
 	if (!thread_group_leader(leader)) {
 		/*
 		 * a race with de_thread from another thread's exec() may strip
@@ -2036,7 +2036,7 @@ int cgroup_attach_proc(struct cgroup *cg
 		 * throw this task away and try again (from cgroup_procs_write);
 		 * this is "double-double-toil-and-trouble-check locking".
 		 */
-		rcu_read_unlock();
+		read_unlock(&tasklist_lock);
 		retval = -EAGAIN;
 		goto out_free_group_list;
 	}
@@ -2057,7 +2057,7 @@ int cgroup_attach_proc(struct cgroup *cg
 	} while_each_thread(leader, tsk);
 	/* remember the number of threads in the array for later. */
 	group_size = i;
-	rcu_read_unlock();
+	read_unlock(&tasklist_lock);
 
 	/*
 	 * step 1: check that we can legitimately attach to the cgroup.
_

Patches currently in -mm which might be from bblum@andrew.cmu.edu are

cgroups-more-safe-tasklist-locking-in-cgroup_attach_proc.patch
cgroups-fix-ordering-of-calls-in-cgroup_attach_proc.patch

Re: + cgroups-more-safe-tasklist-locking-in-cgroup_attach_proc.patch added to -mm tree

[Oleg Nesterov]

> From: Ben Blum <bblum@andrew.cmu.edu>
>
> Fix unstable tasklist locking in cgroup_attach_proc.
>
> According to this thread - https://lkml.org/lkml/2011/7/27/243 - RCU is
> not sufficient to guarantee the tasklist is stable w.r.t.  de_thread and
> exit.  Taking tasklist_lock for reading, instead of rcu_read_lock, ensures
> proper exclusion.

I still think we should avoid the global lock.

In any case, with tasklist or siglock,

> -	rcu_read_lock();
> +	read_lock(&tasklist_lock);
>  	if (!thread_group_leader(leader)) {
>  		/*
>  		 * a race with de_thread from another thread's exec() may strip
> @@ -2036,7 +2036,7 @@ int cgroup_attach_proc(struct cgroup *cg
>  		 * throw this task away and try again (from cgroup_procs_write);
>  		 * this is "double-double-toil-and-trouble-check locking".
>  		 */
> -		rcu_read_unlock();
> +		read_unlock(&tasklist_lock);
>  		retval = -EAGAIN;

this check+comment becomes completely pointless and imho very confusing.

Oleg.

Re: + cgroups-more-safe-tasklist-locking-in-cgroup_attach_proc.patch added to -mm tree

[Oleg Nesterov]

Forgot to mention, sorry...

That said, I believe the patch is correct and should fix the problem.

On 09/02, Oleg Nesterov wrote:
>
> > From: Ben Blum <bblum@andrew.cmu.edu>
> >
> > Fix unstable tasklist locking in cgroup_attach_proc.
> >
> > According to this thread - https://lkml.org/lkml/2011/7/27/243 - RCU is
> > not sufficient to guarantee the tasklist is stable w.r.t.  de_thread and
> > exit.  Taking tasklist_lock for reading, instead of rcu_read_lock, ensures
> > proper exclusion.
>
> I still think we should avoid the global lock.
>
> In any case, with tasklist or siglock,
>
> > -	rcu_read_lock();
> > +	read_lock(&tasklist_lock);
> >  	if (!thread_group_leader(leader)) {
> >  		/*
> >  		 * a race with de_thread from another thread's exec() may strip
> > @@ -2036,7 +2036,7 @@ int cgroup_attach_proc(struct cgroup *cg
> >  		 * throw this task away and try again (from cgroup_procs_write);
> >  		 * this is "double-double-toil-and-trouble-check locking".
> >  		 */
> > -		rcu_read_unlock();
> > +		read_unlock(&tasklist_lock);
> >  		retval = -EAGAIN;
>
> this check+comment becomes completely pointless and imho very confusing.
>
> Oleg.

Re: + cgroups-more-safe-tasklist-locking-in-cgroup_attach_proc.patch added to -mm tree

[Ben Blum]

On Fri, Sep 02, 2011 at 04:00:15PM +0200, Oleg Nesterov wrote:
> Forgot to mention, sorry...
> 
> That said, I believe the patch is correct and should fix the problem.

Thanks!

But I don't think the check becomes pointless? If a sub-thread execs
right before read_lock(&tasklist_lock) (but after the find_task_by_vpid
in attach_task_by_pid), that causes the case that the comment refers to.

-- Ben

> 
> On 09/02, Oleg Nesterov wrote:
> >
> > > From: Ben Blum <bblum@andrew.cmu.edu>
> > >
> > > Fix unstable tasklist locking in cgroup_attach_proc.
> > >
> > > According to this thread - https://lkml.org/lkml/2011/7/27/243 - RCU is
> > > not sufficient to guarantee the tasklist is stable w.r.t.  de_thread and
> > > exit.  Taking tasklist_lock for reading, instead of rcu_read_lock, ensures
> > > proper exclusion.
> >
> > I still think we should avoid the global lock.
> >
> > In any case, with tasklist or siglock,
> >
> > > -	rcu_read_lock();
> > > +	read_lock(&tasklist_lock);
> > >  	if (!thread_group_leader(leader)) {
> > >  		/*
> > >  		 * a race with de_thread from another thread's exec() may strip
> > > @@ -2036,7 +2036,7 @@ int cgroup_attach_proc(struct cgroup *cg
> > >  		 * throw this task away and try again (from cgroup_procs_write);
> > >  		 * this is "double-double-toil-and-trouble-check locking".
> > >  		 */
> > > -		rcu_read_unlock();
> > > +		read_unlock(&tasklist_lock);
> > >  		retval = -EAGAIN;
> >
> > this check+comment becomes completely pointless and imho very confusing.
> >
> > Oleg.
> 
> 

Re: + cgroups-more-safe-tasklist-locking-in-cgroup_attach_proc.patch added to -mm tree

[Oleg Nesterov]

On 09/02, Ben Blum wrote:
>
> On Fri, Sep 02, 2011 at 04:00:15PM +0200, Oleg Nesterov wrote:
> > Forgot to mention, sorry...
> >
> > That said, I believe the patch is correct and should fix the problem.
>
> Thanks!
>
> But I don't think the check becomes pointless? If a sub-thread execs
> right before read_lock(&tasklist_lock) (but after the find_task_by_vpid
> in attach_task_by_pid), that causes the case that the comment refers to.

How so? The comment says:

	* a race with de_thread from another thread's exec() may strip
	* us of our leadership, making while_each_thread unsafe

This is not true.

And. Given that ->group_leader can be changed right after we drop tasklist
this check is pointless. Yes, it can detect the case when this task_struct
has nothing to do with this process sometimes, but not in general. (This
connects to other problems I mentioned).

IOW, personally I think it would be better to update the patch. But I
won't insist.

Oleg.

Re: + cgroups-more-safe-tasklist-locking-in-cgroup_attach_proc.patch added to -mm tree

[Ben Blum]

On Fri, Sep 02, 2011 at 05:55:34PM +0200, Oleg Nesterov wrote:
> On 09/02, Ben Blum wrote:
> >
> > On Fri, Sep 02, 2011 at 04:00:15PM +0200, Oleg Nesterov wrote:
> > > Forgot to mention, sorry...
> > >
> > > That said, I believe the patch is correct and should fix the problem.
> >
> > Thanks!
> >
> > But I don't think the check becomes pointless? If a sub-thread execs
> > right before read_lock(&tasklist_lock) (but after the find_task_by_vpid
> > in attach_task_by_pid), that causes the case that the comment refers to.
> 
> How so? The comment says:
> 
> 	* a race with de_thread from another thread's exec() may strip
> 	* us of our leadership, making while_each_thread unsafe
> 
> This is not true.

Sorry, the comment is unclear. The reason I think this is necessary is
if de_thread happens, the leader would fall off the thread-group list:

de_thread()
=> release_task(leader)
=> __exit_signal(leader)
=> __unhash_process(leader, false)
=> list_del_rcu(&leader->thread_group)

which is the same list that while_each_thread() iterates over.

and this looks like an unconditionally taken path?

> 
> And. Given that ->group_leader can be changed right after we drop tasklist
> this check is pointless. Yes, it can detect the case when this task_struct
> has nothing to do with this process sometimes, but not in general. (This
> connects to other problems I mentioned).

I agree there is a problem later with the ss->attach(leader) calls.

If the above reasoning is right, though, it's necessary here, and also
guarantees that that the later iteration (in cgroup_attach_proc's "step
3") accurately reflects all threads in the group.

Thanks,
Ben

> 
> IOW, personally I think it would be better to update the patch. But I
> won't insist.
> 
> Oleg.
> 
> 

Re: + cgroups-more-safe-tasklist-locking-in-cgroup_attach_proc.patch added to -mm tree

[Oleg Nesterov]

On 09/07, Ben Blum wrote:
>
> On Fri, Sep 02, 2011 at 05:55:34PM +0200, Oleg Nesterov wrote:
> > On 09/02, Ben Blum wrote:
> > >
> > > But I don't think the check becomes pointless? If a sub-thread execs
> > > right before read_lock(&tasklist_lock) (but after the find_task_by_vpid
> > > in attach_task_by_pid), that causes the case that the comment refers to.
> >
> > How so? The comment says:
> >
> > 	* a race with de_thread from another thread's exec() may strip
> > 	* us of our leadership, making while_each_thread unsafe
> >
> > This is not true.
>
> Sorry, the comment is unclear.

No, the comment is clear. In fact it was me who pointed out we can't
do while_each_thread() blindly. And now I am tried to confuse you ;)

So, sorry for noise, and thanks for correcting me. Somehow I forgot
this is not safe even under tasklist.

Partly I was confused because I was thinking about the patch I suggested,
if we use ->siglock we are safe. If lock_task_sighand(task) succeeds,
this task should be on list.

Anyway, I was wrong, sorry.

Oleg.

--- x/kernel/cgroup.c
+++ x/kernel/cgroup.c
@@ -2000,6 +2000,7 @@ int cgroup_attach_proc(struct cgroup *cg
 	/* threadgroup list cursor and array */
 	struct task_struct *tsk;
 	struct flex_array *group;
+	unsigned long flags;
 	/*
 	 * we need to make sure we have css_sets for all the tasks we're
 	 * going to move -before- we actually start moving them, so that in
@@ -2027,19 +2028,10 @@ int cgroup_attach_proc(struct cgroup *cg
 		goto out_free_group_list;
 
 	/* prevent changes to the threadgroup list while we take a snapshot. */
-	rcu_read_lock();
-	if (!thread_group_leader(leader)) {
-		/*
-		 * a race with de_thread from another thread's exec() may strip
-		 * us of our leadership, making while_each_thread unsafe to use
-		 * on this task. if this happens, there is no choice but to
-		 * throw this task away and try again (from cgroup_procs_write);
-		 * this is "double-double-toil-and-trouble-check locking".
-		 */
-		rcu_read_unlock();
-		retval = -EAGAIN;
+	retval = -EAGAIN;
+	if (!lock_task_sighand(leader, &flags))
 		goto out_free_group_list;
-	}
+
 	/* take a reference on each task in the group to go in the array. */
 	tsk = leader;
 	i = 0;
@@ -2055,9 +2047,9 @@ int cgroup_attach_proc(struct cgroup *cg
 		BUG_ON(retval != 0);
 		i++;
 	} while_each_thread(leader, tsk);
+	unlock_task_sighand(leader, &flags);
 	/* remember the number of threads in the array for later. */
 	group_size = i;
-	rcu_read_unlock();
 
 	/*
 	 * step 1: check that we can legitimately attach to the cgroup.

Re: + cgroups-more-safe-tasklist-locking-in-cgroup_attach_proc.patch added to -mm tree

[Ben Blum]

On Thu, Sep 08, 2011 at 07:35:59PM +0200, Oleg Nesterov wrote:
> On 09/07, Ben Blum wrote:
> >
> > On Fri, Sep 02, 2011 at 05:55:34PM +0200, Oleg Nesterov wrote:
> > > On 09/02, Ben Blum wrote:
> > > >
> > > > But I don't think the check becomes pointless? If a sub-thread execs
> > > > right before read_lock(&tasklist_lock) (but after the find_task_by_vpid
> > > > in attach_task_by_pid), that causes the case that the comment refers to.
> > >
> > > How so? The comment says:
> > >
> > > 	* a race with de_thread from another thread's exec() may strip
> > > 	* us of our leadership, making while_each_thread unsafe
> > >
> > > This is not true.
> >
> > Sorry, the comment is unclear.
> 
> No, the comment is clear. In fact it was me who pointed out we can't
> do while_each_thread() blindly. And now I am tried to confuse you ;)
> 
> So, sorry for noise, and thanks for correcting me. Somehow I forgot
> this is not safe even under tasklist.
> 
> Partly I was confused because I was thinking about the patch I suggested,
> if we use ->siglock we are safe. If lock_task_sighand(task) succeeds,
> this task should be on list.
> 
> Anyway, I was wrong, sorry.
> 
> Oleg.

All right, no problem.

As for the patch below (which is the same as it was last time?): did you
mean for Andrew to replace the old tasklist_lock patch with this one, or
should one of us rewrite this against it? either way, it should have
something like the comment I proposed in the first thread.

Thanks,
Ben

> 
> --- x/kernel/cgroup.c
> +++ x/kernel/cgroup.c
> @@ -2000,6 +2000,7 @@ int cgroup_attach_proc(struct cgroup *cg
>  	/* threadgroup list cursor and array */
>  	struct task_struct *tsk;
>  	struct flex_array *group;
> +	unsigned long flags;
>  	/*
>  	 * we need to make sure we have css_sets for all the tasks we're
>  	 * going to move -before- we actually start moving them, so that in
> @@ -2027,19 +2028,10 @@ int cgroup_attach_proc(struct cgroup *cg
>  		goto out_free_group_list;
>  
>  	/* prevent changes to the threadgroup list while we take a snapshot. */
> -	rcu_read_lock();
> -	if (!thread_group_leader(leader)) {
> -		/*
> -		 * a race with de_thread from another thread's exec() may strip
> -		 * us of our leadership, making while_each_thread unsafe to use
> -		 * on this task. if this happens, there is no choice but to
> -		 * throw this task away and try again (from cgroup_procs_write);
> -		 * this is "double-double-toil-and-trouble-check locking".
> -		 */
> -		rcu_read_unlock();
> -		retval = -EAGAIN;
> +	retval = -EAGAIN;
> +	if (!lock_task_sighand(leader, &flags))
>  		goto out_free_group_list;
> -	}
> +
>  	/* take a reference on each task in the group to go in the array. */
>  	tsk = leader;
>  	i = 0;
> @@ -2055,9 +2047,9 @@ int cgroup_attach_proc(struct cgroup *cg
>  		BUG_ON(retval != 0);
>  		i++;
>  	} while_each_thread(leader, tsk);
> +	unlock_task_sighand(leader, &flags);
>  	/* remember the number of threads in the array for later. */
>  	group_size = i;
> -	rcu_read_unlock();
>  
>  	/*
>  	 * step 1: check that we can legitimately attach to the cgroup.
> 
> 

Re: + cgroups-more-safe-tasklist-locking-in-cgroup_attach_proc.patch added to -mm tree

[Oleg Nesterov]

On 09/08, Ben Blum wrote:
>
> As for the patch below (which is the same as it was last time?):

It is the same, yes, I simply copied it from my old email. BTW, it
wasn't tested at all, even compiled.

> did you
> mean for Andrew to replace the old tasklist_lock patch with this one, or
> should one of us rewrite this against it?

This is up to you. And just in case, please feel free to do nothing and
keep the current fix.

> either way, it should have
> something like the comment I proposed in the first thread.

Confused... Aha, I missed another email from you. You mean

	/* If the leader exits, its links on the thread_group list become
	 * invalid. One way this can happen is if a sub-thread does exec() when
	 * de_thread() calls release_task(leader) (and leader->sighand gets set
	 * to NULL, in which case lock_task_sighand will fail). Since in that
	 * case the threadgroup is still around, cgroup_procs_write should try
	 * again (finding the new leader), which EAGAIN indicates here. This is
	 * "double-double-toil-and-trouble-check locking". */

Agreed.





Off-topic question... Looking at this code, it seems that
attach_task_by_pid(zombie_pid, threadgroup => true) returns 0.
single-task-only case fails with -ESRCH in this case. I am not
saying this is wrong, just this looks a bit strange (unless I
misread the code).

Oleg.

Re: + cgroups-more-safe-tasklist-locking-in-cgroup_attach_proc.patch added to -mm tree

[Ben Blum]

On Thu, Sep 08, 2011 at 11:31:30PM +0200, Oleg Nesterov wrote:
> On 09/08, Ben Blum wrote:
> >
> > As for the patch below (which is the same as it was last time?):
> 
> It is the same, yes, I simply copied it from my old email. BTW, it
> wasn't tested at all, even compiled.

Testing is recommended ;)

> 
> > did you
> > mean for Andrew to replace the old tasklist_lock patch with this one, or
> > should one of us rewrite this against it?
> 
> This is up to you. And just in case, please feel free to do nothing and
> keep the current fix.

No, I do like the sighand method better now that I've thought about it.
The way it subsumes the check for consistency of the leader's links is
rather nice (much as it still requires deep understanding of de_thread).

If you polished this patch off, I'd be happier, since I have a lot else
on my plate.

> 
> > either way, it should have
> > something like the comment I proposed in the first thread.
> 
> Confused... Aha, I missed another email from you. You mean
> 
> 	/* If the leader exits, its links on the thread_group list become
> 	 * invalid. One way this can happen is if a sub-thread does exec() when
> 	 * de_thread() calls release_task(leader) (and leader->sighand gets set
> 	 * to NULL, in which case lock_task_sighand will fail). Since in that
> 	 * case the threadgroup is still around, cgroup_procs_write should try
> 	 * again (finding the new leader), which EAGAIN indicates here. This is
> 	 * "double-double-toil-and-trouble-check locking". */
> 
> Agreed.
> 
> 
> 
> 
> 
> Off-topic question... Looking at this code, it seems that
> attach_task_by_pid(zombie_pid, threadgroup => true) returns 0.
> single-task-only case fails with -ESRCH in this case. I am not
> saying this is wrong, just this looks a bit strange (unless I
> misread the code).

yeah, this is a side-effect of cgroup_attach_proc continuing to iterate
in case any one of the sub-threads be still alive. you could keep track
of whether all threads were zombies and return -ESRCH then, but it
shouldn't matter, since the user-facing behaviour is indistinguishable
from if the user had sent the command just before the task turned zombie
but while it was still about to exit.

Thanks,
Ben

> 
> Oleg.
> 
> 

Re: + cgroups-more-safe-tasklist-locking-in-cgroup_attach_proc.patch added to -mm tree

[Oleg Nesterov]

On 09/08, Ben Blum wrote:
>
> On Thu, Sep 08, 2011 at 11:31:30PM +0200, Oleg Nesterov wrote:
> > On 09/08, Ben Blum wrote:
> > >
> > > As for the patch below (which is the same as it was last time?):
> >
> > It is the same, yes, I simply copied it from my old email. BTW, it
> > wasn't tested at all, even compiled.
>
> Testing is recommended ;)

Heh, that is why I didn't send the patch "officially". Not to mention
I never used cgroups.

> If you polished this patch off, I'd be happier, since I have a lot else
> on my plate.

Same here ;)

> > Off-topic question... Looking at this code, it seems that
> > attach_task_by_pid(zombie_pid, threadgroup => true) returns 0.
> > single-task-only case fails with -ESRCH in this case. I am not
> > saying this is wrong, just this looks a bit strange (unless I
> > misread the code).
>
> yeah, this is a side-effect of cgroup_attach_proc continuing to iterate
> in case any one of the sub-threads be still alive. you could keep track
> of whether all threads were zombies and return -ESRCH then,

Yes I see. Although PF_EXITING && thread_group_empty() could help.

> but it
> shouldn't matter, since the user-facing behaviour is indistinguishable
> from if the user had sent the command just before the task turned zombie
> but while it was still about to exit.

Not sure. A task can spend a lot of time being zombie. In this case
we return success or -ESRCH depending on threadgroup.

But I agree, this is minor.

Oleg.