[PATCH] cgroup_freezer: Freezing and task move race fix

[PATCH] cgroup_freezer: Freezing and task move race fix

[Tomasz Buchert]

Writing 'FROZEN' to freezer.state file does not
forbid the task to be moved away from its cgroup
(for a very short time). Nevertheless the moved task
can become frozen OUTSIDE its cgroup which puts
discussed task in a permanent 'D' state.

This patch forbids migration of either FROZEN
or FREEZING tasks.

This behavior was observed and easily reproduced on
a single core laptop. Program and instructions how
to reproduce the bug can be fetched from:
http://pentium.hopto.org/~thinred/repos/linux-misc/freezer_bug.c

Signed-off-by: Tomasz Buchert <tomasz.buchert@inria.fr>
---
 kernel/cgroup_freezer.c |   10 ++++++++--
 1 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/kernel/cgroup_freezer.c b/kernel/cgroup_freezer.c
index ce71ed5..e49aa8c 100644
--- a/kernel/cgroup_freezer.c
+++ b/kernel/cgroup_freezer.c
@@ -161,6 +161,12 @@ static bool is_task_frozen_enough(struct task_struct *task)
 		(task_is_stopped_or_traced(task) && freezing(task));
 }
 
+/* Task is in a state that forbids any cgroup migration. */
+static bool is_task_pinned_down(struct task_struct *task)
+{
+	return freezing(task) || frozen(task);
+}
+
 /*
  * The call to cgroup_lock() in the freezer.state write method prevents
  * a write to that file racing against an attach, and hence the
@@ -179,7 +185,7 @@ static int freezer_can_attach(struct cgroup_subsys *ss,
 	 * frozen, so it's sufficient to check the latter condition.
 	 */
 
-	if (is_task_frozen_enough(task))
+	if (is_task_pinned_down(task))
 		return -EBUSY;
 
 	freezer = cgroup_freezer(new_cgroup);
@@ -191,7 +197,7 @@ static int freezer_can_attach(struct cgroup_subsys *ss,
 
 		rcu_read_lock();
 		list_for_each_entry_rcu(c, &task->thread_group, thread_group) {
-			if (is_task_frozen_enough(c)) {
+			if (is_task_pinned_down(c)) {
 				rcu_read_unlock();
 				return -EBUSY;
 			}
-- 
1.6.3.3

Re: [PATCH] cgroup_freezer: Freezing and task move race fix

[Matt Helsley]

On Tue, Aug 10, 2010 at 09:53:21PM +0200, Tomasz Buchert wrote:
> Writing 'FROZEN' to freezer.state file does not
> forbid the task to be moved away from its cgroup
> (for a very short time). Nevertheless the moved task
> can become frozen OUTSIDE its cgroup which puts
> discussed task in a permanent 'D' state.
> 
> This patch forbids migration of either FROZEN
> or FREEZING tasks.
> 
> This behavior was observed and easily reproduced on
> a single core laptop. Program and instructions how
> to reproduce the bug can be fetched from:
> http://pentium.hopto.org/~thinred/repos/linux-misc/freezer_bug.c

Thanks for the report and the test code.

I'm will try to reproduce this race in the next few hours and analyze
it since I'm not sure the patch really fixes the race -- it may only
make the race trigger less frequently.

At the very least the patch won't break the current code since it's
essentially a more-strict version of is_task_frozen_enough() -- it lets
fewer tasks attach/detach to/from frozen cgroups.

Cheers,
	-Matt Helsley

Re: [PATCH] cgroup_freezer: Freezing and task move race fix

[Tomasz Buchert]

Matt Helsley a écrit :
> On Tue, Aug 10, 2010 at 09:53:21PM +0200, Tomasz Buchert wrote:
>> Writing 'FROZEN' to freezer.state file does not
>> forbid the task to be moved away from its cgroup
>> (for a very short time). Nevertheless the moved task
>> can become frozen OUTSIDE its cgroup which puts
>> discussed task in a permanent 'D' state.
>>
>> This patch forbids migration of either FROZEN
>> or FREEZING tasks.
>>
>> This behavior was observed and easily reproduced on
>> a single core laptop. Program and instructions how
>> to reproduce the bug can be fetched from:
>> http://pentium.hopto.org/~thinred/repos/linux-misc/freezer_bug.c
> 
> Thanks for the report and the test code.
> 
> I'm will try to reproduce this race in the next few hours and analyze
> it since I'm not sure the patch really fixes the race -- it may only
> make the race trigger less frequently.
> 
> At the very least the patch won't break the current code since it's
> essentially a more-strict version of is_task_frozen_enough() -- it lets
> fewer tasks attach/detach to/from frozen cgroups.
> 
> Cheers,
> 	-Matt Helsley

Hi Matt!
I am a novice if it comes to the kernel and I find the cgroup_freezer
code especially complicated, so definetely this may be not enough to fix that.
Notice also that if you uncomment the line 55 in my testcase this will also
trigger the race! This, however, makes sense since process may not be in the cgroup anymore
and consequently won't be thawed.
I think that this patch fixes these problems because it does the flag checking in a right order:
first freezing() is used and then frozen() which assures (see frozen_process()) that
the race will not happen. Right? :)

Tomasz

Re: [PATCH] cgroup_freezer: Freezing and task move race fix

[Matt Helsley]

On Wed, Aug 11, 2010 at 12:18:44AM +0200, Tomasz Buchert wrote:
> Matt Helsley a écrit :
> > On Tue, Aug 10, 2010 at 09:53:21PM +0200, Tomasz Buchert wrote:
> >> Writing 'FROZEN' to freezer.state file does not
> >> forbid the task to be moved away from its cgroup
> >> (for a very short time). Nevertheless the moved task
> >> can become frozen OUTSIDE its cgroup which puts
> >> discussed task in a permanent 'D' state.
> >>
> >> This patch forbids migration of either FROZEN
> >> or FREEZING tasks.
> >>
> >> This behavior was observed and easily reproduced on
> >> a single core laptop. Program and instructions how
> >> to reproduce the bug can be fetched from:
> >> http://pentium.hopto.org/~thinred/repos/linux-misc/freezer_bug.c
> > 
> > Thanks for the report and the test code.
> > 
> > I'm will try to reproduce this race in the next few hours and analyze
> > it since I'm not sure the patch really fixes the race -- it may only
> > make the race trigger less frequently.
> > 
> > At the very least the patch won't break the current code since it's
> > essentially a more-strict version of is_task_frozen_enough() -- it lets
> > fewer tasks attach/detach to/from frozen cgroups.
> > 
> > Cheers,
> > 	-Matt Helsley
> 
> Hi Matt!
> I am a novice if it comes to the kernel and I find the cgroup_freezer
> code especially complicated, so definetely this may be not enough to fix that.
> Notice also that if you uncomment the line 55 in my testcase this will also
> trigger the race! This, however, makes sense since process may not be in the cgroup anymore
> and consequently won't be thawed.

OK, I triggered it with that. Interesting.

> I think that this patch fixes these problems because it does the flag checking in a right order:
> first freezing() is used and then frozen() which assures (see frozen_process()) that
> the race will not happen. Right? :)

I see what you mean. It still seems like it wouldn't actually fix the race -- just make it
harder to trigger. I think you're saying this is what happens without the patch:

Time	"bug" goes through these states		cgroup code checks for these states
-----------------------------------------------------------------------------------
|	freezing
|						is_frozen? Nope.
|	frozen
|						is_freezing? Nope.
|						<move>
V

But, without having carefully investigated the details, this could just as easily happen
with your patch:

Time	"bug" goes through these states		cgroup code checks for these states
-----------------------------------------------------------------------------------
|						is_freezing? Nope.
|						is_frozen? Nope.
|	freezing
|						<move>
|	frozen
V

or:

Time	"bug" goes through these states		cgroup code checks for these states
-----------------------------------------------------------------------------------
|						is_freezing? Nope.
|						is_frozen? Nope.
|	freezing
|	frozen
|						<move>
V

Time	"bug" goes through these states		cgroup code checks for these states
-----------------------------------------------------------------------------------
|						is_freezing? Nope.
|	freezing
|						is_frozen? Nope.
|						<move>
|	frozen
V

or:

Time	"bug" goes through these states		cgroup code checks for these states
-----------------------------------------------------------------------------------
|						is_freezing? Nope.
|	freezing
|						is_frozen? Nope.
|	frozen
|						<move>
V

(even with 1 cpu/core)

Your patch only improves things in the sense that it works for the first
example. We need to prevent the latter cases as well.

Cheers,
	-Matt

Re: [PATCH] cgroup_freezer: Freezing and task move race fix

[Tomasz Buchert]

Matt Helsley a écrit :
> On Wed, Aug 11, 2010 at 12:18:44AM +0200, Tomasz Buchert wrote:
>> Matt Helsley a écrit :
>>> On Tue, Aug 10, 2010 at 09:53:21PM +0200, Tomasz Buchert wrote:
>>>> Writing 'FROZEN' to freezer.state file does not
>>>> forbid the task to be moved away from its cgroup
>>>> (for a very short time). Nevertheless the moved task
>>>> can become frozen OUTSIDE its cgroup which puts
>>>> discussed task in a permanent 'D' state.
>>>>
>>>> This patch forbids migration of either FROZEN
>>>> or FREEZING tasks.
>>>>
>>>> This behavior was observed and easily reproduced on
>>>> a single core laptop. Program and instructions how
>>>> to reproduce the bug can be fetched from:
>>>> http://pentium.hopto.org/~thinred/repos/linux-misc/freezer_bug.c
>>> Thanks for the report and the test code.
>>>
>>> I'm will try to reproduce this race in the next few hours and analyze
>>> it since I'm not sure the patch really fixes the race -- it may only
>>> make the race trigger less frequently.
>>>
>>> At the very least the patch won't break the current code since it's
>>> essentially a more-strict version of is_task_frozen_enough() -- it lets
>>> fewer tasks attach/detach to/from frozen cgroups.
>>>
>>> Cheers,
>>> 	-Matt Helsley
>> Hi Matt!
>> I am a novice if it comes to the kernel and I find the cgroup_freezer
>> code especially complicated, so definetely this may be not enough to fix that.
>> Notice also that if you uncomment the line 55 in my testcase this will also
>> trigger the race! This, however, makes sense since process may not be in the cgroup anymore
>> and consequently won't be thawed.
> 
> OK, I triggered it with that. Interesting.
> 

Good!

>> I think that this patch fixes these problems because it does the flag checking in a right order:
>> first freezing() is used and then frozen() which assures (see frozen_process()) that
>> the race will not happen. Right? :)
> 
> I see what you mean. It still seems like it wouldn't actually fix the race -- just make it
> harder to trigger. I think you're saying this is what happens without the patch:
> 
> Time	"bug" goes through these states		cgroup code checks for these states
> -----------------------------------------------------------------------------------
> |	freezing
> |						is_frozen? Nope.
> |	frozen
> |						is_freezing? Nope.
> |						<move>
> V
> 
My first scenario was a bit different:
Time	"bug" goes through these states		cgroup code checks for these states
-----------------------------------------------------------------------------------
|	freezing
|						is_task_frozen_enough? Nope.
|						<move>
|	frozen
V
but the problem is the same.

> But, without having carefully investigated the details, this could just as easily happen
> with your patch:
> 
> Time	"bug" goes through these states		cgroup code checks for these states
> -----------------------------------------------------------------------------------
> |						is_freezing? Nope.
> |						is_frozen? Nope.
> |	freezing
> |						<move>
> |	frozen
> V
> 

This can't happen as far as I know because there is cgroup_lock around the code in freezer_write()
and freezer_can_attach().
The task can't enter 'freezing' state when can_attach is executed.

> or:
> 
> Time	"bug" goes through these states		cgroup code checks for these states
> -----------------------------------------------------------------------------------
> |						is_freezing? Nope.
> |						is_frozen? Nope.
> |	freezing
> |	frozen
> |						<move>
> V
> 

Same thing here.

> Time	"bug" goes through these states		cgroup code checks for these states
> -----------------------------------------------------------------------------------
> |						is_freezing? Nope.
> |	freezing
> |						is_frozen? Nope.
> |						<move>
> |	frozen
> V
> 

Again.

> or:
> 
> Time	"bug" goes through these states		cgroup code checks for these states
> -----------------------------------------------------------------------------------
> |						is_freezing? Nope.
> |	freezing
> |						is_frozen? Nope.
> |	frozen
> |						<move>
> V
> 
> (even with 1 cpu/core)

Well, once more.

> 
> Your patch only improves things in the sense that it works for the first
> example. We need to prevent the latter cases as well.
> 
> Cheers,
> 	-Matt

What do you think?

Tomasz

Re: [PATCH] cgroup_freezer: Freezing and task move race fix

[Matt Helsley]

On Wed, Aug 11, 2010 at 09:35:43AM +0200, Tomasz Buchert wrote:
> Matt Helsley a écrit :
> > On Wed, Aug 11, 2010 at 12:18:44AM +0200, Tomasz Buchert wrote:
> >> Matt Helsley a écrit :
> >>> On Tue, Aug 10, 2010 at 09:53:21PM +0200, Tomasz Buchert wrote:
> >>>> Writing 'FROZEN' to freezer.state file does not
> >>>> forbid the task to be moved away from its cgroup
> >>>> (for a very short time). Nevertheless the moved task
> >>>> can become frozen OUTSIDE its cgroup which puts
> >>>> discussed task in a permanent 'D' state.
> >>>>
> >>>> This patch forbids migration of either FROZEN
> >>>> or FREEZING tasks.
> >>>>
> >>>> This behavior was observed and easily reproduced on
> >>>> a single core laptop. Program and instructions how
> >>>> to reproduce the bug can be fetched from:
> >>>> http://pentium.hopto.org/~thinred/repos/linux-misc/freezer_bug.c
> >>> Thanks for the report and the test code.
> >>>
> >>> I'm will try to reproduce this race in the next few hours and analyze
> >>> it since I'm not sure the patch really fixes the race -- it may only
> >>> make the race trigger less frequently.
> >>>
> >>> At the very least the patch won't break the current code since it's
> >>> essentially a more-strict version of is_task_frozen_enough() -- it lets
> >>> fewer tasks attach/detach to/from frozen cgroups.
> >>>
> >>> Cheers,
> >>> 	-Matt Helsley
> >> Hi Matt!
> >> I am a novice if it comes to the kernel and I find the cgroup_freezer
> >> code especially complicated, so definetely this may be not enough to fix that.
> >> Notice also that if you uncomment the line 55 in my testcase this will also
> >> trigger the race! This, however, makes sense since process may not be in the cgroup anymore
> >> and consequently won't be thawed.
> > 
> > OK, I triggered it with that. Interesting.
> > 
> 
> Good!
> 
> >> I think that this patch fixes these problems because it does the flag checking in a right order:
> >> first freezing() is used and then frozen() which assures (see frozen_process()) that
> >> the race will not happen. Right? :)
> > 
> > I see what you mean. It still seems like it wouldn't actually fix the race -- just make it
> > harder to trigger. I think you're saying this is what happens without the patch:
> > 
> > Time	"bug" goes through these states		cgroup code checks for these states
> > -----------------------------------------------------------------------------------
> > |	freezing
> > |						is_frozen? Nope.
> > |	frozen
> > |						is_freezing? Nope.
> > |						<move>
> > V
> > 
> My first scenario was a bit different:
> Time	"bug" goes through these states		cgroup code checks for these states
> -----------------------------------------------------------------------------------
> |	freezing
> |						is_task_frozen_enough? Nope.
> |						<move>
> |	frozen
> V
> but the problem is the same.

I think I found a bug in the cgroup freezer which your patch fixes.
However I don't think it's a race.

/* Task is frozen or will freeze immediately when next it gets woken */
static bool is_task_frozen_enough(struct task_struct *task)
{
        return frozen(task) ||
                (task_is_stopped_or_traced(task) && freezing(task));
}

So it will only refuse to attach freezing tasks which have been stopped
or traced! Yet for attach we need to refuse to move _any_ freezing tasks.

Though stopped/traced _is_ relevant to the cgroup freezer state itself.
If we uses frozen(task) || freezing(task) to determine whether a cgroup
is frozen then it would be possible for the task to still be active
when the cgroup is finally reported FROZEN. However that's not possible
when the task is stopped/traced *and* freezing since even if woken it
won't exit the kernel before entering the refrigerator.

> > But, without having carefully investigated the details, this could just as easily happen
> > with your patch:
> > 
> > Time	"bug" goes through these states		cgroup code checks for these states
> > -----------------------------------------------------------------------------------
> > |						is_freezing? Nope.
> > |						is_frozen? Nope.
> > |	freezing
> > |						<move>
> > |	frozen
> > V
> > 
> 
> This can't happen as far as I know because there is cgroup_lock around the code in freezer_write()
> and freezer_can_attach().
> The task can't enter 'freezing' state when can_attach is executed.

You're right, cgroup_mutex should protect against that. However presumably
that same logic applies to the first case. So again I don't think the bug
is a race.

<snipped the remaining cases which should be the same>

At this point I think the bug description in your patch needs to change
but the patch itself is perfect. Assuming you agree with my assessment
of the bug I think the process is: you'll rewrite the description, add -stable
maintaners to your current Cc's (since this bug is simple, exists in previous
versions, and is somewhat nasty), add:

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

and send it to Andrew Morton. Hopefully then (if not before) Paul and Li
will ack it.

Thanks!

Cheers,
	-Matt Helsley

Re: [PATCH] cgroup_freezer: Freezing and task move race fix

[Tomasz Buchert]

Matt Helsley a écrit :
> On Wed, Aug 11, 2010 at 09:35:43AM +0200, Tomasz Buchert wrote:
>> Matt Helsley a écrit :
>>> On Wed, Aug 11, 2010 at 12:18:44AM +0200, Tomasz Buchert wrote:
>>>> Matt Helsley a écrit :
>>>>> On Tue, Aug 10, 2010 at 09:53:21PM +0200, Tomasz Buchert wrote:
>>>>>> Writing 'FROZEN' to freezer.state file does not
>>>>>> forbid the task to be moved away from its cgroup
>>>>>> (for a very short time). Nevertheless the moved task
>>>>>> can become frozen OUTSIDE its cgroup which puts
>>>>>> discussed task in a permanent 'D' state.
>>>>>>
>>>>>> This patch forbids migration of either FROZEN
>>>>>> or FREEZING tasks.
>>>>>>
>>>>>> This behavior was observed and easily reproduced on
>>>>>> a single core laptop. Program and instructions how
>>>>>> to reproduce the bug can be fetched from:
>>>>>> http://pentium.hopto.org/~thinred/repos/linux-misc/freezer_bug.c
>>>>> Thanks for the report and the test code.
>>>>>
>>>>> I'm will try to reproduce this race in the next few hours and analyze
>>>>> it since I'm not sure the patch really fixes the race -- it may only
>>>>> make the race trigger less frequently.
>>>>>
>>>>> At the very least the patch won't break the current code since it's
>>>>> essentially a more-strict version of is_task_frozen_enough() -- it lets
>>>>> fewer tasks attach/detach to/from frozen cgroups.
>>>>>
>>>>> Cheers,
>>>>> 	-Matt Helsley
>>>> Hi Matt!
>>>> I am a novice if it comes to the kernel and I find the cgroup_freezer
>>>> code especially complicated, so definetely this may be not enough to fix that.
>>>> Notice also that if you uncomment the line 55 in my testcase this will also
>>>> trigger the race! This, however, makes sense since process may not be in the cgroup anymore
>>>> and consequently won't be thawed.
>>> OK, I triggered it with that. Interesting.
>>>
>> Good!
>>
>>>> I think that this patch fixes these problems because it does the flag checking in a right order:
>>>> first freezing() is used and then frozen() which assures (see frozen_process()) that
>>>> the race will not happen. Right? :)
>>> I see what you mean. It still seems like it wouldn't actually fix the race -- just make it
>>> harder to trigger. I think you're saying this is what happens without the patch:
>>>
>>> Time	"bug" goes through these states		cgroup code checks for these states
>>> -----------------------------------------------------------------------------------
>>> |	freezing
>>> |						is_frozen? Nope.
>>> |	frozen
>>> |						is_freezing? Nope.
>>> |						<move>
>>> V
>>>
>> My first scenario was a bit different:
>> Time	"bug" goes through these states		cgroup code checks for these states
>> -----------------------------------------------------------------------------------
>> |	freezing
>> |						is_task_frozen_enough? Nope.
>> |						<move>
>> |	frozen
>> V
>> but the problem is the same.
> 
> I think I found a bug in the cgroup freezer which your patch fixes.
> However I don't think it's a race.
> 
> /* Task is frozen or will freeze immediately when next it gets woken */
> static bool is_task_frozen_enough(struct task_struct *task)
> {
>         return frozen(task) ||
>                 (task_is_stopped_or_traced(task) && freezing(task));
> }
> 
> So it will only refuse to attach freezing tasks which have been stopped
> or traced! Yet for attach we need to refuse to move _any_ freezing tasks.
> 
> Though stopped/traced _is_ relevant to the cgroup freezer state itself.
> If we uses frozen(task) || freezing(task) to determine whether a cgroup
> is frozen then it would be possible for the task to still be active
> when the cgroup is finally reported FROZEN. However that's not possible
> when the task is stopped/traced *and* freezing since even if woken it
> won't exit the kernel before entering the refrigerator.
> 
>>> But, without having carefully investigated the details, this could just as easily happen
>>> with your patch:
>>>
>>> Time	"bug" goes through these states		cgroup code checks for these states
>>> -----------------------------------------------------------------------------------
>>> |						is_freezing? Nope.
>>> |						is_frozen? Nope.
>>> |	freezing
>>> |						<move>
>>> |	frozen
>>> V
>>>
>> This can't happen as far as I know because there is cgroup_lock around the code in freezer_write()
>> and freezer_can_attach().
>> The task can't enter 'freezing' state when can_attach is executed.
> 
> You're right, cgroup_mutex should protect against that. However presumably
> that same logic applies to the first case. So again I don't think the bug
> is a race.
> 
> <snipped the remaining cases which should be the same>
> 
> At this point I think the bug description in your patch needs to change
> but the patch itself is perfect. Assuming you agree with my assessment
> of the bug I think the process is: you'll rewrite the description, add -stable
> maintaners to your current Cc's (since this bug is simple, exists in previous
> versions, and is somewhat nasty), add:
> 
> Reviewed-by: Matt Helsley <matthltc@us.ibm.com>
> Tested-by: Matt Helsley <matthltc@us.ibm.com>
> 
> and send it to Andrew Morton. Hopefully then (if not before) Paul and Li
> will ack it.
> 
> Thanks!
> 
> Cheers,
> 	-Matt Helsley

I agree with your assessment, Matt. The wrong the definition of being 'frozen enough'
allows a task to sneak out of its freezing cgroup.
The problem is that I found another, closely related bug. Right now, I have
a new queue of 3 patches to fix both bugs in a more general setting. They are not well tested
yet but I'm fairly confident that they do the right thing. I'm going to test them tomorrow.
Do you still want me to push the first patch? I propose to let me work a bit on
the new patches and prepare the code for the incoming fix to the related bug.
What is your opinion?

Tomasz

Re: [PATCH] cgroup_freezer: Freezing and task move race fix

[Matt Helsley]

On Thu, Aug 12, 2010 at 02:53:25AM +0200, Tomasz Buchert wrote:
> Matt Helsley a écrit :
> > On Wed, Aug 11, 2010 at 09:35:43AM +0200, Tomasz Buchert wrote:
> >> Matt Helsley a écrit :
> >>> On Wed, Aug 11, 2010 at 12:18:44AM +0200, Tomasz Buchert wrote:
> >>>> Matt Helsley a écrit :
> >>>>> On Tue, Aug 10, 2010 at 09:53:21PM +0200, Tomasz Buchert wrote:
> >>>>>> Writing 'FROZEN' to freezer.state file does not
> >>>>>> forbid the task to be moved away from its cgroup
> >>>>>> (for a very short time). Nevertheless the moved task
> >>>>>> can become frozen OUTSIDE its cgroup which puts
> >>>>>> discussed task in a permanent 'D' state.
> >>>>>>
> >>>>>> This patch forbids migration of either FROZEN
> >>>>>> or FREEZING tasks.
> >>>>>>
> >>>>>> This behavior was observed and easily reproduced on
> >>>>>> a single core laptop. Program and instructions how
> >>>>>> to reproduce the bug can be fetched from:
> >>>>>> http://pentium.hopto.org/~thinred/repos/linux-misc/freezer_bug.c
> >>>>> Thanks for the report and the test code.
> >>>>>
> >>>>> I'm will try to reproduce this race in the next few hours and analyze
> >>>>> it since I'm not sure the patch really fixes the race -- it may only
> >>>>> make the race trigger less frequently.
> >>>>>
> >>>>> At the very least the patch won't break the current code since it's
> >>>>> essentially a more-strict version of is_task_frozen_enough() -- it lets
> >>>>> fewer tasks attach/detach to/from frozen cgroups.
> >>>>>
> >>>>> Cheers,
> >>>>> 	-Matt Helsley
> >>>> Hi Matt!
> >>>> I am a novice if it comes to the kernel and I find the cgroup_freezer
> >>>> code especially complicated, so definetely this may be not enough to fix that.
> >>>> Notice also that if you uncomment the line 55 in my testcase this will also
> >>>> trigger the race! This, however, makes sense since process may not be in the cgroup anymore
> >>>> and consequently won't be thawed.
> >>> OK, I triggered it with that. Interesting.
> >>>
> >> Good!
> >>
> >>>> I think that this patch fixes these problems because it does the flag checking in a right order:
> >>>> first freezing() is used and then frozen() which assures (see frozen_process()) that
> >>>> the race will not happen. Right? :)
> >>> I see what you mean. It still seems like it wouldn't actually fix the race -- just make it
> >>> harder to trigger. I think you're saying this is what happens without the patch:
> >>>
> >>> Time	"bug" goes through these states		cgroup code checks for these states
> >>> -----------------------------------------------------------------------------------
> >>> |	freezing
> >>> |						is_frozen? Nope.
> >>> |	frozen
> >>> |						is_freezing? Nope.
> >>> |						<move>
> >>> V
> >>>
> >> My first scenario was a bit different:
> >> Time	"bug" goes through these states		cgroup code checks for these states
> >> -----------------------------------------------------------------------------------
> >> |	freezing
> >> |						is_task_frozen_enough? Nope.
> >> |						<move>
> >> |	frozen
> >> V
> >> but the problem is the same.
> > 
> > I think I found a bug in the cgroup freezer which your patch fixes.
> > However I don't think it's a race.
> > 
> > /* Task is frozen or will freeze immediately when next it gets woken */
> > static bool is_task_frozen_enough(struct task_struct *task)
> > {
> >         return frozen(task) ||
> >                 (task_is_stopped_or_traced(task) && freezing(task));
> > }
> > 
> > So it will only refuse to attach freezing tasks which have been stopped
> > or traced! Yet for attach we need to refuse to move _any_ freezing tasks.
> > 
> > Though stopped/traced _is_ relevant to the cgroup freezer state itself.
> > If we uses frozen(task) || freezing(task) to determine whether a cgroup
> > is frozen then it would be possible for the task to still be active
> > when the cgroup is finally reported FROZEN. However that's not possible
> > when the task is stopped/traced *and* freezing since even if woken it
> > won't exit the kernel before entering the refrigerator.
> > 
> >>> But, without having carefully investigated the details, this could just as easily happen
> >>> with your patch:
> >>>
> >>> Time	"bug" goes through these states		cgroup code checks for these states
> >>> -----------------------------------------------------------------------------------
> >>> |						is_freezing? Nope.
> >>> |						is_frozen? Nope.
> >>> |	freezing
> >>> |						<move>
> >>> |	frozen
> >>> V
> >>>
> >> This can't happen as far as I know because there is cgroup_lock around the code in freezer_write()
> >> and freezer_can_attach().
> >> The task can't enter 'freezing' state when can_attach is executed.
> > 
> > You're right, cgroup_mutex should protect against that. However presumably
> > that same logic applies to the first case. So again I don't think the bug
> > is a race.
> > 
> > <snipped the remaining cases which should be the same>
> > 
> > At this point I think the bug description in your patch needs to change
> > but the patch itself is perfect. Assuming you agree with my assessment
> > of the bug I think the process is: you'll rewrite the description, add -stable
> > maintaners to your current Cc's (since this bug is simple, exists in previous
> > versions, and is somewhat nasty), add:
> > 
> > Reviewed-by: Matt Helsley <matthltc@us.ibm.com>
> > Tested-by: Matt Helsley <matthltc@us.ibm.com>
> > 
> > and send it to Andrew Morton. Hopefully then (if not before) Paul and Li
> > will ack it.
> > 
> > Thanks!
> > 
> > Cheers,
> > 	-Matt Helsley
> 
> I agree with your assessment, Matt. The wrong the definition of being 'frozen enough'
> allows a task to sneak out of its freezing cgroup.
> The problem is that I found another, closely related bug. Right now, I have
> a new queue of 3 patches to fix both bugs in a more general setting. They are not well tested
> yet but I'm fairly confident that they do the right thing. I'm going to test them tomorrow.
> Do you still want me to push the first patch? I propose to let me work a bit on
> the new patches and prepare the code for the incoming fix to the related bug.
> What is your opinion?

OK, I'll have a look at the 3 new patches and your test(s) then we can discuss
what to do.

Cheers,
	-Matt

Re: [PATCH] cgroup_freezer: Freezing and task move race fix

[Tomasz Buchert]

Le 12/08/2010 22:13, Matt Helsley a écrit :
> On Thu, Aug 12, 2010 at 02:53:25AM +0200, Tomasz Buchert wrote:
>> Matt Helsley a écrit :
>>> On Wed, Aug 11, 2010 at 09:35:43AM +0200, Tomasz Buchert wrote:
>>>> Matt Helsley a écrit :
>>>>> On Wed, Aug 11, 2010 at 12:18:44AM +0200, Tomasz Buchert wrote:
>>>>>> Matt Helsley a écrit :
>>>>>>> On Tue, Aug 10, 2010 at 09:53:21PM +0200, Tomasz Buchert wrote:
>>>>>>>> Writing 'FROZEN' to freezer.state file does not
>>>>>>>> forbid the task to be moved away from its cgroup
>>>>>>>> (for a very short time). Nevertheless the moved task
>>>>>>>> can become frozen OUTSIDE its cgroup which puts
>>>>>>>> discussed task in a permanent 'D' state.
>>>>>>>>
>>>>>>>> This patch forbids migration of either FROZEN
>>>>>>>> or FREEZING tasks.
>>>>>>>>
>>>>>>>> This behavior was observed and easily reproduced on
>>>>>>>> a single core laptop. Program and instructions how
>>>>>>>> to reproduce the bug can be fetched from:
>>>>>>>> http://pentium.hopto.org/~thinred/repos/linux-misc/freezer_bug.c
>>>>>>> Thanks for the report and the test code.
>>>>>>>
>>>>>>> I'm will try to reproduce this race in the next few hours and analyze
>>>>>>> it since I'm not sure the patch really fixes the race -- it may only
>>>>>>> make the race trigger less frequently.
>>>>>>>
>>>>>>> At the very least the patch won't break the current code since it's
>>>>>>> essentially a more-strict version of is_task_frozen_enough() -- it lets
>>>>>>> fewer tasks attach/detach to/from frozen cgroups.
>>>>>>>
>>>>>>> Cheers,
>>>>>>> 	-Matt Helsley
>>>>>> Hi Matt!
>>>>>> I am a novice if it comes to the kernel and I find the cgroup_freezer
>>>>>> code especially complicated, so definetely this may be not enough to fix that.
>>>>>> Notice also that if you uncomment the line 55 in my testcase this will also
>>>>>> trigger the race! This, however, makes sense since process may not be in the cgroup anymore
>>>>>> and consequently won't be thawed.
>>>>> OK, I triggered it with that. Interesting.
>>>>>
>>>> Good!
>>>>
>>>>>> I think that this patch fixes these problems because it does the flag checking in a right order:
>>>>>> first freezing() is used and then frozen() which assures (see frozen_process()) that
>>>>>> the race will not happen. Right? :)
>>>>> I see what you mean. It still seems like it wouldn't actually fix the race -- just make it
>>>>> harder to trigger. I think you're saying this is what happens without the patch:
>>>>>
>>>>> Time	"bug" goes through these states		cgroup code checks for these states
>>>>> -----------------------------------------------------------------------------------
>>>>> |	freezing
>>>>> |						is_frozen? Nope.
>>>>> |	frozen
>>>>> |						is_freezing? Nope.
>>>>> |						<move>
>>>>> V
>>>>>
>>>> My first scenario was a bit different:
>>>> Time	"bug" goes through these states		cgroup code checks for these states
>>>> -----------------------------------------------------------------------------------
>>>> |	freezing
>>>> |						is_task_frozen_enough? Nope.
>>>> |						<move>
>>>> |	frozen
>>>> V
>>>> but the problem is the same.
>>>
>>> I think I found a bug in the cgroup freezer which your patch fixes.
>>> However I don't think it's a race.
>>>
>>> /* Task is frozen or will freeze immediately when next it gets woken */
>>> static bool is_task_frozen_enough(struct task_struct *task)
>>> {
>>>          return frozen(task) ||
>>>                  (task_is_stopped_or_traced(task)&&  freezing(task));
>>> }
>>>
>>> So it will only refuse to attach freezing tasks which have been stopped
>>> or traced! Yet for attach we need to refuse to move _any_ freezing tasks.
>>>
>>> Though stopped/traced _is_ relevant to the cgroup freezer state itself.
>>> If we uses frozen(task) || freezing(task) to determine whether a cgroup
>>> is frozen then it would be possible for the task to still be active
>>> when the cgroup is finally reported FROZEN. However that's not possible
>>> when the task is stopped/traced *and* freezing since even if woken it
>>> won't exit the kernel before entering the refrigerator.
>>>
>>>>> But, without having carefully investigated the details, this could just as easily happen
>>>>> with your patch:
>>>>>
>>>>> Time	"bug" goes through these states		cgroup code checks for these states
>>>>> -----------------------------------------------------------------------------------
>>>>> |						is_freezing? Nope.
>>>>> |						is_frozen? Nope.
>>>>> |	freezing
>>>>> |						<move>
>>>>> |	frozen
>>>>> V
>>>>>
>>>> This can't happen as far as I know because there is cgroup_lock around the code in freezer_write()
>>>> and freezer_can_attach().
>>>> The task can't enter 'freezing' state when can_attach is executed.
>>>
>>> You're right, cgroup_mutex should protect against that. However presumably
>>> that same logic applies to the first case. So again I don't think the bug
>>> is a race.
>>>
>>> <snipped the remaining cases which should be the same>
>>>
>>> At this point I think the bug description in your patch needs to change
>>> but the patch itself is perfect. Assuming you agree with my assessment
>>> of the bug I think the process is: you'll rewrite the description, add -stable
>>> maintaners to your current Cc's (since this bug is simple, exists in previous
>>> versions, and is somewhat nasty), add:
>>>
>>> Reviewed-by: Matt Helsley<matthltc@us.ibm.com>
>>> Tested-by: Matt Helsley<matthltc@us.ibm.com>
>>>
>>> and send it to Andrew Morton. Hopefully then (if not before) Paul and Li
>>> will ack it.
>>>
>>> Thanks!
>>>
>>> Cheers,
>>> 	-Matt Helsley
>>
>> I agree with your assessment, Matt. The wrong the definition of being 'frozen enough'
>> allows a task to sneak out of its freezing cgroup.
>> The problem is that I found another, closely related bug. Right now, I have
>> a new queue of 3 patches to fix both bugs in a more general setting. They are not well tested
>> yet but I'm fairly confident that they do the right thing. I'm going to test them tomorrow.
>> Do you still want me to push the first patch? I propose to let me work a bit on
>> the new patches and prepare the code for the incoming fix to the related bug.
>> What is your opinion?
>
> OK, I'll have a look at the 3 new patches and your test(s) then we can discuss
> what to do.
>
> Cheers,
> 	-Matt

Matt,
Am I supposed to do something right now? The discussion
became a bit quiet recently.

Cheers,
Tomasz

Re: [PATCH] cgroup_freezer: Freezing and task move race fix

[Matt Helsley]

On Wed, Aug 18, 2010 at 03:13:02AM +0200, Tomasz Buchert wrote:
> Le 12/08/2010 22:13, Matt Helsley a écrit :

<snip>

> 
> Matt,
> Am I supposed to do something right now? The discussion
> became a bit quiet recently.
> 
> Cheers,
> Tomasz

Hi Tomasz,

	I'm sorry. I haven't had much time to review your latest
patches and comment. I thought Andrew put them in his -mm tree
so I don't think you have anything to do at the moment.

Cheers,
	-Matt

Re: [PATCH] cgroup_freezer: Freezing and task move race fix

[Tomasz Buchert]

Le 18/08/2010 04:22, Matt Helsley a écrit :
> On Wed, Aug 18, 2010 at 03:13:02AM +0200, Tomasz Buchert wrote:
>> Le 12/08/2010 22:13, Matt Helsley a écrit :
>
> <snip>
>
>>
>> Matt,
>> Am I supposed to do something right now? The discussion
>> became a bit quiet recently.
>>
>> Cheers,
>> Tomasz
>
> Hi Tomasz,
>
> 	I'm sorry. I haven't had much time to review your latest
> patches and comment. I thought Andrew put them in his -mm tree
> so I don't think you have anything to do at the moment.
>
> Cheers,
> 	-Matt

Take your time, Matt. I only wish that eventually the problem will be 
fixed, that or another way.

Tomek

Re: [PATCH] cgroup_freezer: Freezing and task move race fix

[Rafael J. Wysocki]

On Thursday, August 12, 2010, Matt Helsley wrote:
> On Wed, Aug 11, 2010 at 09:35:43AM +0200, Tomasz Buchert wrote:
> > Matt Helsley a écrit :
...
> You're right, cgroup_mutex should protect against that. However presumably
> that same logic applies to the first case. So again I don't think the bug
> is a race.
> 
> <snipped the remaining cases which should be the same>
> 
> At this point I think the bug description in your patch needs to change
> but the patch itself is perfect. Assuming you agree with my assessment
> of the bug I think the process is: you'll rewrite the description, add -stable
> maintaners to your current Cc's (since this bug is simple, exists in previous
> versions, and is somewhat nasty), add:
> 
> Reviewed-by: Matt Helsley <matthltc@us.ibm.com>
> Tested-by: Matt Helsley <matthltc@us.ibm.com>
> 
> and send it to Andrew Morton. Hopefully then (if not before) Paul and Li
> will ack it.

Actaully, I maintain the freezer, so please send it to me.

Thanks,
Rafael

Re: [PATCH] cgroup_freezer: Freezing and task move race fix

[Matt Helsley]

On Tue, Aug 10, 2010 at 09:53:21PM +0200, Tomasz Buchert wrote:
> Writing 'FROZEN' to freezer.state file does not
> forbid the task to be moved away from its cgroup
> (for a very short time). Nevertheless the moved task
> can become frozen OUTSIDE its cgroup which puts
> discussed task in a permanent 'D' state.

SUMMARY (for supporting info see the "DETAILS" heading below)

I can't reproduce this.

My preliminary conclusion is that your testcase doesn't really reproduce
what you described. Instead, your testcase prints an incorrect message which
could easily lead the person running it to the wrong conclusion.

DETAILS

I tried it with and without the cpuset portions of the testcase on a dual
core bare metal system with a 2.6.31-derived distro kernel. Since it
*should* be obeying the cpuset portions of the testcase I don't think the
fact that it's dual-core should make a difference.

I also tried with a 2.6.34-rc5 kernel in a single-cpu kvm on a different
distro (but on the same physical hardware). I also tried it with and without
the sleep(1) in the child's for(;;) loop in case the cpu load somehow enabled
me to trigger the race.

I used the following bash snippet to run the testcase variations and did not
observe any tasks in the D state:

mount -t cgroup -o freezer,cpuset none /cg
while /bin/true ; do
	./bug /cg
	ps -C bug -o state= | grep D && break
done

If there is a race then I should be able to run that ps line anytime afterwards
to see the stuck task.

Note that the test message:

	printf("Succesfully moved frozen task!\n");

is bogus. In fact there is no guarantee the task or cgroup is frozen at that
point in the testcase. This should be apparent from a careful reading of
Documentation/cgroups/freezer-subsystem.txt, especially:

	This is the basic mechanism which should do the right thing for user space task
	in a simple scenario.

	It's important to note that freezing can be incomplete. In that case we return
	EBUSY. This means that some tasks in the cgroup are busy doing something that
	prevents us from completely freezing the cgroup at this time. After EBUSY,
	the cgroup will remain partially frozen -- reflected by freezer.state reporting
	"FREEZING" when read. The state will remain "FREEZING" until one of these
	things happens:

		1) Userspace cancels the freezing operation by writing "THAWED" to
			the freezer.state file
		2) Userspace retries the freezing operation by writing "FROZEN" to
			the freezer.state file (writing "FREEZING" is not legal
			and returns EINVAL)
		3) The tasks that blocked the cgroup from entering the "FROZEN"
			state disappear from the cgroup's set of tasks.

So simply writing FROZEN to freezer.state is necessary to initiate freezing
but insufficient to assert that the task and/or cgroup is frozen.
That's why the FREEZING state exists. It's intentionally not specified when/why
we can't immediately enter FROZEN. Thus userspace must read the freezer.state
to determine if the current state matches the requested/expected state.

This is why I have the extra ps step in the script above -- to determine
if the task is actually in D. I should also check that the cgroup it belongs
to is THAWED. However while attempting to reproduce your report that hasn't
been necessary -- none of the tasks have even entered the D state.

Which brings us to the final portion of this analysis: Why isn't anything
entering the D state?

The behavior I have been able to reproduce and which is not a bug is
moving the task immediately after writing FROZEN to freezer.state. We don't
know the state of the task or cgroup at that time (in this testcase) so
this is acceptable. I've even made a sequence of modifications to your
testcase and run it after each modification to bring it successively more
in line with correct use of the cgroup freezer. I still was unable to
reproduce your report.

So I'm fairly confident there is no bug. I say "fairly" because there may
be some aspect of your system that I am not reproducing. At this point it
would be great if you could provide more details so I can more thoroughly
attempt to recreate your conditions.

Cheers,
	-Matt Helsley

> 
> This patch forbids migration of either FROZEN
> or FREEZING tasks.
> 
> This behavior was observed and easily reproduced on
> a single core laptop. Program and instructions how
> to reproduce the bug can be fetched from:
> http://pentium.hopto.org/~thinred/repos/linux-misc/freezer_bug.c
> 
> Signed-off-by: Tomasz Buchert <tomasz.buchert@inria.fr>
> ---
>  kernel/cgroup_freezer.c |   10 ++++++++--
>  1 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/cgroup_freezer.c b/kernel/cgroup_freezer.c
> index ce71ed5..e49aa8c 100644
> --- a/kernel/cgroup_freezer.c
> +++ b/kernel/cgroup_freezer.c
> @@ -161,6 +161,12 @@ static bool is_task_frozen_enough(struct task_struct *task)
>  		(task_is_stopped_or_traced(task) && freezing(task));
>  }
> 
> +/* Task is in a state that forbids any cgroup migration. */
> +static bool is_task_pinned_down(struct task_struct *task)
> +{
> +	return freezing(task) || frozen(task);
> +}
> +
>  /*
>   * The call to cgroup_lock() in the freezer.state write method prevents
>   * a write to that file racing against an attach, and hence the
> @@ -179,7 +185,7 @@ static int freezer_can_attach(struct cgroup_subsys *ss,
>  	 * frozen, so it's sufficient to check the latter condition.
>  	 */
> 
> -	if (is_task_frozen_enough(task))
> +	if (is_task_pinned_down(task))
>  		return -EBUSY;
> 
>  	freezer = cgroup_freezer(new_cgroup);
> @@ -191,7 +197,7 @@ static int freezer_can_attach(struct cgroup_subsys *ss,
> 
>  		rcu_read_lock();
>  		list_for_each_entry_rcu(c, &task->thread_group, thread_group) {
> -			if (is_task_frozen_enough(c)) {
> +			if (is_task_pinned_down(c)) {
>  				rcu_read_unlock();
>  				return -EBUSY;
>  			}
> -- 
> 1.6.3.3
> 
> _______________________________________________
> Containers mailing list
> Containers@lists.linux-foundation.org
> https://lists.linux-foundation.org/mailman/listinfo/containers

Re: [PATCH] cgroup_freezer: Freezing and task move race fix

[Tomasz Buchert]

Matt Helsley a écrit :
> On Tue, Aug 10, 2010 at 09:53:21PM +0200, Tomasz Buchert wrote:
>> Writing 'FROZEN' to freezer.state file does not
>> forbid the task to be moved away from its cgroup
>> (for a very short time). Nevertheless the moved task
>> can become frozen OUTSIDE its cgroup which puts
>> discussed task in a permanent 'D' state.
> 
> SUMMARY (for supporting info see the "DETAILS" heading below)
> 
> I can't reproduce this.
> 
> My preliminary conclusion is that your testcase doesn't really reproduce
> what you described. Instead, your testcase prints an incorrect message which
> could easily lead the person running it to the wrong conclusion.
> 
> DETAILS
> 
> I tried it with and without the cpuset portions of the testcase on a dual
> core bare metal system with a 2.6.31-derived distro kernel. Since it
> *should* be obeying the cpuset portions of the testcase I don't think the
> fact that it's dual-core should make a difference.
> 
> I also tried with a 2.6.34-rc5 kernel in a single-cpu kvm on a different
> distro (but on the same physical hardware). I also tried it with and without
> the sleep(1) in the child's for(;;) loop in case the cpu load somehow enabled
> me to trigger the race.
> 
> I used the following bash snippet to run the testcase variations and did not
> observe any tasks in the D state:
> 
> mount -t cgroup -o freezer,cpuset none /cg
> while /bin/true ; do
> 	./bug /cg
> 	ps -C bug -o state= | grep D && break
> done
> 
> If there is a race then I should be able to run that ps line anytime afterwards
> to see the stuck task.
> 
> Note that the test message:
> 
> 	printf("Succesfully moved frozen task!\n");
> 
> is bogus. In fact there is no guarantee the task or cgroup is frozen at that
> point in the testcase. This should be apparent from a careful reading of
> Documentation/cgroups/freezer-subsystem.txt, especially:
> 
> 	This is the basic mechanism which should do the right thing for user space task
> 	in a simple scenario.
> 
> 	It's important to note that freezing can be incomplete. In that case we return
> 	EBUSY. This means that some tasks in the cgroup are busy doing something that
> 	prevents us from completely freezing the cgroup at this time. After EBUSY,
> 	the cgroup will remain partially frozen -- reflected by freezer.state reporting
> 	"FREEZING" when read. The state will remain "FREEZING" until one of these
> 	things happens:
> 
> 		1) Userspace cancels the freezing operation by writing "THAWED" to
> 			the freezer.state file
> 		2) Userspace retries the freezing operation by writing "FROZEN" to
> 			the freezer.state file (writing "FREEZING" is not legal
> 			and returns EINVAL)
> 		3) The tasks that blocked the cgroup from entering the "FROZEN"
> 			state disappear from the cgroup's set of tasks.
> 
> So simply writing FROZEN to freezer.state is necessary to initiate freezing
> but insufficient to assert that the task and/or cgroup is frozen.
> That's why the FREEZING state exists. It's intentionally not specified when/why
> we can't immediately enter FROZEN. Thus userspace must read the freezer.state
> to determine if the current state matches the requested/expected state.
> 
> This is why I have the extra ps step in the script above -- to determine
> if the task is actually in D. I should also check that the cgroup it belongs
> to is THAWED. However while attempting to reproduce your report that hasn't
> been necessary -- none of the tasks have even entered the D state.
> 
> Which brings us to the final portion of this analysis: Why isn't anything
> entering the D state?
> 
> The behavior I have been able to reproduce and which is not a bug is
> moving the task immediately after writing FROZEN to freezer.state. We don't
> know the state of the task or cgroup at that time (in this testcase) so
> this is acceptable. I've even made a sequence of modifications to your
> testcase and run it after each modification to bring it successively more
> in line with correct use of the cgroup freezer. I still was unable to
> reproduce your report.
> 
> So I'm fairly confident there is no bug. I say "fairly" because there may
> be some aspect of your system that I am not reproducing. At this point it
> would be great if you could provide more details so I can more thoroughly
> attempt to recreate your conditions.
> 
> Cheers,
> 	-Matt Helsley
> 

Maybe this is a problem with different timings. I have a qemu minimal image
with two different kernels 2.6.35 - vanilla and patached. I don't use kvm
with qemu though. You can get it from here:
	http://pentium.hopto.org/~thinred/files/qemu.tar.bz2
in the package there is also minimal '.config' for current Linus's tree.
You run it with ./run-linux <bzImageOfTheKernel>. In /root you will have
'minimal' program which is my testcase. The small problem with this image is that
ps segfaults but it's not a big problem. :)

I am also aware that:
	printf("Succesfully moved frozen task!\n");
does not show that task was moved. Only the message is just wrong. What I wanted to observe
is that the kernel actually ALLOWED to start the process of freezing.
When you run the testcase with 'strace' you'll see that 'write' returns a positive number
instead of -EBUSY. And that's the bug.

Anyway, I deduce from your later mail that you actually reproduced the problem.

Cheers,
Tomasz

Re: [PATCH] cgroup_freezer: Freezing and task move race fix

[Tomasz Buchert]

Tomasz Buchert a écrit :
> Matt Helsley a écrit :
>> On Tue, Aug 10, 2010 at 09:53:21PM +0200, Tomasz Buchert wrote:
>>> Writing 'FROZEN' to freezer.state file does not
>>> forbid the task to be moved away from its cgroup
>>> (for a very short time). Nevertheless the moved task
>>> can become frozen OUTSIDE its cgroup which puts
>>> discussed task in a permanent 'D' state.
>> SUMMARY (for supporting info see the "DETAILS" heading below)
>>
>> I can't reproduce this.
>>
>> My preliminary conclusion is that your testcase doesn't really reproduce
>> what you described. Instead, your testcase prints an incorrect message which
>> could easily lead the person running it to the wrong conclusion.
>>
>> DETAILS
>>
>> I tried it with and without the cpuset portions of the testcase on a dual
>> core bare metal system with a 2.6.31-derived distro kernel. Since it
>> *should* be obeying the cpuset portions of the testcase I don't think the
>> fact that it's dual-core should make a difference.
>>
>> I also tried with a 2.6.34-rc5 kernel in a single-cpu kvm on a different
>> distro (but on the same physical hardware). I also tried it with and without
>> the sleep(1) in the child's for(;;) loop in case the cpu load somehow enabled
>> me to trigger the race.
>>
>> I used the following bash snippet to run the testcase variations and did not
>> observe any tasks in the D state:
>>
>> mount -t cgroup -o freezer,cpuset none /cg
>> while /bin/true ; do
>> 	./bug /cg
>> 	ps -C bug -o state= | grep D && break
>> done
>>
>> If there is a race then I should be able to run that ps line anytime afterwards
>> to see the stuck task.
>>
>> Note that the test message:
>>
>> 	printf("Succesfully moved frozen task!\n");
>>
>> is bogus. In fact there is no guarantee the task or cgroup is frozen at that
>> point in the testcase. This should be apparent from a careful reading of
>> Documentation/cgroups/freezer-subsystem.txt, especially:
>>
>> 	This is the basic mechanism which should do the right thing for user space task
>> 	in a simple scenario.
>>
>> 	It's important to note that freezing can be incomplete. In that case we return
>> 	EBUSY. This means that some tasks in the cgroup are busy doing something that
>> 	prevents us from completely freezing the cgroup at this time. After EBUSY,
>> 	the cgroup will remain partially frozen -- reflected by freezer.state reporting
>> 	"FREEZING" when read. The state will remain "FREEZING" until one of these
>> 	things happens:
>>
>> 		1) Userspace cancels the freezing operation by writing "THAWED" to
>> 			the freezer.state file
>> 		2) Userspace retries the freezing operation by writing "FROZEN" to
>> 			the freezer.state file (writing "FREEZING" is not legal
>> 			and returns EINVAL)
>> 		3) The tasks that blocked the cgroup from entering the "FROZEN"
>> 			state disappear from the cgroup's set of tasks.
>>
>> So simply writing FROZEN to freezer.state is necessary to initiate freezing
>> but insufficient to assert that the task and/or cgroup is frozen.
>> That's why the FREEZING state exists. It's intentionally not specified when/why
>> we can't immediately enter FROZEN. Thus userspace must read the freezer.state
>> to determine if the current state matches the requested/expected state.
>>
>> This is why I have the extra ps step in the script above -- to determine
>> if the task is actually in D. I should also check that the cgroup it belongs
>> to is THAWED. However while attempting to reproduce your report that hasn't
>> been necessary -- none of the tasks have even entered the D state.
>>
>> Which brings us to the final portion of this analysis: Why isn't anything
>> entering the D state?
>>
>> The behavior I have been able to reproduce and which is not a bug is
>> moving the task immediately after writing FROZEN to freezer.state. We don't
>> know the state of the task or cgroup at that time (in this testcase) so
>> this is acceptable. I've even made a sequence of modifications to your
>> testcase and run it after each modification to bring it successively more
>> in line with correct use of the cgroup freezer. I still was unable to
>> reproduce your report.
>>
>> So I'm fairly confident there is no bug. I say "fairly" because there may
>> be some aspect of your system that I am not reproducing. At this point it
>> would be great if you could provide more details so I can more thoroughly
>> attempt to recreate your conditions.
>>
>> Cheers,
>> 	-Matt Helsley
>>
> 
> Maybe this is a problem with different timings. I have a qemu minimal image
> with two different kernels 2.6.35 - vanilla and patached. I don't use kvm
> with qemu though. You can get it from here:
> 	http://pentium.hopto.org/~thinred/files/qemu.tar.bz2
> in the package there is also minimal '.config' for current Linus's tree.
> You run it with ./run-linux <bzImageOfTheKernel>. In /root you will have
> 'minimal' program which is my testcase. The small problem with this image is that
> ps segfaults but it's not a big problem. :)
> 
> I am also aware that:
> 	printf("Succesfully moved frozen task!\n");
> does not show that task was moved. Only the message is just wrong. What I wanted to observe
> is that the kernel actually ALLOWED to start the process of freezing.
> When you run the testcase with 'strace' you'll see that 'write' returns a positive number
> instead of -EBUSY. And that's the bug.
> 
> Anyway, I deduce from your later mail that you actually reproduced the problem.
> 
> Cheers,
> Tomasz
> 
> 
The penultimate paragraph should be:

I am also aware that:
	printf("Succesfully moved frozen task!\n");
does not show that task was moved. Only the message is just wrong. What I wanted to observe
is that the kernel actually ALLOWED to start the process of **MOVING**.
When you run the testcase with 'strace' you'll see that 'write' returns a positive number
instead of -EBUSY. And that's the bug.

Cheers again!
Tomasz

[PATCH 0/3] Two bugfixes for cgroup freezer.

[Tomasz Buchert]

These patches fixes two bugs I discovered when working with
the cgroup freezer. These patches fix them (and probably also
some other variations of them) and now the kernel passes
testcases that uncovered the bugs in the first place.

Please review.

Tomasz

Tomasz Buchert (3):
  cgroup_freezer: Unnecessary test in cgroup_freezing_or_frozen
  cgroup_freezer: Fix can_attach to prohibit moving from/to
    freezing/frozen cgroups
  cgroup_freezer: update_freezer_state does incorrect state
    transactions

 kernel/cgroup_freezer.c |   74 +++++++++++++++++++++-------------------------
 1 files changed, 34 insertions(+), 40 deletions(-)

[PATCH 1/3] cgroup_freezer: Unnecessary test in cgroup_freezing_or_frozen

[Tomasz Buchert]

The root freezer_state is always CGROUP_THAWED so we can remove
the special case from the code. The test itself can be handy
and is extracted to static function.

Signed-off-by: Tomasz Buchert <tomasz.buchert@inria.fr>
---
 kernel/cgroup_freezer.c |   19 +++++++++----------
 1 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/kernel/cgroup_freezer.c b/kernel/cgroup_freezer.c
index ce71ed5..321e2a0 100644
--- a/kernel/cgroup_freezer.c
+++ b/kernel/cgroup_freezer.c
@@ -48,20 +48,19 @@ static inline struct freezer *task_freezer(struct task_struct *task)
 			    struct freezer, css);
 }
 
-int cgroup_freezing_or_frozen(struct task_struct *task)
+static inline int __cgroup_freezing_or_frozen(struct task_struct *task)
 {
-	struct freezer *freezer;
-	enum freezer_state state;
+	enum freezer_state state = task_freezer(task)->state;
+	return (state == CGROUP_FREEZING) || (state == CGROUP_FROZEN);
+}
 
+int cgroup_freezing_or_frozen(struct task_struct *task)
+{
+	int result;
 	task_lock(task);
-	freezer = task_freezer(task);
-	if (!freezer->css.cgroup->parent)
-		state = CGROUP_THAWED; /* root cgroup can't be frozen */
-	else
-		state = freezer->state;
+	result = __cgroup_freezing_or_frozen(task);
 	task_unlock(task);
-
-	return (state == CGROUP_FREEZING) || (state == CGROUP_FROZEN);
+	return result;
 }
 
 /*
-- 
1.6.3.3

[PATCH 2/3] cgroup_freezer: Fix can_attach to prohibit moving from/to freezing/frozen cgroups

[Tomasz Buchert]

It is possible to move a task from its cgroup
even if this group is 'FREEZING'. This results in
a nasty bug - the moved task will become frozen OUTSIDE
its original cgroup and will remain in a permanent 'D' state.

This patch allows to migrate the task only between THAWED cgroups.

This behavior was observed and easily reproduced on
a single core laptop. Notice that reproducibility depends
highly on the machine used. Program and instructions how
to reproduce the bug can be fetched from:
http://pentium.hopto.org/~thinred/repos/linux-misc/freezer_bug.c

Signed-off-by: Tomasz Buchert <tomasz.buchert@inria.fr>
---
 kernel/cgroup_freezer.c |   15 ++++++++-------
 1 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/kernel/cgroup_freezer.c b/kernel/cgroup_freezer.c
index 321e2a0..c287627 100644
--- a/kernel/cgroup_freezer.c
+++ b/kernel/cgroup_freezer.c
@@ -173,24 +173,25 @@ static int freezer_can_attach(struct cgroup_subsys *ss,
 
 	/*
 	 * 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))
+	freezer = cgroup_freezer(new_cgroup);
+	if (freezer->state != CGROUP_THAWED)
 		return -EBUSY;
 
-	freezer = cgroup_freezer(new_cgroup);
-	if (freezer->state == CGROUP_FROZEN)
+	rcu_read_lock();
+	if (__cgroup_freezing_or_frozen(task)) {
+		rcu_read_unlock();
 		return -EBUSY;
+	}
+	rcu_read_unlock();
 
 	if (threadgroup) {
 		struct task_struct *c;
 
 		rcu_read_lock();
 		list_for_each_entry_rcu(c, &task->thread_group, thread_group) {
-			if (is_task_frozen_enough(c)) {
+			if (__cgroup_freezing_or_frozen(c)) {
 				rcu_read_unlock();
 				return -EBUSY;
 			}
-- 
1.6.3.3

[PATCH 3/3] cgroup_freezer: update_freezer_state does incorrect state transactions

[Tomasz Buchert]

There are 4 state transitions possible for a freezer.
Only FREEZING -> FROZEN transaction is done lazily.
This patch allows update_freezer_state  only to perform
this transaction and renames the function to update_if_frozen.

Moreover is_task_frozen_enough function is removed and its
every occurence is replaced with frozen(). Therefore for a group
to become FROZEN every task must be frozen.

The previous version could trigger a following bug:
When cgroup is in the process of freezing (but none of its tasks
are frozen yet), update_freezer_state() (called from freezer_read
or freezer_write) would incorrectly report that a group is 'THAWED'
(because nfrozen = 0), allowing the transaction FREEZING -> THAWED
without writing anything to 'freezer.state'. This is incorrect
according to the documentation. This could result in a 'THAWED' cgroup
with frozen tasks inside.

A code to reproduce this bug is available here:
http://pentium.hopto.org/~thinred/repos/linux-misc/freezer_bug2.c

Signed-off-by: Tomasz Buchert <tomasz.buchert@inria.fr>
---
 kernel/cgroup_freezer.c |   40 +++++++++++++++++-----------------------
 1 files changed, 17 insertions(+), 23 deletions(-)

diff --git a/kernel/cgroup_freezer.c b/kernel/cgroup_freezer.c
index c287627..a5f7cb6 100644
--- a/kernel/cgroup_freezer.c
+++ b/kernel/cgroup_freezer.c
@@ -153,13 +153,6 @@ static void freezer_destroy(struct cgroup_subsys *ss,
 	kfree(cgroup_freezer(cgroup));
 }
 
-/* Task is frozen or will freeze immediately when next it gets woken */
-static bool is_task_frozen_enough(struct task_struct *task)
-{
-	return frozen(task) ||
-		(task_is_stopped_or_traced(task) && freezing(task));
-}
-
 /*
  * The call to cgroup_lock() in the freezer.state write method prevents
  * a write to that file racing against an attach, and hence the
@@ -236,31 +229,32 @@ static void freezer_fork(struct cgroup_subsys *ss, struct task_struct *task)
 /*
  * caller must hold freezer->lock
  */
-static void update_freezer_state(struct cgroup *cgroup,
+static void update_if_frozen(struct cgroup *cgroup,
 				 struct freezer *freezer)
 {
 	struct cgroup_iter it;
 	struct task_struct *task;
 	unsigned int nfrozen = 0, ntotal = 0;
+	enum freezer_state old_state = freezer->state;
 
 	cgroup_iter_start(cgroup, &it);
 	while ((task = cgroup_iter_next(cgroup, &it))) {
 		ntotal++;
-		if (is_task_frozen_enough(task))
+		if (frozen(task))
 			nfrozen++;
 	}
 
-	/*
-	 * Transition to FROZEN when no new tasks can be added ensures
-	 * that we never exist in the FROZEN state while there are unfrozen
-	 * tasks.
-	 */
-	if (nfrozen == ntotal)
-		freezer->state = CGROUP_FROZEN;
-	else if (nfrozen > 0)
-		freezer->state = CGROUP_FREEZING;
-	else
-		freezer->state = CGROUP_THAWED;
+	if (old_state == CGROUP_THAWED) {
+		BUG_ON(nfrozen > 0);
+	}
+	else if (old_state == CGROUP_FREEZING) {
+		if (nfrozen == ntotal)
+			freezer->state = CGROUP_FROZEN;
+	}
+	else { /* old_state == CGROUP_FROZEN */
+		BUG_ON(nfrozen != ntotal);
+	}
+
 	cgroup_iter_end(cgroup, &it);
 }
 
@@ -279,7 +273,7 @@ static int freezer_read(struct cgroup *cgroup, struct cftype *cft,
 	if (state == CGROUP_FREEZING) {
 		/* We change from FREEZING to FROZEN lazily if the cgroup was
 		 * only partially frozen when we exitted write. */
-		update_freezer_state(cgroup, freezer);
+		update_if_frozen(cgroup, freezer);
 		state = freezer->state;
 	}
 	spin_unlock_irq(&freezer->lock);
@@ -301,7 +295,7 @@ static int try_to_freeze_cgroup(struct cgroup *cgroup, struct freezer *freezer)
 	while ((task = cgroup_iter_next(cgroup, &it))) {
 		if (!freeze_task(task, true))
 			continue;
-		if (is_task_frozen_enough(task))
+		if (frozen(task))
 			continue;
 		if (!freezing(task) && !freezer_should_skip(task))
 			num_cant_freeze_now++;
@@ -335,7 +329,7 @@ static int freezer_change_state(struct cgroup *cgroup,
 
 	spin_lock_irq(&freezer->lock);
 
-	update_freezer_state(cgroup, freezer);
+	update_if_frozen(cgroup, freezer);
 	if (goal_state == freezer->state)
 		goto out;
 
-- 
1.6.3.3