[RC][PATCH -mm] PM: Prevent frozen user mode helpers from failing the freezing of tasks

[RC][PATCH -mm] PM: Prevent frozen user mode helpers from failing the freezing of tasks

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>

At present, if a user mode helper is running while usermodehelper_pm_callback()
is executed, the helper may be frozen and the completion in
call_usermodehelper_exec() won't be completed until user space processes are
thawed.  As a result, the freezing of kernel threads may fail, which is not
desirable.

Prevent this from happening by introducing a counter of running user mode
helpers and allowing usermodehelper_pm_callback() to succeed for
action = PM_HIBERNATION_PREPARE or action = PM_SUSPEND_PREPARE only if there
are no helpers running.  [Namely, usermodehelper_pm_callback() waits for at most
RUNNING_HELPERS_TIMEOUT for the number of running helpers to become zero and
fails if that doesn't happen.]

Special thanks to Uli Luckas <u.luckas@road.de> for reviewing the previous
versions of this patch and for very useful comments.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
Acked-by: Nigel Cunningham <nigel@nigel.suspend2.net>
Acked-by: Uli Luckas <u.luckas@road.de>
Reviewed-by: Pavel Machek <pavel@ucw.cz>
---
 kernel/kmod.c |   70 +++++++++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 60 insertions(+), 10 deletions(-)

Index: linux-2.6.22-rc4-mm2/kernel/kmod.c
===================================================================
--- linux-2.6.22-rc4-mm2.orig/kernel/kmod.c
+++ linux-2.6.22-rc4-mm2/kernel/kmod.c
@@ -41,14 +41,6 @@ extern int max_threads;
 
 static struct workqueue_struct *khelper_wq;
 
-/*
- * If set, both call_usermodehelper_keys() and call_usermodehelper_pipe() exit
- * immediately returning -EBUSY.  Used for preventing user land processes from
- * being created after the user land has been frozen during a system-wide
- * hibernation or suspend operation.
- */
-static int usermodehelper_disabled;
-
 #ifdef CONFIG_KMOD
 
 /*
@@ -275,15 +267,48 @@ static void __call_usermodehelper(struct
 	}
 }
 
+#ifdef CONFIG_PM
+/*
+ * If set, call_usermodehelper_exec() will exit immediately returning -EBUSY
+ * (used for preventing user land processes from being created after the user
+ * land has been frozen during a system-wide hibernation or suspend operation).
+ */
+static int usermodehelper_disabled;
+
+/* Number of helpers running */
+static atomic_t running_helpers = ATOMIC_INIT(0);
+
+/*
+ * Wait queue head used by usermodehelper_pm_callback() to wait for all running
+ * helpers to finish.
+ */
+static DECLARE_WAIT_QUEUE_HEAD(running_helpers_waitq);
+
+/*
+ * Time to wait for running_helpers to become zero before the setting of
+ * usermodehelper_disabled in usermodehelper_pm_callback() fails
+ */
+#define RUNNING_HELPERS_TIMEOUT	(5 * HZ)
+
 static int usermodehelper_pm_callback(struct notifier_block *nfb,
 					unsigned long action,
 					void *ignored)
 {
+	long retval;
+
 	switch (action) {
 	case PM_HIBERNATION_PREPARE:
 	case PM_SUSPEND_PREPARE:
 		usermodehelper_disabled = 1;
-		return NOTIFY_OK;
+		retval = wait_event_timeout(running_helpers_waitq,
+					atomic_read(&running_helpers) == 0,
+					RUNNING_HELPERS_TIMEOUT);
+		if (retval) {
+			return NOTIFY_OK;
+		} else {
+			usermodehelper_disabled = 0;
+			return NOTIFY_BAD;
+		}
 	case PM_POST_HIBERNATION:
 	case PM_POST_SUSPEND:
 		usermodehelper_disabled = 0;
@@ -293,6 +318,29 @@ static int usermodehelper_pm_callback(st
 	return NOTIFY_DONE;
 }
 
+static void new_helper(void)
+{
+	atomic_inc(&running_helpers);
+}
+
+static void helper_finished(void)
+{
+	if (atomic_dec_and_test(&running_helpers))
+		wake_up(&running_helpers_waitq);
+}
+
+static void register_pm_notifier_callback(void)
+{
+	pm_notifier(usermodehelper_pm_callback, 0);
+}
+#else /* CONFIG_PM */
+#define usermodehelper_disabled	0
+
+static inline void new_helper(void) {}
+static inline void helper_finished(void) {}
+static inline void register_pm_notifier_callback(void) {}
+#endif /* CONFIG_PM */
+
 /**
  * call_usermodehelper_setup - prepare to call a usermode helper
  * @path - path to usermode executable
@@ -397,6 +445,7 @@ int call_usermodehelper_exec(struct subp
 	DECLARE_COMPLETION_ONSTACK(done);
 	int retval;
 
+	new_helper();
 	if (sub_info->path[0] == '\0') {
 		retval = 0;
 		goto out;
@@ -418,6 +467,7 @@ int call_usermodehelper_exec(struct subp
 
   out:
 	call_usermodehelper_freeinfo(sub_info);
+	helper_finished();
 	return retval;
 }
 EXPORT_SYMBOL(call_usermodehelper_exec);
@@ -459,5 +509,5 @@ void __init usermodehelper_init(void)
 {
 	khelper_wq = create_singlethread_workqueue("khelper");
 	BUG_ON(!khelper_wq);
-	pm_notifier(usermodehelper_pm_callback, 0);
+	register_pm_notifier_callback();
 }

Re: [RC][PATCH -mm] PM: Prevent frozen user mode helpers from failing the freezing of tasks

[Pavel Machek]

Hi!

> From: Rafael J. Wysocki <rjw@sisk.pl>
> 
> At present, if a user mode helper is running while usermodehelper_pm_callback()
> is executed, the helper may be frozen and the completion in
> call_usermodehelper_exec() won't be completed until user space processes are
> thawed.  As a result, the freezing of kernel threads may fail, which is not
> desirable.
> 
> Prevent this from happening by introducing a counter of running user mode
> helpers and allowing usermodehelper_pm_callback() to succeed for
> action = PM_HIBERNATION_PREPARE or action = PM_SUSPEND_PREPARE only if there
> are no helpers running.  [Namely, usermodehelper_pm_callback() waits for at most
> RUNNING_HELPERS_TIMEOUT for the number of running helpers to become zero and
> fails if that doesn't happen.]
> 
> Special thanks to Uli Luckas <u.luckas@road.de> for reviewing the previous
> versions of this patch and for very useful comments.
...
> Reviewed-by: Pavel Machek <pavel@ucw.cz>

Eh, not sure this header is worth anything. Sometimes I'm lazy and
stop when I see first problem.

>  	switch (action) {
>  	case PM_HIBERNATION_PREPARE:
>  	case PM_SUSPEND_PREPARE:
>  		usermodehelper_disabled = 1;
> -		return NOTIFY_OK;
> +		retval = wait_event_timeout(running_helpers_waitq,
> +					atomic_read(&running_helpers) == 0,

Are you sure here? What happens when atomic variable changes between
the atomic_read and the function call?
						Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [RC][PATCH -mm] PM: Prevent frozen user mode helpers from failing the freezing of tasks

[Rafael J. Wysocki]

On Friday, 22 June 2007 23:07, Pavel Machek wrote:
> Hi!
> 
> > From: Rafael J. Wysocki <rjw@sisk.pl>
> > 
> > At present, if a user mode helper is running while usermodehelper_pm_callback()
> > is executed, the helper may be frozen and the completion in
> > call_usermodehelper_exec() won't be completed until user space processes are
> > thawed.  As a result, the freezing of kernel threads may fail, which is not
> > desirable.
> > 
> > Prevent this from happening by introducing a counter of running user mode
> > helpers and allowing usermodehelper_pm_callback() to succeed for
> > action = PM_HIBERNATION_PREPARE or action = PM_SUSPEND_PREPARE only if there
> > are no helpers running.  [Namely, usermodehelper_pm_callback() waits for at most
> > RUNNING_HELPERS_TIMEOUT for the number of running helpers to become zero and
> > fails if that doesn't happen.]
> > 
> > Special thanks to Uli Luckas <u.luckas@road.de> for reviewing the previous
> > versions of this patch and for very useful comments.
> ...
> > Reviewed-by: Pavel Machek <pavel@ucw.cz>
> 
> Eh, not sure this header is worth anything. Sometimes I'm lazy and
> stop when I see first problem.
> 
> >  	switch (action) {
> >  	case PM_HIBERNATION_PREPARE:
> >  	case PM_SUSPEND_PREPARE:
> >  		usermodehelper_disabled = 1;
> > -		return NOTIFY_OK;
> > +		retval = wait_event_timeout(running_helpers_waitq,
> > +					atomic_read(&running_helpers) == 0,
> 
> Are you sure here? What happens when atomic variable changes between
> the atomic_read and the function call?

Er, this is a macro. :-)

In fact we rely only on atomic_read(&running_helpers) being still zero after
helper_finished() has woken us up, but I think that's acceptable.

IOW, if the wait_event_timeout() returns with retval different from zero, this
means that atomic_read(&running_helpers) returned zero at one point after
we'd set usermodehelper_disabled, which is enough.  OTOH, if it doesn't
return with retval different from zero, this means that either one or more
helpers are waited for to finish or they are coming and going very quickly,
which would be suspicious enough to cancel the suspend, IMHO.

Greetings,
Rafael


-- 
"Premature optimization is the root of all evil." - Donald Knuth

Re: [RC][PATCH -mm] PM: Prevent frozen user mode helpers from failing the freezing of tasks

[Pavel Machek]

Hi!

> > > Special thanks to Uli Luckas <u.luckas@road.de> for reviewing the previous
> > > versions of this patch and for very useful comments.
> > ...
> > > Reviewed-by: Pavel Machek <pavel@ucw.cz>
> > 
> > Eh, not sure this header is worth anything. Sometimes I'm lazy and
> > stop when I see first problem.
> > 
> > >  	switch (action) {
> > >  	case PM_HIBERNATION_PREPARE:
> > >  	case PM_SUSPEND_PREPARE:
> > >  		usermodehelper_disabled = 1;
> > > -		return NOTIFY_OK;
> > > +		retval = wait_event_timeout(running_helpers_waitq,
> > > +					atomic_read(&running_helpers) == 0,
> > 
> > Are you sure here? What happens when atomic variable changes between
> > the atomic_read and the function call?
> 
> Er, this is a macro. :-)
> 
> In fact we rely only on atomic_read(&running_helpers) being still zero after
> helper_finished() has woken us up, but I think that's acceptable.
> 
> IOW, if the wait_event_timeout() returns with retval different from zero, this
> means that atomic_read(&running_helpers) returned zero at one point after
> we'd set usermodehelper_disabled, which is enough.  OTOH, if it doesn't

Ok, can you write short comment explaining that? /* We have set
usermodehelper_disabled, so any new usermode helpers are not a problem
*/.

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [RC][PATCH -mm] PM: Prevent frozen user mode helpers from failing the freezing of tasks

[Rafael J. Wysocki]

Hi,

On Sunday, 24 June 2007 19:05, Pavel Machek wrote:
> Hi!
> 
> > > > Special thanks to Uli Luckas <u.luckas@road.de> for reviewing the previous
> > > > versions of this patch and for very useful comments.
> > > ...
> > > > Reviewed-by: Pavel Machek <pavel@ucw.cz>
> > > 
> > > Eh, not sure this header is worth anything. Sometimes I'm lazy and
> > > stop when I see first problem.
> > > 
> > > >  	switch (action) {
> > > >  	case PM_HIBERNATION_PREPARE:
> > > >  	case PM_SUSPEND_PREPARE:
> > > >  		usermodehelper_disabled = 1;
> > > > -		return NOTIFY_OK;
> > > > +		retval = wait_event_timeout(running_helpers_waitq,
> > > > +					atomic_read(&running_helpers) == 0,
> > > 
> > > Are you sure here? What happens when atomic variable changes between
> > > the atomic_read and the function call?
> > 
> > Er, this is a macro. :-)
> > 
> > In fact we rely only on atomic_read(&running_helpers) being still zero after
> > helper_finished() has woken us up, but I think that's acceptable.
> > 
> > IOW, if the wait_event_timeout() returns with retval different from zero, this
> > means that atomic_read(&running_helpers) returned zero at one point after
> > we'd set usermodehelper_disabled, which is enough.  OTOH, if it doesn't
> 
> Ok, can you write short comment explaining that? /* We have set
> usermodehelper_disabled, so any new usermode helpers are not a problem
> */.

OK, please have a look at the updated patch below.

Greetings,
Rafael

---
From: Rafael J. Wysocki <rjw@sisk.pl>

At present, if a user mode helper is running while usermodehelper_pm_callback()
is executed, the helper may be frozen and the completion in
call_usermodehelper_exec() won't be completed until user space processes are
thawed.  As a result, the freezing of kernel threads may fail, which is not
desirable.

Prevent this from happening by introducing a counter of running user mode
helpers and allowing usermodehelper_pm_callback() to succeed for
action = PM_HIBERNATION_PREPARE or action = PM_SUSPEND_PREPARE only if there
are no helpers running.  [Namely, usermodehelper_pm_callback() waits for at most
RUNNING_HELPERS_TIMEOUT for the number of running helpers to become zero and
fails if that doesn't happen.]

Special thanks to Uli Luckas <u.luckas@road.de> for reviewing the previous
versions of this patch and for very useful comments.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 kernel/kmod.c |   76 ++++++++++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 66 insertions(+), 10 deletions(-)

Index: linux-2.6.22-rc4-mm2/kernel/kmod.c
===================================================================
--- linux-2.6.22-rc4-mm2.orig/kernel/kmod.c
+++ linux-2.6.22-rc4-mm2/kernel/kmod.c
@@ -41,14 +41,6 @@ extern int max_threads;
 
 static struct workqueue_struct *khelper_wq;
 
-/*
- * If set, both call_usermodehelper_keys() and call_usermodehelper_pipe() exit
- * immediately returning -EBUSY.  Used for preventing user land processes from
- * being created after the user land has been frozen during a system-wide
- * hibernation or suspend operation.
- */
-static int usermodehelper_disabled;
-
 #ifdef CONFIG_KMOD
 
 /*
@@ -275,15 +267,54 @@ static void __call_usermodehelper(struct
 	}
 }
 
+#ifdef CONFIG_PM
+/*
+ * If set, call_usermodehelper_exec() will exit immediately returning -EBUSY
+ * (used for preventing user land processes from being created after the user
+ * land has been frozen during a system-wide hibernation or suspend operation).
+ */
+static int usermodehelper_disabled;
+
+/* Number of helpers running */
+static atomic_t running_helpers = ATOMIC_INIT(0);
+
+/*
+ * Wait queue head used by usermodehelper_pm_callback() to wait for all running
+ * helpers to finish.
+ */
+static DECLARE_WAIT_QUEUE_HEAD(running_helpers_waitq);
+
+/*
+ * Time to wait for running_helpers to become zero before the setting of
+ * usermodehelper_disabled in usermodehelper_pm_callback() fails
+ */
+#define RUNNING_HELPERS_TIMEOUT	(5 * HZ)
+
 static int usermodehelper_pm_callback(struct notifier_block *nfb,
 					unsigned long action,
 					void *ignored)
 {
+	long retval;
+
 	switch (action) {
 	case PM_HIBERNATION_PREPARE:
 	case PM_SUSPEND_PREPARE:
 		usermodehelper_disabled = 1;
-		return NOTIFY_OK;
+		/*
+		 * From now on call_usermodehelper_exec() won't start any new
+		 * helpers, so it is sufficient if running_helpers turns out to
+		 * be zero at one point (it may be increased later, but that
+		 * doesn't matter).
+		 */
+		retval = wait_event_timeout(running_helpers_waitq,
+					atomic_read(&running_helpers) == 0,
+					RUNNING_HELPERS_TIMEOUT);
+		if (retval) {
+			return NOTIFY_OK;
+		} else {
+			usermodehelper_disabled = 0;
+			return NOTIFY_BAD;
+		}
 	case PM_POST_HIBERNATION:
 	case PM_POST_SUSPEND:
 		usermodehelper_disabled = 0;
@@ -293,6 +324,29 @@ static int usermodehelper_pm_callback(st
 	return NOTIFY_DONE;
 }
 
+static void new_helper(void)
+{
+	atomic_inc(&running_helpers);
+}
+
+static void helper_finished(void)
+{
+	if (atomic_dec_and_test(&running_helpers))
+		wake_up(&running_helpers_waitq);
+}
+
+static void register_pm_notifier_callback(void)
+{
+	pm_notifier(usermodehelper_pm_callback, 0);
+}
+#else /* CONFIG_PM */
+#define usermodehelper_disabled	0
+
+static inline void new_helper(void) {}
+static inline void helper_finished(void) {}
+static inline void register_pm_notifier_callback(void) {}
+#endif /* CONFIG_PM */
+
 /**
  * call_usermodehelper_setup - prepare to call a usermode helper
  * @path - path to usermode executable
@@ -397,6 +451,7 @@ int call_usermodehelper_exec(struct subp
 	DECLARE_COMPLETION_ONSTACK(done);
 	int retval;
 
+	new_helper();
 	if (sub_info->path[0] == '\0') {
 		retval = 0;
 		goto out;
@@ -418,6 +473,7 @@ int call_usermodehelper_exec(struct subp
 
   out:
 	call_usermodehelper_freeinfo(sub_info);
+	helper_finished();
 	return retval;
 }
 EXPORT_SYMBOL(call_usermodehelper_exec);
@@ -459,5 +515,5 @@ void __init usermodehelper_init(void)
 {
 	khelper_wq = create_singlethread_workqueue("khelper");
 	BUG_ON(!khelper_wq);
-	pm_notifier(usermodehelper_pm_callback, 0);
+	register_pm_notifier_callback();
 }

Re: [RC][PATCH -mm] PM: Prevent frozen user mode helpers from failing the freezing of tasks

[Pavel Machek]

Hi!

> > > > > Special thanks to Uli Luckas <u.luckas@road.de> for reviewing the previous
> > > > > versions of this patch and for very useful comments.
> > > > ...
> > > > > Reviewed-by: Pavel Machek <pavel@ucw.cz>
> > > > 
> > > > Eh, not sure this header is worth anything. Sometimes I'm lazy and
> > > > stop when I see first problem.
> > > > 
> > > > >  	switch (action) {
> > > > >  	case PM_HIBERNATION_PREPARE:
> > > > >  	case PM_SUSPEND_PREPARE:
> > > > >  		usermodehelper_disabled = 1;
> > > > > -		return NOTIFY_OK;
> > > > > +		retval = wait_event_timeout(running_helpers_waitq,
> > > > > +					atomic_read(&running_helpers) == 0,
> > > > 
> > > > Are you sure here? What happens when atomic variable changes between
> > > > the atomic_read and the function call?
> > > 
> > > Er, this is a macro. :-)
> > > 
> > > In fact we rely only on atomic_read(&running_helpers) being still zero after
> > > helper_finished() has woken us up, but I think that's acceptable.
> > > 
> > > IOW, if the wait_event_timeout() returns with retval different from zero, this
> > > means that atomic_read(&running_helpers) returned zero at one point after
> > > we'd set usermodehelper_disabled, which is enough.  OTOH, if it doesn't
> > 
> > Ok, can you write short comment explaining that? /* We have set
> > usermodehelper_disabled, so any new usermode helpers are not a problem
> > */.
> 
> OK, please have a look at the updated patch below.

ACK, and thanks for your patience.
									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html