[PATCH pm-freezer 1/4] cgroup_freezer: fix freezer->state setting bug in freezer_change_state()

[PATCH pm-freezer 1/4] cgroup_freezer: fix freezer->state setting bug in freezer_change_state()

[Tejun Heo]

d02f52811d0e "cgroup_freezer: prepare for removal of TIF_FREEZE" moved
setting of freezer->state into freezer_change_state(); unfortunately,
while doing so, when it's beginning to freeze tasks, it sets the state
to CGROUP_FROZEN instead of CGROUP_FREEZING ending up skipping the
whole freezing state.  Fix it.

-v2: Oleg pointed out that re-freezing FROZEN cgroup could increment
     system_freezing_cnt.  Fixed.

Signed-off-by: Tejun Heo <tj@kernel.org>
Reported-by: Oleg Nesterov <oleg@redhat.com>
Cc: Paul Menage <paul@paulmenage.org>
Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
---
I'm in the process of moving and can only use a quite old laptop.  I
tested compile but couldn't really do much else, so please proceed
with caution.  Oleg, can you please ack the patches if you agree with
the updated versions?

Thanks.

 kernel/cgroup_freezer.c |   20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

Index: work/kernel/cgroup_freezer.c
===================================================================
--- work.orig/kernel/cgroup_freezer.c
+++ work/kernel/cgroup_freezer.c
@@ -308,24 +308,26 @@ static int freezer_change_state(struct c
 	spin_lock_irq(&freezer->lock);
 
 	update_if_frozen(cgroup, freezer);
-	if (goal_state == freezer->state)
-		goto out;
-
-	freezer->state = goal_state;
 
 	switch (goal_state) {
 	case CGROUP_THAWED:
-		atomic_dec(&system_freezing_cnt);
-		unfreeze_cgroup(cgroup, freezer);
+		if (freezer->state != CGROUP_THAWED) {
+			freezer->state = CGROUP_THAWED;
+			atomic_dec(&system_freezing_cnt);
+			unfreeze_cgroup(cgroup, freezer);
+		}
 		break;
 	case CGROUP_FROZEN:
-		atomic_inc(&system_freezing_cnt);
-		retval = try_to_freeze_cgroup(cgroup, freezer);
+		if (freezer->state == CGROUP_THAWED) {
+			freezer->state = CGROUP_FREEZING;
+			atomic_inc(&system_freezing_cnt);
+			retval = try_to_freeze_cgroup(cgroup, freezer);
+		}
 		break;
 	default:
 		BUG();
 	}
-out:
+
 	spin_unlock_irq(&freezer->lock);
 
 	return retval;

[PATCH pm-freezer 2/4] freezer: set PF_NOFREEZE on a dying task right before TASK_DEAD setting bug in freezer_change_state()

[Tejun Heo]

3fb45733df "freezer: make exiting tasks properly unfreezable" removed
clear_freeze_flag() from exit path and set PF_NOFREEZE right after
PTRACE_EVENT_EXIT; however, Oleg pointed out that following exit paths
may cause interaction with device drivers after PM freezer consider
the system frozen.

There's no try_to_freeze() call in the exit path and the only
necessary guarantee is that freezer doesn't hang waiting for zombies.
Set PF_NOFREEZE right before setting tsk->state to TASK_DEAD instead.

Signed-off-by: Tejun Heo <tj@kernel.org>
Reported-by: Oleg Nesterov <oleg@redhat.com>
Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
---
 kernel/exit.c |   10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

Index: work/kernel/exit.c
===================================================================
--- work.orig/kernel/exit.c
+++ work/kernel/exit.c
@@ -913,12 +913,6 @@ NORET_TYPE void do_exit(long code)
 
 	ptrace_event(PTRACE_EVENT_EXIT, code);
 
-	/*
-	 * With ptrace notification done, there's no point in freezing from
-	 * here on.  Disallow freezing.
-	 */
-	current->flags |= PF_NOFREEZE;
-
 	validate_creds_for_do_exit(tsk);
 
 	/*
@@ -1044,6 +1038,10 @@ NORET_TYPE void do_exit(long code)
 
 	preempt_disable();
 	exit_rcu();
+
+	/* this task is now dead and freezer should ignore it */
+	current->flags |= PF_NOFREEZE;
+
 	/* causes final put_task_struct in finish_task_switch(). */
 	tsk->state = TASK_DEAD;
 	schedule();

[PATCH pm-freezer 3/4] freezer: restructure __refrigerator()

[Tejun Heo]

If another freeze happens before all tasks leave FROZEN state after
being thawed, the freezer can see the existing FROZEN and consider the
tasks to be frozen but they can clear FROZEN without checking the new
freezing().

Oleg suggested restructuring __refrigerator() such that there's single
condition check section inside freezer_lock and sigpending is cleared
afterwards, which fixes the problem and simplifies the code.
Restructure accordingly.

Signed-off-by: Tejun Heo <tj@kernel.org>
Reported-by: Oleg Nesterov <oleg@redhat.com>
Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
---
 kernel/freezer.c |   33 ++++++++++++++-------------------
 1 file changed, 14 insertions(+), 19 deletions(-)

Index: work/kernel/freezer.c
===================================================================
--- work.orig/kernel/freezer.c
+++ work/kernel/freezer.c
@@ -52,36 +52,31 @@ bool __refrigerator(bool check_kthr_stop
 	/* Hmm, should we be allowed to suspend when there are realtime
 	   processes around? */
 	bool was_frozen = false;
-	long save;
+	long save = current->state;
 
-	/*
-	 * No point in checking freezing() again - the caller already did.
-	 * Proceed to enter FROZEN.
-	 */
-	spin_lock_irq(&freezer_lock);
-	current->flags |= PF_FROZEN;
-	spin_unlock_irq(&freezer_lock);
-
-	save = current->state;
 	pr_debug("%s entered refrigerator\n", current->comm);
 
-	spin_lock_irq(&current->sighand->siglock);
-	recalc_sigpending(); /* We sent fake signal, clean it up */
-	spin_unlock_irq(&current->sighand->siglock);
-
 	for (;;) {
 		set_current_state(TASK_UNINTERRUPTIBLE);
+
+		spin_lock_irq(&freezer_lock);
+		current->flags |= PF_FROZEN;
 		if (!freezing(current) ||
-		    (check_kthr_stop && kthread_should_stop()))
+		    (check_kthr_stop && kthread_should_stop())) {
+			current->flags &= ~PF_FROZEN;
+			break;
+		}
+		spin_unlock_irq(&freezer_lock);
+
+		if (!(current->flags & PF_FROZEN))
 			break;
 		was_frozen = true;
 		schedule();
 	}
 
-	/* leave FROZEN */
-	spin_lock_irq(&freezer_lock);
-	current->flags &= ~PF_FROZEN;
-	spin_unlock_irq(&freezer_lock);
+	spin_lock_irq(&current->sighand->siglock);
+	recalc_sigpending(); /* We sent fake signal, clean it up */
+	spin_unlock_irq(&current->sighand->siglock);
 
 	pr_debug("%s left refrigerator\n", current->comm);
 

[PATCH pm-freezer 4/4] freezer: use lock_task_sighand() in fake_signal_wake_up()

[Tejun Heo]

cgroup_freezer calls freeze_task() without holding tasklist_lock and,
if the task is exiting, its ->sighand may be gone by the time
fake_signal_wake_up() is called.  Use lock_task_sighand() instead of
accessing ->sighand directly.

Signed-off-by: Tejun Heo <tj@kernel.org>
Reported-by: Oleg Nesterov <oleg@redhat.com>
Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
Cc: Paul Menage <paul@paulmenage.org>
---
 kernel/freezer.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

Index: work/kernel/freezer.c
===================================================================
--- work.orig/kernel/freezer.c
+++ work/kernel/freezer.c
@@ -95,9 +95,10 @@ static void fake_signal_wake_up(struct t
 {
 	unsigned long flags;
 
-	spin_lock_irqsave(&p->sighand->siglock, flags);
-	signal_wake_up(p, 0);
-	spin_unlock_irqrestore(&p->sighand->siglock, flags);
+	if (lock_task_sighand(p, &flags)) {
+		signal_wake_up(p, 0);
+		unlock_task_sighand(p, &flags);
+	}
 }
 
 /**

Re: [PATCH pm-freezer 3/4] freezer: restructure __refrigerator()

[Oleg Nesterov]

On 08/31, Tejun Heo wrote:
>
>  	for (;;) {
>  		set_current_state(TASK_UNINTERRUPTIBLE);
> +
> +		spin_lock_irq(&freezer_lock);
> +		current->flags |= PF_FROZEN;
>  		if (!freezing(current) ||
> -		    (check_kthr_stop && kthread_should_stop()))
> +		    (check_kthr_stop && kthread_should_stop())) {
> +			current->flags &= ~PF_FROZEN;
> +			break;

Argh! sorry I missed this...

This "break" is typo, it should be removed.

Otherwise I believe this is correct.

Oleg.

Re: [PATCH pm-freezer 1/4] cgroup_freezer: fix freezer->state setting bug in freezer_change_state()

[Oleg Nesterov]

On 08/31, Tejun Heo wrote:
>
> I'm in the process of moving and can only use a quite old laptop.  I
> tested compile but couldn't really do much else, so please proceed
> with caution.  Oleg, can you please ack the patches if you agree with
> the updated versions?

Everything looks fine. But I am already sleeping now ;)

Rafael, Tejun, I'll try to re-read 1-4 tomorrow. I do not expect I'll
find something interesting, just I am paranoid.

Looks like, 1/4 could have an additional note in the changelog, with
this patch we avoid the unnecessary try_to_freeze_cgroup() and this
looks like a win to me...

Oleg.

Re: [PATCH pm-freezer 1/4] cgroup_freezer: fix freezer->state setting bug in freezer_change_state()

[Matt Helsley]

On Wed, Aug 31, 2011 at 12:21:07PM +0200, Tejun Heo wrote:
> d02f52811d0e "cgroup_freezer: prepare for removal of TIF_FREEZE" moved
> setting of freezer->state into freezer_change_state(); unfortunately,
> while doing so, when it's beginning to freeze tasks, it sets the state
> to CGROUP_FROZEN instead of CGROUP_FREEZING ending up skipping the
> whole freezing state.  Fix it.
> 
> -v2: Oleg pointed out that re-freezing FROZEN cgroup could increment
>      system_freezing_cnt.  Fixed.
> 
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Reported-by: Oleg Nesterov <oleg@redhat.com>
> Cc: Paul Menage <paul@paulmenage.org>
> Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
> ---
> I'm in the process of moving and can only use a quite old laptop.  I
> tested compile but couldn't really do much else, so please proceed
> with caution.  Oleg, can you please ack the patches if you agree with
> the updated versions?
> 
> Thanks.
> 
>  kernel/cgroup_freezer.c |   20 +++++++++++---------
>  1 file changed, 11 insertions(+), 9 deletions(-)
> 
> Index: work/kernel/cgroup_freezer.c
> ===================================================================
> --- work.orig/kernel/cgroup_freezer.c
> +++ work/kernel/cgroup_freezer.c
> @@ -308,24 +308,26 @@ static int freezer_change_state(struct c
>  	spin_lock_irq(&freezer->lock);
> 
>  	update_if_frozen(cgroup, freezer);
> -	if (goal_state == freezer->state)
> -		goto out;
> -
> -	freezer->state = goal_state;
> 
>  	switch (goal_state) {
>  	case CGROUP_THAWED:
> -		atomic_dec(&system_freezing_cnt);
> -		unfreeze_cgroup(cgroup, freezer);
> +		if (freezer->state != CGROUP_THAWED) {
> +			freezer->state = CGROUP_THAWED;
> +			atomic_dec(&system_freezing_cnt);
> +			unfreeze_cgroup(cgroup, freezer);
> +		}
>  		break;
>  	case CGROUP_FROZEN:
> -		atomic_inc(&system_freezing_cnt);
> -		retval = try_to_freeze_cgroup(cgroup, freezer);
> +		if (freezer->state == CGROUP_THAWED) {
> +			freezer->state = CGROUP_FREEZING;
> +			atomic_inc(&system_freezing_cnt);
> +			retval = try_to_freeze_cgroup(cgroup, freezer);

This still doesn't look quite right. If the cgroup is FREEZING it should
also call try_to_freeze_cgroup(). I think this is what's needed:

		if (freezer->state == CGROUP_THAWED)
			atomic_inc(&system_freezing_cnt);
		freezer->state = CGROUP_FREEZING;
		retval = try_to_freeze_cgroup(cgroup, freezer);

> +		}
>  		break;
>  	default:
>  		BUG();
>  	}

Cheers,
	-Matt Helsley

Re: [PATCH pm-freezer 1/4] cgroup_freezer: fix freezer->state setting bug in freezer_change_state()

[Tejun Heo]

Hello, Matt.

On Thu, Sep 01, 2011 at 05:42:31PM -0700, Matt Helsley wrote:
> >  	case CGROUP_FROZEN:
> > -		atomic_inc(&system_freezing_cnt);
> > -		retval = try_to_freeze_cgroup(cgroup, freezer);
> > +		if (freezer->state == CGROUP_THAWED) {
> > +			freezer->state = CGROUP_FREEZING;
> > +			atomic_inc(&system_freezing_cnt);
> > +			retval = try_to_freeze_cgroup(cgroup, freezer);
> 
> This still doesn't look quite right. If the cgroup is FREEZING it should
> also call try_to_freeze_cgroup(). I think this is what's needed:
> 
> 		if (freezer->state == CGROUP_THAWED)
> 			atomic_inc(&system_freezing_cnt);
> 		freezer->state = CGROUP_FREEZING;
> 		retval = try_to_freeze_cgroup(cgroup, freezer);

Does this make any difference?  Tasks can't migrate if the cgroups are
freezing and freezing state is inherited through forks.  But yeah
doing that for both THAWED and FROZEN might still be a good idea for
safety.

Thanks.

-- 
tejun

Re: [PATCH pm-freezer 1/4] cgroup_freezer: fix freezer->state setting bug in freezer_change_state()

[Oleg Nesterov]

On 09/01, Matt Helsley wrote:
>
> On Wed, Aug 31, 2011 at 12:21:07PM +0200, Tejun Heo wrote:
> >  	case CGROUP_FROZEN:
> > -		atomic_inc(&system_freezing_cnt);
> > -		retval = try_to_freeze_cgroup(cgroup, freezer);
> > +		if (freezer->state == CGROUP_THAWED) {
> > +			freezer->state = CGROUP_FREEZING;
> > +			atomic_inc(&system_freezing_cnt);
> > +			retval = try_to_freeze_cgroup(cgroup, freezer);
>
> This still doesn't look quite right. If the cgroup is FREEZING it should
> also call try_to_freeze_cgroup(). I think this is what's needed:
>
> 		if (freezer->state == CGROUP_THAWED)
> 			atomic_inc(&system_freezing_cnt);
> 		freezer->state = CGROUP_FREEZING;
> 		retval = try_to_freeze_cgroup(cgroup, freezer);

This is what I mentioned before, to me this looks like a win.

Why do we need try_to_freeze_cgroup() in this case? "for safety"
could actually mean "hide the bug" ;)


But I agree either way. Rafael, I think 1-4 are fine, but I think
we need the simple 5/4, will send in a minute...

Oleg.

Re: [PATCH pm-freezer 1/4] cgroup_freezer: fix freezer->state setting bug in freezer_change_state()

[Tejun Heo]

Hello,

On Fri, Sep 02, 2011 at 06:58:39PM +0200, Oleg Nesterov wrote:
> > This still doesn't look quite right. If the cgroup is FREEZING it should
> > also call try_to_freeze_cgroup(). I think this is what's needed:
> >
> > 		if (freezer->state == CGROUP_THAWED)
> > 			atomic_inc(&system_freezing_cnt);
> > 		freezer->state = CGROUP_FREEZING;
> > 		retval = try_to_freeze_cgroup(cgroup, freezer);
> 
> This is what I mentioned before, to me this looks like a win.
> 
> Why do we need try_to_freeze_cgroup() in this case? "for safety"
> could actually mean "hide the bug" ;)

I guess it depends on the viewpoint.  A simple analogy would be using
WARN_ON_ONCE() instead of BUG_ON() so that the mode of failure is
softer.  This change isn't likely to make bugs significantly more
difficult to discover so why not?

> But I agree either way. Rafael, I think 1-4 are fine, but I think
> we need the simple 5/4, will send in a minute...

Can you please wait a bit?  The second one was broken (missing unlock)
and I made some small adjustments.  Will repost soon.

Thanks.

-- 
tejun

Re: [PATCH pm-freezer 1/4] cgroup_freezer: fix freezer->state setting bug in freezer_change_state()

[Oleg Nesterov]

On 09/03, Tejun Heo wrote:
>
> Hello,
>
> On Fri, Sep 02, 2011 at 06:58:39PM +0200, Oleg Nesterov wrote:
> > > This still doesn't look quite right. If the cgroup is FREEZING it should
> > > also call try_to_freeze_cgroup(). I think this is what's needed:
> > >
> > > 		if (freezer->state == CGROUP_THAWED)
> > > 			atomic_inc(&system_freezing_cnt);
> > > 		freezer->state = CGROUP_FREEZING;
> > > 		retval = try_to_freeze_cgroup(cgroup, freezer);
> >
> > This is what I mentioned before, to me this looks like a win.
> >
> > Why do we need try_to_freeze_cgroup() in this case? "for safety"
> > could actually mean "hide the bug" ;)
>
> I guess it depends on the viewpoint.  A simple analogy would be using
> WARN_ON_ONCE() instead of BUG_ON() so that the mode of failure is
> softer.  This change isn't likely to make bugs significantly more
> difficult to discover so why not?

I agree either way.

Personally I prefer your current patch. Because it is not clear why
do we call try_to_freeze_cgroup() if it was already called. And, the
2nd call can silently hide the problem if we have some bug.

But of course, this is up to you and Matt.



> > But I agree either way. Rafael, I think 1-4 are fine, but I think
> > we need the simple 5/4, will send in a minute...
>
> Can you please wait a bit?  The second one was broken (missing unlock)

Yes, I just noticed the small problem too, hopefully we mean the same
bug ;)

Oleg.

Re: [PATCH pm-freezer 1/4] cgroup_freezer: fix freezer->state setting bug in freezer_change_state()

[Tejun Heo]

Hello,

On Fri, Sep 02, 2011 at 07:15:17PM +0200, Oleg Nesterov wrote:
> > I guess it depends on the viewpoint.  A simple analogy would be using
> > WARN_ON_ONCE() instead of BUG_ON() so that the mode of failure is
> > softer.  This change isn't likely to make bugs significantly more
> > difficult to discover so why not?
> 
> I agree either way.
> 
> Personally I prefer your current patch. Because it is not clear why
> do we call try_to_freeze_cgroup() if it was already called. And, the
> 2nd call can silently hide the problem if we have some bug.
> 
> But of course, this is up to you and Matt.

I'm okay either way too.  It would make a bug in that area a bit less
annoying and thus may decrease the chance of bug report, but it means
that we ship with built-in easy work around (if it doesn't work at the
first kick, kick again), which can be beneficial in practice.

> > > But I agree either way. Rafael, I think 1-4 are fine, but I think
> > > we need the simple 5/4, will send in a minute...
> >
> > Can you please wait a bit?  The second one was broken (missing unlock)
> 
> Yes, I just noticed the small problem too, hopefully we mean the same
> bug ;)

Yeap, the same one.  I thought it was the second patch instead of the
third. :)

Thanks.

-- 
tejun

Re: [PATCH pm-freezer 1/4] cgroup_freezer: fix freezer->state setting bug in freezer_change_state()

[Oleg Nesterov]

On 09/03, Tejun Heo wrote:
>
> Can you please wait a bit?

Sure. But just in case, I mean the simple patch below.

Feel free to incorporate into 4/4, or I can resend it later.

Oleg.

------------------------------------------------------------------------------
[PATCH] freezer: remove the pointless/unsafe __thaw_task()->recalc_sigpending_and_wake()

Remove __thaw_task()->recalc_sigpending_and_wake(). It was copied from
cancel_freezing() recently, but it was always wrong.

It is pointless, contary to the comment it can't clear TIF_SIGPENDING.
Not to mention, we must never do this with !current task, this is wrong.

The usage of ->sighand is not safe if the caller is cgroup_freezer, we
can racw with exit.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---

 kernel/freezer.c |   10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

--- 3.1/kernel/freezer.c~kill_rspaw	2011-09-02 18:52:46.000000000 +0200
+++ 3.1/kernel/freezer.c	2011-09-02 19:03:30.000000000 +0200
@@ -150,18 +150,10 @@ void __thaw_task(struct task_struct *p)
 	 * be visible to @p as waking up implies wmb.  Waking up inside
 	 * freezer_lock also prevents wakeups from leaking outside
 	 * refrigerator.
-	 *
-	 * If !FROZEN, @p hasn't reached refrigerator, recalc sigpending to
-	 * avoid leaving dangling TIF_SIGPENDING behind.
 	 */
 	spin_lock_irqsave(&freezer_lock, flags);
-	if (frozen(p)) {
+	if (frozen(p))
 		wake_up_process(p);
-	} else {
-		spin_lock(&p->sighand->siglock);
-		recalc_sigpending_and_wake(p);
-		spin_unlock(&p->sighand->siglock);
-	}
 	spin_unlock_irqrestore(&freezer_lock, flags);
 }
 

Re: [PATCH pm-freezer 1/4] cgroup_freezer: fix freezer->state setting bug in freezer_change_state()

[Matt Helsley]

On Fri, Sep 02, 2011 at 06:58:39PM +0200, Oleg Nesterov wrote:
> On 09/01, Matt Helsley wrote:
> >
> > On Wed, Aug 31, 2011 at 12:21:07PM +0200, Tejun Heo wrote:
> > >  	case CGROUP_FROZEN:
> > > -		atomic_inc(&system_freezing_cnt);
> > > -		retval = try_to_freeze_cgroup(cgroup, freezer);
> > > +		if (freezer->state == CGROUP_THAWED) {
> > > +			freezer->state = CGROUP_FREEZING;
> > > +			atomic_inc(&system_freezing_cnt);
> > > +			retval = try_to_freeze_cgroup(cgroup, freezer);
> >
> > This still doesn't look quite right. If the cgroup is FREEZING it should
> > also call try_to_freeze_cgroup(). I think this is what's needed:
> >
> > 		if (freezer->state == CGROUP_THAWED)
> > 			atomic_inc(&system_freezing_cnt);
> > 		freezer->state = CGROUP_FREEZING;
> > 		retval = try_to_freeze_cgroup(cgroup, freezer);
> 
> This is what I mentioned before, to me this looks like a win.
> 
> Why do we need try_to_freeze_cgroup() in this case? "for safety"
> could actually mean "hide the bug" ;)

Well, I need to check Tejun's latest freezer bits to see if this is
still the case but it was possible to get to the FREEZING state and
not enter FROZEN before returning to userspace. So you could come back
into the state change function in the FREEZING state with FROZEN as
the goal state. Note that for the cgroup freezer the FREEZING state is
optional -- so skipping it is fine so long was we guarantee that by the
time we exit to userspace with a FROZEN state all the tasks in the cgroup
are actually frozen (in the refrigerator loop) or frozen enough (about to
enter the refrigerator loop without causing more IO -- e.g. stopped).

Cheers,
	-Matt