[PATCH 0/7] regulator: fix deadlock vs memory reclaim

[PATCH 0/7] regulator: fix deadlock vs memory reclaim

[Michał Mirosław]

For systems that have eg. eMMC storage using voltage regulator, memory
reclaim path might call back into regulator subsystem. This means we
have to make sure no allocations happen with a regulator or regulator
list locked.

After this series I see no more lockdep complaints on my test system,
but please review and test further.

First four patches move allocations out of locked regions, next three
came as a drive-by cleanups.


Michał Mirosław (7):
  regulator: push allocation in regulator_init_coupling() outside of
    lock
  regulator: push allocation in regulator_ena_gpio_request() out of lock
  regulator: push allocations in create_regulator() outside of lock
  regulator: push allocation in set_consumer_device_supply() out of lock
  regulator: plug of_node leak in regulator_register()'s error path
  regulator: cleanup regulator_ena_gpio_free()
  regulator: remove superfluous lock in regulator_resolve_coupling()

 drivers/regulator/core.c | 162 +++++++++++++++++++++------------------
 1 file changed, 86 insertions(+), 76 deletions(-)

-- 
2.20.1

[PATCH 1/7] regulator: push allocation in regulator_init_coupling() outside of lock

[Michał Mirosław]

Allocating memory with regulator_list_mutex held makes lockdep unhappy
when memory pressure makes the system do fs_reclaim on eg. eMMC using
a regulator. Push the lock inside regulator_init_coupling() after the
allocation.

======================================================
WARNING: possible circular locking dependency detected
5.7.13+ #533 Not tainted
------------------------------------------------------
kswapd0/383 is trying to acquire lock:
cca78ca4 (&sbi->write_io[i][j].io_rwsem){++++}-{3:3}, at: __submit_merged_write_cond+0x104/0x154
but task is already holding lock:
c0e38518 (fs_reclaim){+.+.}-{0:0}, at: __fs_reclaim_acquire+0x0/0x50
which lock already depends on the new lock.
the existing dependency chain (in reverse order) is:
-> #2 (fs_reclaim){+.+.}-{0:0}:
       fs_reclaim_acquire.part.11+0x40/0x50
       fs_reclaim_acquire+0x24/0x28
       __kmalloc+0x54/0x218
       regulator_register+0x860/0x1584
       dummy_regulator_probe+0x60/0xa8
[...]
other info that might help us debug this:

Chain exists of:
  &sbi->write_io[i][j].io_rwsem --> regulator_list_mutex --> fs_reclaim

Possible unsafe locking scenario:

       CPU0                    CPU1
       ----                    ----
  lock(fs_reclaim);
                               lock(regulator_list_mutex);
                               lock(fs_reclaim);
  lock(&sbi->write_io[i][j].io_rwsem);
 *** DEADLOCK ***

1 lock held by kswapd0/383:
 #0: c0e38518 (fs_reclaim){+.+.}-{0:0}, at: __fs_reclaim_acquire+0x0/0x50
[...]

Cc: stable@vger.kernel.org
Fixes: d8ca7d184b33 ("regulator: core: Introduce API for regulators coupling customization")
Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
 drivers/regulator/core.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 0a32c3da0e26..510d234f6c46 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -5010,7 +5010,10 @@ static int regulator_init_coupling(struct regulator_dev *rdev)
 	if (!of_check_coupling_data(rdev))
 		return -EPERM;
 
+	mutex_lock(&regulator_list_mutex);
 	rdev->coupling_desc.coupler = regulator_find_coupler(rdev);
+	mutex_unlock(&regulator_list_mutex);
+
 	if (IS_ERR(rdev->coupling_desc.coupler)) {
 		err = PTR_ERR(rdev->coupling_desc.coupler);
 		rdev_err(rdev, "failed to get coupler: %d\n", err);
@@ -5218,9 +5221,7 @@ regulator_register(const struct regulator_desc *regulator_desc,
 	if (ret < 0)
 		goto wash;
 
-	mutex_lock(&regulator_list_mutex);
 	ret = regulator_init_coupling(rdev);
-	mutex_unlock(&regulator_list_mutex);
 	if (ret < 0)
 		goto wash;
 
-- 
2.20.1

Re: [PATCH 1/7] regulator: push allocation in regulator_init_coupling() outside of lock

[Dmitry Osipenko]

11.08.2020 04:07, Michał Mirosław пишет:
> Allocating memory with regulator_list_mutex held makes lockdep unhappy
> when memory pressure makes the system do fs_reclaim on eg. eMMC using
> a regulator. Push the lock inside regulator_init_coupling() after the
> allocation.
...

Reviewed-by: Dmitry Osipenko <digetx@gmail.com>

Re: [PATCH 1/7] regulator: push allocation in regulator_init_coupling() outside of lock

[Dmitry Osipenko]

11.08.2020 18:59, Dmitry Osipenko пишет:
> 11.08.2020 04:07, Michał Mirosław пишет:
>> Allocating memory with regulator_list_mutex held makes lockdep unhappy
>> when memory pressure makes the system do fs_reclaim on eg. eMMC using
>> a regulator. Push the lock inside regulator_init_coupling() after the
>> allocation.
> ...
> 
> Reviewed-by: Dmitry Osipenko <digetx@gmail.com>
> 

On the other hand, couldn't it be better to just remove taking the
list_mutex from the regulator_lock_dependent()?

I think the list_mutex is only needed to protect from supply/couple
regulator being removed during of the locking process, but maybe this is
not something we should worry about?

Re: [PATCH 1/7] regulator: push allocation in regulator_init_coupling() outside of lock

[Michał Mirosław]

On Tue, Aug 11, 2020 at 07:27:43PM +0300, Dmitry Osipenko wrote:
> 11.08.2020 18:59, Dmitry Osipenko пишет:
> > 11.08.2020 04:07, Michał Mirosław пишет:
> >> Allocating memory with regulator_list_mutex held makes lockdep unhappy
> >> when memory pressure makes the system do fs_reclaim on eg. eMMC using
> >> a regulator. Push the lock inside regulator_init_coupling() after the
> >> allocation.
> > ...
> > 
> > Reviewed-by: Dmitry Osipenko <digetx@gmail.com>
> On the other hand, couldn't it be better to just remove taking the
> list_mutex from the regulator_lock_dependent()?
> 
> I think the list_mutex is only needed to protect from supply/couple
> regulator being removed during of the locking process, but maybe this is
> not something we should worry about?

This is what I would like to see in the end, but it requires more
thought, at least around interaction with regulator_resolve_coupling()
and the regulator removal.

Best Regards,
Michał Mirosław

Re: [PATCH 1/7] regulator: push allocation in regulator_init_coupling() outside of lock

[Dmitry Osipenko]

11.08.2020 20:20, Michał Mirosław пишет:
> On Tue, Aug 11, 2020 at 07:27:43PM +0300, Dmitry Osipenko wrote:
>> 11.08.2020 18:59, Dmitry Osipenko пишет:
>>> 11.08.2020 04:07, Michał Mirosław пишет:
>>>> Allocating memory with regulator_list_mutex held makes lockdep unhappy
>>>> when memory pressure makes the system do fs_reclaim on eg. eMMC using
>>>> a regulator. Push the lock inside regulator_init_coupling() after the
>>>> allocation.
>>> ...
>>>
>>> Reviewed-by: Dmitry Osipenko <digetx@gmail.com>
>> On the other hand, couldn't it be better to just remove taking the
>> list_mutex from the regulator_lock_dependent()?
>>
>> I think the list_mutex is only needed to protect from supply/couple
>> regulator being removed during of the locking process, but maybe this is
>> not something we should worry about?
> 
> This is what I would like to see in the end, but it requires more
> thought, at least around interaction with regulator_resolve_coupling()
> and the regulator removal.

I meant that it's very unlikely to have regulator gone while it's
in-use. Hence it could be okay to ignore this rare case, and thus,
simplify the fix significantly by removing the offending lock.

Still this won't solve the root of the problem because potentially
reclaim could happen while storage regulator (or its supply) is locked,
although it should be a very unlikely condition in practice.

[PATCH 3/7] regulator: push allocations in create_regulator() outside of lock

[Michał Mirosław]

Move all allocations outside of the regulator_lock()ed section.

======================================================
WARNING: possible circular locking dependency detected
5.7.13+ #535 Not tainted
------------------------------------------------------
f2fs_discard-179:7/702 is trying to acquire lock:
c0e5d920 (regulator_list_mutex){+.+.}-{3:3}, at: regulator_lock_dependent+0x54/0x2c0

but task is already holding lock:
cb95b080 (&dcc->cmd_lock){+.+.}-{3:3}, at: __issue_discard_cmd+0xec/0x5f8

which lock already depends on the new lock.

the existing dependency chain (in reverse order) is:

[...]

-> #3 (fs_reclaim){+.+.}-{0:0}:
       fs_reclaim_acquire.part.11+0x40/0x50
       fs_reclaim_acquire+0x24/0x28
       __kmalloc_track_caller+0x54/0x218
       kstrdup+0x40/0x5c
       create_regulator+0xf4/0x368
       regulator_resolve_supply+0x1a0/0x200
       regulator_register+0x9c8/0x163c

[...]

other info that might help us debug this:

Chain exists of:
  regulator_list_mutex --> &sit_i->sentry_lock --> &dcc->cmd_lock

[...]

Cc: stable@vger.kernel.org
Fixes: f8702f9e4aa7 ("regulator: core: Use ww_mutex for regulators locking")
Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
 drivers/regulator/core.c | 53 +++++++++++++++++++++-------------------
 1 file changed, 28 insertions(+), 25 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 3dd4d4914075..c95397798275 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1553,44 +1553,53 @@ static struct regulator *create_regulator(struct regulator_dev *rdev,
 					  const char *supply_name)
 {
 	struct regulator *regulator;
-	char buf[REG_STR_SIZE];
-	int err, size;
+	int err;
+
+	if (dev) {
+		char buf[REG_STR_SIZE];
+		int size;
+
+		size = snprintf(buf, REG_STR_SIZE, "%s-%s",
+				dev->kobj.name, supply_name);
+		if (size >= REG_STR_SIZE)
+			return NULL;
+
+		supply_name = kstrdup(buf, GFP_KERNEL);
+		if (supply_name == NULL)
+			return NULL;
+	} else {
+		supply_name = kstrdup_const(supply_name, GFP_KERNEL);
+		if (supply_name == NULL)
+			return NULL;
+	}
 
 	regulator = kzalloc(sizeof(*regulator), GFP_KERNEL);
-	if (regulator == NULL)
+	if (regulator == NULL) {
+		kfree(supply_name);
 		return NULL;
+	}
 
-	regulator_lock(rdev);
 	regulator->rdev = rdev;
+	regulator->supply_name = supply_name;
+
+	regulator_lock(rdev);
 	list_add(&regulator->list, &rdev->consumer_list);
+	regulator_unlock(rdev);
 
 	if (dev) {
 		regulator->dev = dev;
 
 		/* Add a link to the device sysfs entry */
-		size = snprintf(buf, REG_STR_SIZE, "%s-%s",
-				dev->kobj.name, supply_name);
-		if (size >= REG_STR_SIZE)
-			goto overflow_err;
-
-		regulator->supply_name = kstrdup(buf, GFP_KERNEL);
-		if (regulator->supply_name == NULL)
-			goto overflow_err;
-
 		err = sysfs_create_link_nowarn(&rdev->dev.kobj, &dev->kobj,
-					buf);
+					       supply_name);
 		if (err) {
 			rdev_dbg(rdev, "could not add device link %s err %d\n",
 				  dev->kobj.name, err);
 			/* non-fatal */
 		}
-	} else {
-		regulator->supply_name = kstrdup_const(supply_name, GFP_KERNEL);
-		if (regulator->supply_name == NULL)
-			goto overflow_err;
 	}
 
-	regulator->debugfs = debugfs_create_dir(regulator->supply_name,
+	regulator->debugfs = debugfs_create_dir(supply_name,
 						rdev->debugfs);
 	if (!regulator->debugfs) {
 		rdev_dbg(rdev, "Failed to create debugfs directory\n");
@@ -1615,13 +1624,7 @@ static struct regulator *create_regulator(struct regulator_dev *rdev,
 	    _regulator_is_enabled(rdev))
 		regulator->always_on = true;
 
-	regulator_unlock(rdev);
 	return regulator;
-overflow_err:
-	list_del(&regulator->list);
-	kfree(regulator);
-	regulator_unlock(rdev);
-	return NULL;
 }
 
 static int _regulator_get_enable_time(struct regulator_dev *rdev)
-- 
2.20.1

[PATCH 2/7] regulator: push allocation in regulator_ena_gpio_request() out of lock

[Michał Mirosław]

Move another allocation out of regulator_list_mutex-protected region, as
reclaim might want to take the same lock.

WARNING: possible circular locking dependency detected
5.7.13+ #534 Not tainted
------------------------------------------------------
kswapd0/383 is trying to acquire lock:
c0e5d920 (regulator_list_mutex){+.+.}-{3:3}, at: regulator_lock_dependent+0x54/0x2c0

but task is already holding lock:
c0e38518 (fs_reclaim){+.+.}-{0:0}, at: __fs_reclaim_acquire+0x0/0x50

which lock already depends on the new lock.

the existing dependency chain (in reverse order) is:

-> #1 (fs_reclaim){+.+.}-{0:0}:
       fs_reclaim_acquire.part.11+0x40/0x50
       fs_reclaim_acquire+0x24/0x28
       kmem_cache_alloc_trace+0x40/0x1e8
       regulator_register+0x384/0x1630
       devm_regulator_register+0x50/0x84
       reg_fixed_voltage_probe+0x248/0x35c
[...]
other info that might help us debug this:

 Possible unsafe locking scenario:

       CPU0                    CPU1
       ----                    ----
  lock(fs_reclaim);
                               lock(regulator_list_mutex);
                               lock(fs_reclaim);
  lock(regulator_list_mutex);

 *** DEADLOCK ***
[...]
2 locks held by kswapd0/383:
 #0: c0e38518 (fs_reclaim){+.+.}-{0:0}, at: __fs_reclaim_acquire+0x0/0x50
 #1: cb70e5e0 (hctx->srcu){....}-{0:0}, at: hctx_lock+0x60/0xb8
[...]

Fixes: 541d052d7215 ("regulator: core: Only support passing enable GPIO descriptors")
[this commit only changes context]
Fixes: f8702f9e4aa7 ("regulator: core: Use ww_mutex for regulators locking")
[this is when the regulator_list_mutex was introduced in reclaim locking path]

Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
 drivers/regulator/core.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 510d234f6c46..3dd4d4914075 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -2203,10 +2203,13 @@ EXPORT_SYMBOL_GPL(regulator_bulk_unregister_supply_alias);
 static int regulator_ena_gpio_request(struct regulator_dev *rdev,
 				const struct regulator_config *config)
 {
-	struct regulator_enable_gpio *pin;
+	struct regulator_enable_gpio *pin, *new_pin;
 	struct gpio_desc *gpiod;
 
 	gpiod = config->ena_gpiod;
+	new_pin = kzalloc(sizeof(*new_pin), GFP_KERNEL);
+
+	mutex_lock(&regulator_list_mutex);
 
 	list_for_each_entry(pin, &regulator_ena_gpio_list, list) {
 		if (pin->gpiod == gpiod) {
@@ -2215,9 +2218,13 @@ static int regulator_ena_gpio_request(struct regulator_dev *rdev,
 		}
 	}
 
-	pin = kzalloc(sizeof(struct regulator_enable_gpio), GFP_KERNEL);
-	if (pin == NULL)
+	if (new_pin == NULL) {
+		mutex_unlock(&regulator_list_mutex);
 		return -ENOMEM;
+	}
+
+	pin = new_pin;
+	new_pin = NULL;
 
 	pin->gpiod = gpiod;
 	list_add(&pin->list, &regulator_ena_gpio_list);
@@ -2225,6 +2232,10 @@ static int regulator_ena_gpio_request(struct regulator_dev *rdev,
 update_ena_gpio_to_rdev:
 	pin->request_count++;
 	rdev->ena_pin = pin;
+
+	mutex_unlock(&regulator_list_mutex);
+	kfree(new_pin);
+
 	return 0;
 }
 
@@ -5179,9 +5190,7 @@ regulator_register(const struct regulator_desc *regulator_desc,
 	}
 
 	if (config->ena_gpiod) {
-		mutex_lock(&regulator_list_mutex);
 		ret = regulator_ena_gpio_request(rdev, config);
-		mutex_unlock(&regulator_list_mutex);
 		if (ret != 0) {
 			rdev_err(rdev, "Failed to request enable GPIO: %d\n",
 				 ret);
-- 
2.20.1

[PATCH 4/7] regulator: push allocation in set_consumer_device_supply() out of lock

[Michał Mirosław]

Pull regulator_list_mutex into set_consumer_device_supply() and keep
allocations outside of it. Fourth of the fs_reclaim deadlock case.

Cc: stable@vger.kernel.org
Fixes: 45389c47526d ("regulator: core: Add early supply resolution for regulators")
Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
 drivers/regulator/core.c | 44 +++++++++++++++++++++++-----------------
 1 file changed, 25 insertions(+), 19 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index c95397798275..0408c4e1d9a8 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1445,6 +1445,22 @@ static int set_consumer_device_supply(struct regulator_dev *rdev,
 	else
 		has_dev = 0;
 
+	node = kzalloc(sizeof(struct regulator_map), GFP_KERNEL);
+	if (node == NULL)
+		return -ENOMEM;
+
+	node->regulator = rdev;
+	node->supply = supply;
+
+	if (has_dev) {
+		node->dev_name = kstrdup(consumer_dev_name, GFP_KERNEL);
+		if (node->dev_name == NULL) {
+			kfree(node);
+			return -ENOMEM;
+		}
+	}
+
+	mutex_lock(&regulator_list_mutex);
 	list_for_each_entry(node, &regulator_map_list, list) {
 		if (node->dev_name && consumer_dev_name) {
 			if (strcmp(node->dev_name, consumer_dev_name) != 0)
@@ -1462,26 +1478,19 @@ static int set_consumer_device_supply(struct regulator_dev *rdev,
 			 node->regulator->desc->name,
 			 supply,
 			 dev_name(&rdev->dev), rdev_get_name(rdev));
-		return -EBUSY;
-	}
-
-	node = kzalloc(sizeof(struct regulator_map), GFP_KERNEL);
-	if (node == NULL)
-		return -ENOMEM;
-
-	node->regulator = rdev;
-	node->supply = supply;
-
-	if (has_dev) {
-		node->dev_name = kstrdup(consumer_dev_name, GFP_KERNEL);
-		if (node->dev_name == NULL) {
-			kfree(node);
-			return -ENOMEM;
-		}
+		goto fail;
 	}
 
 	list_add(&node->list, &regulator_map_list);
+	mutex_unlock(&regulator_list_mutex);
+
 	return 0;
+
+fail:
+	mutex_unlock(&regulator_list_mutex);
+	kfree(node->dev_name);
+	kfree(node);
+	return -EBUSY;
 }
 
 static void unset_regulator_supplies(struct regulator_dev *rdev)
@@ -5239,19 +5248,16 @@ regulator_register(const struct regulator_desc *regulator_desc,
 
 	/* add consumers devices */
 	if (init_data) {
-		mutex_lock(&regulator_list_mutex);
 		for (i = 0; i < init_data->num_consumer_supplies; i++) {
 			ret = set_consumer_device_supply(rdev,
 				init_data->consumer_supplies[i].dev_name,
 				init_data->consumer_supplies[i].supply);
 			if (ret < 0) {
-				mutex_unlock(&regulator_list_mutex);
 				dev_err(dev, "Failed to set supply %s\n",
 					init_data->consumer_supplies[i].supply);
 				goto unset_supplies;
 			}
 		}
-		mutex_unlock(&regulator_list_mutex);
 	}
 
 	if (!rdev->desc->ops->get_voltage &&
-- 
2.20.1

Re: [PATCH 4/7] regulator: push allocation in set_consumer_device_supply() out of lock

[kernel test robot]

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

Hi "Michał,

I love your patch! Perhaps something to improve:

[auto build test WARNING on regulator/for-next]
[also build test WARNING on v5.8 next-20200810]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Micha-Miros-aw/regulator-fix-deadlock-vs-memory-reclaim/20200811-091414
base:   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-next
config: x86_64-randconfig-m001-20200810 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

New smatch warnings:
drivers/regulator/core.c:1491 set_consumer_device_supply() warn: overwrite may leak 'node'

Old smatch warnings:
drivers/regulator/core.c:129 regulator_ops_is_valid() error: we previously assumed 'rdev->constraints' could be null (see line 128)
drivers/regulator/core.c:3051 regulator_get_hardware_vsel_register() warn: inconsistent indenting

vim +/node +1491 drivers/regulator/core.c

3801b86aa482d2 Mark Brown      2011-06-09  1448  
a5766f11cfd3a0 Liam Girdwood   2008-10-10  1449  /**
06c63f9396133f Randy Dunlap    2010-11-18  1450   * set_consumer_device_supply - Bind a regulator to a symbolic supply
69279fb9a95051 Mark Brown      2008-12-31  1451   * @rdev:         regulator source
40f9244f4da897 Mark Brown      2009-06-17  1452   * @consumer_dev_name: dev_name() string for device supply applies to
a5766f11cfd3a0 Liam Girdwood   2008-10-10  1453   * @supply:       symbolic name for supply
a5766f11cfd3a0 Liam Girdwood   2008-10-10  1454   *
a5766f11cfd3a0 Liam Girdwood   2008-10-10  1455   * Allows platform initialisation code to map physical regulator
a5766f11cfd3a0 Liam Girdwood   2008-10-10  1456   * sources to symbolic names for supplies for use by devices.  Devices
a5766f11cfd3a0 Liam Girdwood   2008-10-10  1457   * should use these symbolic names to request regulators, avoiding the
a5766f11cfd3a0 Liam Girdwood   2008-10-10  1458   * need to provide board-specific regulator names as platform data.
a5766f11cfd3a0 Liam Girdwood   2008-10-10  1459   */
a5766f11cfd3a0 Liam Girdwood   2008-10-10  1460  static int set_consumer_device_supply(struct regulator_dev *rdev,
737f360d5bef5e Mark Brown      2012-02-02  1461  				      const char *consumer_dev_name,
40f9244f4da897 Mark Brown      2009-06-17  1462  				      const char *supply)
a5766f11cfd3a0 Liam Girdwood   2008-10-10  1463  {
a5766f11cfd3a0 Liam Girdwood   2008-10-10  1464  	struct regulator_map *node;
9ed2099edca26d Mark Brown      2009-07-21  1465  	int has_dev;
a5766f11cfd3a0 Liam Girdwood   2008-10-10  1466  
a5766f11cfd3a0 Liam Girdwood   2008-10-10  1467  	if (supply == NULL)
a5766f11cfd3a0 Liam Girdwood   2008-10-10  1468  		return -EINVAL;
a5766f11cfd3a0 Liam Girdwood   2008-10-10  1469  
9ed2099edca26d Mark Brown      2009-07-21  1470  	if (consumer_dev_name != NULL)
9ed2099edca26d Mark Brown      2009-07-21  1471  		has_dev = 1;
9ed2099edca26d Mark Brown      2009-07-21  1472  	else
9ed2099edca26d Mark Brown      2009-07-21  1473  		has_dev = 0;
9ed2099edca26d Mark Brown      2009-07-21  1474  
bcadf558ba875d Michał Mirosław 2020-08-11  1475  	node = kzalloc(sizeof(struct regulator_map), GFP_KERNEL);
bcadf558ba875d Michał Mirosław 2020-08-11  1476  	if (node == NULL)
bcadf558ba875d Michał Mirosław 2020-08-11  1477  		return -ENOMEM;
bcadf558ba875d Michał Mirosław 2020-08-11  1478  
bcadf558ba875d Michał Mirosław 2020-08-11  1479  	node->regulator = rdev;
bcadf558ba875d Michał Mirosław 2020-08-11  1480  	node->supply = supply;
bcadf558ba875d Michał Mirosław 2020-08-11  1481  
bcadf558ba875d Michał Mirosław 2020-08-11  1482  	if (has_dev) {
bcadf558ba875d Michał Mirosław 2020-08-11  1483  		node->dev_name = kstrdup(consumer_dev_name, GFP_KERNEL);
bcadf558ba875d Michał Mirosław 2020-08-11  1484  		if (node->dev_name == NULL) {
bcadf558ba875d Michał Mirosław 2020-08-11  1485  			kfree(node);
bcadf558ba875d Michał Mirosław 2020-08-11  1486  			return -ENOMEM;
bcadf558ba875d Michał Mirosław 2020-08-11  1487  		}
bcadf558ba875d Michał Mirosław 2020-08-11  1488  	}
bcadf558ba875d Michał Mirosław 2020-08-11  1489  
bcadf558ba875d Michał Mirosław 2020-08-11  1490  	mutex_lock(&regulator_list_mutex);
6001e13c5f708e David Brownell  2008-12-31 @1491  	list_for_each_entry(node, &regulator_map_list, list) {
23b5cc2ab67832 Jani Nikula     2010-04-29  1492  		if (node->dev_name && consumer_dev_name) {
23b5cc2ab67832 Jani Nikula     2010-04-29  1493  			if (strcmp(node->dev_name, consumer_dev_name) != 0)
6001e13c5f708e David Brownell  2008-12-31  1494  				continue;
23b5cc2ab67832 Jani Nikula     2010-04-29  1495  		} else if (node->dev_name || consumer_dev_name) {
23b5cc2ab67832 Jani Nikula     2010-04-29  1496  			continue;
23b5cc2ab67832 Jani Nikula     2010-04-29  1497  		}
23b5cc2ab67832 Jani Nikula     2010-04-29  1498  
6001e13c5f708e David Brownell  2008-12-31  1499  		if (strcmp(node->supply, supply) != 0)
6001e13c5f708e David Brownell  2008-12-31  1500  			continue;
6001e13c5f708e David Brownell  2008-12-31  1501  
737f360d5bef5e Mark Brown      2012-02-02  1502  		pr_debug("%s: %s/%s is '%s' supply; fail %s/%s\n",
737f360d5bef5e Mark Brown      2012-02-02  1503  			 consumer_dev_name,
6001e13c5f708e David Brownell  2008-12-31  1504  			 dev_name(&node->regulator->dev),
6001e13c5f708e David Brownell  2008-12-31  1505  			 node->regulator->desc->name,
6001e13c5f708e David Brownell  2008-12-31  1506  			 supply,
1083c39346d482 Mark Brown      2009-10-22  1507  			 dev_name(&rdev->dev), rdev_get_name(rdev));
bcadf558ba875d Michał Mirosław 2020-08-11  1508  		goto fail;
6001e13c5f708e David Brownell  2008-12-31  1509  	}
6001e13c5f708e David Brownell  2008-12-31  1510  
bcadf558ba875d Michał Mirosław 2020-08-11  1511  	list_add(&node->list, &regulator_map_list);
bcadf558ba875d Michał Mirosław 2020-08-11  1512  	mutex_unlock(&regulator_list_mutex);
a5766f11cfd3a0 Liam Girdwood   2008-10-10  1513  
bcadf558ba875d Michał Mirosław 2020-08-11  1514  	return 0;
a5766f11cfd3a0 Liam Girdwood   2008-10-10  1515  
bcadf558ba875d Michał Mirosław 2020-08-11  1516  fail:
bcadf558ba875d Michał Mirosław 2020-08-11  1517  	mutex_unlock(&regulator_list_mutex);
bcadf558ba875d Michał Mirosław 2020-08-11  1518  	kfree(node->dev_name);
40f9244f4da897 Mark Brown      2009-06-17  1519  	kfree(node);
bcadf558ba875d Michał Mirosław 2020-08-11  1520  	return -EBUSY;
a5766f11cfd3a0 Liam Girdwood   2008-10-10  1521  }
a5766f11cfd3a0 Liam Girdwood   2008-10-10  1522  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 37205 bytes --]

Re: [PATCH 4/7] regulator: push allocation in set_consumer_device_supply() out of lock

[Michał Mirosław]

On Tue, Aug 11, 2020 at 01:23:47PM +0800, kernel test robot wrote:
[...]
> New smatch warnings:
> drivers/regulator/core.c:1491 set_consumer_device_supply() warn: overwrite may leak 'node'

Ah! Good catch!

[PATCH 6/7] regulator: cleanup regulator_ena_gpio_free()

[Michał Mirosław]

Since only regulator_ena_gpio_request() allocates rdev->ena_pin, and it
guarantees that same gpiod gets same pin structure, it is enough to
compare just the pointers. Also we know there can be only one matching
entry on the list. Rework the code take advantage of the facts.

Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
 drivers/regulator/core.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index a9ff2ad55ee7..b85ec974944e 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -2260,19 +2260,19 @@ static void regulator_ena_gpio_free(struct regulator_dev *rdev)
 
 	/* Free the GPIO only in case of no use */
 	list_for_each_entry_safe(pin, n, &regulator_ena_gpio_list, list) {
-		if (pin->gpiod == rdev->ena_pin->gpiod) {
-			if (pin->request_count <= 1) {
-				pin->request_count = 0;
-				gpiod_put(pin->gpiod);
-				list_del(&pin->list);
-				kfree(pin);
-				rdev->ena_pin = NULL;
-				return;
-			} else {
-				pin->request_count--;
-			}
-		}
+		if (pin != rdev->ena_pin)
+			continue;
+
+		if (--pin->request_count)
+			break;
+
+		gpiod_put(pin->gpiod);
+		list_del(&pin->list);
+		kfree(pin);
+		break;
 	}
+
+	rdev->ena_pin = NULL;
 }
 
 /**
-- 
2.20.1

[PATCH 5/7] regulator: plug of_node leak in regulator_register()'s error path

[Michał Mirosław]

By calling device_initialize() earlier and noting that kfree(NULL) is
ok, we can save a bit of code in error handling and plug of_node leak.
Fixed commit already did part of the work.

Cc: stable@vger.kernel.org
Fixes: 9177514ce349 ("regulator: fix memory leak on error path of regulator_register()")
Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
 drivers/regulator/core.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 0408c4e1d9a8..a9ff2ad55ee7 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -5137,6 +5137,7 @@ regulator_register(const struct regulator_desc *regulator_desc,
 		ret = -ENOMEM;
 		goto rinse;
 	}
+	device_initialize(&rdev->dev);
 
 	/*
 	 * Duplicate the config so the driver could override it after
@@ -5144,9 +5145,8 @@ regulator_register(const struct regulator_desc *regulator_desc,
 	 */
 	config = kmemdup(cfg, sizeof(*cfg), GFP_KERNEL);
 	if (config == NULL) {
-		kfree(rdev);
 		ret = -ENOMEM;
-		goto rinse;
+		goto clean;
 	}
 
 	init_data = regulator_of_get_init_data(dev, regulator_desc, config,
@@ -5158,10 +5158,8 @@ regulator_register(const struct regulator_desc *regulator_desc,
 	 * from a gpio extender or something else.
 	 */
 	if (PTR_ERR(init_data) == -EPROBE_DEFER) {
-		kfree(config);
-		kfree(rdev);
 		ret = -EPROBE_DEFER;
-		goto rinse;
+		goto clean;
 	}
 
 	/*
@@ -5214,7 +5212,6 @@ regulator_register(const struct regulator_desc *regulator_desc,
 	}
 
 	/* register with sysfs */
-	device_initialize(&rdev->dev);
 	rdev->dev.class = &regulator_class;
 	rdev->dev.parent = dev;
 	dev_set_name(&rdev->dev, "regulator.%lu",
@@ -5292,13 +5289,11 @@ regulator_register(const struct regulator_desc *regulator_desc,
 	mutex_lock(&regulator_list_mutex);
 	regulator_ena_gpio_free(rdev);
 	mutex_unlock(&regulator_list_mutex);
-	put_device(&rdev->dev);
-	rdev = NULL;
 clean:
 	if (dangling_of_gpiod)
 		gpiod_put(config->ena_gpiod);
-	kfree(rdev);
 	kfree(config);
+	put_device(&rdev->dev);
 rinse:
 	if (dangling_cfg_gpiod)
 		gpiod_put(cfg->ena_gpiod);
-- 
2.20.1

Re: [PATCH 5/7] regulator: plug of_node leak in regulator_register()'s error path

[Vladimir Zapolskiy]

Hi Michał,

On 8/11/20 4:07 AM, Michał Mirosław wrote:
> By calling device_initialize() earlier and noting that kfree(NULL) is
> ok, we can save a bit of code in error handling and plug of_node leak.
> Fixed commit already did part of the work.
> 
> Cc: stable@vger.kernel.org
> Fixes: 9177514ce349 ("regulator: fix memory leak on error path of regulator_register()")
> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>

thank you for the patch!

I was worried about a potentially remaining of_node reference leak,
but I was not able to reproduce it on practice without code fuzzing.

The change looks valid and it's a nice simplification.

Acked-by: Vladimir Zapolskiy <vz@mleia.com>

--
Best wishes,
Vladimir

[PATCH 7/7] regulator: remove superfluous lock in regulator_resolve_coupling()

[Michał Mirosław]

The code modifies rdev, but locks c_rdev instead. The bug remains:
stored c_rdev could be freed just after unlock anyway. This doesn't blow
up because regulator_list_mutex taken outside holds it together.

Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
 drivers/regulator/core.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index b85ec974944e..f8834559a2fb 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -4942,13 +4942,9 @@ static void regulator_resolve_coupling(struct regulator_dev *rdev)
 			return;
 		}
 
-		regulator_lock(c_rdev);
-
 		c_desc->coupled_rdevs[i] = c_rdev;
 		c_desc->n_resolved++;
 
-		regulator_unlock(c_rdev);
-
 		regulator_resolve_coupling(c_rdev);
 	}
 }
-- 
2.20.1

Re: [PATCH 7/7] regulator: remove superfluous lock in regulator_resolve_coupling()

[Dmitry Osipenko]

11.08.2020 04:07, Michał Mirosław пишет:
> The code modifies rdev, but locks c_rdev instead. The bug remains:
> stored c_rdev could be freed just after unlock anyway. This doesn't blow
> up because regulator_list_mutex taken outside holds it together.
> 
> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> ---
>  drivers/regulator/core.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index b85ec974944e..f8834559a2fb 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -4942,13 +4942,9 @@ static void regulator_resolve_coupling(struct regulator_dev *rdev)
>  			return;
>  		}
>  
> -		regulator_lock(c_rdev);
> -
>  		c_desc->coupled_rdevs[i] = c_rdev;
>  		c_desc->n_resolved++;
>  
> -		regulator_unlock(c_rdev);
> -
>  		regulator_resolve_coupling(c_rdev);
>  	}
>  }
> 

As I replied to the other email, there is no real bug here. The
regulators are uncoupled before regulator is freed and the uncoupling is
also protected by the list_mutex.

Hence the resolve_coupling() doesn't need to lock regulators and this
change looks like a good cleanup.

Perhaps the commit message could be improved a tad, either way:

Reviewed-by: Dmitry Osipenko <digetx@gmail.com>