[PATCH 1/4] freezer_cg: fix improper BUG_ON() causing oops

[PATCH 1/4] freezer_cg: fix improper BUG_ON() causing oops

[Li Zefan]

The BUG_ON() should be protected by freezer->lock, otherwise it
can be triggered easily when a task has been unfreezed but the
corresponding cgroup hasn't been changed to FROZEN state.

Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
---
 kernel/cgroup_freezer.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/kernel/cgroup_freezer.c b/kernel/cgroup_freezer.c
index e950569..7f54d1c 100644
--- a/kernel/cgroup_freezer.c
+++ b/kernel/cgroup_freezer.c
@@ -190,8 +190,9 @@ static void freezer_fork(struct cgroup_subsys *ss, struct task_struct *task)
 	freezer = task_freezer(task);
 	task_unlock(task);
 
-	BUG_ON(freezer->state == CGROUP_FROZEN);
 	spin_lock_irq(&freezer->lock);
+	BUG_ON(freezer->state == CGROUP_FROZEN);
+
 	/* Locking avoids race with FREEZING -> THAWED transitions. */
 	if (freezer->state == CGROUP_FREEZING)
 		freeze_task(task, true);
-- 
1.5.4.rc3

[PATCH 2/4] freezer_cg: remove redundant check in freezer_can_attach()

[Li Zefan]

It is sufficient to check if @task is frozen, and no need to check if
the original freezer is frozen.

Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
---
 kernel/cgroup_freezer.c |   16 +++++++---------
 1 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/kernel/cgroup_freezer.c b/kernel/cgroup_freezer.c
index 7f54d1c..6fadafe 100644
--- a/kernel/cgroup_freezer.c
+++ b/kernel/cgroup_freezer.c
@@ -162,9 +162,13 @@ static int freezer_can_attach(struct cgroup_subsys *ss,
 			      struct task_struct *task)
 {
 	struct freezer *freezer;
-	int retval;
 
-	/* Anything frozen can't move or be moved to/from */
+	/*
+	 * Anything frozen can't move or be moved to/from.
+	 *
+	 * Since orig_freezer->state == FROZEN means that @task has been
+	 * frozen, so it's sufficient to check the latter condition.
+	 */
 
 	if (is_task_frozen_enough(task))
 		return -EBUSY;
@@ -173,13 +177,7 @@ static int freezer_can_attach(struct cgroup_subsys *ss,
 	if (freezer->state == CGROUP_FROZEN)
 		return -EBUSY;
 
-	retval = 0;
-	task_lock(task);
-	freezer = task_freezer(task);
-	if (freezer->state == CGROUP_FROZEN)
-		retval = -EBUSY;
-	task_unlock(task);
-	return retval;
+	return 0;
 }
 
 static void freezer_fork(struct cgroup_subsys *ss, struct task_struct *task)
-- 
1.5.4.rc3

[PATCH 3/4] freezer_cg: use thaw_process() in unfreeze_cgroup()

[Li Zefan]

Don't duplicate the implementation of thaw_process().

Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
---
 kernel/cgroup_freezer.c |   15 ++++-----------
 1 files changed, 4 insertions(+), 11 deletions(-)

diff --git a/kernel/cgroup_freezer.c b/kernel/cgroup_freezer.c
index 6fadafe..3ea57e4 100644
--- a/kernel/cgroup_freezer.c
+++ b/kernel/cgroup_freezer.c
@@ -275,25 +275,18 @@ static int try_to_freeze_cgroup(struct cgroup *cgroup, struct freezer *freezer)
 	return num_cant_freeze_now ? -EBUSY : 0;
 }
 
-static int unfreeze_cgroup(struct cgroup *cgroup, struct freezer *freezer)
+static void unfreeze_cgroup(struct cgroup *cgroup, struct freezer *freezer)
 {
 	struct cgroup_iter it;
 	struct task_struct *task;
 
 	cgroup_iter_start(cgroup, &it);
 	while ((task = cgroup_iter_next(cgroup, &it))) {
-		int do_wake;
-
-		task_lock(task);
-		do_wake = __thaw_process(task);
-		task_unlock(task);
-		if (do_wake)
-			wake_up_process(task);
+		thaw_process(task);
 	}
 	cgroup_iter_end(cgroup, &it);
-	freezer->state = CGROUP_THAWED;
 
-	return 0;
+	freezer->state = CGROUP_THAWED;
 }
 
 static int freezer_change_state(struct cgroup *cgroup,
@@ -320,7 +313,7 @@ static int freezer_change_state(struct cgroup *cgroup,
 		}
 		/* state == FREEZING and goal_state == THAWED, so unfreeze */
 	case CGROUP_FROZEN:
-		retval = unfreeze_cgroup(cgroup, freezer);
+		unfreeze_cgroup(cgroup, freezer);
 		break;
 	default:
 		break;
-- 
1.5.4.rc3

[PATCH 4/4] freezer_cg: simplify freezer_change_state()

[Li Zefan]

Just call unfreeze_cgroup() if goal_state == THAWED, and call
try_to_freeze_cgroup() if goal_state == FROZEN.

No behavior has been changed.

Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
---
 kernel/cgroup_freezer.c |   19 +++++++------------
 1 files changed, 7 insertions(+), 12 deletions(-)

diff --git a/kernel/cgroup_freezer.c b/kernel/cgroup_freezer.c
index 3ea57e4..cdef2d8 100644
--- a/kernel/cgroup_freezer.c
+++ b/kernel/cgroup_freezer.c
@@ -296,27 +296,22 @@ static int freezer_change_state(struct cgroup *cgroup,
 	int retval = 0;
 
 	freezer = cgroup_freezer(cgroup);
+
 	spin_lock_irq(&freezer->lock);
+
 	update_freezer_state(cgroup, freezer);
 	if (goal_state == freezer->state)
 		goto out;
-	switch (freezer->state) {
+
+	switch (goal_state) {
 	case CGROUP_THAWED:
-		retval = try_to_freeze_cgroup(cgroup, freezer);
+		unfreeze_cgroup(cgroup, freezer);
 		break;
-	case CGROUP_FREEZING:
-		if (goal_state == CGROUP_FROZEN) {
-			/* Userspace is retrying after
-			 * "/bin/echo FROZEN > freezer.state" returned -EBUSY */
-			retval = try_to_freeze_cgroup(cgroup, freezer);
-			break;
-		}
-		/* state == FREEZING and goal_state == THAWED, so unfreeze */
 	case CGROUP_FROZEN:
-		unfreeze_cgroup(cgroup, freezer);
+		retval = try_to_freeze_cgroup(cgroup, freezer);
 		break;
 	default:
-		break;
+		BUG();
 	}
 out:
 	spin_unlock_irq(&freezer->lock);
-- 
1.5.4.rc3

Re: [PATCH 4/4] freezer_cg: simplify freezer_change_state()

[Cedric Le Goater]

Li Zefan wrote:
> Just call unfreeze_cgroup() if goal_state == THAWED, and call
> try_to_freeze_cgroup() if goal_state == FROZEN.
> 
> No behavior has been changed.

Looks good.

Acked-by: Cedric Le Goater <clg@fr.ibm.com>

thanks,

C.

> 
> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
> ---
>  kernel/cgroup_freezer.c |   19 +++++++------------
>  1 files changed, 7 insertions(+), 12 deletions(-)
> 
> diff --git a/kernel/cgroup_freezer.c b/kernel/cgroup_freezer.c
> index 3ea57e4..cdef2d8 100644
> --- a/kernel/cgroup_freezer.c
> +++ b/kernel/cgroup_freezer.c
> @@ -296,27 +296,22 @@ static int freezer_change_state(struct cgroup *cgroup,
>  	int retval = 0;
> 
>  	freezer = cgroup_freezer(cgroup);
> +
>  	spin_lock_irq(&freezer->lock);
> +
>  	update_freezer_state(cgroup, freezer);
>  	if (goal_state == freezer->state)
>  		goto out;
> -	switch (freezer->state) {
> +
> +	switch (goal_state) {
>  	case CGROUP_THAWED:
> -		retval = try_to_freeze_cgroup(cgroup, freezer);
> +		unfreeze_cgroup(cgroup, freezer);
>  		break;
> -	case CGROUP_FREEZING:
> -		if (goal_state == CGROUP_FROZEN) {
> -			/* Userspace is retrying after
> -			 * "/bin/echo FROZEN > freezer.state" returned -EBUSY */
> -			retval = try_to_freeze_cgroup(cgroup, freezer);
> -			break;
> -		}
> -		/* state == FREEZING and goal_state == THAWED, so unfreeze */
>  	case CGROUP_FROZEN:
> -		unfreeze_cgroup(cgroup, freezer);
> +		retval = try_to_freeze_cgroup(cgroup, freezer);
>  		break;
>  	default:
> -		break;
> +		BUG();
>  	}
>  out:
>  	spin_unlock_irq(&freezer->lock);

Re: [PATCH 4/4] freezer_cg: simplify freezer_change_state()

[Matt Helsley]

On Tue, 2008-10-21 at 08:52 +0800, Li Zefan wrote:
> Just call unfreeze_cgroup() if goal_state == THAWED, and call
> try_to_freeze_cgroup() if goal_state == FROZEN.
> 
> No behavior has been changed.

Another good one. Thanks!

Acked-by: Matt Helsley <matthltc@us.ibm.com>

> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
> ---
>  kernel/cgroup_freezer.c |   19 +++++++------------
>  1 files changed, 7 insertions(+), 12 deletions(-)
> 
> diff --git a/kernel/cgroup_freezer.c b/kernel/cgroup_freezer.c
> index 3ea57e4..cdef2d8 100644
> --- a/kernel/cgroup_freezer.c
> +++ b/kernel/cgroup_freezer.c
> @@ -296,27 +296,22 @@ static int freezer_change_state(struct cgroup *cgroup,
>  	int retval = 0;
> 
>  	freezer = cgroup_freezer(cgroup);
> +
>  	spin_lock_irq(&freezer->lock);
> +
>  	update_freezer_state(cgroup, freezer);
>  	if (goal_state == freezer->state)
>  		goto out;
> -	switch (freezer->state) {
> +
> +	switch (goal_state) {
>  	case CGROUP_THAWED:
> -		retval = try_to_freeze_cgroup(cgroup, freezer);
> +		unfreeze_cgroup(cgroup, freezer);
>  		break;
> -	case CGROUP_FREEZING:
> -		if (goal_state == CGROUP_FROZEN) {
> -			/* Userspace is retrying after
> -			 * "/bin/echo FROZEN > freezer.state" returned -EBUSY */
> -			retval = try_to_freeze_cgroup(cgroup, freezer);
> -			break;
> -		}
> -		/* state == FREEZING and goal_state == THAWED, so unfreeze */
>  	case CGROUP_FROZEN:
> -		unfreeze_cgroup(cgroup, freezer);
> +		retval = try_to_freeze_cgroup(cgroup, freezer);
>  		break;
>  	default:
> -		break;
> +		BUG();
>  	}
>  out:
>  	spin_unlock_irq(&freezer->lock);

Re: [PATCH 3/4] freezer_cg: use thaw_process() in unfreeze_cgroup()

[Cedric Le Goater]

Li Zefan wrote:
> Don't duplicate the implementation of thaw_process().

looks OK but you should remove  __thaw_process() which is unused 
now.

thanks,

C.

> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
> ---
>  kernel/cgroup_freezer.c |   15 ++++-----------
>  1 files changed, 4 insertions(+), 11 deletions(-)
> 
> diff --git a/kernel/cgroup_freezer.c b/kernel/cgroup_freezer.c
> index 6fadafe..3ea57e4 100644
> --- a/kernel/cgroup_freezer.c
> +++ b/kernel/cgroup_freezer.c
> @@ -275,25 +275,18 @@ static int try_to_freeze_cgroup(struct cgroup *cgroup, struct freezer *freezer)
>  	return num_cant_freeze_now ? -EBUSY : 0;
>  }
>  
> -static int unfreeze_cgroup(struct cgroup *cgroup, struct freezer *freezer)
> +static void unfreeze_cgroup(struct cgroup *cgroup, struct freezer *freezer)
>  {
>  	struct cgroup_iter it;
>  	struct task_struct *task;
>  
>  	cgroup_iter_start(cgroup, &it);
>  	while ((task = cgroup_iter_next(cgroup, &it))) {
> -		int do_wake;
> -
> -		task_lock(task);
> -		do_wake = __thaw_process(task);
> -		task_unlock(task);
> -		if (do_wake)
> -			wake_up_process(task);
> +		thaw_process(task);
>  	}
>  	cgroup_iter_end(cgroup, &it);
> -	freezer->state = CGROUP_THAWED;
>  
> -	return 0;
> +	freezer->state = CGROUP_THAWED;
>  }
>  
>  static int freezer_change_state(struct cgroup *cgroup,
> @@ -320,7 +313,7 @@ static int freezer_change_state(struct cgroup *cgroup,
>  		}
>  		/* state == FREEZING and goal_state == THAWED, so unfreeze */
>  	case CGROUP_FROZEN:
> -		retval = unfreeze_cgroup(cgroup, freezer);
> +		unfreeze_cgroup(cgroup, freezer);
>  		break;
>  	default:
>  		break;

Re: [PATCH 3/4] freezer_cg: use thaw_process() in unfreeze_cgroup()

[Li Zefan]

Cedric Le Goater wrote:
> Li Zefan wrote:
>> Don't duplicate the implementation of thaw_process().
> 
> looks OK but you should remove  __thaw_process() which is unused 
> now.
> 

Then I'll make it a static function and remove the declaration from .h file.

Re: [PATCH 3/4] freezer_cg: use thaw_process() in unfreeze_cgroup()

[Andrew Morton]

On Tue, 21 Oct 2008 09:16:08 +0200
Cedric Le Goater <clg@fr.ibm.com> wrote:

> Li Zefan wrote:
> > Don't duplicate the implementation of thaw_process().
> 
> looks OK but you should remove  __thaw_process() which is unused 
> now.

It's called by thaw_process().

But that's the only callsite, I believe, so...

--- a/include/linux/freezer.h~freezer_cg-use-thaw_process-in-unfreeze_cgroup-fix
+++ a/include/linux/freezer.h
@@ -44,11 +44,6 @@ static inline bool should_send_signal(st
 	return !(p->flags & PF_FREEZER_NOSIG);
 }
 
-/*
- * Wake up a frozen process
- */
-extern int __thaw_process(struct task_struct *p);
-
 /* Takes and releases task alloc lock using task_lock() */
 extern int thaw_process(struct task_struct *p);
 
diff -puN kernel/freezer.c~freezer_cg-use-thaw_process-in-unfreeze_cgroup-fix kernel/freezer.c
--- a/kernel/freezer.c~freezer_cg-use-thaw_process-in-unfreeze_cgroup-fix
+++ a/kernel/freezer.c
@@ -121,16 +121,7 @@ void cancel_freezing(struct task_struct 
 	}
 }
 
-/*
- * Wake up a frozen process
- *
- * task_lock() is needed to prevent the race with refrigerator() which may
- * occur if the freezing of tasks fails.  Namely, without the lock, if the
- * freezing of tasks failed, thaw_tasks() might have run before a task in
- * refrigerator() could call frozen_process(), in which case the task would be
- * frozen and no one would thaw it.
- */
-int __thaw_process(struct task_struct *p)
+static int __thaw_process(struct task_struct *p)
 {
 	if (frozen(p)) {
 		p->flags &= ~PF_FROZEN;
@@ -140,6 +131,15 @@ int __thaw_process(struct task_struct *p
 	return 0;
 }
 
+/*
+ * Wake up a frozen process
+ *
+ * task_lock() is needed to prevent the race with refrigerator() which may
+ * occur if the freezing of tasks fails.  Namely, without the lock, if the
+ * freezing of tasks failed, thaw_tasks() might have run before a task in
+ * refrigerator() could call frozen_process(), in which case the task would be
+ * frozen and no one would thaw it.
+ */
 int thaw_process(struct task_struct *p)
 {
 	task_lock(p);
_

Re: [PATCH 3/4] freezer_cg: use thaw_process() in unfreeze_cgroup()

[Cedric Le Goater]

Andrew Morton wrote:
> On Tue, 21 Oct 2008 09:16:08 +0200
> Cedric Le Goater <clg@fr.ibm.com> wrote:
> 
>> Li Zefan wrote:
>>> Don't duplicate the implementation of thaw_process().
>> looks OK but you should remove  __thaw_process() which is unused 
>> now.
> 
> It's called by thaw_process().
> 
> But that's the only callsite, I believe, so...

yes it is. it was added by dc52ddc0e6f45b04780b26fc0813509f8e798c42
and was inline before.

thanks,

C.

> 
> --- a/include/linux/freezer.h~freezer_cg-use-thaw_process-in-unfreeze_cgroup-fix
> +++ a/include/linux/freezer.h
> @@ -44,11 +44,6 @@ static inline bool should_send_signal(st
>  	return !(p->flags & PF_FREEZER_NOSIG);
>  }
>  
> -/*
> - * Wake up a frozen process
> - */
> -extern int __thaw_process(struct task_struct *p);
> -
>  /* Takes and releases task alloc lock using task_lock() */
>  extern int thaw_process(struct task_struct *p);
>  
> diff -puN kernel/freezer.c~freezer_cg-use-thaw_process-in-unfreeze_cgroup-fix kernel/freezer.c
> --- a/kernel/freezer.c~freezer_cg-use-thaw_process-in-unfreeze_cgroup-fix
> +++ a/kernel/freezer.c
> @@ -121,16 +121,7 @@ void cancel_freezing(struct task_struct 
>  	}
>  }
>  
> -/*
> - * Wake up a frozen process
> - *
> - * task_lock() is needed to prevent the race with refrigerator() which may
> - * occur if the freezing of tasks fails.  Namely, without the lock, if the
> - * freezing of tasks failed, thaw_tasks() might have run before a task in
> - * refrigerator() could call frozen_process(), in which case the task would be
> - * frozen and no one would thaw it.
> - */
> -int __thaw_process(struct task_struct *p)
> +static int __thaw_process(struct task_struct *p)
>  {
>  	if (frozen(p)) {
>  		p->flags &= ~PF_FROZEN;
> @@ -140,6 +131,15 @@ int __thaw_process(struct task_struct *p
>  	return 0;
>  }
>  
> +/*
> + * Wake up a frozen process
> + *
> + * task_lock() is needed to prevent the race with refrigerator() which may
> + * occur if the freezing of tasks fails.  Namely, without the lock, if the
> + * freezing of tasks failed, thaw_tasks() might have run before a task in
> + * refrigerator() could call frozen_process(), in which case the task would be
> + * frozen and no one would thaw it.
> + */
>  int thaw_process(struct task_struct *p)
>  {
>  	task_lock(p);
> _
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

Re: [PATCH 3/4] freezer_cg: use thaw_process() in unfreeze_cgroup()

[Matt Helsley]

On Tue, 2008-10-21 at 08:51 +0800, Li Zefan wrote:
> Don't duplicate the implementation of thaw_process().
> 
> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>

Acked-by: Matt Helsley <matthltc@us.ibm.com>

> ---
>  kernel/cgroup_freezer.c |   15 ++++-----------
>  1 files changed, 4 insertions(+), 11 deletions(-)
> 
> diff --git a/kernel/cgroup_freezer.c b/kernel/cgroup_freezer.c
> index 6fadafe..3ea57e4 100644
> --- a/kernel/cgroup_freezer.c
> +++ b/kernel/cgroup_freezer.c
> @@ -275,25 +275,18 @@ static int try_to_freeze_cgroup(struct cgroup *cgroup, struct freezer *freezer)
>  	return num_cant_freeze_now ? -EBUSY : 0;
>  }
> 
> -static int unfreeze_cgroup(struct cgroup *cgroup, struct freezer *freezer)
> +static void unfreeze_cgroup(struct cgroup *cgroup, struct freezer *freezer)
>  {
>  	struct cgroup_iter it;
>  	struct task_struct *task;
> 
>  	cgroup_iter_start(cgroup, &it);
>  	while ((task = cgroup_iter_next(cgroup, &it))) {
> -		int do_wake;
> -
> -		task_lock(task);
> -		do_wake = __thaw_process(task);
> -		task_unlock(task);
> -		if (do_wake)
> -			wake_up_process(task);
> +		thaw_process(task);
>  	}
>  	cgroup_iter_end(cgroup, &it);
> -	freezer->state = CGROUP_THAWED;
> 
> -	return 0;
> +	freezer->state = CGROUP_THAWED;
>  }
> 
>  static int freezer_change_state(struct cgroup *cgroup,
> @@ -320,7 +313,7 @@ static int freezer_change_state(struct cgroup *cgroup,
>  		}
>  		/* state == FREEZING and goal_state == THAWED, so unfreeze */
>  	case CGROUP_FROZEN:
> -		retval = unfreeze_cgroup(cgroup, freezer);
> +		unfreeze_cgroup(cgroup, freezer);
>  		break;
>  	default:
>  		break;

Re: [PATCH 2/4] freezer_cg: remove redundant check in freezer_can_attach()

[Cedric Le Goater]

Li Zefan wrote:
> It is sufficient to check if @task is frozen, and no need to check if
> the original freezer is frozen.

hmm, a task being frozen does not mean that its freezer cgroup is 
frozen. So the extra check seems valid but looking at the comment :

	/*
	 * The call to cgroup_lock() in the freezer.state write method prevents
	 * a write to that file racing against an attach, and hence the
	 * can_attach() result will remain valid until the attach completes.
	 */
	static int freezer_can_attach(struct cgroup_subsys *ss,

how do we know that the task_freezer(task), which is not protected by
the cgroup_lock(), is not going to change its state to CGROUP_FROZEN 
it looks racy.

C.

> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
> ---
>  kernel/cgroup_freezer.c |   16 +++++++---------
>  1 files changed, 7 insertions(+), 9 deletions(-)
> 
> diff --git a/kernel/cgroup_freezer.c b/kernel/cgroup_freezer.c
> index 7f54d1c..6fadafe 100644
> --- a/kernel/cgroup_freezer.c
> +++ b/kernel/cgroup_freezer.c
> @@ -162,9 +162,13 @@ static int freezer_can_attach(struct cgroup_subsys *ss,
>  			      struct task_struct *task)
>  {
>  	struct freezer *freezer;
> -	int retval;
>  
> -	/* Anything frozen can't move or be moved to/from */
> +	/*
> +	 * Anything frozen can't move or be moved to/from.
> +	 *
> +	 * Since orig_freezer->state == FROZEN means that @task has been
> +	 * frozen, so it's sufficient to check the latter condition.
> +	 */
>  
>  	if (is_task_frozen_enough(task))
>  		return -EBUSY;
> @@ -173,13 +177,7 @@ static int freezer_can_attach(struct cgroup_subsys *ss,
>  	if (freezer->state == CGROUP_FROZEN)
>  		return -EBUSY;
>  
> -	retval = 0;
> -	task_lock(task);
> -	freezer = task_freezer(task);
> -	if (freezer->state == CGROUP_FROZEN)
> -		retval = -EBUSY;
> -	task_unlock(task);
> -	return retval;
> +	return 0;
>  }
>  
>  static void freezer_fork(struct cgroup_subsys *ss, struct task_struct *task)

Re: [PATCH 2/4] freezer_cg: remove redundant check in freezer_can_attach()

[Li Zefan]

Cedric Le Goater wrote:
> Li Zefan wrote:
>> It is sufficient to check if @task is frozen, and no need to check if
>> the original freezer is frozen.
> 
> hmm, a task being frozen does not mean that its freezer cgroup is 
> frozen.

If a task has being frozen, then can_attach() returns -EBUSY at once.
If a task isn't frozen, then we have orig_freezer->state == THAWED.

So we need to check the state of the task only.

> So the extra check seems valid but looking at the comment :
> 
> 	/*
> 	 * The call to cgroup_lock() in the freezer.state write method prevents
> 	 * a write to that file racing against an attach, and hence the
> 	 * can_attach() result will remain valid until the attach completes.
> 	 */
> 	static int freezer_can_attach(struct cgroup_subsys *ss,
> 
> how do we know that the task_freezer(task), which is not protected by
> the cgroup_lock(), is not going to change its state to CGROUP_FROZEN 
> it looks racy.
> 

Since freezer_can_attach() is called by cgroup core with cgroup_lock held, there is
no race with task attaching and state changing.

> C.
> 
>> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
>> ---
>>  kernel/cgroup_freezer.c |   16 +++++++---------
>>  1 files changed, 7 insertions(+), 9 deletions(-)
>>
>> diff --git a/kernel/cgroup_freezer.c b/kernel/cgroup_freezer.c
>> index 7f54d1c..6fadafe 100644
>> --- a/kernel/cgroup_freezer.c
>> +++ b/kernel/cgroup_freezer.c
>> @@ -162,9 +162,13 @@ static int freezer_can_attach(struct cgroup_subsys *ss,
>>  			      struct task_struct *task)
>>  {
>>  	struct freezer *freezer;
>> -	int retval;
>>  
>> -	/* Anything frozen can't move or be moved to/from */
>> +	/*
>> +	 * Anything frozen can't move or be moved to/from.
>> +	 *
>> +	 * Since orig_freezer->state == FROZEN means that @task has been
>> +	 * frozen, so it's sufficient to check the latter condition.
>> +	 */
>>  
>>  	if (is_task_frozen_enough(task))
>>  		return -EBUSY;
>> @@ -173,13 +177,7 @@ static int freezer_can_attach(struct cgroup_subsys *ss,
>>  	if (freezer->state == CGROUP_FROZEN)
>>  		return -EBUSY;
>>  
>> -	retval = 0;
>> -	task_lock(task);
>> -	freezer = task_freezer(task);
>> -	if (freezer->state == CGROUP_FROZEN)
>> -		retval = -EBUSY;
>> -	task_unlock(task);
>> -	return retval;
>> +	return 0;
>>  }
>>  
>>  static void freezer_fork(struct cgroup_subsys *ss, struct task_struct *task)
> 
> 

Re: [PATCH 2/4] freezer_cg: remove redundant check in freezer_can_attach()

[Cedric Le Goater]

Li Zefan wrote:
> Cedric Le Goater wrote:
>> Li Zefan wrote:
>>> It is sufficient to check if @task is frozen, and no need to check if
>>> the original freezer is frozen.
>> hmm, a task being frozen does not mean that its freezer cgroup is 
>> frozen.
> 
> If a task has being frozen, then can_attach() returns -EBUSY at once.
> If a task isn't frozen, then we have orig_freezer->state == THAWED.
> 
> So we need to check the state of the task only.
> 
>> So the extra check seems valid but looking at the comment :
>>
>> 	/*
>> 	 * The call to cgroup_lock() in the freezer.state write method prevents
>> 	 * a write to that file racing against an attach, and hence the
>> 	 * can_attach() result will remain valid until the attach completes.
>> 	 */
>> 	static int freezer_can_attach(struct cgroup_subsys *ss,
>>
>> how do we know that the task_freezer(task), which is not protected by
>> the cgroup_lock(), is not going to change its state to CGROUP_FROZEN 
>> it looks racy.
>>
> 
> Since freezer_can_attach() is called by cgroup core with cgroup_lock held, there is
> no race with task attaching and state changing.

ok I see. cgroup_mutex is global, I thought it had changed and that we 
were only locking the cgroup the task was being attached to. 

Acked-by: Cedric Le Goater <clg@fr.ibm.com>

thanks,

C. 

 

Re: [PATCH 2/4] freezer_cg: remove redundant check in freezer_can_attach()

[Matt Helsley]

On Tue, 2008-10-21 at 08:50 +0800, Li Zefan wrote:
> It is sufficient to check if @task is frozen, and no need to check if
> the original freezer is frozen.

Looks great! Thanks!

Acked-by: Matt Helsley <matthltc@us.ibm.com>

> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
> ---
>  kernel/cgroup_freezer.c |   16 +++++++---------
>  1 files changed, 7 insertions(+), 9 deletions(-)
> 
> diff --git a/kernel/cgroup_freezer.c b/kernel/cgroup_freezer.c
> index 7f54d1c..6fadafe 100644
> --- a/kernel/cgroup_freezer.c
> +++ b/kernel/cgroup_freezer.c
> @@ -162,9 +162,13 @@ static int freezer_can_attach(struct cgroup_subsys *ss,
>  			      struct task_struct *task)
>  {
>  	struct freezer *freezer;
> -	int retval;
> 
> -	/* Anything frozen can't move or be moved to/from */
> +	/*
> +	 * Anything frozen can't move or be moved to/from.
> +	 *
> +	 * Since orig_freezer->state == FROZEN means that @task has been
> +	 * frozen, so it's sufficient to check the latter condition.
> +	 */
> 
>  	if (is_task_frozen_enough(task))
>  		return -EBUSY;
> @@ -173,13 +177,7 @@ static int freezer_can_attach(struct cgroup_subsys *ss,
>  	if (freezer->state == CGROUP_FROZEN)
>  		return -EBUSY;
> 
> -	retval = 0;
> -	task_lock(task);
> -	freezer = task_freezer(task);
> -	if (freezer->state == CGROUP_FROZEN)
> -		retval = -EBUSY;
> -	task_unlock(task);
> -	return retval;
> +	return 0;
>  }
> 
>  static void freezer_fork(struct cgroup_subsys *ss, struct task_struct *task)

Re: [PATCH 1/4] freezer_cg: fix improper BUG_ON() causing oops

[Cedric Le Goater]

Li Zefan wrote:
> The BUG_ON() should be protected by freezer->lock, otherwise it
> can be triggered easily when a task has been unfreezed but the
> corresponding cgroup hasn't been changed to FROZEN state.

yes. thanks,

Acked-by: Cedric Le Goater <clg@fr.ibm.com>

> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
> ---
>  kernel/cgroup_freezer.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/kernel/cgroup_freezer.c b/kernel/cgroup_freezer.c
> index e950569..7f54d1c 100644
> --- a/kernel/cgroup_freezer.c
> +++ b/kernel/cgroup_freezer.c
> @@ -190,8 +190,9 @@ static void freezer_fork(struct cgroup_subsys *ss, struct task_struct *task)
>  	freezer = task_freezer(task);
>  	task_unlock(task);
>  
> -	BUG_ON(freezer->state == CGROUP_FROZEN);
>  	spin_lock_irq(&freezer->lock);
> +	BUG_ON(freezer->state == CGROUP_FROZEN);
> +
>  	/* Locking avoids race with FREEZING -> THAWED transitions. */
>  	if (freezer->state == CGROUP_FREEZING)
>  		freeze_task(task, true);

Re: [PATCH 1/4] freezer_cg: fix improper BUG_ON() causing oops

[Matt Helsley]

On Tue, 2008-10-21 at 08:48 +0800, Li Zefan wrote:
> The BUG_ON() should be protected by freezer->lock, otherwise it
> can be triggered easily when a task has been unfreezed but the
> corresponding cgroup hasn't been changed to FROZEN state.
> 
> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>

Acked-by: Matt Helsley <matthltc@us.ibm.com>

> ---
>  kernel/cgroup_freezer.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/kernel/cgroup_freezer.c b/kernel/cgroup_freezer.c
> index e950569..7f54d1c 100644
> --- a/kernel/cgroup_freezer.c
> +++ b/kernel/cgroup_freezer.c
> @@ -190,8 +190,9 @@ static void freezer_fork(struct cgroup_subsys *ss, struct task_struct *task)
>  	freezer = task_freezer(task);
>  	task_unlock(task);
> 
> -	BUG_ON(freezer->state == CGROUP_FROZEN);
>  	spin_lock_irq(&freezer->lock);
> +	BUG_ON(freezer->state == CGROUP_FROZEN);
> +
>  	/* Locking avoids race with FREEZING -> THAWED transitions. */
>  	if (freezer->state == CGROUP_FREEZING)
>  		freeze_task(task, true);