freezer: fix wait_event_freezable/__thaw_task races

PATCH Applied Tejun Heo 2 participants 10 messages Nov 1, 2011 – Nov 3, 2011

Subsystems (3)

Suspend to RAM Hibernation Freezer

lore

Share & tools

lore

Tejun Heo

author

2011-11-01 02:15 UTC #0 lore raw

PATCH 1/1 freezer: fix wait_event_freezable/__thaw_task races

From: Oleg Nesterov <[email protected]>

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 <[email protected]>

Acked-by Tejun Heo <[email protected]>

---

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

include/linux/freezer.h +14 ~~-13~~

index 5f32321..4a1d933 100644
@@ -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; }

Signature · 1 line

1.7.3.1

Rafael J. Wysocki

Applied

2011-11-03 00:18 UTC #1 lore raw

Quoted 13 lines · Tejun Heo

From: Oleg Nesterov <[email protected]>
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.

Quoted 7 lines

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 <[email protected]>
Acked-by Tejun Heo <[email protected]>

Applied to linux-pm/linux-next.

Thanks,
Rafael

Quoted 84 lines

---

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

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

include/linux/freezer.h +14 ~~-13~~

index 5f32321..4a1d933 100644
@@ -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; }

Tejun Heo

author

2011-11-03 00:43 UTC #2 lore raw

Quoted 17 lines · Rafael J. Wysocki

Quoted 16 lines · Tejun Heo
From: Oleg Nesterov <[email protected]>
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.

Quoted 6 lines

Quoted 4 lines
tj: Updated to apply to wait_event_freezekillable() too.
Signed-off-by Oleg Nesterov <[email protected]>
Acked-by Tejun Heo <[email protected]>
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.

Signature · 1 line

tejun

Rafael J. Wysocki

2011-11-03 00:58 UTC #3 lore raw

Quoted 20 lines · Tejun Heo

Quoted 19 lines · Rafael J. Wysocki
Quoted 18 lines · Tejun Heo
From: Oleg Nesterov <[email protected]>
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.

Quoted 7 lines

Quoted 4 lines
Quoted 2 lines
Signed-off-by Oleg Nesterov <[email protected]>
Acked-by Tejun Heo <[email protected]>
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

Tejun Heo

author

2011-11-03 06:38 UTC #4 lore raw

Hello,

Quoted 1 line · Rafael J. Wysocki

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.

Signature · 1 line

tejun

Rafael J. Wysocki

2011-11-03 10:40 UTC #5 lore raw

Quoted 7 lines · Tejun Heo

Hello,
Quoted 1 line · Rafael J. Wysocki
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

Tejun Heo

author

2011-11-03 15:07 UTC #6 lore raw

Hello, Rafael.

Quoted 11 lines · Rafael J. Wysocki

Quoted 7 lines · Tejun Heo
Hello,
Quoted 1 line · Rafael J. Wysocki
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.

Signature · 8 lines

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

Rafael J. Wysocki

2011-11-03 22:10 UTC #7 lore raw

Quoted 17 lines · Tejun Heo

Hello, Rafael.
Quoted 11 lines · Rafael J. Wysocki
Quoted 7 lines · Tejun Heo
Hello,
Quoted 1 line · Rafael J. Wysocki
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.

Quoted 7 lines

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.

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

Quoted 11 lines

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

Tejun Heo

author

2011-11-03 22:16 UTC #8 lore raw

Hello,

Quoted 2 lines · Rafael J. Wysocki

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. :)

Quoted 5 lines

Quoted 2 lines
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.

Signature · 1 line

tejun

Rafael J. Wysocki

2011-11-03 22:20 UTC #9 lore raw

Quoted 7 lines · you

Hello,
Quoted 2 lines · Rafael J. Wysocki
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!

Quoted 8 lines

Quoted 5 lines
Quoted 2 lines
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