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

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

[Oleg Nesterov]

On 09/03, Tejun Heo wrote:
>
> @@ -308,24 +308,24 @@ static int freezer_change_state(struct cgroup *cgroup,
>  	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);
> +		if (freezer->state != CGROUP_THAWED)
> +			atomic_dec(&system_freezing_cnt);
> +		freezer->state = CGROUP_THAWED;
>  		unfreeze_cgroup(cgroup, freezer);
>  		break;
>  	case CGROUP_FROZEN:
> -		atomic_inc(&system_freezing_cnt);
> +		if (freezer->state == CGROUP_THAWED)
> +			atomic_inc(&system_freezing_cnt);
> +		freezer->state = CGROUP_FREEZING;
>  		retval = try_to_freeze_cgroup(cgroup, freezer);

Cough. Now it doesn't look right to me ;)

If the user write "FROZEN" into the control file, we should not
turn the CGROUP_FROZEN state into CGROUP_FREEZING. Probably this
is harmless, but this looks wrong and this doesn't match the
current behaviour, user-visible change.

Oleg.

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

[Tejun Heo]

On Sun, Sep 04, 2011 at 08:02:06PM +0200, Oleg Nesterov wrote:
> Cough. Now it doesn't look right to me ;)
> 
> If the user write "FROZEN" into the control file, we should not
> turn the CGROUP_FROZEN state into CGROUP_FREEZING. Probably this
> is harmless, but this looks wrong and this doesn't match the
> current behaviour, user-visible change.

It's not user-visible as the state is always updated before shown to
userland.  If we're gonna be re-priming freezing on each FROZEN write,
resetting the state to freezing to force state re-calculation is
logical thing to do.

Thanks.

-- 
tejun

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

[Oleg Nesterov]

On 09/05, Tejun Heo wrote:
>
> On Sun, Sep 04, 2011 at 08:02:06PM +0200, Oleg Nesterov wrote:
> > Cough. Now it doesn't look right to me ;)
> >
> > If the user write "FROZEN" into the control file, we should not
> > turn the CGROUP_FROZEN state into CGROUP_FREEZING. Probably this
> > is harmless, but this looks wrong and this doesn't match the
> > current behaviour, user-visible change.
>
> It's not user-visible as the state is always updated before shown to
> userland.

Ah, indeed, thanks.

OK.

So, the only proble I can see is that update_if_frozen() can hit
BUG_ON(nfrozen > 0), but this is off-topic.

I think this patch is correct.

Oleg.

Re: [PATCH 6/6] freezer: kill unused set_freezable_with_signal()

[Oleg Nesterov]

On 09/03, Tejun Heo wrote:
>
> This patch removes set_freezable_with_signal() along with
> PF_FREEZER_NOSIG

Great. I never understood PF_FREEZER_NOSIG logic ;)

One question,

> @@ -72,10 +72,6 @@ bool __refrigerator(bool check_kthr_stop)
>  		schedule();
>  	}
>
> -	spin_lock_irq(&current->sighand->siglock);
> -	recalc_sigpending(); /* We sent fake signal, clean it up */
> -	spin_unlock_irq(&current->sighand->siglock);
> -

Why? This recalc_sigpending() makes sense imho. Otherwise the user-space
tasks (almost) always return with TIF_SIGPENDING. May be we can do this
under "if (PF_KTRHREAD)".

For example. Suppose the user-space task does wait_event_freezable()...

Hmm. OTOH, wait_event_freezable() looks wrong anyway... So probably
this doesn't matter. ptrace_stop/get_signal_to_deliver doesn't need
this, probably we do not care about the other callers.

It seems, a lot of get_signal_to_deliver() calles also call
try_to_freeze() for no reason.


So, yes, I am starting to think this change is fine too ;)

Oleg.

Re: [PATCH 6/6] freezer: kill unused set_freezable_with_signal()

[Tejun Heo]

Hello, Oleg.

On Sun, Sep 04, 2011 at 08:46:26PM +0200, Oleg Nesterov wrote:
> > @@ -72,10 +72,6 @@ bool __refrigerator(bool check_kthr_stop)
> >  		schedule();
> >  	}
> >
> > -	spin_lock_irq(&current->sighand->siglock);
> > -	recalc_sigpending(); /* We sent fake signal, clean it up */
> > -	spin_unlock_irq(&current->sighand->siglock);
> > -
> 
> Why? This recalc_sigpending() makes sense imho. Otherwise the user-space
> tasks (almost) always return with TIF_SIGPENDING. May be we can do this
> under "if (PF_KTRHREAD)".

PF_KTHREAD no longer gets TIF_SIGPENDING set, so...

> For example. Suppose the user-space task does wait_event_freezable()...
> 
> Hmm. OTOH, wait_event_freezable() looks wrong anyway... So probably
> this doesn't matter. ptrace_stop/get_signal_to_deliver doesn't need
> this, probably we do not care about the other callers.

Can you elaborate on it being wrong?  Do you mean the possibility of
leaking spurious TIF_SIGPENDING?

> It seems, a lot of get_signal_to_deliver() calles also call
> try_to_freeze() for no reason.

Yeap, it doesn't really matter.  In most cases userland tasks would
get parked in the signal delivery path anyway and if a task is deep in
the kernel, it should and usually does hit syscall restart path.
Except for few special cases (maybe some of unfailable NFS calls),
there'userland tasks shouldn't take try_to_freeze() path outside of
signal delivery / job control path.

Also, in general, I don't think it's a good idea to add non-trivial
optimization like above to code path which is as cold as it gets
without any backing data.  The PM freezer used to busy loop until all
tasks enter refrigerator.  Nobody cares whether some tasks enter
signal delivery path one more time.

> So, yes, I am starting to think this change is fine too ;)

Yeay. :)

Thanks.

-- 
tejun

Re: [PATCH 6/6] freezer: kill unused set_freezable_with_signal()

[Tejun Heo]

On Mon, Sep 05, 2011 at 11:33:15AM +0900, Tejun Heo wrote:
> > So, yes, I am starting to think this change is fine too ;)
> 
> Yeay. :)

Ooh, can I add your Acked-by before sending pull request to Rafael?

Thanks.

-- 
tejun

Re: [PATCH 6/6] freezer: kill unused set_freezable_with_signal()

[Oleg Nesterov]

On 09/05, Tejun Heo wrote:
>
> Hello, Oleg.
>
> On Sun, Sep 04, 2011 at 08:46:26PM +0200, Oleg Nesterov wrote:
> > > @@ -72,10 +72,6 @@ bool __refrigerator(bool check_kthr_stop)
> > >  		schedule();
> > >  	}
> > >
> > > -	spin_lock_irq(&current->sighand->siglock);
> > > -	recalc_sigpending(); /* We sent fake signal, clean it up */
> > > -	spin_unlock_irq(&current->sighand->siglock);
> > > -
> >
> > Why? This recalc_sigpending() makes sense imho. Otherwise the user-space
> > tasks (almost) always return with TIF_SIGPENDING. May be we can do this
> > under "if (PF_KTRHREAD)".
>
> PF_KTHREAD no longer gets TIF_SIGPENDING set, so...

Yes,

> > For example. Suppose the user-space task does wait_event_freezable()...
> >
> > Hmm. OTOH, wait_event_freezable() looks wrong anyway... So probably
> > this doesn't matter. ptrace_stop/get_signal_to_deliver doesn't need
> > this, probably we do not care about the other callers.
>
> Can you elaborate on it being wrong?  Do you mean the possibility of
> leaking spurious TIF_SIGPENDING?

Perhaps it is correct... Just I do not understand what it should do.
I thought it is "wait_for_event && do_not_block_freezer". And at first
glance the code looks as if it tries to do this. Say, in the "likely"
case we restart wait_event_interruptible() after refrigerator().

But this looks racy. Suppose that freezing() is already false when
try_to_freeze() or __refrigerator() is called. Say, cgroup_freezer does
freeze_task() + __thaw_task(). Why it returns -ERESTARTSYS in this case?

And if it can be used by the userspace thread, then we should probably
do recalc_sigpending() somewhere, otherwise wait_event_freezable() will
always return -ERESTARTSYS after __refrigerator().

Oleg.

Re: [PATCH 6/6] freezer: kill unused set_freezable_with_signal()

[Oleg Nesterov]

On 09/05, Tejun Heo wrote:
>
> Ooh, can I add your Acked-by before sending pull request to Rafael?

Yes, thanks, please feel free.

Oleg.

Re: [PATCH 6/6] freezer: kill unused set_freezable_with_signal()

[Tejun Heo]

Hello,

On Mon, Sep 05, 2011 at 06:20:12PM +0200, Oleg Nesterov wrote:
> Perhaps it is correct... Just I do not understand what it should do.
> I thought it is "wait_for_event && do_not_block_freezer". And at first
> glance the code looks as if it tries to do this. Say, in the "likely"
> case we restart wait_event_interruptible() after refrigerator().
> 
> But this looks racy. Suppose that freezing() is already false when
> try_to_freeze() or __refrigerator() is called. Say, cgroup_freezer does
> freeze_task() + __thaw_task(). Why it returns -ERESTARTSYS in this case?

It may return -ERESTARTSYS when not strictly necessary but given that
it's supposed to trigger restart anyway I don't think it's actually
broken (it doesn't break the contract of the wait).  Another thing to
note is that kthread freezing via cgroup is almost fundamentally
broken.  With the removal of freezable_with_signal, this shouldn't be
an issue anymore although cgroup_freezer still needs to be fixed to
account for kthreads for xstate transitions (it currently triggers
BUG_ON).

> And if it can be used by the userspace thread, then we should probably
> do recalc_sigpending() somewhere, otherwise wait_event_freezable() will
> always return -ERESTARTSYS after __refrigerator().

Thankfully, currently, all the few users are kthreads.  I don't think
it makes sense for userland tasks at all.  Interruptible sleeps for
userland tasks necessiate the ability to return to signal delivery
path and restart or abort the current operation.  There's no point in
using wait_event_freezable() instead of wait_event_interruptible().

Thanks.

-- 
tejun

Re: [PATCH 6/6] freezer: kill unused set_freezable_with_signal()

[Oleg Nesterov]

On 09/06, Oleg Nesterov wrote:
>
> Yes, agreed. In this case I think it should be
>
> 	#define wait_event_freezable(wq, condition)				\
> 	({									\
> 		int __retval;							\
> 		for (;;) {							\
> 			__retval = wait_event_interruptible(wq, 		\
> 					(condition) || freezing(current));	\
> 			if (__retval || (condition))				\
> 				break;						\
> 			try_to_freeze();					\
> 		}								\
> 		__retval;							\
> 	})
>
> __retval/ERESTARTSYS is only needed for kthreads which play with allow_signal(),
> probably nobody should do this.

I meant, unless the caller plays with allow_signal(), it has all rights to do

	if (wait_event_freezable(...))
		BUG();

This becomes correct with the code above.

Oleg.

Re: [PATCH 6/6] freezer: kill unused set_freezable_with_signal()

[Tejun Heo]

Hello,

On Tue, Sep 06, 2011 at 05:25:39PM +0200, Oleg Nesterov wrote:
> On 09/06, Oleg Nesterov wrote:
> >
> > Yes, agreed. In this case I think it should be
> >
> > 	#define wait_event_freezable(wq, condition)				\
> > 	({									\
> > 		int __retval;							\
> > 		for (;;) {							\
> > 			__retval = wait_event_interruptible(wq, 		\
> > 					(condition) || freezing(current));	\
> > 			if (__retval || (condition))				\
> > 				break;						\
> > 			try_to_freeze();					\
> > 		}								\
> > 		__retval;							\
> > 	})
> >
> > __retval/ERESTARTSYS is only needed for kthreads which play with allow_signal(),
> > probably nobody should do this.
> 
> I meant, unless the caller plays with allow_signal(), it has all rights to do
> 
> 	if (wait_event_freezable(...))
> 		BUG();
> 
> This becomes correct with the code above.

Yeap, sure, w/ freezable_with_signal gone, the above should work fine.
Care to create a patch?

Thanks.

-- 
tejun

[PATCH 0/1] freezer: fix wait_event_freezable/__thaw_task races

[Oleg Nesterov]

Hi,

On 09/07, Tejun Heo wrote:
> >
> > I meant, unless the caller plays with allow_signal(), it has all rights to do
> >
> > 	if (wait_event_freezable(...))
> > 		BUG();
> >
> > This becomes correct with the code above.
>
> Yeap, sure, w/ freezable_with_signal gone, the above should work fine.
> Care to create a patch?

Sure.

But. Tejun, Rafael, I have to rely on your review. I have no idea how
can I test this change. Hopfully it is simple enough, though.

And. wait_event_freezable_timeout() obviously has another problem,
although I hope this is fine. The caller can spend a lot of time in
refrigerator, shouldn't we update "timeout" in this case? I hope we
shouldn't.

Oleg.

[PATCH 1/1] freezer: fix wait_event_freezable/__thaw_task races

[Oleg Nesterov]

wait_event_freezable() and wait_event_freezable_timeout() stop
the waiting if try_to_freeze() fails. This is not right, we can
race with __thaw_task() and in this case

	- wait_event_freezable() returns the wrong ERESTARTSYS

	- wait_event_freezable_timeout() can return the positive
	  value while condition == F

Change the code to always check __retval/condition before return.

Note: with or without this patch the timeout logic looks strange,
probably we should recalc timeout if try_to_freeze() returns T.

Signed-off-by: Oleg Nesterov <oleg-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---

 include/linux/freezer.h |   19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

--- 3.1/include/linux/freezer.h~w_e_f	2011-09-04 20:23:30.000000000 +0200
+++ 3.1/include/linux/freezer.h	2011-09-07 20:00:27.000000000 +0200
@@ -107,32 +107,33 @@ static inline int freezer_should_skip(st
  * Freezer-friendly wrappers around wait_event_interruptible() and
  * wait_event_interruptible_timeout(), originally defined in <linux/wait.h>
  */
-
 #define wait_event_freezable(wq, condition)				\
 ({									\
 	int __retval;							\
-	do {								\
+	for (;;) {							\
 		__retval = wait_event_interruptible(wq, 		\
 				(condition) || freezing(current));	\
-		if (__retval && !freezing(current))			\
+		if (__retval || (condition))				\
 			break;						\
-		else if (!(condition))					\
-			__retval = -ERESTARTSYS;			\
-	} while (try_to_freeze());					\
+		try_to_freeze();					\
+	}								\
 	__retval;							\
 })
 
-
 #define wait_event_freezable_timeout(wq, condition, timeout)		\
 ({									\
 	long __retval = timeout;					\
-	do {								\
+	for (;;) {							\
 		__retval = wait_event_interruptible_timeout(wq,		\
 				(condition) || freezing(current),	\
 				__retval); 				\
-	} while (try_to_freeze());					\
+		if (__retval <= 0 || (condition))			\
+			break;						\
+		try_to_freeze();					\
+	}								\
 	__retval;							\
 })
+
 #else /* !CONFIG_FREEZER */
 static inline bool frozen(struct task_struct *p) { return false; }
 static inline bool freezing(struct task_struct *p) { return false; }

Re: [PATCH 1/1] freezer: fix wait_event_freezable/__thaw_task races

[Tejun Heo]

Hello,

On Wed, Sep 07, 2011 at 08:22:17PM +0200, Oleg Nesterov wrote:
> wait_event_freezable() and wait_event_freezable_timeout() stop
> the waiting if try_to_freeze() fails. This is not right, we can
> race with __thaw_task() and in this case
> 
> 	- wait_event_freezable() returns the wrong ERESTARTSYS
> 
> 	- wait_event_freezable_timeout() can return the positive
> 	  value while condition == F

Indeed, nice catch.  This one actually is pretty dangerous.  We
probably want to make a separate fix for this and backport it to
-stable?

> Change the code to always check __retval/condition before return.
> 
> Note: with or without this patch the timeout logic looks strange,
> probably we should recalc timeout if try_to_freeze() returns T.
> 
> Signed-off-by: Oleg Nesterov <oleg-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Yeap, with freezable_with_signal gone, this looks correct & simpler to
me but it would be nice if you can sprinkle some documentation while
at it. :)

Thanks.

-- 
tejun

Re: [PATCH 1/1] freezer: fix wait_event_freezable/__thaw_task races

[Oleg Nesterov]

Hi,

On 09/08, Tejun Heo wrote:
>
> Hello,
>
> On Wed, Sep 07, 2011 at 08:22:17PM +0200, Oleg Nesterov wrote:
> > wait_event_freezable() and wait_event_freezable_timeout() stop
> > the waiting if try_to_freeze() fails. This is not right, we can
> > race with __thaw_task() and in this case
> >
> > 	- wait_event_freezable() returns the wrong ERESTARTSYS
> >
> > 	- wait_event_freezable_timeout() can return the positive
> > 	  value while condition == F
>
> Indeed, nice catch.  This one actually is pretty dangerous.  We
> probably want to make a separate fix for this and backport it to
> -stable?

And I forgot to mention that wait_event_freezable_timeout() doesn't
handle -ERESTARTSYS at all.

But I don't think -stable needs this fix. According to grep, nobody
check the returned value, and none of the callers plays with signals.

> > Change the code to always check __retval/condition before return.
> >
> > Note: with or without this patch the timeout logic looks strange,
> > probably we should recalc timeout if try_to_freeze() returns T.
> >
> > Signed-off-by: Oleg Nesterov <oleg-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>
> Yeap, with freezable_with_signal gone, this looks correct & simpler to
> me

I don't really understand this... set_freezable_with_signal() has a
lot of problems, yes... But even if it wasn't removed this fix makes
sense anyway, afaics.

If freezable_with_signal caller does wait_event_freezable_timeout(),
__retval becomes -ERESTARTSYS after freeze_task(). The next iteration
will return 0 with the KERN_ERR message from schedule_timeout().

> but it would be nice if you can sprinkle some documentation while
> at it. :)

But they already have the comment ;) What can I add?

Oleg.

Re: [PATCH 1/1] freezer: fix wait_event_freezable/__thaw_task races

[Tejun Heo]

Hello,

On Thu, Sep 08, 2011 at 07:59:26PM +0200, Oleg Nesterov wrote:
> > Indeed, nice catch.  This one actually is pretty dangerous.  We
> > probably want to make a separate fix for this and backport it to
> > -stable?
> 
> And I forgot to mention that wait_event_freezable_timeout() doesn't
> handle -ERESTARTSYS at all.
> 
> But I don't think -stable needs this fix. According to grep, nobody
> check the returned value, and none of the callers plays with signals.

Ah, no user, okay.

> > Yeap, with freezable_with_signal gone, this looks correct & simpler to
> > me
> 
> I don't really understand this... set_freezable_with_signal() has a
> lot of problems, yes... But even if it wasn't removed this fix makes
> sense anyway, afaics.
> 
> If freezable_with_signal caller does wait_event_freezable_timeout(),
> __retval becomes -ERESTARTSYS after freeze_task(). The next iteration
> will return 0 with the KERN_ERR message from schedule_timeout().

Hmmm... but with the change, if a kthread gets TIF_SIGPENDING from
freezer and then thawed, it would not enter try_to_freeze() and thus
won't clear TIF_SIGPENDING.  The original code was racy too but the
window would be much larger afterwards.  Anyways, this doesn't matter
anymore.

> > but it would be nice if you can sprinkle some documentation while
> > at it. :)
> 
> But they already have the comment ;) What can I add?

Proper /** - */ comment w/ return value documentation? :P

Thanks.

-- 
tejun

Re: [PATCH 6/6] freezer: kill unused set_freezable_with_signal()

[Oleg Nesterov]

Hi,

On 09/06, Tejun Heo wrote:
>
> Hello,
>
> On Mon, Sep 05, 2011 at 06:20:12PM +0200, Oleg Nesterov wrote:
> > Perhaps it is correct... Just I do not understand what it should do.
> > I thought it is "wait_for_event && do_not_block_freezer". And at first
> > glance the code looks as if it tries to do this. Say, in the "likely"
> > case we restart wait_event_interruptible() after refrigerator().
> >
> > But this looks racy. Suppose that freezing() is already false when
> > try_to_freeze() or __refrigerator() is called. Say, cgroup_freezer does
> > freeze_task() + __thaw_task(). Why it returns -ERESTARTSYS in this case?
>
> It may return -ERESTARTSYS when not strictly necessary but given that
> it's supposed to trigger restart anyway I don't think it's actually
> broken (it doesn't break the contract of the wait).

OK, but still this doesn't look right.

> Another thing to
> note is that kthread freezing via cgroup is almost fundamentally
> broken.

OK, let it be freeze_processes()+thaw_tasks(). Of course this is mostly
theoretical.

> > And if it can be used by the userspace thread, then we should probably
> > do recalc_sigpending() somewhere, otherwise wait_event_freezable() will
> > always return -ERESTARTSYS after __refrigerator().
>
> Thankfully, currently, all the few users are kthreads.  I don't think
> it makes sense for userland tasks at all.

Yes, agreed. In this case I think it should be

	#define wait_event_freezable(wq, condition)				\
	({									\
		int __retval;							\
		for (;;) {							\
			__retval = wait_event_interruptible(wq, 		\
					(condition) || freezing(current));	\
			if (__retval || (condition))				\
				break;						\
			try_to_freeze();					\
		}								\
		__retval;							\
	})

__retval/ERESTARTSYS is only needed for kthreads which play with allow_signal(),
probably nobody should do this.

Oleg.

Re: [PATCH 6/6] freezer: kill unused set_freezable_with_signal()

[Matt Helsley]

On Tue, Sep 06, 2011 at 12:28:55PM +0900, Tejun Heo wrote:
> Hello,
> 
> On Mon, Sep 05, 2011 at 06:20:12PM +0200, Oleg Nesterov wrote:
> > Perhaps it is correct... Just I do not understand what it should do.
> > I thought it is "wait_for_event && do_not_block_freezer". And at first
> > glance the code looks as if it tries to do this. Say, in the "likely"
> > case we restart wait_event_interruptible() after refrigerator().
> > 
> > But this looks racy. Suppose that freezing() is already false when
> > try_to_freeze() or __refrigerator() is called. Say, cgroup_freezer does
> > freeze_task() + __thaw_task(). Why it returns -ERESTARTSYS in this case?
> 
> It may return -ERESTARTSYS when not strictly necessary but given that
> it's supposed to trigger restart anyway I don't think it's actually
> broken (it doesn't break the contract of the wait).  Another thing to
> note is that kthread freezing via cgroup is almost fundamentally
> broken.  With the removal of freezable_with_signal, this shouldn't be
> an issue anymore although cgroup_freezer still needs to be fixed to
> account for kthreads for xstate transitions (it currently triggers
> BUG_ON).

Looking at the code and docs I realize I didn't explicitly mention that
kthreads could not be frozen by the cgroup freezer. However, the code did
not allow it. When freezing tasks the cgroup freezer always did:

	freeze_task(task, true)

which would only freeze tasks without PF_FREEZER_NOSIG due to the second
"sig_only" parameter. I believe this means it could not be used to
freeze kthreads.

My feeling is kthreads should not be "managed" directly by userspace.
Especially if that includes the ability to arbitrarily stop or freeze them.
So it seems more appropriate to explicitly disallow freezing of kthreads
via the cgroup freezer.

Cheers,
	-Matt Helsley

Re: [PATCH 6/6] freezer: kill unused set_freezable_with_signal()

[Tejun Heo]

Hello, Matt.

On Thu, Sep 08, 2011 at 11:01:59AM -0700, Matt Helsley wrote:
> Looking at the code and docs I realize I didn't explicitly mention that
> kthreads could not be frozen by the cgroup freezer. However, the code did
> not allow it. When freezing tasks the cgroup freezer always did:
> 
> 	freeze_task(task, true)
> 
> which would only freeze tasks without PF_FREEZER_NOSIG due to the second
> "sig_only" parameter. I believe this means it could not be used to
> freeze kthreads.
> 
> My feeling is kthreads should not be "managed" directly by userspace.
> Especially if that includes the ability to arbitrarily stop or freeze them.
> So it seems more appropriate to explicitly disallow freezing of kthreads
> via the cgroup freezer.

Oh yeah, that's what we should do and maybe that's what the current
code intends to achieve too but currently it ends up triggering
BUG_ON().

Thanks.

-- 
tejun

[PATCH 1/6] 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.

-v3: Matt pointed out that setting CGROUP_FROZEN always invoked
     try_to_freeze_cgroup() regardless of the current state.  Patch
     updated such that the actual freeze/thaw operations are always
     performed on state changes.  This shouldn't make any difference
     unless something is broken.

Signed-off-by: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Reported-by: Oleg Nesterov <oleg-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Cc: Paul Menage <paul-inf54ven1CmVyaH7bEyXVA@public.gmane.org>
Cc: "Rafael J. Wysocki" <rjw-KKrjLPT3xs0@public.gmane.org>
Cc: Matt Helsley <matthltc-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
---
 kernel/cgroup_freezer.c |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/kernel/cgroup_freezer.c b/kernel/cgroup_freezer.c
index 4e82525..56a457a 100644
--- a/kernel/cgroup_freezer.c
+++ b/kernel/cgroup_freezer.c
@@ -308,24 +308,24 @@ static int freezer_change_state(struct cgroup *cgroup,
 	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);
+		if (freezer->state != CGROUP_THAWED)
+			atomic_dec(&system_freezing_cnt);
+		freezer->state = CGROUP_THAWED;
 		unfreeze_cgroup(cgroup, freezer);
 		break;
 	case CGROUP_FROZEN:
-		atomic_inc(&system_freezing_cnt);
+		if (freezer->state == CGROUP_THAWED)
+			atomic_inc(&system_freezing_cnt);
+		freezer->state = CGROUP_FREEZING;
 		retval = try_to_freeze_cgroup(cgroup, freezer);
 		break;
 	default:
 		BUG();
 	}
-out:
+
 	spin_unlock_irq(&freezer->lock);
 
 	return retval;
-- 
1.7.6

[PATCH 2/6] 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-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 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/kernel/exit.c b/kernel/exit.c
index ac58259..7b6c4fa 100644
--- a/kernel/exit.c
+++ b/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();
-- 
1.7.6

[PATCH 3/6] 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.

-v2: Frozen loop exited without releasing freezer_lock.  Fixed.

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 |   29 +++++++++++------------------
 1 files changed, 11 insertions(+), 18 deletions(-)

diff --git a/kernel/freezer.c b/kernel/freezer.c
index 466ea6b..ebee1ab 100644
--- a/kernel/freezer.c
+++ b/kernel/freezer.c
@@ -52,36 +52,29 @@ 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()))
+			current->flags &= ~PF_FROZEN;
+		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);
 
-- 
1.7.6

[PATCH 4/6] 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 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/kernel/freezer.c b/kernel/freezer.c
index ebee1ab..9846133 100644
--- a/kernel/freezer.c
+++ b/kernel/freezer.c
@@ -93,9 +93,10 @@ static void fake_signal_wake_up(struct task_struct *p)
 {
 	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);
+	}
 }
 
 /**
-- 
1.7.6

[PATCH 5/6] freezer: remove unused @sig_only from freeze_task()

[Tejun Heo]

After 25a16d02ba "freezer: make freezing() test freeze conditions in
effect instead of TIF_FREEZE", freezing() returns authoritative answer
on whether the current task should freeze or not and freeze_task()
doesn't need or use @sig_only.  Remove it.

While at it, rewrite function comment for freeze_task() and rename
@sig_only to @user_only in try_to_freeze_tasks().

This patch doesn't cause any functional change.

Signed-off-by: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 include/linux/freezer.h |    2 +-
 kernel/cgroup_freezer.c |    4 ++--
 kernel/freezer.c        |   21 +++++++++------------
 kernel/power/process.c  |    8 ++++----
 4 files changed, 16 insertions(+), 19 deletions(-)

diff --git a/include/linux/freezer.h b/include/linux/freezer.h
index 122b5ce..9193bae 100644
--- a/include/linux/freezer.h
+++ b/include/linux/freezer.h
@@ -47,7 +47,7 @@ static inline bool try_to_freeze(void)
 	return __refrigerator(false);
 }
 
-extern bool freeze_task(struct task_struct *p, bool sig_only);
+extern bool freeze_task(struct task_struct *p);
 extern bool __set_freezable(bool with_signal);
 
 #ifdef CONFIG_CGROUP_FREEZER
diff --git a/kernel/cgroup_freezer.c b/kernel/cgroup_freezer.c
index 56a457a..8753e7b 100644
--- a/kernel/cgroup_freezer.c
+++ b/kernel/cgroup_freezer.c
@@ -206,7 +206,7 @@ static void freezer_fork(struct cgroup_subsys *ss, struct task_struct *task)
 
 	/* Locking avoids race with FREEZING -> THAWED transitions. */
 	if (freezer->state == CGROUP_FREEZING)
-		freeze_task(task, true);
+		freeze_task(task);
 	spin_unlock_irq(&freezer->lock);
 }
 
@@ -274,7 +274,7 @@ static int try_to_freeze_cgroup(struct cgroup *cgroup, struct freezer *freezer)
 
 	cgroup_iter_start(cgroup, &it);
 	while ((task = cgroup_iter_next(cgroup, &it))) {
-		if (!freeze_task(task, true))
+		if (!freeze_task(task))
 			continue;
 		if (frozen(task))
 			continue;
diff --git a/kernel/freezer.c b/kernel/freezer.c
index 9846133..da76b64 100644
--- a/kernel/freezer.c
+++ b/kernel/freezer.c
@@ -100,20 +100,17 @@ static void fake_signal_wake_up(struct task_struct *p)
 }
 
 /**
- *	freeze_task - send a freeze request to given task
- *	@p: task to send the request to
- *	@sig_only: if set, the request will only be sent if the task has the
- *		PF_FREEZER_NOSIG flag unset
- *	Return value: 'false', if @sig_only is set and the task has
- *		PF_FREEZER_NOSIG set or the task is frozen, 'true', otherwise
+ * freeze_task - send a freeze request to given task
+ * @p: task to send the request to
  *
- *	The freeze request is sent by setting the tasks's TIF_FREEZE flag and
- *	either sending a fake signal to it or waking it up, depending on whether
- *	or not it has PF_FREEZER_NOSIG set.  If @sig_only is set and the task
- *	has PF_FREEZER_NOSIG set (ie. it is a typical kernel thread), its
- *	TIF_FREEZE flag will not be set.
+ * If @p is freezing, the freeze request is sent by setting %TIF_FREEZE
+ * flag and either sending a fake signal to it or waking it up, depending
+ * on whether it has %PF_FREEZER_NOSIG set.
+ *
+ * RETURNS:
+ * %false, if @p is not freezing or already frozen; %true, otherwise
  */
-bool freeze_task(struct task_struct *p, bool sig_only)
+bool freeze_task(struct task_struct *p)
 {
 	unsigned long flags;
 
diff --git a/kernel/power/process.c b/kernel/power/process.c
index fe06ddf..ab4fd7b 100644
--- a/kernel/power/process.c
+++ b/kernel/power/process.c
@@ -22,7 +22,7 @@
  */
 #define TIMEOUT	(20 * HZ)
 
-static int try_to_freeze_tasks(bool sig_only)
+static int try_to_freeze_tasks(bool user_only)
 {
 	struct task_struct *g, *p;
 	unsigned long end_time;
@@ -37,14 +37,14 @@ static int try_to_freeze_tasks(bool sig_only)
 
 	end_time = jiffies + TIMEOUT;
 
-	if (!sig_only)
+	if (!user_only)
 		freeze_workqueues_begin();
 
 	while (true) {
 		todo = 0;
 		read_lock(&tasklist_lock);
 		do_each_thread(g, p) {
-			if (p == current || !freeze_task(p, sig_only))
+			if (p == current || !freeze_task(p))
 				continue;
 
 			/*
@@ -65,7 +65,7 @@ static int try_to_freeze_tasks(bool sig_only)
 		} while_each_thread(g, p);
 		read_unlock(&tasklist_lock);
 
-		if (!sig_only) {
+		if (!user_only) {
 			wq_busy = freeze_workqueues_busy();
 			todo += wq_busy;
 		}
-- 
1.7.6

[PATCH 6/6] freezer: kill unused set_freezable_with_signal()

[Tejun Heo]

There's no in-kernel user of set_freezable_with_signal() and there's
no plan to add one.  Mixing TIF_SIGPENDING with kernel threads can
lead to nasty corner cases as kernel threads never travel signal
delivery path on their own.

e.g. the current implementation is buggy in the cancelation path of
__thaw_task().  It calls recalc_sigpending_and_wake() in an attempt to
clear TIF_SIGPENDING but the function never clears it regardless of
sigpending state.  This means that signallable freezable kthreads may
continue executing with !freezing() && stuck TIF_SIGPENDING, which can
be troublesome.

This patch removes set_freezable_with_signal() along with
PF_FREEZER_NOSIG and recalc_sigpending*() calls in freezer.  User
tasks get TIF_SIGPENDING, kernel tasks get woken up and the spurious
sigpending is dealt with in the usual signal delivery path.

Signed-off-by: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 include/linux/freezer.h |   20 +-------------------
 include/linux/sched.h   |    1 -
 kernel/freezer.c        |   27 ++++++---------------------
 kernel/kthread.c        |    2 +-
 4 files changed, 8 insertions(+), 42 deletions(-)

diff --git a/include/linux/freezer.h b/include/linux/freezer.h
index 9193bae..7ffbf04 100644
--- a/include/linux/freezer.h
+++ b/include/linux/freezer.h
@@ -48,7 +48,7 @@ static inline bool try_to_freeze(void)
 }
 
 extern bool freeze_task(struct task_struct *p);
-extern bool __set_freezable(bool with_signal);
+extern bool set_freezable(void);
 
 #ifdef CONFIG_CGROUP_FREEZER
 extern bool cgroup_freezing(struct task_struct *task);
@@ -104,23 +104,6 @@ static inline int freezer_should_skip(struct task_struct *p)
 }
 
 /*
- * Tell the freezer that the current task should be frozen by it
- */
-static inline bool set_freezable(void)
-{
-	return __set_freezable(false);
-}
-
-/*
- * Tell the freezer that the current task should be frozen by it and that it
- * should send a fake signal to the task to freeze it.
- */
-static inline bool set_freezable_with_signal(void)
-{
-	return __set_freezable(true);
-}
-
-/*
  * Freezer-friendly wrappers around wait_event_interruptible() and
  * wait_event_interruptible_timeout(), originally defined in <linux/wait.h>
  */
@@ -164,7 +147,6 @@ static inline void freezer_do_not_count(void) {}
 static inline void freezer_count(void) {}
 static inline int freezer_should_skip(struct task_struct *p) { return 0; }
 static inline void set_freezable(void) {}
-static inline void set_freezable_with_signal(void) {}
 
 #define wait_event_freezable(wq, condition)				\
 		wait_event_interruptible(wq, condition)
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 1bb3356..6d45ce7 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1784,7 +1784,6 @@ extern void thread_group_times(struct task_struct *p, cputime_t *ut, cputime_t *
 #define PF_MEMPOLICY	0x10000000	/* Non-default NUMA mempolicy */
 #define PF_MUTEX_TESTER	0x20000000	/* Thread belongs to the rt mutex tester */
 #define PF_FREEZER_SKIP	0x40000000	/* Freezer should not count it as freezable */
-#define PF_FREEZER_NOSIG 0x80000000	/* Freezer won't send signals to it */
 
 /*
  * Only the _current_ task can read/write to tsk->flags, but other
diff --git a/kernel/freezer.c b/kernel/freezer.c
index da76b64..770e6f5 100644
--- a/kernel/freezer.c
+++ b/kernel/freezer.c
@@ -39,7 +39,7 @@ bool freezing_slow_path(struct task_struct *p)
 	if (pm_nosig_freezing || cgroup_freezing(p))
 		return true;
 
-	if (pm_freezing && !(p->flags & PF_FREEZER_NOSIG))
+	if (pm_freezing && !(p->flags & PF_KTHREAD))
 		return true;
 
 	return false;
@@ -72,10 +72,6 @@ bool __refrigerator(bool check_kthr_stop)
 		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);
 
 	/*
@@ -120,7 +116,7 @@ bool freeze_task(struct task_struct *p)
 		return false;
 	}
 
-	if (!(p->flags & PF_FREEZER_NOSIG)) {
+	if (!(p->flags & PF_KTHREAD)) {
 		fake_signal_wake_up(p);
 		/*
 		 * fake_signal_wake_up() goes through p's scheduler
@@ -145,28 +141,19 @@ 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);
 }
 
 /**
- * __set_freezable - make %current freezable
- * @with_signal: do we want %TIF_SIGPENDING for notification too?
+ * set_freezable - make %current freezable
  *
  * Mark %current freezable and enter refrigerator if necessary.
  */
-bool __set_freezable(bool with_signal)
+bool set_freezable(void)
 {
 	might_sleep();
 
@@ -177,10 +164,8 @@ bool __set_freezable(bool with_signal)
 	 */
 	spin_lock_irq(&freezer_lock);
 	current->flags &= ~PF_NOFREEZE;
-	if (with_signal)
-		current->flags &= ~PF_FREEZER_NOSIG;
 	spin_unlock_irq(&freezer_lock);
 
 	return try_to_freeze();
 }
-EXPORT_SYMBOL(__set_freezable);
+EXPORT_SYMBOL(set_freezable);
diff --git a/kernel/kthread.c b/kernel/kthread.c
index a6cbeea..7a4c862 100644
--- a/kernel/kthread.c
+++ b/kernel/kthread.c
@@ -282,7 +282,7 @@ int kthreadd(void *unused)
 	set_cpus_allowed_ptr(tsk, cpu_all_mask);
 	set_mems_allowed(node_states[N_HIGH_MEMORY]);
 
-	current->flags |= PF_NOFREEZE | PF_FREEZER_NOSIG;
+	current->flags |= PF_NOFREEZE;
 
 	for (;;) {
 		set_current_state(TASK_INTERRUPTIBLE);
-- 
1.7.6

Re: [PATCHSET pm-freezer] freezer: fixes & simplifications

[Oleg Nesterov]

On 09/03, Tejun Heo wrote:
>
> These six patches are fixes and simplifications for the recent freezer
> changes.  The first four have been posted twice.  Changes since the
> last posting[L] are
>
> * freezer->state setting bug fix updated per Matt Helsley so that the
>   actual per-task freeze/thaw operations are always performed.
>
> * fixed a bug caused by forgetting to unlock freezer_lock in "freezer:
>   restructure __refrigerator()".
>
> * Two patches added.  The fifth one is mostly trivial.  The sixth a
>   bit more involved but still shouldn't cause any noticeable
>   functional difference.  This removes the buggy and unused
>   freezable_with_signal.

Afaics, everything is fine.

Oleg.