| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 28 Mar 2011 16:53:51 -0700
From: David Collins <collinsd@...eaurora.org>
To: Liam Girdwood <lrg@...mlogic.co.uk>,
Mark Brown <broonie@...nsource.wolfsonmicro.com>
Cc: linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org,
linux-arm-msm-owner@...r.kernel.org,
David Collins <collinsd@...eaurora.org>
Subject: [PATCH v2 1/2] regulator: Remove possible deadlock from regulator_enable
Assume a regulator supply chain in which S1 supplies L2 (S1 --> L2).
It is possible to achieve deadlock using two threads if one thread
calls regulator_enable on a regulator L2 while the other thread calls
regulator_disable on S1. This happens because _regulator_enable(L2)
holds a lock for L2 and then takes out a lock on S1.
_regulator_disable(S1) on the other hand, is called with a lock taken
for S1 and then it calls _notifier_call_chain which attempts to take a
lock for L2. Because these locks are taken in opposite orders, deadlock
will result.
Change regulator_enable and _regulator_enable so that only one lock is
held at a time to avoid this deadlock scenario. Rename
_regulator_enable to regulator_dev_enable to avoid confusion with other
_* functions which assume that all locks are already taken.
Signed-off-by: David Collins <collinsd@...eaurora.org>
---
drivers/regulator/core.c | 82 +++++++++++++++++++++++++++++++---------------
1 files changed, 55 insertions(+), 27 deletions(-)
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 9fa2095..05904be 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1279,38 +1279,54 @@ static int _regulator_can_change_status(struct regulator_dev *rdev)
return 0;
}
-/* locks held by regulator_enable() */
-static int _regulator_enable(struct regulator_dev *rdev)
+/* Locks are *not* held by regulator_enable(). */
+static int regulator_dev_enable(struct regulator_dev *rdev)
{
- int ret, delay;
+ struct regulator_dev *supply_rdev = NULL;
+ int ret = 0, delay;
+ mutex_lock(&rdev->mutex);
if (rdev->use_count == 0) {
+ /*
+ * Incrementing here removes a race condition for simultaneous
+ * regulator_enable calls with use_count == 0.
+ */
+ rdev->use_count++;
+ mutex_unlock(&rdev->mutex);
+
/* do we need to enable the supply regulator first */
if (rdev->supply) {
- mutex_lock(&rdev->supply->mutex);
- ret = _regulator_enable(rdev->supply);
- mutex_unlock(&rdev->supply->mutex);
+ ret = regulator_dev_enable(rdev->supply);
if (ret < 0) {
rdev_err(rdev, "failed to enable: %d\n", ret);
+ mutex_lock(&rdev->mutex);
+ rdev->use_count--;
+ mutex_unlock(&rdev->mutex);
return ret;
}
+ supply_rdev = rdev->supply;
}
- }
- /* check voltage and requested load before enabling */
- if (rdev->constraints &&
- (rdev->constraints->valid_ops_mask & REGULATOR_CHANGE_DRMS))
- drms_uA_update(rdev);
+ mutex_lock(&rdev->mutex);
+ rdev->use_count--;
+
+ /* check voltage and requested load before enabling */
+ if (rdev->constraints &&
+ (rdev->constraints->valid_ops_mask & REGULATOR_CHANGE_DRMS))
+ drms_uA_update(rdev);
- if (rdev->use_count == 0) {
- /* The regulator may on if it's not switchable or left on */
+ /* The regulator may be on if it's not switchable or left on */
ret = _regulator_is_enabled(rdev);
if (ret == -EINVAL || ret == 0) {
- if (!_regulator_can_change_status(rdev))
- return -EPERM;
+ if (!_regulator_can_change_status(rdev)) {
+ ret = -EPERM;
+ goto out;
+ }
- if (!rdev->desc->ops->enable)
- return -EINVAL;
+ if (!rdev->desc->ops->enable) {
+ ret = -EINVAL;
+ goto out;
+ }
/* Query before enabling in case configuration
* dependant. */
@@ -1330,7 +1346,7 @@ static int _regulator_enable(struct regulator_dev *rdev)
* regulators can ramp together. */
ret = rdev->desc->ops->enable(rdev);
if (ret < 0)
- return ret;
+ goto out;
trace_regulator_enable_delay(rdev_get_name(rdev));
@@ -1345,14 +1361,32 @@ static int _regulator_enable(struct regulator_dev *rdev)
} else if (ret < 0) {
rdev_err(rdev, "is_enabled() failed: %d\n", ret);
- return ret;
+ goto out;
}
/* Fallthrough on positive return values - already enabled */
+ } else {
+ /* Check voltage and requested load. */
+ if (rdev->constraints &&
+ (rdev->constraints->valid_ops_mask & REGULATOR_CHANGE_DRMS))
+ drms_uA_update(rdev);
}
rdev->use_count++;
+out:
+ mutex_unlock(&rdev->mutex);
- return 0;
+ if (ret < 0) {
+ /* Enable has failed, disable supplies if necessary. */
+ while (supply_rdev != NULL) {
+ rdev = supply_rdev;
+
+ mutex_lock(&rdev->mutex);
+ _regulator_disable(rdev, &supply_rdev);
+ mutex_unlock(&rdev->mutex);
+ }
+ }
+
+ return ret;
}
/**
@@ -1368,13 +1402,7 @@ static int _regulator_enable(struct regulator_dev *rdev)
*/
int regulator_enable(struct regulator *regulator)
{
- struct regulator_dev *rdev = regulator->rdev;
- int ret = 0;
-
- mutex_lock(&rdev->mutex);
- ret = _regulator_enable(rdev);
- mutex_unlock(&rdev->mutex);
- return ret;
+ return regulator_dev_enable(regulator->rdev);
}
EXPORT_SYMBOL_GPL(regulator_enable);
--
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists