Is not locking task_lock in cgroup_fork() safe?

7e381b0eb1 ("cgroup: Drop task_lock(parent) on cgroup_fork()") removed task_lock from cgroup_fork citing that current->cgroups can't change due to threadgroup_change locking; however, threadgroup_change locking is used only during CLONE_THREAD forking. If @current is forking a new process, there's nothing preventing someone else to migrate the parent while forking is in progress and delete the css_set it currently is using. Am I confused somewhere?

Quoted 7 lines · Tejun Heo

7e381b0eb1 ("cgroup: Drop task_lock(parent) on cgroup_fork()") removed task_lock from cgroup_fork citing that current->cgroups can't change due to threadgroup_change locking; however, threadgroup_change locking is used only during CLONE_THREAD forking. If @current is forking a new process, there's nothing preventing someone else to migrate the parent while forking is in progress and delete the css_set it currently is using. Am I confused somewhere?

Also, please note that task_lock is likely to be hot on local CPU at that point and avoiding it there might not really buy much.

You're right. threadgroup lock is held unconditionally in attach_task_py_pid(), but it's held only for CLONE_THREAD in fork path, which I guess I overlooked when reviewing the patch.

Quoted 2 lines

Also, please note that task_lock is likely to be hot on local CPU at that point and avoiding it there might not really buy much.

Reverting that commit should be fine.

Hello, Li, Frederic.

Quoted 7 lines · Li Zefan

You're right. threadgroup lock is held unconditionally in attach_task_py_pid(), but it's held only for CLONE_THREAD in fork path, which I guess I overlooked when reviewing the patch.

Quoted 2 lines

Also, please note that task_lock is likely to be hot on local CPU at that point and avoiding it there might not really buy much.

Quoted 1 line

Reverting that commit should be fine.

There are other commits which perform similar optimization

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

Are they wrong too?

Quoted 12 lines · Tejun Heo

Hello, Li, Frederic.

Quoted 8 lines · Li Zefan

You're right. threadgroup lock is held unconditionally in attach_task_py_pid(), but it's held only for CLONE_THREAD in fork path, which I guess I overlooked when reviewing the patch.

Quoted 2 lines

Also, please note that task_lock is likely to be hot on local CPU at that point and avoiding it there might not really buy much.

Reverting that commit should be fine.

Quoted 6 lines

There are other commits which perform similar optimization

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

Are they wrong too?

Frederic, Li, Ping?

Quoted 12 lines · Tejun Heo

Hello, Li, Frederic.

Quoted 8 lines · Li Zefan

You're right. threadgroup lock is held unconditionally in attach_task_py_pid(), but it's held only for CLONE_THREAD in fork path, which I guess I overlooked when reviewing the patch.

Quoted 2 lines

Also, please note that task_lock is likely to be hot on local CPU at that point and avoiding it there might not really buy much.

Reverting that commit should be fine.

Quoted 6 lines

There are other commits which perform similar optimization

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

Are they wrong too?

Frederic, Li, Ping?

You're right. threadgroup lock is held unconditionally in attach_task_py_pid(), but it's held only for CLONE_THREAD in fork path, which I guess I overlooked when reviewing the patch.

Quoted 2 lines

Also, please note that task_lock is likely to be hot on local CPU at that point and avoiding it there might not really buy much.

Reverting that commit should be fine.

Quoted 7 lines · Tejun Heo

7e381b0eb1 ("cgroup: Drop task_lock(parent) on cgroup_fork()") removed task_lock from cgroup_fork citing that current->cgroups can't change due to threadgroup_change locking; however, threadgroup_change locking is used only during CLONE_THREAD forking. If @current is forking a new process, there's nothing preventing someone else to migrate the parent while forking is in progress and delete the css_set it currently is using. Am I confused somewhere?

Also, please note that task_lock is likely to be hot on local CPU at that point and avoiding it there might not really buy much.

Quoted 7 lines

7e381b0eb1 ("cgroup: Drop task_lock(parent) on cgroup_fork()") removed task_lock from cgroup_fork citing that current->cgroups can't change due to threadgroup_change locking; however, threadgroup_change locking is used only during CLONE_THREAD forking. If @current is forking a new process, there's nothing preventing someone else to migrate the parent while forking is in progress and delete the css_set it currently is using. Am I confused somewhere?

Also, please note that task_lock is likely to be hot on local CPU at that point and avoiding it there might not really buy much.

Quoted 7 lines

7e381b0eb1 ("cgroup: Drop task_lock(parent) on cgroup_fork()") removed task_lock from cgroup_fork citing that current->cgroups can't change due to threadgroup_change locking; however, threadgroup_change locking is used only during CLONE_THREAD forking. If @current is forking a new process, there's nothing preventing someone else to migrate the parent while forking is in progress and delete the css_set it currently is using. Am I confused somewhere?

Also, please note that task_lock is likely to be hot on local CPU at that point and avoiding it there might not really buy much.

Hello, Frederic.

7e381b0eb1 ("cgroup: Drop task_lock(parent) on cgroup_fork()") removed task_lock from cgroup_fork citing that current->cgroups can't change due to threadgroup_change locking; however, threadgroup_change locking is used only during CLONE_THREAD forking. If @current is forking a new process, there's nothing preventing someone else to migrate the parent while forking is in progress and delete the css_set it currently is using. Am I confused somewhere?

Yeah I missed this one.
Now the whole cgroup_attach_task() is clusteracy without the

threadgroup lock anyway:

* The PF_EXITING check is racy (we are neither holding tsk->flags nor threagroup lock).

* The cgrp == oldcgrp is racy (exit() can change the oldcgrp anytime.

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

It has been designed to be called under that lock. So I suspect the

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.

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.

Hey, Frederic.

Quoted 2 lines · Frederic Weisbecker

Yeah I missed this one.
Now the whole cgroup_attach_task() is clusteracy without the

Clusteracy?

Quoted 4 lines

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().

Quoted 1 line

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

Quoted 4 lines

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

Quoted 1 line

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.

Quoted 3 lines

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.

Quoted 5 lines

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.

Quoted 4 lines

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?

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.

Quoted 2 lines

I really don't know. Why isn't it locking the threadgroup to begin with?

No idea, sounds like something to fix.

Hello, Frederic.

Quoted 5 lines · Frederic Weisbecker

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.

Quoted 1 line

Don't know which is better.

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

Quoted 4 lines

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.

Quoted 5 lines

Quoted 3 lines

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

Quoted 2 lines

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

Hello, again.

Quoted 5 lines · Tejun Heo

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.

Quoted 1 line

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?

Hello, again.

Quoted 5 lines · Tejun Heo

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.

Quoted 1 line

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?

Quoted 2 lines

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.

Hey, Frederic.

Quoted 2 lines · Frederic Weisbecker

Yeah I missed this one.
Now the whole cgroup_attach_task() is clusteracy without the

Clusteracy?

Quoted 4 lines

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().

Quoted 1 line

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

Quoted 4 lines

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

Quoted 1 line

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.

Quoted 3 lines

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.

Quoted 5 lines

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.

Quoted 4 lines

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?

Hello, Frederic.

7e381b0eb1 ("cgroup: Drop task_lock(parent) on cgroup_fork()") removed task_lock from cgroup_fork citing that current->cgroups can't change due to threadgroup_change locking; however, threadgroup_change locking is used only during CLONE_THREAD forking. If @current is forking a new process, there's nothing preventing someone else to migrate the parent while forking is in progress and delete the css_set it currently is using. Am I confused somewhere?

Quoted 1 line

From d935a5d6832a264ce52f4257e176f4f96cbaf048 Mon Sep 17 00:00:00 2001

From: Tejun Heo <[email protected]>
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.

Signed-off-by Tejun Heo <[email protected]>

LKML-Reference: <20121008020000.GB2575@localhost>
Cc: Frederic Weisbecker <[email protected]>
Cc: Li Zefan <[email protected]>
Cc: [email protected]

From d935a5d6832a264ce52f4257e176f4f96cbaf048 Mon Sep 17 00:00:00 2001
From: Tejun Heo <[email protected]>
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.

Signed-off-by Tejun Heo <[email protected]>

LKML-Reference: <20121008020000.GB2575@localhost>
Cc: Frederic Weisbecker <[email protected]>
Cc: Li Zefan <[email protected]>
Cc: [email protected]

---
 kernel/cgroup.c |   15 +++------------
 1 files changed, 3 insertions(+), 12 deletions(-)

kernel/cgroup.c +3 -13

index d1739fc..2990dc7 100644
@@ -4894,19 +4894,10 @@ void cgroup_post_fork(struct task_struct *child)
         */
        if (use_task_css_set_links) {
                write_lock(&css_set_lock);
-               if (list_empty(&child->cg_list)) {
-                       /*
-                        * It's safe to use child->cgroups without task_lock()
-                        * here because we are protected through
-                        * threadgroup_change_begin() against concurrent
-                        * css_set change in cgroup_task_migrate(). Also
-                        * the task can't exit at that point until
-                        * wake_up_new_task() is called, so we are protected
-                        * against cgroup_exit() setting child->cgroup to
-                        * init_css_set.
-                        */
+               task_lock(child);
+               if (list_empty(&child->cg_list))
                        list_add(&child->cg_list, &child->cgroups->tasks);
-               }
+               task_unlock(child);
                write_unlock(&css_set_lock);
        }
 }
--

1.7.7.3

2012/10/18 Tejun Heo <[email protected]>:

Quoted 16 lines

From d935a5d6832a264ce52f4257e176f4f96cbaf048 Mon Sep 17 00:00:00 2001
From: Tejun Heo <[email protected]>
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.

Quoted 1 line

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.

Quoted 17 lines

2012/10/18 Tejun Heo <[email protected]>:

Quoted 16 lines

From d935a5d6832a264ce52f4257e176f4f96cbaf048 Mon Sep 17 00:00:00 2001
From: Tejun Heo <[email protected]>
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.

Quoted 5 lines

Quoted 1 line

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.

Quoted 2 lines

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.

Hello, Frederic.

Quoted 3 lines · Frederic Weisbecker

Quoted 2 lines

For -stable, I think it's better to revert. If you want to remove task_lock, let's do it for 3.8.

Quoted 1 line

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?

2012/10/19 Tejun Heo <[email protected]>:

Quoted 6 lines

Hello, Frederic.

Quoted 3 lines · Frederic Weisbecker

Quoted 2 lines

For -stable, I think it's better to revert. If you want to remove task_lock, let's do it for 3.8.

Quoted 6 lines

Quoted 1 line

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)

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.

Hello, Frederic.

Quoted 13 lines · Frederic Weisbecker

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

Quoted 3 lines

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

Man, that's confusing. :)

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

Hello, Frederic.

Quoted 13 lines · Frederic Weisbecker

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

Quoted 3 lines

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

Man, that's confusing. :)

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

Quoted 2 lines

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.

Quoted 17 lines

2012/10/18 Tejun Heo <[email protected]>:

Quoted 16 lines

From d935a5d6832a264ce52f4257e176f4f96cbaf048 Mon Sep 17 00:00:00 2001
From: Tejun Heo <[email protected]>
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.

Quoted 5 lines

Quoted 1 line

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.

2012/10/18 Tejun Heo <[email protected]>:

Quoted 16 lines

From d935a5d6832a264ce52f4257e176f4f96cbaf048 Mon Sep 17 00:00:00 2001
From: Tejun Heo <[email protected]>
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.

Quoted 1 line

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.

From d935a5d6832a264ce52f4257e176f4f96cbaf048 Mon Sep 17 00:00:00 2001
From: Tejun Heo <[email protected]>
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.

Signed-off-by Tejun Heo <[email protected]>

LKML-Reference: <20121008020000.GB2575@localhost>
Cc: Frederic Weisbecker <[email protected]>
Cc: Li Zefan <[email protected]>
Cc: [email protected]

---
 kernel/cgroup.c |   15 +++------------
 1 files changed, 3 insertions(+), 12 deletions(-)

kernel/cgroup.c +3 -13

index d1739fc..2990dc7 100644
@@ -4894,19 +4894,10 @@ void cgroup_post_fork(struct task_struct *child)
         */
        if (use_task_css_set_links) {
                write_lock(&css_set_lock);
-               if (list_empty(&child->cg_list)) {
-                       /*
-                        * It's safe to use child->cgroups without task_lock()
-                        * here because we are protected through
-                        * threadgroup_change_begin() against concurrent
-                        * css_set change in cgroup_task_migrate(). Also
-                        * the task can't exit at that point until
-                        * wake_up_new_task() is called, so we are protected
-                        * against cgroup_exit() setting child->cgroup to
-                        * init_css_set.
-                        */
+               task_lock(child);
+               if (list_empty(&child->cg_list))
                        list_add(&child->cg_list, &child->cgroups->tasks);
-               }
+               task_unlock(child);
                write_unlock(&css_set_lock);
        }
 }
--

1.7.7.3

Quoted 1 line

From c8b27924a8b6fd74066088f1cf07c256bbc6ed74 Mon Sep 17 00:00:00 2001

From: Tejun Heo <[email protected]>
Date: Thu, 18 Oct 2012 17:52:07 -0700

This reverts commit 7e381b0eb1e1a9805c37335562e8dc02e7d7848c.

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.

Signed-off-by Tejun Heo <[email protected]>

LKML-Reference: <20121008020000.GB2575@localhost>
Cc: Frederic Weisbecker <[email protected]>
Cc: Li Zefan <[email protected]>
Cc: [email protected]

Quoted 1 line

From c8b27924a8b6fd74066088f1cf07c256bbc6ed74 Mon Sep 17 00:00:00 2001

From: Tejun Heo <[email protected]>
Date: Thu, 18 Oct 2012 17:52:07 -0700

This reverts commit 7e381b0eb1e1a9805c37335562e8dc02e7d7848c.

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.

Signed-off-by Tejun Heo <[email protected]>

LKML-Reference: <20121008020000.GB2575@localhost>
Cc: Frederic Weisbecker <[email protected]>
Cc: Li Zefan <[email protected]>
Cc: [email protected]

From c8b27924a8b6fd74066088f1cf07c256bbc6ed74 Mon Sep 17 00:00:00 2001
From: Tejun Heo <[email protected]>
Date: Thu, 18 Oct 2012 17:52:07 -0700

This reverts commit 7e381b0eb1e1a9805c37335562e8dc02e7d7848c.

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.

Signed-off-by Tejun Heo <[email protected]>

LKML-Reference: <20121008020000.GB2575@localhost>

Cc Li Zefan <[email protected]>

Cc [email protected]

---
 kernel/cgroup.c |   23 ++++++-----------------
 1 files changed, 6 insertions(+), 17 deletions(-)

kernel/cgroup.c +6 -18

index 2990dc7..f24f724 100644
@@ -4814,31 +4814,20 @@ static const struct file_operations proc_cgroupstats_operations = {
  *
  * A pointer to the shared css_set was automatically copied in
  * fork.c by dup_task_struct().  However, we ignore that copy, since
- * it was not made under the protection of RCU, cgroup_mutex or
- * threadgroup_change_begin(), so it might no longer be a valid
- * cgroup pointer.  cgroup_attach_task() might have already changed
- * current->cgroups, allowing the previously referenced cgroup
- * group to be removed and freed.
- *
- * Outside the pointer validity we also need to process the css_set
- * inheritance between threadgoup_change_begin() and
- * threadgoup_change_end(), this way there is no leak in any process
- * wide migration performed by cgroup_attach_proc() that could otherwise
- * miss a thread because it is too early or too late in the fork stage.
+ * it was not made under the protection of RCU or cgroup_mutex, so
+ * might no longer be a valid cgroup pointer.  cgroup_attach_task() might
+ * have already changed current->cgroups, allowing the previously
+ * referenced cgroup group to be removed and freed.
  *
  * At the point that cgroup_fork() is called, 'current' is the parent
  * task, and the passed argument 'child' points to the child task.
  */
 void cgroup_fork(struct task_struct *child)
 {
-       /*
-        * We don't need to task_lock() current because current->cgroups
-        * can't be changed concurrently here. The parent obviously hasn't
-        * exited and called cgroup_exit(), and we are synchronized against
-        * cgroup migration through threadgroup_change_begin().
-        */
+       task_lock(current);
        child->cgroups = current->cgroups;
        get_css_set(child->cgroups);
+       task_unlock(current);
        INIT_LIST_HEAD(&child->cg_list);
 }
 
--

1.7.7.3

From c8b27924a8b6fd74066088f1cf07c256bbc6ed74 Mon Sep 17 00:00:00 2001
From: Tejun Heo <[email protected]>
Date: Thu, 18 Oct 2012 17:52:07 -0700

This reverts commit 7e381b0eb1e1a9805c37335562e8dc02e7d7848c.

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.

Signed-off-by Tejun Heo <[email protected]>

LKML-Reference: <20121008020000.GB2575@localhost>

Cc Li Zefan <[email protected]>

Cc [email protected]

---
 kernel/cgroup.c |   23 ++++++-----------------
 1 files changed, 6 insertions(+), 17 deletions(-)

kernel/cgroup.c +6 -18

index 2990dc7..f24f724 100644
@@ -4814,31 +4814,20 @@ static const struct file_operations proc_cgroupstats_operations = {
  *
  * A pointer to the shared css_set was automatically copied in
  * fork.c by dup_task_struct().  However, we ignore that copy, since
- * it was not made under the protection of RCU, cgroup_mutex or
- * threadgroup_change_begin(), so it might no longer be a valid
- * cgroup pointer.  cgroup_attach_task() might have already changed
- * current->cgroups, allowing the previously referenced cgroup
- * group to be removed and freed.
- *
- * Outside the pointer validity we also need to process the css_set
- * inheritance between threadgoup_change_begin() and
- * threadgoup_change_end(), this way there is no leak in any process
- * wide migration performed by cgroup_attach_proc() that could otherwise
- * miss a thread because it is too early or too late in the fork stage.
+ * it was not made under the protection of RCU or cgroup_mutex, so
+ * might no longer be a valid cgroup pointer.  cgroup_attach_task() might
+ * have already changed current->cgroups, allowing the previously
+ * referenced cgroup group to be removed and freed.
  *
  * At the point that cgroup_fork() is called, 'current' is the parent
  * task, and the passed argument 'child' points to the child task.
  */
 void cgroup_fork(struct task_struct *child)
 {
-       /*
-        * We don't need to task_lock() current because current->cgroups
-        * can't be changed concurrently here. The parent obviously hasn't
-        * exited and called cgroup_exit(), and we are synchronized against
-        * cgroup migration through threadgroup_change_begin().
-        */
+       task_lock(current);
        child->cgroups = current->cgroups;
        get_css_set(child->cgroups);
+       task_unlock(current);
        INIT_LIST_HEAD(&child->cg_list);
 }
 
--

1.7.7.3