[PATCH] regulator: core: fix race condition in regulator_put()

[PATCH] regulator: core: fix race condition in regulator_put()

[Ashay Jaiswal]

The regulator framework maintains a list of consumer regulators
for a regulator device and protects it from concurrent access
using the regulator device's mutex lock.

In the case of regulator_put() the consumer is removed without
holding the regulator device's mutex, resulting in a race condition
between any regulator operation which traverses the consumer list
and regulator_put() which releases the consumer regulator.
Fix this race condition by holding the regulator device's mutex while
removing and releasing the consumer regulator.

Signed-off-by: Ashay Jaiswal <ashayj@codeaurora.org>
---
 drivers/regulator/core.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index c2554d8..3845397 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1498,6 +1498,7 @@ static void _regulator_put(struct regulator *regulator)

 	rdev = regulator->rdev;

+	mutex_lock(&rdev->mutex);
 	debugfs_remove_recursive(regulator->debugfs);

 	/* remove any sysfs entries */
@@ -1511,6 +1512,7 @@ static void _regulator_put(struct regulator *regulator)
 	rdev->exclusive = 0;

 	module_put(rdev->owner);
+	mutex_unlock(&rdev->mutex);
 }

 /**
--
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH] regulator: core: fix race condition in regulator_put()

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1569 bytes --]

On Wed, Jan 07, 2015 at 07:21:23PM +0530, Ashay Jaiswal wrote:

> The regulator framework maintains a list of consumer regulators
> for a regulator device and protects it from concurrent access
> using the regulator device's mutex lock.

> In the case of regulator_put() the consumer is removed without
> holding the regulator device's mutex, resulting in a race condition
> between any regulator operation which traverses the consumer list
> and regulator_put() which releases the consumer regulator.
> Fix this race condition by holding the regulator device's mutex while
> removing and releasing the consumer regulator.

This is a good spot thanks but I think your analysis here is missing a
bit - it's not just the list manipulation that affects the rdev, it's
also the reference count in the rdev and the exclusive flag.  Indeed
some of this issue applies on the _get() side too, while we do add the
regulator to the list under the rdev mutex we don't have the mutex when
we update the reference count meaning that we've got a potential issue
with that.  That *is* kind of separate though so could be dealt with in
a separate patch.

The lock region also seems too wide, the lock is only needed for the
operations that affect the rdev not for the operations only on the
object being freed - holding the lock for too long means impacting other
users and some of the cleanup is potentially expensive.  The comment at
the top of the function needs updating too, it currently says that the
lock is held in the caller but this applies only to the regulator_list_mutex.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH] regulator: core: fix race condition in regulator_put()

[ashayj]

Thanks Mark for your review comments.

> On Wed, Jan 07, 2015 at 07:21:23PM +0530, Ashay Jaiswal wrote:
>
>> The regulator framework maintains a list of consumer regulators
>> for a regulator device and protects it from concurrent access
>> using the regulator device's mutex lock.
>
>> In the case of regulator_put() the consumer is removed without
>> holding the regulator device's mutex, resulting in a race condition
>> between any regulator operation which traverses the consumer list
>> and regulator_put() which releases the consumer regulator.
>> Fix this race condition by holding the regulator device's mutex while
>> removing and releasing the consumer regulator.
>
> This is a good spot thanks but I think your analysis here is missing a
> bit - it's not just the list manipulation that affects the rdev, it's
> also the reference count in the rdev and the exclusive flag.  Indeed
> some of this issue applies on the _get() side too, while we do add the
> regulator to the list under the rdev mutex we don't have the mutex when
> we update the reference count meaning that we've got a potential issue
> with that.  That *is* kind of separate though so could be dealt with in
> a separate patch.
>

I have updated the commit message as per your suggestion.
I will try to fix the regulator_get path.

> The lock region also seems too wide, the lock is only needed for the
> operations that affect the rdev not for the operations only on the
> object being freed - holding the lock for too long means impacting other
> users and some of the cleanup is potentially expensive.  The comment at
> the top of the function needs updating too, it currently says that the
> lock is held in the caller but this applies only to the
> regulator_list_mutex.
>
Yes I too agree with your suggestion, updated the lock region to protect
only rdev parameters.
Also updated the comment on top of the _regulator_put function to reflect
regulator_list_mutex lock is held by caller.

I will upload next version of patch incorporating all the review comments.

Ashay Jaiswal