Commit ea84753c authored by Anjana V Kumar's avatar Anjana V Kumar Committed by Tejun Heo

cgroup: fix to break the while loop in cgroup_attach_task() correctly

Both Anjana and Eunki reported a stall in the while_each_thread loop
in cgroup_attach_task().

It's because, when we attach a single thread to a cgroup, if the cgroup
is exiting or is already in that cgroup, we won't break the loop.

If the task is already in the cgroup, the bug can lead to another thread
being attached to the cgroup unexpectedly:

  # echo 5207 > tasks
  # cat tasks
  5207
  # echo 5207 > tasks
  # cat tasks
  5207
  5215

What's worse, if the task to be attached isn't the leader of the thread
group, we might never exit the loop, hence cpu stall. Thanks for Oleg's
analysis.

This bug was introduced by commit 081aa458
("cgroup: consolidate cgroup_attach_task() and cgroup_attach_proc()")

[ lizf: - fixed the first continue, pointed out by Oleg,
        - rewrote changelog. ]

Cc: <stable@vger.kernel.org> # 3.9+
Reported-by: default avatarEunki Kim <eunki_kim@samsung.com>
Reported-by: default avatarAnjana V Kumar <anjanavk12@gmail.com>
Signed-off-by: default avatarAnjana V Kumar <anjanavk12@gmail.com>
Signed-off-by: default avatarLi Zefan <lizefan@huawei.com>
Signed-off-by: default avatarTejun Heo <tj@kernel.org>
parent 58b79a91
...@@ -2038,7 +2038,7 @@ static int cgroup_attach_task(struct cgroup *cgrp, struct task_struct *tsk, ...@@ -2038,7 +2038,7 @@ static int cgroup_attach_task(struct cgroup *cgrp, struct task_struct *tsk,
/* @tsk either already exited or can't exit until the end */ /* @tsk either already exited or can't exit until the end */
if (tsk->flags & PF_EXITING) if (tsk->flags & PF_EXITING)
continue; goto next;
/* as per above, nr_threads may decrease, but not increase. */ /* as per above, nr_threads may decrease, but not increase. */
BUG_ON(i >= group_size); BUG_ON(i >= group_size);
...@@ -2046,7 +2046,7 @@ static int cgroup_attach_task(struct cgroup *cgrp, struct task_struct *tsk, ...@@ -2046,7 +2046,7 @@ static int cgroup_attach_task(struct cgroup *cgrp, struct task_struct *tsk,
ent.cgrp = task_cgroup_from_root(tsk, root); ent.cgrp = task_cgroup_from_root(tsk, root);
/* nothing to do if this task is already in the cgroup */ /* nothing to do if this task is already in the cgroup */
if (ent.cgrp == cgrp) if (ent.cgrp == cgrp)
continue; goto next;
/* /*
* saying GFP_ATOMIC has no effect here because we did prealloc * saying GFP_ATOMIC has no effect here because we did prealloc
* earlier, but it's good form to communicate our expectations. * earlier, but it's good form to communicate our expectations.
...@@ -2054,7 +2054,7 @@ static int cgroup_attach_task(struct cgroup *cgrp, struct task_struct *tsk, ...@@ -2054,7 +2054,7 @@ static int cgroup_attach_task(struct cgroup *cgrp, struct task_struct *tsk,
retval = flex_array_put(group, i, &ent, GFP_ATOMIC); retval = flex_array_put(group, i, &ent, GFP_ATOMIC);
BUG_ON(retval != 0); BUG_ON(retval != 0);
i++; i++;
next:
if (!threadgroup) if (!threadgroup)
break; break;
} while_each_thread(leader, tsk); } while_each_thread(leader, tsk);
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment