[PATCH pm] freezer: fix wait_event_freezable/__thaw_task races

[PATCH pm] freezer: fix wait_event_freezable/__thaw_task races

[Tejun Heo]

From: Oleg Nesterov <oleg@redhat.com>

wait_event_freezable() and friends 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.

tj: Updated to apply to wait_event_freezekillable() too.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Acked-by: Tejun Heo <tj@kernel.org>
---

So, this should be the last of lost patches.  It's on top of

linus/master
+[1] freezer: fix various bugs and simplify implementation, take#2
+[2] usb_storage: don't use set_freezable_with_signal()" + [3]
+[3] freezer: kill unused set_freezable_with_signal()

and available in the following git branch.

 git://git.kernel.org/pub/scm/linux/kernel/git/tj/misc.git pm-fix-wait_freezable

Thank you.

[1] http://thread.gmane.org/gmane.linux.kernel/1209247
[2] http://thread.gmane.org/gmane.linux.kernel/1209416
[3] http://thread.gmane.org/gmane.linux.kernel/1209416/focus=1209417

 include/linux/freezer.h |   27 ++++++++++++++-------------
 1 files changed, 14 insertions(+), 13 deletions(-)

diff --git a/include/linux/freezer.h b/include/linux/freezer.h
index 5f32321..4a1d933 100644
--- a/include/linux/freezer.h
+++ b/include/linux/freezer.h
@@ -113,42 +113,43 @@ static inline int freezer_should_skip(struct task_struct *p)
 #define wait_event_freezekillable(wq, condition)			\
 ({									\
 	int __retval;							\
-	do {								\
+	for (;;) {							\
 		__retval = wait_event_killable(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(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; }
-- 
1.7.3.1

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

[Rafael J. Wysocki]

On Tuesday, November 01, 2011, Tejun Heo wrote:
> From: Oleg Nesterov <oleg@redhat.com>
> 
> wait_event_freezable() and friends 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.
> 
> tj: Updated to apply to wait_event_freezekillable() too.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> Acked-by: Tejun Heo <tj@kernel.org>

Applied to linux-pm/linux-next.

Thanks,
Rafael


> ---
> 
> So, this should be the last of lost patches.  It's on top of
> 
> linus/master
> +[1] freezer: fix various bugs and simplify implementation, take#2
> +[2] usb_storage: don't use set_freezable_with_signal()" + [3]
> +[3] freezer: kill unused set_freezable_with_signal()
> 
> and available in the following git branch.
> 
>  git://git.kernel.org/pub/scm/linux/kernel/git/tj/misc.git pm-fix-wait_freezable
> 
> Thank you.
> 
> [1] http://thread.gmane.org/gmane.linux.kernel/1209247
> [2] http://thread.gmane.org/gmane.linux.kernel/1209416
> [3] http://thread.gmane.org/gmane.linux.kernel/1209416/focus=1209417
> 
>  include/linux/freezer.h |   27 ++++++++++++++-------------
>  1 files changed, 14 insertions(+), 13 deletions(-)
> 
> diff --git a/include/linux/freezer.h b/include/linux/freezer.h
> index 5f32321..4a1d933 100644
> --- a/include/linux/freezer.h
> +++ b/include/linux/freezer.h
> @@ -113,42 +113,43 @@ static inline int freezer_should_skip(struct task_struct *p)
>  #define wait_event_freezekillable(wq, condition)			\
>  ({									\
>  	int __retval;							\
> -	do {								\
> +	for (;;) {							\
>  		__retval = wait_event_killable(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(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 pm] freezer: fix wait_event_freezable/__thaw_task races

[Tejun Heo]

On Thu, Nov 03, 2011 at 01:20:41AM +0100, Rafael J. Wysocki wrote:
> On Tuesday, November 01, 2011, Tejun Heo wrote:
> > From: Oleg Nesterov <oleg@redhat.com>
> > 
> > wait_event_freezable() and friends 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.
> > 
> > tj: Updated to apply to wait_event_freezekillable() too.
> > 
> > Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> > Acked-by: Tejun Heo <tj@kernel.org>
> 
> Applied to linux-pm/linux-next.

Just in case, this patch isn't correct with preceding patches (the
freezer update patchset), so should only be applied after them.

Thanks.

-- 
tejun

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

[Rafael J. Wysocki]

On Thursday, November 03, 2011, Tejun Heo wrote:
> On Thu, Nov 03, 2011 at 01:20:41AM +0100, Rafael J. Wysocki wrote:
> > On Tuesday, November 01, 2011, Tejun Heo wrote:
> > > From: Oleg Nesterov <oleg@redhat.com>
> > > 
> > > wait_event_freezable() and friends 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.
> > > 
> > > tj: Updated to apply to wait_event_freezekillable() too.
> > > 
> > > Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> > > Acked-by: Tejun Heo <tj@kernel.org>
> > 
> > Applied to linux-pm/linux-next.
> 
> Just in case, this patch isn't correct with preceding patches (the
> freezer update patchset), so should only be applied after them.

OK, so I guess I should drop it from my linux-next branch?

Rafael

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

[Tejun Heo]

Hello,

On Wed, Nov 2, 2011 at 6:01 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> OK, so I guess I should drop it from my linux-next branch?

Hmmm... yes, the preceding patches should be applied first and then
this one. Any problems with earlier ones?

Thank you.

-- 
tejun

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

[Rafael J. Wysocki]

On Thursday, November 03, 2011, Tejun Heo wrote:
> Hello,
> 
> On Wed, Nov 2, 2011 at 6:01 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > OK, so I guess I should drop it from my linux-next branch?
> 
> Hmmm... yes, the preceding patches should be applied first and then
> this one. Any problems with earlier ones?

The only problem I currently have is to figure out what patches to apply
and in which order.  Care to help? ;-)

Rafael

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

[Tejun Heo]

Hello, Rafael.

On Thu, Nov 03, 2011 at 11:43:13AM +0100, Rafael J. Wysocki wrote:
> On Thursday, November 03, 2011, Tejun Heo wrote:
> > Hello,
> > 
> > On Wed, Nov 2, 2011 at 6:01 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > > OK, so I guess I should drop it from my linux-next branch?
> > 
> > Hmmm... yes, the preceding patches should be applied first and then
> > this one. Any problems with earlier ones?
> 
> The only problem I currently have is to figure out what patches to apply
> and in which order.  Care to help? ;-)

Heh, sorry about that, so to sum up the outstanding patches.

for-fixes (current merge window)

 [1] wait_event_freezekillable: use freezer_do_not_count/freezer_count
 [2] freezer: revert 27920651fe "PM / Freezer: Make fake_signal_wake_up() wake TASK_KILLABLE tasks too"

  It would be a good idea to note that the first patch makes thes
  change made by commit 27920651fe and thus reverting is safe.

for-next

 [3] freezer: fix various bugs and simplify implementation, take#2
 [4] usb_storage: don't use set_freezable_with_signal()
 [5] freezer: kill unused set_freezable_with_signal()
 [6] freezer: fix wait_event_freezable/__thaw_task races

All the for-next patches are in the following git branch in the above
order.

 git://git.kernel.org/pub/scm/linux/kernel/git/tj/misc.git pm-fix-wait_freezable

Thank you.

-- 
tejun

[1] http://thread.gmane.org/gmane.linux.kernel.cifs/4823/focus=4864
[2] http://thread.gmane.org/gmane.linux.kernel.cifs/4823
[3] http://thread.gmane.org/gmane.linux.kernel/1209247
[4] http://thread.gmane.org/gmane.linux.kernel/1209416
[5] http://thread.gmane.org/gmane.linux.kernel/1209416/focus=1209417
[6] http://thread.gmane.org/gmane.linux.kernel/1209444

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

[Rafael J. Wysocki]

On Thursday, November 03, 2011, Tejun Heo wrote:
> Hello, Rafael.
> 
> On Thu, Nov 03, 2011 at 11:43:13AM +0100, Rafael J. Wysocki wrote:
> > On Thursday, November 03, 2011, Tejun Heo wrote:
> > > Hello,
> > > 
> > > On Wed, Nov 2, 2011 at 6:01 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > > > OK, so I guess I should drop it from my linux-next branch?
> > > 
> > > Hmmm... yes, the preceding patches should be applied first and then
> > > this one. Any problems with earlier ones?
> > 
> > The only problem I currently have is to figure out what patches to apply
> > and in which order.  Care to help? ;-)
> 
> Heh, sorry about that, so to sum up the outstanding patches.
> 
> for-fixes (current merge window)
> 
>  [1] wait_event_freezekillable: use freezer_do_not_count/freezer_count
>  [2] freezer: revert 27920651fe "PM / Freezer: Make fake_signal_wake_up() wake TASK_KILLABLE tasks too"
> 
>   It would be a good idea to note that the first patch makes thes
>   change made by commit 27920651fe and thus reverting is safe.

OK

Can you repost these two with new changelogs so that there's no confusion
about what they are doing and why exactly, pretty please?

> for-next
> 
>  [3] freezer: fix various bugs and simplify implementation, take#2
>  [4] usb_storage: don't use set_freezable_with_signal()
>  [5] freezer: kill unused set_freezable_with_signal()
>  [6] freezer: fix wait_event_freezable/__thaw_task races
> 
> All the for-next patches are in the following git branch in the above
> order.
> 
>  git://git.kernel.org/pub/scm/linux/kernel/git/tj/misc.git pm-fix-wait_freezable

Good.  I'll put them into a separate branch and merge them into my linux-next
branch when 3.2-rc1 is out.

Thanks,
Rafael

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

[Tejun Heo]

Hello,

On Thu, Nov 3, 2011 at 3:12 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> Can you repost these two with new changelogs so that there's no confusion
> about what they are doing and why exactly, pretty please?

Will do. :)

>> for-next
>>  git://git.kernel.org/pub/scm/linux/kernel/git/tj/misc.git pm-fix-wait_freezable
>
> Good.  I'll put them into a separate branch and merge them into my linux-next
> branch when 3.2-rc1 is out.

Can you please hold fifteen minutes?  I'm updating the series to
reflect Srivatsa's review.  I'll send pull request soon.

Thank you.

-- 
tejun

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

[Rafael J. Wysocki]

On Thursday, November 03, 2011, you wrote:
> Hello,
> 
> On Thu, Nov 3, 2011 at 3:12 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > Can you repost these two with new changelogs so that there's no confusion
> > about what they are doing and why exactly, pretty please?
> 
> Will do. :)

Great, thanks!

> >> for-next
> >>  git://git.kernel.org/pub/scm/linux/kernel/git/tj/misc.git pm-fix-wait_freezable
> >
> > Good.  I'll put them into a separate branch and merge them into my linux-next
> > branch when 3.2-rc1 is out.
> 
> Can you please hold fifteen minutes?  I'm updating the series to
> reflect Srivatsa's review.  I'll send pull request soon.

OK, no problem.

Thanks,
Rafael