Re: Is not locking task_lock in cgroup_fork() safe?

Re: Is not locking task_lock in cgroup_fork() safe?

[Frederic Weisbecker]

2012/10/16 Tejun Heo <tj@kernel.org>:
> Hey, Frederic.
>
> On Mon, Oct 08, 2012 at 02:48:58PM +0200, Frederic Weisbecker wrote:
>> Yeah I missed this one.
>> Now the whole cgroup_attach_task() is clusteracy without the
>
> Clusteracy?
>
>> threadgroup lock anyway:
>>
>> * The PF_EXITING check is racy (we are neither holding tsk->flags nor
>> threagroup lock).
>
> PF_EXITING is *always* protected by threadgroup_change_begin/end().
>
>> * The cgrp == oldcgrp is racy (exit() can change the oldcgrp anytime.
>
> So, as long as this happens after PF_EXITING check, it should be safe.
>
>> * can_attach / attach / cancel_attach can race against fork/exit (and
>> post_fork if you consider those interested in cgroup task link like
>> the freezer. But that is racy in any case already even with
>> threadgroup lock)
>
> Against exit, no.  Against forking a new process, can they?  If so, we
> need to fix it.
>
>> It has been designed to be called under that lock. So I suspect the
>
> Ummm.... threadgroup_lock is a recent addition so things couldn't have
> been designed to be called under that lock.  threadgroup_lock protects
> the *threadgroup* - creating a new task in the same process or a task
> of the process exiting.  It doesn't do anything about other processes.
> In fact, the lock itself is per-process.
>
>> best, at least for now, is to threadgroup lock from
>> cgroup_attach_task_all(). And also make cgroup_attach_task() static to
>> avoid future unsafe callers.
>
> Oh, from that call path, sure.  Can someone teach me why we need that
> one at all?  I think we're confusing each other here.  I was talking
> about the usual migration path not protected against forking a new
> process.

Ah right I was confused. Hmm, indeed we have a race here on
cgroup_fork(). How about using css_try_get() in cgroup_fork() and
refetch the parent's css until we succeed? This requires rcu_read_lock
though, and freeing the css_set under RCU.

Don't know which is better.

Different problem but I really would like we sanitize the cgroup hooks
in fork. There is cgroup_fork(), cgroup_post_fork() which takes that
big css_set_lock, plus the big threadgroup lock... I hope we can
simplify the mess there.

>
>> There is no harm yet because the only user of it calls that with
>> current as the "task" parameter, in a place that is
>> not in the middle of a fork. So no need to worry about some stable backport.
>>
>> Also, looking at cgroup_attach_task_all(), what guarantee do we have
>> that "from" is not concurrently exiting and removing its cgrp. Which
>> is a separate problem. But we probably need to do some css_set_get()
>> before playing with it.
>
> I really don't know.  Why isn't it locking the threadgroup to begin
> with?

No idea, sounds like something to fix.

Re: Is not locking task_lock in cgroup_fork() safe?

[Tejun Heo]

Hello, Frederic.

On Thu, Oct 18, 2012 at 04:50:59PM +0200, Frederic Weisbecker wrote:
> Ah right I was confused. Hmm, indeed we have a race here on
> cgroup_fork(). How about using css_try_get() in cgroup_fork() and
> refetch the parent's css until we succeed? This requires rcu_read_lock
> though, and freeing the css_set under RCU.
> 
> Don't know which is better.

For now, I'll revert the patches and cc stable.  Let's think about
improving it later.

> Different problem but I really would like we sanitize the cgroup hooks
> in fork. There is cgroup_fork(), cgroup_post_fork() which takes that
> big css_set_lock, plus the big threadgroup lock... I hope we can
> simplify the mess there.

Oh yeah, I've been looking at that one too.  There are a few problems
in that area.  I think all we need is clearing ->cgroups to NULL on
copy_process() and all the rest can be moved to cgroup_post_fork().
I'd also like to make it very explicit that migration can't happen
before post_fork is complete.

> > I really don't know.  Why isn't it locking the threadgroup to begin
> > with?
> 
> No idea, sounds like something to fix.

Alrighty.

Thanks.

-- 
tejun

Re: Is not locking task_lock in cgroup_fork() safe?

[Frederic Weisbecker]

2012/10/18 Tejun Heo <tj@kernel.org>:
> Hello, Frederic.
>
> On Thu, Oct 18, 2012 at 04:50:59PM +0200, Frederic Weisbecker wrote:
>> Ah right I was confused. Hmm, indeed we have a race here on
>> cgroup_fork(). How about using css_try_get() in cgroup_fork() and
>> refetch the parent's css until we succeed? This requires rcu_read_lock
>> though, and freeing the css_set under RCU.
>>
>> Don't know which is better.
>
> For now, I'll revert the patches and cc stable.  Let's think about
> improving it later.

Ok for reverting in cgroup_fork(). Is it necessary for the
cgroup_post_fork() thing? I don't immediately see any race involved
there.

>> Different problem but I really would like we sanitize the cgroup hooks
>> in fork. There is cgroup_fork(), cgroup_post_fork() which takes that
>> big css_set_lock, plus the big threadgroup lock... I hope we can
>> simplify the mess there.
>
> Oh yeah, I've been looking at that one too.  There are a few problems
> in that area.  I think all we need is clearing ->cgroups to NULL on
> copy_process() and all the rest can be moved to cgroup_post_fork().
> I'd also like to make it very explicit that migration can't happen
> before post_fork is complete.

Sounds right.

>
>> > I really don't know.  Why isn't it locking the threadgroup to begin
>> > with?
>>
>> No idea, sounds like something to fix.
>
> Alrighty.

Ok thanks.

> Thanks.
>
> --
> tejun

Re: Is not locking task_lock in cgroup_fork() safe?

[Tejun Heo]

Hello, Frederic.

On Thu, Oct 18, 2012 at 10:53:47PM +0200, Frederic Weisbecker wrote:
> > For now, I'll revert the patches and cc stable.  Let's think about
> > improving it later.
> 
> Ok for reverting in cgroup_fork(). Is it necessary for the
> cgroup_post_fork() thing? I don't immediately see any race involved
> there.

Even if there isn't an actual race, the comment is dead wrong.  I'm
reverting the following three patches.  Let's try again later.

  7e381b0eb1 ("cgroup: Drop task_lock(parent) on cgroup_fork()")
  7e3aa30ac8 ("cgroup: Remove task_lock() from cgroup_post_fork()")
  c84cdf75cc ("cgroup: Remove unnecessary task_lock before fetching css_set on migration")

Thanks.

-- 
tejun

Re: Is not locking task_lock in cgroup_fork() safe?

[Tejun Heo]

Hello, again.

On Thu, Oct 18, 2012 at 05:38:35PM -0700, Tejun Heo wrote:
> Even if there isn't an actual race, the comment is dead wrong.  I'm
> reverting the following three patches.  Let's try again later.
> 
>   7e381b0eb1 ("cgroup: Drop task_lock(parent) on cgroup_fork()")
>   7e3aa30ac8 ("cgroup: Remove task_lock() from cgroup_post_fork()")

So, after some more looking, I think the following is correct and
doesn't need to be reverted.  It's depending on threadgroup locking
from migration path to synchronize against exit path which is always
performed.

>   c84cdf75cc ("cgroup: Remove unnecessary task_lock before fetching css_set on migration")

Frederic, were you trying to say that the above commit is correct?
Li, do you agree?

Thanks.

-- 
tejun

Re: Is not locking task_lock in cgroup_fork() safe?

[Li Zefan]

On 2012/10/19 8:58, Tejun Heo wrote:
> Hello, again.
> 
> On Thu, Oct 18, 2012 at 05:38:35PM -0700, Tejun Heo wrote:
>> Even if there isn't an actual race, the comment is dead wrong.  I'm
>> reverting the following three patches.  Let's try again later.
>>
>>   7e381b0eb1 ("cgroup: Drop task_lock(parent) on cgroup_fork()")
>>   7e3aa30ac8 ("cgroup: Remove task_lock() from cgroup_post_fork()")
> 
> So, after some more looking, I think the following is correct and
> doesn't need to be reverted.  It's depending on threadgroup locking
> from migration path to synchronize against exit path which is always
> performed.
> 
>>   c84cdf75cc ("cgroup: Remove unnecessary task_lock before fetching css_set on migration")
> 
> Frederic, were you trying to say that the above commit is correct?
> Li, do you agree?
> 

This one does look innocent.

Re: [PATCH cgroup/for-3.7-fixes 1/2] Revert "cgroup: Remove task_lock() from cgroup_post_fork()"

[Frederic Weisbecker]

2012/10/19 Tejun Heo <tj@kernel.org>:
> On Fri, Oct 19, 2012 at 09:35:26AM -0400, Frederic Weisbecker wrote:
>> 2012/10/18 Tejun Heo <tj@kernel.org>:
>> > From d935a5d6832a264ce52f4257e176f4f96cbaf048 Mon Sep 17 00:00:00 2001
>> > From: Tejun Heo <tj@kernel.org>
>> > Date: Thu, 18 Oct 2012 17:40:30 -0700
>> >
>> > This reverts commit 7e3aa30ac8c904a706518b725c451bb486daaae9.
>> >
>> > The commit incorrectly assumed that fork path always performed
>> > threadgroup_change_begin/end() and depended on that for
>> > synchronization against task exit and cgroup migration paths instead
>> > of explicitly grabbing task_lock().
>> >
>> > threadgroup_change is not locked when forking a new process (as
>> > opposed to a new thread in the same process) and even if it were it
>> > wouldn't be effective as different processes use different threadgroup
>> > locks.
>> >
>> > Revert the incorrect optimization.
>>
>> Ok but there is still no good reason to task_lock() there. But the
>> comment is indeed wrong,  how about fixing it instead? I can send you
>> a patch for that.
>
> For -stable, I think it's better to revert.  If you want to remove
> task_lock, let's do it for 3.8.

I don't think that a wrong comment justifies a patch to stable.

Re: [PATCH cgroup/for-3.7-fixes 1/2] Revert "cgroup: Remove task_lock() from cgroup_post_fork()"

[Tejun Heo]

Hello, Frederic.

On Fri, Oct 19, 2012 at 03:44:20PM -0400, Frederic Weisbecker wrote:
> > For -stable, I think it's better to revert.  If you want to remove
> > task_lock, let's do it for 3.8.
> 
> I don't think that a wrong comment justifies a patch to stable.

I'm not really sure whether it's safe or not.  It seems all usages are
protected by write locking css_set_lock but maybe I'm missing
something and as the commit is born out of confusion, I'm very
inclined to revert it by default.  Are you sure this one is safe?

Thanks.

-- 
tejun

Re: [PATCH cgroup/for-3.7-fixes 1/2] Revert "cgroup: Remove task_lock() from cgroup_post_fork()"

[Frederic Weisbecker]

2012/10/19 Tejun Heo <tj@kernel.org>:
> Hello, Frederic.
>
> On Fri, Oct 19, 2012 at 03:44:20PM -0400, Frederic Weisbecker wrote:
>> > For -stable, I think it's better to revert.  If you want to remove
>> > task_lock, let's do it for 3.8.
>>
>> I don't think that a wrong comment justifies a patch to stable.
>
> I'm not really sure whether it's safe or not.  It seems all usages are
> protected by write locking css_set_lock but maybe I'm missing
> something and as the commit is born out of confusion, I'm very
> inclined to revert it by default.  Are you sure this one is safe?

Thinking about it further, one scenario is worrying me but it
eventually looks safe but by accident.

CPU 0
                    CPU 1

cgroup_task_migrate {
        task_lock(p)
        rcu_assign_pointer(tsk->cgroups, newcg);
        task_unlock(tsk);

        write_lock(&css_set_lock);
        if (!list_empty(&tsk->cg_list))
            list_move(&tsk->cg_list, &newcg->tasks);
        write_unlock(&css_set_lock);

                          write_lock(&css_set_lock);
	put_css_set(oldcg);
         list_add(&child->cg_list, &child->cgroups->tasks); (1)

On (1), child->cgroups should have the value of newcg and not oldcg
due to the memory ordering implied by the locking of css_set_lock. Now
I can't guarantee that because I'm no memory ordering expert. And even
if it's safe, it's so very non obvious that I now agree with you:
let's revert  the patch and restart with a better base by gathering
all the cgroup fork code in the current cgroup_post_fork place.

Thanks.

Re: [PATCH cgroup/for-3.7-fixes 1/2] Revert "cgroup: Remove task_lock() from cgroup_post_fork()"

[Frederic Weisbecker]

2012/10/20 Frederic Weisbecker <fweisbec@gmail.com>:
> 2012/10/19 Tejun Heo <tj@kernel.org>:
>> Hello, Frederic.
>>
>> On Fri, Oct 19, 2012 at 03:44:20PM -0400, Frederic Weisbecker wrote:
>>> > For -stable, I think it's better to revert.  If you want to remove
>>> > task_lock, let's do it for 3.8.
>>>
>>> I don't think that a wrong comment justifies a patch to stable.
>>
>> I'm not really sure whether it's safe or not.  It seems all usages are
>> protected by write locking css_set_lock but maybe I'm missing
>> something and as the commit is born out of confusion, I'm very
>> inclined to revert it by default.  Are you sure this one is safe?
>
> Thinking about it further, one scenario is worrying me but it
> eventually looks safe but by accident.
>
> CPU 0
>                     CPU 1
>
> cgroup_task_migrate {
>         task_lock(p)
>         rcu_assign_pointer(tsk->cgroups, newcg);
>         task_unlock(tsk);
>
>         write_lock(&css_set_lock);
>         if (!list_empty(&tsk->cg_list))
>             list_move(&tsk->cg_list, &newcg->tasks);
>         write_unlock(&css_set_lock);
>
>                           write_lock(&css_set_lock);
>         put_css_set(oldcg);
>          list_add(&child->cg_list, &child->cgroups->tasks); (1)

gmail mangled everything :(

Re: [PATCH cgroup/for-3.7-fixes 1/2] Revert "cgroup: Remove task_lock() from cgroup_post_fork()"

[Tejun Heo]

Hello, Frederic.

On Sat, Oct 20, 2012 at 02:21:43PM -0400, Frederic Weisbecker wrote:
> CPU 0
>                     CPU 1
> 
> cgroup_task_migrate {
>         task_lock(p)
>         rcu_assign_pointer(tsk->cgroups, newcg);
>         task_unlock(tsk);
> 
>         write_lock(&css_set_lock);
>         if (!list_empty(&tsk->cg_list))
>             list_move(&tsk->cg_list, &newcg->tasks);
>         write_unlock(&css_set_lock);
> 
>                           write_lock(&css_set_lock);
> 	put_css_set(oldcg);
>          list_add(&child->cg_list, &child->cgroups->tasks); (1)

Man, that's confusing. :)

> On (1), child->cgroups should have the value of newcg and not oldcg
> due to the memory ordering implied by the locking of css_set_lock. Now
> I can't guarantee that because I'm no memory ordering expert. And even
> if it's safe, it's so very non obvious that I now agree with you:
> let's revert  the patch and restart with a better base by gathering
> all the cgroup fork code in the current cgroup_post_fork place.

Aye aye, let's move everything to cgroup_post_fork() and then we don't
have to worry about grabbing task_lock multiple times.

Thanks.

-- 
tejun

Re: [PATCH cgroup/for-3.7-fixes 1/2] Revert "cgroup: Remove task_lock() from cgroup_post_fork()"

[Frederic Weisbecker]

2012/10/21 Tejun Heo <tj@kernel.org>:
> Hello, Frederic.
>
> On Sat, Oct 20, 2012 at 02:21:43PM -0400, Frederic Weisbecker wrote:
>> CPU 0
>>                     CPU 1
>>
>> cgroup_task_migrate {
>>         task_lock(p)
>>         rcu_assign_pointer(tsk->cgroups, newcg);
>>         task_unlock(tsk);
>>
>>         write_lock(&css_set_lock);
>>         if (!list_empty(&tsk->cg_list))
>>             list_move(&tsk->cg_list, &newcg->tasks);
>>         write_unlock(&css_set_lock);
>>
>>                           write_lock(&css_set_lock);
>>       put_css_set(oldcg);
>>          list_add(&child->cg_list, &child->cgroups->tasks); (1)
>
> Man, that's confusing. :)

Sorry and I'm currently stuck in some airport and too lazy to reorder
the above lines :)

>
>> On (1), child->cgroups should have the value of newcg and not oldcg
>> due to the memory ordering implied by the locking of css_set_lock. Now
>> I can't guarantee that because I'm no memory ordering expert. And even
>> if it's safe, it's so very non obvious that I now agree with you:
>> let's revert  the patch and restart with a better base by gathering
>> all the cgroup fork code in the current cgroup_post_fork place.
>
> Aye aye, let's move everything to cgroup_post_fork() and then we don't
> have to worry about grabbing task_lock multiple times.

Agreed. and Acked-by: Frederic Weisbecker <fweisbec@gmail.com>