2012-10-04 07:48:53

by Daisuke Nishimura

Subject: [REGRESSION] cgroup: notify_on_release may not be triggered in some cases

notify_on_release must be triggered when the last process in a cgroup is
move to another. But if the first(and only) process in a cgroup is moved to
another, notify_on_release is not triggered.

# mkdir /cgroup/cpu/SRC
# mkdir /cgroup/cpu/DST
#
# echo 1 >/cgroup/cpu/SRC/notify_on_release
# echo 1 >/cgroup/cpu/DST/notify_on_release
#
# sleep 300 &
[1] 8629
#
# echo 8629 >/cgroup/cpu/SRC/tasks
# echo 8629 >/cgroup/cpu/DST/tasks
-> notify_on_release for /SRC must be triggered at this point,
but it isn't.

This is because put_css_set() is called before setting CGRP_RELEASABLE
in cgroup_task_migrate(), and is a regression introduce by the
commit:74a1166d(cgroups: make procs file writable), which was merged
into v3.0.

Cc: Ben Blum <[email protected]>
Cc: Tejun Heo <[email protected]>
Cc: Li Zefan <[email protected]>
Cc: <[email protected]> # v3.0.x and later
Signed-off-by: Daisuke Nishimura <[email protected]>
---
This patch is based on v3.6.

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

diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index 7981850..7d01182 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -1923,9 +1923,8 @@ static void cgroup_task_migrate(struct cgroup *cgrp, struct cgroup *oldcgrp,
* trading it for newcg is protected by cgroup_mutex, we're safe to drop
* it here; it will be freed under RCU.
*/
- put_css_set(oldcg);
-
set_bit(CGRP_RELEASABLE, &oldcgrp->flags);
+ put_css_set(oldcg);
}

/**
--
1.7.1

2012-10-08 06:13:16

by Zefan Li

Subject: Re: [REGRESSION] cgroup: notify_on_release may not be triggered in some cases

On 2012/10/4 15:37, Daisuke Nishimura wrote:
> notify_on_release must be triggered when the last process in a cgroup is
> move to another. But if the first(and only) process in a cgroup is moved to
> another, notify_on_release is not triggered.
>
> # mkdir /cgroup/cpu/SRC
> # mkdir /cgroup/cpu/DST
> #
> # echo 1 >/cgroup/cpu/SRC/notify_on_release
> # echo 1 >/cgroup/cpu/DST/notify_on_release
> #
> # sleep 300 &
> [1] 8629
> #
> # echo 8629 >/cgroup/cpu/SRC/tasks
> # echo 8629 >/cgroup/cpu/DST/tasks
> -> notify_on_release for /SRC must be triggered at this point,
> but it isn't.
>
> This is because put_css_set() is called before setting CGRP_RELEASABLE
> in cgroup_task_migrate(), and is a regression introduce by the
> commit:74a1166d(cgroups: make procs file writable), which was merged
> into v3.0.
>
> Cc: Ben Blum <[email protected]>
> Cc: Tejun Heo <[email protected]>
> Cc: Li Zefan <[email protected]>
> Cc: <[email protected]> # v3.0.x and later
> Signed-off-by: Daisuke Nishimura <[email protected]>
> ---
> This patch is based on v3.6.
>
> kernel/cgroup.c | 3 +--
> 1 files changed, 1 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/cgroup.c b/kernel/cgroup.c
> index 7981850..7d01182 100644
> --- a/kernel/cgroup.c
> +++ b/kernel/cgroup.c
> @@ -1923,9 +1923,8 @@ static void cgroup_task_migrate(struct cgroup *cgrp, struct cgroup *oldcgrp,
> * trading it for newcg is protected by cgroup_mutex, we're safe to drop
> * it here; it will be freed under RCU.
> */
> - put_css_set(oldcg);
> -
> set_bit(CGRP_RELEASABLE, &oldcgrp->flags);
> + put_css_set(oldcg);

Good catch. Also we might end up with invalid memory access!

Acked-by: Li Zefan <[email protected]>

> }
>
> /**
>

2012-10-17 00:11:48

by Tejun Heo

Subject: Re: [REGRESSION] cgroup: notify_on_release may not be triggered in some cases

Hello,

On Thu, Oct 04, 2012 at 04:37:16PM +0900, Daisuke Nishimura wrote:
> notify_on_release must be triggered when the last process in a cgroup is
> move to another. But if the first(and only) process in a cgroup is moved to
> another, notify_on_release is not triggered.
>
> # mkdir /cgroup/cpu/SRC
> # mkdir /cgroup/cpu/DST
> #
> # echo 1 >/cgroup/cpu/SRC/notify_on_release
> # echo 1 >/cgroup/cpu/DST/notify_on_release
> #
> # sleep 300 &
> [1] 8629
> #
> # echo 8629 >/cgroup/cpu/SRC/tasks
> # echo 8629 >/cgroup/cpu/DST/tasks
> -> notify_on_release for /SRC must be triggered at this point,
> but it isn't.
>
> This is because put_css_set() is called before setting CGRP_RELEASABLE
> in cgroup_task_migrate(), and is a regression introduce by the
> commit:74a1166d(cgroups: make procs file writable), which was merged
> into v3.0.
>
> Cc: Ben Blum <[email protected]>
> Cc: Tejun Heo <[email protected]>
> Cc: Li Zefan <[email protected]>
> Cc: <[email protected]> # v3.0.x and later
> Signed-off-by: Daisuke Nishimura <[email protected]>

Applied to cgroup/for-3.7-fixes w/ Li's ack added.

Thanks!

--
tejun