Bug#689096: ipw2200 crashes on wakeup
Stefan Ott wrote:
> Do you have a particular commit in mind that might be responsible?
Here's a series[1] that's more likely to work as a full fix than the
single patch I sent (though I'm interested in both). Applies to a
3.2.y-based tree with "git am -3sc $(ls -1 /path/to/patches/0*)".
[1] http://thread.gmane.org/gmane.linux.kernel/1273980
From: "Srivatsa S. Bhat" <srivatsa.bhat@linux.vnet.ibm.com>
Date: Fri, 9 Dec 2011 23:36:36 +0100
Subject: PM / Sleep: Fix freezer failures due to racy usermodehelper_is_disabled()
commit b298d289c79211508f11cb50749b0d1d54eb244a upstream.
Commit a144c6a (PM: Print a warning if firmware is requested when tasks
are frozen) introduced usermodehelper_is_disabled() to warn and exit
immediately if firmware is requested when usermodehelpers are disabled.
However, it is racy. Consider the following scenario, currently used in
drivers/base/firmware_class.c:
...
if (usermodehelper_is_disabled())
goto out;
/* Do actual work */
...
out:
return err;
Nothing prevents someone from disabling usermodehelpers just after the check
in the 'if' condition, which means that it is quite possible to try doing the
"actual work" with usermodehelpers disabled, leading to undesirable
consequences.
In particular, this race condition in _request_firmware() causes task freezing
failures whenever suspend/hibernation is in progress because, it wrongly waits
to get the firmware/microcode image from userspace when actually the
usermodehelpers are disabled or userspace has been frozen.
Some of the example scenarios that cause freezing failures due to this race
are those that depend on userspace via request_firmware(), such as x86
microcode module initialization and microcode image reload.
Previous discussions about this issue can be found at:
http://thread.gmane.org/gmane.linux.kernel/1198291/focus=1200591
This patch adds proper synchronization to fix this issue.
It is to be noted that this patchset fixes the freezing failures but doesn't
remove the warnings. IOW, it does not attempt to add explicit synchronization
to x86 microcode driver to avoid requesting microcode image at inopportune
moments. Because, the warnings were introduced to highlight such cases, in the
first place. And we need not silence the warnings, since we take care of the
*real* problem (freezing failure) and hence, after that, the warnings are
pretty harmless anyway.
Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
drivers/base/firmware_class.c | 4 ++++
include/linux/kmod.h | 2 ++
kernel/kmod.c | 23 ++++++++++++++++++++++-
3 files changed, 28 insertions(+), 1 deletion(-)
diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 3719c94be19c..26ab358dac62 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -534,6 +534,8 @@ static int _request_firmware(const struct firmware **firmware_p,
return 0;
}
+ read_lock_usermodehelper();
+
if (WARN_ON(usermodehelper_is_disabled())) {
dev_err(device, "firmware: %s will not be loaded\n", name);
retval = -EBUSY;
@@ -572,6 +574,8 @@ static int _request_firmware(const struct firmware **firmware_p,
fw_destroy_instance(fw_priv);
out:
+ read_unlock_usermodehelper();
+
if (retval) {
release_firmware(firmware);
*firmware_p = NULL;
diff --git a/include/linux/kmod.h b/include/linux/kmod.h
index b16f65390734..722f477c4ef7 100644
--- a/include/linux/kmod.h
+++ b/include/linux/kmod.h
@@ -117,5 +117,7 @@ extern void usermodehelper_init(void);
extern int usermodehelper_disable(void);
extern void usermodehelper_enable(void);
extern bool usermodehelper_is_disabled(void);
+extern void read_lock_usermodehelper(void);
+extern void read_unlock_usermodehelper(void);
#endif /* __LINUX_KMOD_H__ */
diff --git a/kernel/kmod.c b/kernel/kmod.c
index a4bea97c75b6..81b4a27261b2 100644
--- a/kernel/kmod.c
+++ b/kernel/kmod.c
@@ -36,6 +36,7 @@
#include <linux/resource.h>
#include <linux/notifier.h>
#include <linux/suspend.h>
+#include <linux/rwsem.h>
#include <asm/uaccess.h>
#include <trace/events/module.h>
@@ -50,6 +51,7 @@ static struct workqueue_struct *khelper_wq;
static kernel_cap_t usermodehelper_bset = CAP_FULL_SET;
static kernel_cap_t usermodehelper_inheritable = CAP_FULL_SET;
static DEFINE_SPINLOCK(umh_sysctl_lock);
+static DECLARE_RWSEM(umhelper_sem);
#ifdef CONFIG_MODULES
@@ -275,6 +277,7 @@ static void __call_usermodehelper(struct work_struct *work)
* 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).
+ * Should always be manipulated under umhelper_sem acquired for write.
*/
static int usermodehelper_disabled = 1;
@@ -293,6 +296,18 @@ static DECLARE_WAIT_QUEUE_HEAD(running_helpers_waitq);
*/
#define RUNNING_HELPERS_TIMEOUT (5 * HZ)
+void read_lock_usermodehelper(void)
+{
+ down_read(&umhelper_sem);
+}
+EXPORT_SYMBOL_GPL(read_lock_usermodehelper);
+
+void read_unlock_usermodehelper(void)
+{
+ up_read(&umhelper_sem);
+}
+EXPORT_SYMBOL_GPL(read_unlock_usermodehelper);
+
/**
* usermodehelper_disable - prevent new helpers from being started
*/
@@ -300,8 +315,10 @@ int usermodehelper_disable(void)
{
long retval;
+ down_write(&umhelper_sem);
usermodehelper_disabled = 1;
- smp_mb();
+ up_write(&umhelper_sem);
+
/*
* From now on call_usermodehelper_exec() won't start any new
* helpers, so it is sufficient if running_helpers turns out to
@@ -314,7 +331,9 @@ int usermodehelper_disable(void)
if (retval)
return 0;
+ down_write(&umhelper_sem);
usermodehelper_disabled = 0;
+ up_write(&umhelper_sem);
return -EAGAIN;
}
@@ -323,7 +342,9 @@ int usermodehelper_disable(void)
*/
void usermodehelper_enable(void)
{
+ down_write(&umhelper_sem);
usermodehelper_disabled = 0;
+ up_write(&umhelper_sem);
}
/**
--
1.7.10.4
From: "Rafael J. Wysocki" <rjw@sisk.pl>
Date: Wed, 28 Mar 2012 23:29:45 +0200
Subject: firmware_class: Rework usermodehelper check
commit fe2e39d8782d885755139304d8dba0b3e5bfa878 upstream.
Instead of two functions, read_lock_usermodehelper() and
usermodehelper_is_disabled(), used in combination, introduce
usermodehelper_read_trylock() that will only return with umhelper_sem
held if usermodehelper_disabled is unset (and will return -EAGAIN
otherwise) and make _request_firmware() use it.
Rename read_unlock_usermodehelper() to
usermodehelper_read_unlock() to follow the naming convention of the
new function.
Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
drivers/base/firmware_class.c | 11 +++++------
include/linux/kmod.h | 5 ++---
kernel/kmod.c | 24 +++++++++++-------------
3 files changed, 18 insertions(+), 22 deletions(-)
diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 26ab358dac62..653def0648c5 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -534,12 +534,10 @@ static int _request_firmware(const struct firmware **firmware_p,
return 0;
}
- read_lock_usermodehelper();
-
- if (WARN_ON(usermodehelper_is_disabled())) {
+ retval = usermodehelper_read_trylock();
+ if (WARN_ON(retval)) {
dev_err(device, "firmware: %s will not be loaded\n", name);
- retval = -EBUSY;
- goto out;
+ goto out_nolock;
}
if (uevent)
@@ -574,8 +572,9 @@ static int _request_firmware(const struct firmware **firmware_p,
fw_destroy_instance(fw_priv);
out:
- read_unlock_usermodehelper();
+ usermodehelper_read_unlock();
+out_nolock:
if (retval) {
release_firmware(firmware);
*firmware_p = NULL;
diff --git a/include/linux/kmod.h b/include/linux/kmod.h
index 722f477c4ef7..b25e2b80c059 100644
--- a/include/linux/kmod.h
+++ b/include/linux/kmod.h
@@ -116,8 +116,7 @@ extern void usermodehelper_init(void);
extern int usermodehelper_disable(void);
extern void usermodehelper_enable(void);
-extern bool usermodehelper_is_disabled(void);
-extern void read_lock_usermodehelper(void);
-extern void read_unlock_usermodehelper(void);
+extern int usermodehelper_read_trylock(void);
+extern void usermodehelper_read_unlock(void);
#endif /* __LINUX_KMOD_H__ */
diff --git a/kernel/kmod.c b/kernel/kmod.c
index 81b4a27261b2..350ca30787c0 100644
--- a/kernel/kmod.c
+++ b/kernel/kmod.c
@@ -296,17 +296,24 @@ static DECLARE_WAIT_QUEUE_HEAD(running_helpers_waitq);
*/
#define RUNNING_HELPERS_TIMEOUT (5 * HZ)
-void read_lock_usermodehelper(void)
+int usermodehelper_read_trylock(void)
{
+ int ret = 0;
+
down_read(&umhelper_sem);
+ if (usermodehelper_disabled) {
+ up_read(&umhelper_sem);
+ ret = -EAGAIN;
+ }
+ return ret;
}
-EXPORT_SYMBOL_GPL(read_lock_usermodehelper);
+EXPORT_SYMBOL_GPL(usermodehelper_read_trylock);
-void read_unlock_usermodehelper(void)
+void usermodehelper_read_unlock(void)
{
up_read(&umhelper_sem);
}
-EXPORT_SYMBOL_GPL(read_unlock_usermodehelper);
+EXPORT_SYMBOL_GPL(usermodehelper_read_unlock);
/**
* usermodehelper_disable - prevent new helpers from being started
@@ -347,15 +354,6 @@ void usermodehelper_enable(void)
up_write(&umhelper_sem);
}
-/**
- * usermodehelper_is_disabled - check if new helpers are allowed to be started
- */
-bool usermodehelper_is_disabled(void)
-{
- return usermodehelper_disabled;
-}
-EXPORT_SYMBOL_GPL(usermodehelper_is_disabled);
-
static void helper_lock(void)
{
atomic_inc(&running_helpers);
--
1.7.10.4
From: "Rafael J. Wysocki" <rjw@sisk.pl>
Date: Wed, 28 Mar 2012 23:29:55 +0200
Subject: firmware_class: Split _request_firmware() into three functions, v2
commit 811fa4004485dec8977176bf605a5b0508ee206c upstream.
Split _request_firmware() into three functions,
_request_firmware_prepare() doing preparatory work that need not be
done under umhelper_sem, _request_firmware_cleanup() doing the
post-error cleanup and _request_firmware() carrying out the remaining
operations.
This change is requisite for moving the acquisition of umhelper_sem
from _request_firmware() to the callers, which is going to be done
subsequently.
Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
drivers/base/firmware_class.c | 58 +++++++++++++++++++++++++++++------------
1 file changed, 41 insertions(+), 17 deletions(-)
diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 653def0648c5..efc61501f025 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -435,7 +435,7 @@ static void firmware_class_timeout(u_long data)
}
static struct firmware_priv *
-fw_create_instance(struct firmware *firmware, const char *fw_name,
+fw_create_instance(const struct firmware *firmware, const char *fw_name,
struct device *device, bool uevent, bool nowait)
{
struct firmware_priv *fw_priv;
@@ -449,7 +449,7 @@ fw_create_instance(struct firmware *firmware, const char *fw_name,
goto err_out;
}
- fw_priv->fw = firmware;
+ fw_priv->fw = (struct firmware *)firmware;
fw_priv->nowait = nowait;
strcpy(fw_priv->fw_id, fw_name);
init_completion(&fw_priv->completion);
@@ -510,13 +510,10 @@ static void fw_destroy_instance(struct firmware_priv *fw_priv)
device_unregister(f_dev);
}
-static int _request_firmware(const struct firmware **firmware_p,
- const char *name, struct device *device,
- bool uevent, bool nowait)
+static int _request_firmware_prepare(const struct firmware **firmware_p,
+ const char *name, struct device *device)
{
- struct firmware_priv *fw_priv;
struct firmware *firmware;
- int retval = 0;
if (!firmware_p)
return -EINVAL;
@@ -534,10 +531,26 @@ static int _request_firmware(const struct firmware **firmware_p,
return 0;
}
+ return 1;
+}
+
+static void _request_firmware_cleanup(const struct firmware **firmware_p)
+{
+ release_firmware(*firmware_p);
+ *firmware_p = NULL;
+}
+
+static int _request_firmware(const struct firmware *firmware,
+ const char *name, struct device *device,
+ bool uevent, bool nowait)
+{
+ struct firmware_priv *fw_priv;
+ int retval;
+
retval = usermodehelper_read_trylock();
if (WARN_ON(retval)) {
dev_err(device, "firmware: %s will not be loaded\n", name);
- goto out_nolock;
+ return retval;
}
if (uevent)
@@ -573,13 +586,6 @@ static int _request_firmware(const struct firmware **firmware_p,
out:
usermodehelper_read_unlock();
-
-out_nolock:
- if (retval) {
- release_firmware(firmware);
- *firmware_p = NULL;
- }
-
return retval;
}
@@ -602,7 +608,17 @@ int
request_firmware(const struct firmware **firmware_p, const char *name,
struct device *device)
{
- return _request_firmware(firmware_p, name, device, true, false);
+ int ret;
+
+ ret = _request_firmware_prepare(firmware_p, name, device);
+ if (ret <= 0)
+ return ret;
+
+ ret = _request_firmware(*firmware_p, name, device, true, false);
+ if (ret)
+ _request_firmware_cleanup(firmware_p);
+
+ return ret;
}
/**
@@ -640,8 +656,16 @@ static int request_firmware_work_func(void *arg)
return 0;
}
- ret = _request_firmware(&fw, fw_work->name, fw_work->device,
+ ret = _request_firmware_prepare(&fw, fw_work->name, fw_work->device);
+ if (ret <= 0)
+ goto out;
+
+ ret = _request_firmware(fw, fw_work->name, fw_work->device,
fw_work->uevent, true);
+ if (ret)
+ _request_firmware_cleanup(&fw);
+
+ out:
fw_work->cont(fw, fw_work->context);
module_put(fw_work->module);
--
1.7.10.4
From: "Rafael J. Wysocki" <rjw@sisk.pl>
Date: Wed, 28 Mar 2012 23:30:02 +0200
Subject: firmware_class: Do not warn that system is not ready from async loads
commit 9b78c1da60b3c62ccdd1509f0902ad19ceaf776b upstream.
If firmware is requested asynchronously, by calling
request_firmware_nowait(), there is no reason to fail the request
(and warn the user) when the system is (presumably temporarily)
unready to handle it (because user space is not available yet or
frozen). For this reason, introduce an alternative routine for
read-locking umhelper_sem, usermodehelper_read_lock_wait(), that
will wait for usermodehelper_disabled to be unset (possibly with
a timeout) and make request_firmware_work_func() use it instead of
usermodehelper_read_trylock().
Accordingly, modify request_firmware() so that it uses
usermodehelper_read_trylock() to acquire umhelper_sem and remove
the code related to that lock from _request_firmware().
Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
drivers/base/firmware_class.c | 51 +++++++++++++++++++++---------------
include/linux/kmod.h | 1 +
kernel/kmod.c | 58 ++++++++++++++++++++++++++++++++---------
3 files changed, 76 insertions(+), 34 deletions(-)
diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index efc61501f025..7904310e5528 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -81,6 +81,11 @@ enum {
static int loading_timeout = 60; /* In seconds */
+static inline long firmware_loading_timeout(void)
+{
+ return loading_timeout > 0 ? loading_timeout * HZ : MAX_SCHEDULE_TIMEOUT;
+}
+
/* fw_lock could be moved to 'struct firmware_priv' but since it is just
* guarding for corner cases a global lock should be OK */
static DEFINE_MUTEX(fw_lock);
@@ -542,31 +547,22 @@ static void _request_firmware_cleanup(const struct firmware **firmware_p)
static int _request_firmware(const struct firmware *firmware,
const char *name, struct device *device,
- bool uevent, bool nowait)
+ bool uevent, bool nowait, long timeout)
{
struct firmware_priv *fw_priv;
- int retval;
-
- retval = usermodehelper_read_trylock();
- if (WARN_ON(retval)) {
- dev_err(device, "firmware: %s will not be loaded\n", name);
- return retval;
- }
+ int retval = 0;
if (uevent)
dev_dbg(device, "firmware: requesting %s\n", name);
fw_priv = fw_create_instance(firmware, name, device, uevent, nowait);
- if (IS_ERR(fw_priv)) {
- retval = PTR_ERR(fw_priv);
- goto out;
- }
+ if (IS_ERR(fw_priv))
+ return PTR_ERR(fw_priv);
if (uevent) {
- if (loading_timeout > 0)
+ if (timeout != MAX_SCHEDULE_TIMEOUT)
mod_timer(&fw_priv->timeout,
- round_jiffies_up(jiffies +
- loading_timeout * HZ));
+ round_jiffies_up(jiffies + timeout));
kobject_uevent(&fw_priv->dev.kobj, KOBJ_ADD);
}
@@ -583,9 +579,6 @@ static int _request_firmware(const struct firmware *firmware,
mutex_unlock(&fw_lock);
fw_destroy_instance(fw_priv);
-
-out:
- usermodehelper_read_unlock();
return retval;
}
@@ -614,7 +607,14 @@ request_firmware(const struct firmware **firmware_p, const char *name,
if (ret <= 0)
return ret;
- ret = _request_firmware(*firmware_p, name, device, true, false);
+ ret = usermodehelper_read_trylock();
+ if (WARN_ON(ret)) {
+ dev_err(device, "firmware: %s will not be loaded\n", name);
+ } else {
+ ret = _request_firmware(*firmware_p, name, device, true, false,
+ firmware_loading_timeout());
+ usermodehelper_read_unlock();
+ }
if (ret)
_request_firmware_cleanup(firmware_p);
@@ -649,6 +649,7 @@ static int request_firmware_work_func(void *arg)
{
struct firmware_work *fw_work = arg;
const struct firmware *fw;
+ long timeout;
int ret;
if (!arg) {
@@ -660,8 +661,16 @@ static int request_firmware_work_func(void *arg)
if (ret <= 0)
goto out;
- ret = _request_firmware(fw, fw_work->name, fw_work->device,
- fw_work->uevent, true);
+ timeout = usermodehelper_read_lock_wait(firmware_loading_timeout());
+ if (timeout) {
+ ret = _request_firmware(fw, fw_work->name, fw_work->device,
+ fw_work->uevent, true, timeout);
+ usermodehelper_read_unlock();
+ } else {
+ dev_dbg(fw_work->device, "firmware: %s loading timed out\n",
+ fw_work->name);
+ ret = -EAGAIN;
+ }
if (ret)
_request_firmware_cleanup(&fw);
diff --git a/include/linux/kmod.h b/include/linux/kmod.h
index b25e2b80c059..ce9d9a13d777 100644
--- a/include/linux/kmod.h
+++ b/include/linux/kmod.h
@@ -117,6 +117,7 @@ extern void usermodehelper_init(void);
extern int usermodehelper_disable(void);
extern void usermodehelper_enable(void);
extern int usermodehelper_read_trylock(void);
+extern long usermodehelper_read_lock_wait(long timeout);
extern void usermodehelper_read_unlock(void);
#endif /* __LINUX_KMOD_H__ */
diff --git a/kernel/kmod.c b/kernel/kmod.c
index 350ca30787c0..d5648b7efcb2 100644
--- a/kernel/kmod.c
+++ b/kernel/kmod.c
@@ -291,6 +291,12 @@ static atomic_t running_helpers = ATOMIC_INIT(0);
static DECLARE_WAIT_QUEUE_HEAD(running_helpers_waitq);
/*
+ * Used by usermodehelper_read_lock_wait() to wait for usermodehelper_disabled
+ * to become 'false'.
+ */
+static DECLARE_WAIT_QUEUE_HEAD(usermodehelper_disabled_waitq);
+
+/*
* Time to wait for running_helpers to become zero before the setting of
* usermodehelper_disabled in usermodehelper_pm_callback() fails
*/
@@ -309,6 +315,33 @@ int usermodehelper_read_trylock(void)
}
EXPORT_SYMBOL_GPL(usermodehelper_read_trylock);
+long usermodehelper_read_lock_wait(long timeout)
+{
+ DEFINE_WAIT(wait);
+
+ if (timeout < 0)
+ return -EINVAL;
+
+ down_read(&umhelper_sem);
+ for (;;) {
+ prepare_to_wait(&usermodehelper_disabled_waitq, &wait,
+ TASK_UNINTERRUPTIBLE);
+ if (!usermodehelper_disabled)
+ break;
+
+ up_read(&umhelper_sem);
+
+ timeout = schedule_timeout(timeout);
+ if (!timeout)
+ break;
+
+ down_read(&umhelper_sem);
+ }
+ finish_wait(&usermodehelper_disabled_waitq, &wait);
+ return timeout;
+}
+EXPORT_SYMBOL_GPL(usermodehelper_read_lock_wait);
+
void usermodehelper_read_unlock(void)
{
up_read(&umhelper_sem);
@@ -316,6 +349,17 @@ void usermodehelper_read_unlock(void)
EXPORT_SYMBOL_GPL(usermodehelper_read_unlock);
/**
+ * usermodehelper_enable - allow new helpers to be started again
+ */
+void usermodehelper_enable(void)
+{
+ down_write(&umhelper_sem);
+ usermodehelper_disabled = 0;
+ wake_up(&usermodehelper_disabled_waitq);
+ up_write(&umhelper_sem);
+}
+
+/**
* usermodehelper_disable - prevent new helpers from being started
*/
int usermodehelper_disable(void)
@@ -338,22 +382,10 @@ int usermodehelper_disable(void)
if (retval)
return 0;
- down_write(&umhelper_sem);
- usermodehelper_disabled = 0;
- up_write(&umhelper_sem);
+ usermodehelper_enable();
return -EAGAIN;
}
-/**
- * usermodehelper_enable - allow new helpers to be started again
- */
-void usermodehelper_enable(void)
-{
- down_write(&umhelper_sem);
- usermodehelper_disabled = 0;
- up_write(&umhelper_sem);
-}
-
static void helper_lock(void)
{
atomic_inc(&running_helpers);
--
1.7.10.4
From: "Rafael J. Wysocki" <rjw@sisk.pl>
Date: Wed, 28 Mar 2012 23:30:14 +0200
Subject: PM / Hibernate: Disable usermode helpers right before freezing tasks
commit 7b5179ac14dbad945647ac9e76bbbf14ed9e0dbe upstream.
There is no reason to call usermodehelper_disable() before creating
memory bitmaps in hibernate() and software_resume(), so call it right
before freeze_processes(), in accordance with the other suspend and
hibernation code. Consequently, call usermodehelper_enable() right
after the thawing of tasks rather than after freeing the memory
bitmaps.
Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
kernel/power/hibernate.c | 19 +++++++++----------
1 file changed, 9 insertions(+), 10 deletions(-)
diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
index 013bd2ef53e2..e16b013ec540 100644
--- a/kernel/power/hibernate.c
+++ b/kernel/power/hibernate.c
@@ -647,19 +647,19 @@ int hibernate(void)
if (error)
goto Exit;
- error = usermodehelper_disable();
- if (error)
- goto Exit;
-
/* Allocate memory management structures */
error = create_basic_memory_bitmaps();
if (error)
- goto Enable_umh;
+ goto Exit;
printk(KERN_INFO "PM: Syncing filesystems ... ");
sys_sync();
printk("done.\n");
+ error = usermodehelper_disable();
+ if (error)
+ goto Exit;
+
error = prepare_processes();
if (error)
goto Free_bitmaps;
@@ -696,9 +696,8 @@ int hibernate(void)
Thaw:
thaw_processes();
Free_bitmaps:
- free_basic_memory_bitmaps();
- Enable_umh:
usermodehelper_enable();
+ free_basic_memory_bitmaps();
Exit:
pm_notifier_call_chain(PM_POST_HIBERNATION);
pm_restore_console();
@@ -813,11 +812,11 @@ static int software_resume(void)
if (error)
goto close_finish;
- error = usermodehelper_disable();
+ error = create_basic_memory_bitmaps();
if (error)
goto close_finish;
- error = create_basic_memory_bitmaps();
+ error = usermodehelper_disable();
if (error)
goto close_finish;
@@ -839,8 +838,8 @@ static int software_resume(void)
swsusp_free();
thaw_processes();
Done:
- free_basic_memory_bitmaps();
usermodehelper_enable();
+ free_basic_memory_bitmaps();
Finish:
pm_notifier_call_chain(PM_POST_RESTORE);
pm_restore_console();
--
1.7.10.4
From: "Rafael J. Wysocki" <rjw@sisk.pl>
Date: Wed, 28 Mar 2012 23:30:21 +0200
Subject: PM / Sleep: Move disabling of usermode helpers to the freezer
commit 1e73203cd1157a03facc41ffb54050f5b28e55bd upstream.
The core suspend/hibernation code calls usermodehelper_disable() to
avoid race conditions between the freezer and the starting of
usermode helpers and each code path has to do that on its own.
However, it is always called right before freeze_processes()
and usermodehelper_enable() is always called right after
thaw_processes(). For this reason, to avoid code duplication and
to make the connection between usermodehelper_disable() and the
freezer more visible, make freeze_processes() call it and remove the
direct usermodehelper_disable() and usermodehelper_enable() calls
from all suspend/hibernation code paths.
Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
kernel/power/hibernate.c | 11 -----------
kernel/power/process.c | 8 ++++++++
kernel/power/suspend.c | 7 -------
kernel/power/user.c | 10 +---------
4 files changed, 9 insertions(+), 27 deletions(-)
diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
index e16b013ec540..0eb552829280 100644
--- a/kernel/power/hibernate.c
+++ b/kernel/power/hibernate.c
@@ -16,7 +16,6 @@
#include <linux/string.h>
#include <linux/device.h>
#include <linux/async.h>
-#include <linux/kmod.h>
#include <linux/delay.h>
#include <linux/fs.h>
#include <linux/mount.h>
@@ -656,10 +655,6 @@ int hibernate(void)
sys_sync();
printk("done.\n");
- error = usermodehelper_disable();
- if (error)
- goto Exit;
-
error = prepare_processes();
if (error)
goto Free_bitmaps;
@@ -696,7 +691,6 @@ int hibernate(void)
Thaw:
thaw_processes();
Free_bitmaps:
- usermodehelper_enable();
free_basic_memory_bitmaps();
Exit:
pm_notifier_call_chain(PM_POST_HIBERNATION);
@@ -816,10 +810,6 @@ static int software_resume(void)
if (error)
goto close_finish;
- error = usermodehelper_disable();
- if (error)
- goto close_finish;
-
pr_debug("PM: Preparing processes for restore.\n");
error = prepare_processes();
if (error) {
@@ -838,7 +828,6 @@ static int software_resume(void)
swsusp_free();
thaw_processes();
Done:
- usermodehelper_enable();
free_basic_memory_bitmaps();
Finish:
pm_notifier_call_chain(PM_POST_RESTORE);
diff --git a/kernel/power/process.c b/kernel/power/process.c
index 3d4b9544eeba..e8675aeb2e32 100644
--- a/kernel/power/process.c
+++ b/kernel/power/process.c
@@ -16,6 +16,7 @@
#include <linux/freezer.h>
#include <linux/delay.h>
#include <linux/workqueue.h>
+#include <linux/kmod.h>
/*
* Timeout for stopping processes
@@ -141,6 +142,10 @@ int freeze_processes(void)
{
int error;
+ error = usermodehelper_disable();
+ if (error)
+ return error;
+
printk("Freezing user space processes ... ");
error = try_to_freeze_tasks(true);
if (!error) {
@@ -199,6 +204,9 @@ void thaw_processes(void)
thaw_workqueues();
thaw_tasks(true);
thaw_tasks(false);
+
+ usermodehelper_enable();
+
schedule();
printk("done.\n");
}
diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
index af48faa3537f..a69117291abd 100644
--- a/kernel/power/suspend.c
+++ b/kernel/power/suspend.c
@@ -12,7 +12,6 @@
#include <linux/delay.h>
#include <linux/errno.h>
#include <linux/init.h>
-#include <linux/kmod.h>
#include <linux/console.h>
#include <linux/cpu.h>
#include <linux/syscalls.h>
@@ -102,10 +101,6 @@ static int suspend_prepare(void)
if (error)
goto Finish;
- error = usermodehelper_disable();
- if (error)
- goto Finish;
-
error = suspend_freeze_processes();
if (error) {
suspend_stats.failed_freeze++;
@@ -114,7 +109,6 @@ static int suspend_prepare(void)
return 0;
suspend_thaw_processes();
- usermodehelper_enable();
Finish:
pm_notifier_call_chain(PM_POST_SUSPEND);
pm_restore_console();
@@ -264,7 +258,6 @@ int suspend_devices_and_enter(suspend_state_t state)
static void suspend_finish(void)
{
suspend_thaw_processes();
- usermodehelper_enable();
pm_notifier_call_chain(PM_POST_SUSPEND);
pm_restore_console();
}
diff --git a/kernel/power/user.c b/kernel/power/user.c
index f08d227faaf9..e9b817afe102 100644
--- a/kernel/power/user.c
+++ b/kernel/power/user.c
@@ -12,7 +12,6 @@
#include <linux/suspend.h>
#include <linux/syscalls.h>
#include <linux/reboot.h>
-#include <linux/kmod.h>
#include <linux/string.h>
#include <linux/device.h>
#include <linux/miscdevice.h>
@@ -252,15 +251,9 @@ static long snapshot_ioctl(struct file *filp, unsigned int cmd,
sys_sync();
printk("done.\n");
- error = usermodehelper_disable();
- if (error)
- break;
-
error = freeze_processes();
- if (error) {
+ if (error)
thaw_processes();
- usermodehelper_enable();
- }
if (!error)
data->frozen = 1;
break;
@@ -270,7 +263,6 @@ static long snapshot_ioctl(struct file *filp, unsigned int cmd,
break;
pm_restore_gfp_mask();
thaw_processes();
- usermodehelper_enable();
data->frozen = 0;
break;
--
1.7.10.4
From: "Rafael J. Wysocki" <rjw@sisk.pl>
Date: Wed, 28 Mar 2012 23:30:28 +0200
Subject: PM / Sleep: Mitigate race between the freezer and request_firmware()
commit 247bc03742545fec2f79939a3b9f738392a0f7b4 upstream.
There is a race condition between the freezer and request_firmware()
such that if request_firmware() is run on one CPU and
freeze_processes() is run on another CPU and usermodehelper_disable()
called by it succeeds to grab umhelper_sem for writing before
usermodehelper_read_trylock() called from request_firmware()
acquires it for reading, the request_firmware() will fail and
trigger a WARN_ON() complaining that it was called at a wrong time.
However, in fact, it wasn't called at a wrong time and
freeze_processes() simply happened to be executed simultaneously.
To avoid this race, at least in some cases, modify
usermodehelper_read_trylock() so that it doesn't fail if the
freezing of tasks has just started and hasn't been completed yet.
Instead, during the freezing of tasks, it will try to freeze the
task that has called it so that it can wait until user space is
thawed without triggering the scary warning.
For this purpose, change usermodehelper_disabled so that it can
take three different values, UMH_ENABLED (0), UMH_FREEZING and
UMH_DISABLED. The first one means that usermode helpers are
enabled, the last one means "hard disable" (i.e. the system is not
ready for usermode helpers to be used) and the second one
is reserved for the freezer. Namely, when freeze_processes() is
started, it sets usermodehelper_disabled to UMH_FREEZING which
tells usermodehelper_read_trylock() that it shouldn't fail just
yet and should call try_to_freeze() if woken up and cannot
return immediately. This way all freezable tasks that happen
to call request_firmware() right before freeze_processes() is
started and lose the race for umhelper_sem with it will be
frozen and will sleep until thaw_processes() unsets
usermodehelper_disabled. [For the non-freezable callers of
request_firmware() the race for umhelper_sem against
freeze_processes() is unfortunately unavoidable.]
Reported-by: Stephen Boyd <sboyd@codeaurora.org>
Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
[jn: backport to 3.2.y: include linux/freezer.h]
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
include/linux/kmod.h | 21 +++++++++++++++++++--
kernel/kmod.c | 48 ++++++++++++++++++++++++++++++++++++++----------
kernel/power/process.c | 3 ++-
3 files changed, 59 insertions(+), 13 deletions(-)
diff --git a/include/linux/kmod.h b/include/linux/kmod.h
index ce9d9a13d777..0fb48ef0cf44 100644
--- a/include/linux/kmod.h
+++ b/include/linux/kmod.h
@@ -112,10 +112,27 @@ call_usermodehelper(char *path, char **argv, char **envp, enum umh_wait wait)
extern struct ctl_table usermodehelper_table[];
+enum umh_disable_depth {
+ UMH_ENABLED = 0,
+ UMH_FREEZING,
+ UMH_DISABLED,
+};
+
extern void usermodehelper_init(void);
-extern int usermodehelper_disable(void);
-extern void usermodehelper_enable(void);
+extern int __usermodehelper_disable(enum umh_disable_depth depth);
+extern void __usermodehelper_set_disable_depth(enum umh_disable_depth depth);
+
+static inline int usermodehelper_disable(void)
+{
+ return __usermodehelper_disable(UMH_DISABLED);
+}
+
+static inline void usermodehelper_enable(void)
+{
+ __usermodehelper_set_disable_depth(UMH_ENABLED);
+}
+
extern int usermodehelper_read_trylock(void);
extern long usermodehelper_read_lock_wait(long timeout);
extern void usermodehelper_read_unlock(void);
diff --git a/kernel/kmod.c b/kernel/kmod.c
index d5648b7efcb2..a62a52115ecb 100644
--- a/kernel/kmod.c
+++ b/kernel/kmod.c
@@ -35,6 +35,7 @@
#include <linux/init.h>
#include <linux/resource.h>
#include <linux/notifier.h>
+#include <linux/freezer.h>
#include <linux/suspend.h>
#include <linux/rwsem.h>
#include <asm/uaccess.h>
@@ -279,7 +280,7 @@ static void __call_usermodehelper(struct work_struct *work)
* land has been frozen during a system-wide hibernation or suspend operation).
* Should always be manipulated under umhelper_sem acquired for write.
*/
-static int usermodehelper_disabled = 1;
+static enum umh_disable_depth usermodehelper_disabled = UMH_DISABLED;
/* Number of helpers running */
static atomic_t running_helpers = ATOMIC_INIT(0);
@@ -304,13 +305,30 @@ static DECLARE_WAIT_QUEUE_HEAD(usermodehelper_disabled_waitq);
int usermodehelper_read_trylock(void)
{
+ DEFINE_WAIT(wait);
int ret = 0;
down_read(&umhelper_sem);
- if (usermodehelper_disabled) {
+ for (;;) {
+ prepare_to_wait(&usermodehelper_disabled_waitq, &wait,
+ TASK_INTERRUPTIBLE);
+ if (!usermodehelper_disabled)
+ break;
+
+ if (usermodehelper_disabled == UMH_DISABLED)
+ ret = -EAGAIN;
+
up_read(&umhelper_sem);
- ret = -EAGAIN;
+
+ if (ret)
+ break;
+
+ schedule();
+ try_to_freeze();
+
+ down_read(&umhelper_sem);
}
+ finish_wait(&usermodehelper_disabled_waitq, &wait);
return ret;
}
EXPORT_SYMBOL_GPL(usermodehelper_read_trylock);
@@ -349,25 +367,35 @@ void usermodehelper_read_unlock(void)
EXPORT_SYMBOL_GPL(usermodehelper_read_unlock);
/**
- * usermodehelper_enable - allow new helpers to be started again
+ * __usermodehelper_set_disable_depth - Modify usermodehelper_disabled.
+ * depth: New value to assign to usermodehelper_disabled.
+ *
+ * Change the value of usermodehelper_disabled (under umhelper_sem locked for
+ * writing) and wakeup tasks waiting for it to change.
*/
-void usermodehelper_enable(void)
+void __usermodehelper_set_disable_depth(enum umh_disable_depth depth)
{
down_write(&umhelper_sem);
- usermodehelper_disabled = 0;
+ usermodehelper_disabled = depth;
wake_up(&usermodehelper_disabled_waitq);
up_write(&umhelper_sem);
}
/**
- * usermodehelper_disable - prevent new helpers from being started
+ * __usermodehelper_disable - Prevent new helpers from being started.
+ * @depth: New value to assign to usermodehelper_disabled.
+ *
+ * Set usermodehelper_disabled to @depth and wait for running helpers to exit.
*/
-int usermodehelper_disable(void)
+int __usermodehelper_disable(enum umh_disable_depth depth)
{
long retval;
+ if (!depth)
+ return -EINVAL;
+
down_write(&umhelper_sem);
- usermodehelper_disabled = 1;
+ usermodehelper_disabled = depth;
up_write(&umhelper_sem);
/*
@@ -382,7 +410,7 @@ int usermodehelper_disable(void)
if (retval)
return 0;
- usermodehelper_enable();
+ __usermodehelper_set_disable_depth(UMH_ENABLED);
return -EAGAIN;
}
diff --git a/kernel/power/process.c b/kernel/power/process.c
index e8675aeb2e32..d915761bbc4f 100644
--- a/kernel/power/process.c
+++ b/kernel/power/process.c
@@ -142,7 +142,7 @@ int freeze_processes(void)
{
int error;
- error = usermodehelper_disable();
+ error = __usermodehelper_disable(UMH_FREEZING);
if (error)
return error;
@@ -150,6 +150,7 @@ int freeze_processes(void)
error = try_to_freeze_tasks(true);
if (!error) {
printk("done.");
+ __usermodehelper_set_disable_depth(UMH_DISABLED);
oom_killer_disable();
}
printk("\n");
--
1.7.10.4
Reply to: