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

[Matt Helsley <matthltc-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>]

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-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
Tested-by: Matt Helsley <matthltc-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>

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

Thanks!

Cheers,
	-Matt Helsley