Message-ID: <4C6B339E.6010907@inria.fr>
Date:	Wed, 18 Aug 2010 03:13:02 +0200
From:	Tomasz Buchert <tomasz.buchert@...ia.fr>
To:	Matt Helsley <matthltc@...ibm.com>
CC:	Paul Menage <menage@...gle.com>, Li Zefan <lizf@...fujitsu.com>,
	containers@...ts.linux-foundation.org,
	linux-kernel@...r.kernel.org, "Rafael J. Wysocki" <rjw@...k.pl>
Subject: Re: [PATCH] cgroup_freezer: Freezing and task move race fix

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@...ibm.com>
>>> Tested-by: Matt Helsley<matthltc@...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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists