[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.

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>
---
Rafael, these four patches fix the issues spotted by Oleg during
review of the freezer patchset.  Please apply them on pm-freezer once
Oleg acks them.  Thanks.

 kernel/cgroup_freezer.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Index: work/kernel/cgroup_freezer.c
===================================================================
--- work.orig/kernel/cgroup_freezer.c
+++ work/kernel/cgroup_freezer.c
@@ -311,14 +311,14 @@ static int freezer_change_state(struct c
 	if (goal_state == freezer->state)
 		goto out;
 
-	freezer->state = goal_state;
-
 	switch (goal_state) {
 	case CGROUP_THAWED:
+		freezer->state = CGROUP_THAWED;
 		atomic_dec(&system_freezing_cnt);
 		unfreeze_cgroup(cgroup, freezer);
 		break;
 	case CGROUP_FROZEN:
+		freezer->state = CGROUP_FREEZING;
 		atomic_inc(&system_freezing_cnt);
 		retval = try_to_freeze_cgroup(cgroup, freezer);
 		break;

[PATCH pm-freezer 2/4] freezer: set PF_NOFREEZE on a dying task right before TASK_DEAD

[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-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Reported-by: Oleg Nesterov <oleg-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Cc: "Rafael J. Wysocki" <rjw-KKrjLPT3xs0@public.gmane.org>
---
 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();

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

[Oleg Nesterov]

On 08/29, Tejun Heo wrote:
>
> --- work.orig/kernel/cgroup_freezer.c
> +++ work/kernel/cgroup_freezer.c
> @@ -311,14 +311,14 @@ static int freezer_change_state(struct c
>  	if (goal_state == freezer->state)
>  		goto out;
>  
> -	freezer->state = goal_state;
> -
>  	switch (goal_state) {
>  	case CGROUP_THAWED:
> +		freezer->state = CGROUP_THAWED;
>  		atomic_dec(&system_freezing_cnt);
>  		unfreeze_cgroup(cgroup, freezer);
>  		break;
>  	case CGROUP_FROZEN:
> +		freezer->state = CGROUP_FREEZING;

At first glance, this is correct. I'll try to recheck.

But,

>  		atomic_inc(&system_freezing_cnt);

iiuc this becomes wrong... Suppose a user writes "FROZEN" twice,
before freezer->state becomes CGROUP_FROZEN.

I think we should actually fix the "goal_state == freezer->state"
check above.

Oleg.

[PATCH pm-freezer 2/4] freezer: set PF_NOFREEZE on a dying task right before TASK_DEAD

[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: check freezing() before leaving FROZEN state

[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().  Check freezing() while holding freezer_lock before
clearing FROZEN.

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 |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Index: work/kernel/freezer.c
===================================================================
--- work.orig/kernel/freezer.c
+++ work/kernel/freezer.c
@@ -60,6 +60,7 @@ bool __refrigerator(bool check_kthr_stop
 	 */
 	spin_lock_irq(&freezer_lock);
 	current->flags |= PF_FROZEN;
+refreeze:
 	spin_unlock_irq(&freezer_lock);
 
 	save = current->state;
@@ -78,8 +79,10 @@ bool __refrigerator(bool check_kthr_stop
 		schedule();
 	}
 
-	/* leave FROZEN */
+	/* leave FROZEN after checking freezing() holding freezer_lock */
 	spin_lock_irq(&freezer_lock);
+	if (freezing(current))
+		goto refreeze;
 	current->flags &= ~PF_FROZEN;
 	spin_unlock_irq(&freezer_lock);
 

[PATCH pm-freezer 3/4] freezer: check freezing() before leaving FROZEN state

[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().  Check freezing() while holding freezer_lock before
clearing FROZEN.

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 |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Index: work/kernel/freezer.c
===================================================================
--- work.orig/kernel/freezer.c
+++ work/kernel/freezer.c
@@ -60,6 +60,7 @@ bool __refrigerator(bool check_kthr_stop
 	 */
 	spin_lock_irq(&freezer_lock);
 	current->flags |= PF_FROZEN;
+refreeze:
 	spin_unlock_irq(&freezer_lock);
 
 	save = current->state;
@@ -78,8 +79,10 @@ bool __refrigerator(bool check_kthr_stop
 		schedule();
 	}
 
-	/* leave FROZEN */
+	/* leave FROZEN after checking freezing() holding freezer_lock */
 	spin_lock_irq(&freezer_lock);
+	if (freezing(current))
+		goto refreeze;
 	current->flags &= ~PF_FROZEN;
 	spin_unlock_irq(&freezer_lock);
 

[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
@@ -103,9 +103,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 4/4] freezer: use lock_task_sighand() in fake_signal_wake_up()

[Oleg Nesterov]

On 08/29, Tejun Heo wrote:
>
> --- work.orig/kernel/freezer.c
> +++ work/kernel/freezer.c
> @@ -103,9 +103,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);
> +	}

Yes, this looks correct.

Oleg.

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

[Oleg Nesterov]

On 08/29, Tejun Heo wrote:
>
> --- work.orig/kernel/freezer.c
> +++ work/kernel/freezer.c
> @@ -103,9 +103,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);
> +	}

Yes, this looks correct.

Oleg.

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

[Oleg Nesterov]

On 08/29, Tejun Heo wrote:
>
> --- work.orig/kernel/freezer.c
> +++ work/kernel/freezer.c
> @@ -103,9 +103,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);
> +	}

Yes, this looks correct.

Oleg.

[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
@@ -103,9 +103,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);
+	}
 }
 
 /**

[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-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Reported-by: Oleg Nesterov <oleg-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Cc: "Rafael J. Wysocki" <rjw-KKrjLPT3xs0@public.gmane.org>
Cc: Paul Menage <paul-inf54ven1CmVyaH7bEyXVA@public.gmane.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
@@ -103,9 +103,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: check freezing() before leaving FROZEN state

[Oleg Nesterov]

On 08/29, Tejun Heo wrote:
>
> --- work.orig/kernel/freezer.c
> +++ work/kernel/freezer.c
> @@ -60,6 +60,7 @@ bool __refrigerator(bool check_kthr_stop
>  	 */
>  	spin_lock_irq(&freezer_lock);
>  	current->flags |= PF_FROZEN;
> +refreeze:
>  	spin_unlock_irq(&freezer_lock);
>
>  	save = current->state;
> @@ -78,8 +79,10 @@ bool __refrigerator(bool check_kthr_stop
>  		schedule();
>  	}
>
> -	/* leave FROZEN */
> +	/* leave FROZEN after checking freezing() holding freezer_lock */
>  	spin_lock_irq(&freezer_lock);
> +	if (freezing(current))
> +		goto refreeze;

Looks like, you should move "save = current->state" up then.

Oleg.

Re: [PATCH pm-freezer 3/4] freezer: check freezing() before leaving FROZEN state

[Oleg Nesterov]

On 08/29, Tejun Heo wrote:
>
> --- work.orig/kernel/freezer.c
> +++ work/kernel/freezer.c
> @@ -60,6 +60,7 @@ bool __refrigerator(bool check_kthr_stop
>  	 */
>  	spin_lock_irq(&freezer_lock);
>  	current->flags |= PF_FROZEN;
> +refreeze:
>  	spin_unlock_irq(&freezer_lock);
>
>  	save = current->state;
> @@ -78,8 +79,10 @@ bool __refrigerator(bool check_kthr_stop
>  		schedule();
>  	}
>
> -	/* leave FROZEN */
> +	/* leave FROZEN after checking freezing() holding freezer_lock */
>  	spin_lock_irq(&freezer_lock);
> +	if (freezing(current))
> +		goto refreeze;

Looks like, you should move "save = current->state" up then.

Oleg.

Re: [PATCH pm-freezer 3/4] freezer: check freezing() before leaving FROZEN state

[Oleg Nesterov]

On 08/29, Tejun Heo wrote:
>
> --- work.orig/kernel/freezer.c
> +++ work/kernel/freezer.c
> @@ -60,6 +60,7 @@ bool __refrigerator(bool check_kthr_stop
>  	 */
>  	spin_lock_irq(&freezer_lock);
>  	current->flags |= PF_FROZEN;
> +refreeze:
>  	spin_unlock_irq(&freezer_lock);
>
>  	save = current->state;
> @@ -78,8 +79,10 @@ bool __refrigerator(bool check_kthr_stop
>  		schedule();
>  	}
>
> -	/* leave FROZEN */
> +	/* leave FROZEN after checking freezing() holding freezer_lock */
>  	spin_lock_irq(&freezer_lock);
> +	if (freezing(current))
> +		goto refreeze;

Looks like, you should move "save = current->state" up then.

Oleg.

Re: [PATCH pm-freezer 3/4] freezer: check freezing() before leaving FROZEN state

[Oleg Nesterov]

On 08/29, Oleg Nesterov wrote:
>
> On 08/29, Tejun Heo wrote:
> >
> > --- work.orig/kernel/freezer.c
> > +++ work/kernel/freezer.c
> > @@ -60,6 +60,7 @@ bool __refrigerator(bool check_kthr_stop
> >  	 */
> >  	spin_lock_irq(&freezer_lock);
> >  	current->flags |= PF_FROZEN;
> > +refreeze:
> >  	spin_unlock_irq(&freezer_lock);
> >
> >  	save = current->state;
> > @@ -78,8 +79,10 @@ bool __refrigerator(bool check_kthr_stop
> >  		schedule();
> >  	}
> >
> > -	/* leave FROZEN */
> > +	/* leave FROZEN after checking freezing() holding freezer_lock */
> >  	spin_lock_irq(&freezer_lock);
> > +	if (freezing(current))
> > +		goto refreeze;
>
> Looks like, you should move "save = current->state" up then.

Hmm. And afaics there is another problem. This can "livelock" if
check_kthr_stop && kthread_should_stop().

May be we should consolidate the freezer_lock's sections, something
like below?

Hmm. But I got lost a bit. Why do we need freezer_lock to set/clear
PF_FROZEN ? OK, the code below takes freezer_lock for freezing().
Is there any other reason?

Oleg.

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;

	save = current->state;
	pr_debug("%s entered refrigerator\n", current->comm);

	for (;;) {
		set_current_state(TASK_UNINTERRUPTIBLE);

		spin_lock_irq(&freezer_lock);
		current->flags |= PF_FROZEN;
		if (!freezing(current) ||
		    (check_kthr_stop && kthread_should_stop()))
			current->flags &= ~PF_FROZEN;
		spin_unlock_irq(&freezer_lock);

		if (!current->flags & PF_FROZEN)
			break;

		was_frozen = true;
		schedule();
	}

	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);
	/*
	 * Restore saved task state before returning.  The mb'd version
	 * needs to be used; otherwise, it might silently break
	 * synchronization which depends on ordered task state change.
	 */
	set_current_state(save);

	return was_frozen;
}

Re: [PATCH pm-freezer 3/4] freezer: check freezing() before leaving FROZEN state

[Oleg Nesterov]

On 08/29, Oleg Nesterov wrote:
>
> On 08/29, Tejun Heo wrote:
> >
> > --- work.orig/kernel/freezer.c
> > +++ work/kernel/freezer.c
> > @@ -60,6 +60,7 @@ bool __refrigerator(bool check_kthr_stop
> >  	 */
> >  	spin_lock_irq(&freezer_lock);
> >  	current->flags |= PF_FROZEN;
> > +refreeze:
> >  	spin_unlock_irq(&freezer_lock);
> >
> >  	save = current->state;
> > @@ -78,8 +79,10 @@ bool __refrigerator(bool check_kthr_stop
> >  		schedule();
> >  	}
> >
> > -	/* leave FROZEN */
> > +	/* leave FROZEN after checking freezing() holding freezer_lock */
> >  	spin_lock_irq(&freezer_lock);
> > +	if (freezing(current))
> > +		goto refreeze;
>
> Looks like, you should move "save = current->state" up then.

Hmm. And afaics there is another problem. This can "livelock" if
check_kthr_stop && kthread_should_stop().

May be we should consolidate the freezer_lock's sections, something
like below?

Hmm. But I got lost a bit. Why do we need freezer_lock to set/clear
PF_FROZEN ? OK, the code below takes freezer_lock for freezing().
Is there any other reason?

Oleg.

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;

	save = current->state;
	pr_debug("%s entered refrigerator\n", current->comm);

	for (;;) {
		set_current_state(TASK_UNINTERRUPTIBLE);

		spin_lock_irq(&freezer_lock);
		current->flags |= PF_FROZEN;
		if (!freezing(current) ||
		    (check_kthr_stop && kthread_should_stop()))
			current->flags &= ~PF_FROZEN;
		spin_unlock_irq(&freezer_lock);

		if (!current->flags & PF_FROZEN)
			break;

		was_frozen = true;
		schedule();
	}

	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);
	/*
	 * Restore saved task state before returning.  The mb'd version
	 * needs to be used; otherwise, it might silently break
	 * synchronization which depends on ordered task state change.
	 */
	set_current_state(save);

	return was_frozen;
}

Re: [PATCH pm-freezer 3/4] freezer: check freezing() before leaving FROZEN state

[Oleg Nesterov]

On 08/29, Oleg Nesterov wrote:
>
> Hmm. But I got lost a bit. Why do we need freezer_lock to set/clear
> PF_FROZEN ? OK, the code below takes freezer_lock for freezing().
> Is there any other reason?

Ah, at least we need it to serialize with __thaw_task() which does
"if (frozen) wake_up()".

Oleg.

Re: [PATCH pm-freezer 3/4] freezer: check freezing() before leaving FROZEN state

[Oleg Nesterov]

On 08/29, Oleg Nesterov wrote:
>
> Hmm. But I got lost a bit. Why do we need freezer_lock to set/clear
> PF_FROZEN ? OK, the code below takes freezer_lock for freezing().
> Is there any other reason?

Ah, at least we need it to serialize with __thaw_task() which does
"if (frozen) wake_up()".

Oleg.

Re: [PATCH pm-freezer 3/4] freezer: check freezing() before leaving FROZEN state

[Oleg Nesterov]

On 08/29, Oleg Nesterov wrote:
>
> Hmm. But I got lost a bit. Why do we need freezer_lock to set/clear
> PF_FROZEN ? OK, the code below takes freezer_lock for freezing().
> Is there any other reason?

Ah, at least we need it to serialize with __thaw_task() which does
"if (frozen) wake_up()".

Oleg.

Re: [PATCH pm-freezer 3/4] freezer: check freezing() before leaving FROZEN state

[Oleg Nesterov]

On 08/29, Oleg Nesterov wrote:
>
> On 08/29, Tejun Heo wrote:
> >
> > --- work.orig/kernel/freezer.c
> > +++ work/kernel/freezer.c
> > @@ -60,6 +60,7 @@ bool __refrigerator(bool check_kthr_stop
> >  	 */
> >  	spin_lock_irq(&freezer_lock);
> >  	current->flags |= PF_FROZEN;
> > +refreeze:
> >  	spin_unlock_irq(&freezer_lock);
> >
> >  	save = current->state;
> > @@ -78,8 +79,10 @@ bool __refrigerator(bool check_kthr_stop
> >  		schedule();
> >  	}
> >
> > -	/* leave FROZEN */
> > +	/* leave FROZEN after checking freezing() holding freezer_lock */
> >  	spin_lock_irq(&freezer_lock);
> > +	if (freezing(current))
> > +		goto refreeze;
>
> Looks like, you should move "save = current->state" up then.

Hmm. And afaics there is another problem. This can "livelock" if
check_kthr_stop && kthread_should_stop().

May be we should consolidate the freezer_lock's sections, something
like below?

Hmm. But I got lost a bit. Why do we need freezer_lock to set/clear
PF_FROZEN ? OK, the code below takes freezer_lock for freezing().
Is there any other reason?

Oleg.

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;

	save = current->state;
	pr_debug("%s entered refrigerator\n", current->comm);

	for (;;) {
		set_current_state(TASK_UNINTERRUPTIBLE);

		spin_lock_irq(&freezer_lock);
		current->flags |= PF_FROZEN;
		if (!freezing(current) ||
		    (check_kthr_stop && kthread_should_stop()))
			current->flags &= ~PF_FROZEN;
		spin_unlock_irq(&freezer_lock);

		if (!current->flags & PF_FROZEN)
			break;

		was_frozen = true;
		schedule();
	}

	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);
	/*
	 * Restore saved task state before returning.  The mb'd version
	 * needs to be used; otherwise, it might silently break
	 * synchronization which depends on ordered task state change.
	 */
	set_current_state(save);

	return was_frozen;
}

Re: [PATCH pm-freezer 2/4] freezer: set PF_NOFREEZE on a dying task right before TASK_DEAD

[Oleg Nesterov]

On 08/29, Tejun Heo wrote:
>
> 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.

Agreed.

But I'd like to repeat, this looks "asymmetrical". do_each_thread()
can't see the (auto)reaped tasks after they do exit_notify(). So we
can only see this PF_NOFREEZE if the thread becomes a zombie.

> @@ -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;

May be freezing_slow_path() can check TASK_DEAD along with PF_NOFREEZE
instead? (or tsk->exit_state != 0 to avoid the asymmetry above). Just
to keep this logic in the freezer code. I dunno.

But this all is up to you and Rafael, I am not arguing. Just random
thoughts.

Oleg.

Re: [PATCH pm-freezer 2/4] freezer: set PF_NOFREEZE on a dying task right before TASK_DEAD

[Oleg Nesterov]

On 08/29, Tejun Heo wrote:
>
> 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.

Agreed.

But I'd like to repeat, this looks "asymmetrical". do_each_thread()
can't see the (auto)reaped tasks after they do exit_notify(). So we
can only see this PF_NOFREEZE if the thread becomes a zombie.

> @@ -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;

May be freezing_slow_path() can check TASK_DEAD along with PF_NOFREEZE
instead? (or tsk->exit_state != 0 to avoid the asymmetry above). Just
to keep this logic in the freezer code. I dunno.

But this all is up to you and Rafael, I am not arguing. Just random
thoughts.

Oleg.

[PATCH pm-freezer 3/4] freezer: check freezing() before leaving FROZEN state

[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().  Check freezing() while holding freezer_lock before
clearing FROZEN.

Signed-off-by: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Reported-by: Oleg Nesterov <oleg-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Cc: "Rafael J. Wysocki" <rjw-KKrjLPT3xs0@public.gmane.org>
---
 kernel/freezer.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Index: work/kernel/freezer.c
===================================================================
--- work.orig/kernel/freezer.c
+++ work/kernel/freezer.c
@@ -60,6 +60,7 @@ bool __refrigerator(bool check_kthr_stop
 	 */
 	spin_lock_irq(&freezer_lock);
 	current->flags |= PF_FROZEN;
+refreeze:
 	spin_unlock_irq(&freezer_lock);
 
 	save = current->state;
@@ -78,8 +79,10 @@ bool __refrigerator(bool check_kthr_stop
 		schedule();
 	}
 
-	/* leave FROZEN */
+	/* leave FROZEN after checking freezing() holding freezer_lock */
 	spin_lock_irq(&freezer_lock);
+	if (freezing(current))
+		goto refreeze;
 	current->flags &= ~PF_FROZEN;
 	spin_unlock_irq(&freezer_lock);

Re: [PATCH pm-freezer 2/4] freezer: set PF_NOFREEZE on a dying task right before TASK_DEAD

[Oleg Nesterov]

On 08/29, Tejun Heo wrote:
>
> 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.

Agreed.

But I'd like to repeat, this looks "asymmetrical". do_each_thread()
can't see the (auto)reaped tasks after they do exit_notify(). So we
can only see this PF_NOFREEZE if the thread becomes a zombie.

> @@ -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;

May be freezing_slow_path() can check TASK_DEAD along with PF_NOFREEZE
instead? (or tsk->exit_state != 0 to avoid the asymmetry above). Just
to keep this logic in the freezer code. I dunno.

But this all is up to you and Rafael, I am not arguing. Just random
thoughts.

Oleg.

[PATCH pm-freezer 2/4] freezer: set PF_NOFREEZE on a dying task right before TASK_DEAD

[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();

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

[Oleg Nesterov]

On 08/29, Tejun Heo wrote:
>
> --- work.orig/kernel/cgroup_freezer.c
> +++ work/kernel/cgroup_freezer.c
> @@ -311,14 +311,14 @@ static int freezer_change_state(struct c
>  	if (goal_state == freezer->state)
>  		goto out;
>  
> -	freezer->state = goal_state;
> -
>  	switch (goal_state) {
>  	case CGROUP_THAWED:
> +		freezer->state = CGROUP_THAWED;
>  		atomic_dec(&system_freezing_cnt);
>  		unfreeze_cgroup(cgroup, freezer);
>  		break;
>  	case CGROUP_FROZEN:
> +		freezer->state = CGROUP_FREEZING;

At first glance, this is correct. I'll try to recheck.

But,

>  		atomic_inc(&system_freezing_cnt);

iiuc this becomes wrong... Suppose a user writes "FROZEN" twice,
before freezer->state becomes CGROUP_FROZEN.

I think we should actually fix the "goal_state == freezer->state"
check above.

Oleg.

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

[Oleg Nesterov]

On 08/29, Tejun Heo wrote:
>
> --- work.orig/kernel/cgroup_freezer.c
> +++ work/kernel/cgroup_freezer.c
> @@ -311,14 +311,14 @@ static int freezer_change_state(struct c
>  	if (goal_state == freezer->state)
>  		goto out;
>  
> -	freezer->state = goal_state;
> -
>  	switch (goal_state) {
>  	case CGROUP_THAWED:
> +		freezer->state = CGROUP_THAWED;
>  		atomic_dec(&system_freezing_cnt);
>  		unfreeze_cgroup(cgroup, freezer);
>  		break;
>  	case CGROUP_FROZEN:
> +		freezer->state = CGROUP_FREEZING;

At first glance, this is correct. I'll try to recheck.

But,

>  		atomic_inc(&system_freezing_cnt);

iiuc this becomes wrong... Suppose a user writes "FROZEN" twice,
before freezer->state becomes CGROUP_FROZEN.

I think we should actually fix the "goal_state == freezer->state"
check above.

Oleg.