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

[Mark Brown <broonie@kernel.org>]

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