2020-08-12 01:33:50

by Michał Mirosław

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

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.

---
v2: fix bug in patch #4 spotted by kernel test robot
reworded commit #7 description

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 | 164 +++++++++++++++++++++------------------
1 file changed, 87 insertions(+), 77 deletions(-)

--
2.20.1

2020-08-12 01:34:29

by Michał Mirosław

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

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: [email protected]
Fixes: 9177514ce349 ("regulator: fix memory leak on error path of regulator_register()")
Signed-off-by: Michał Mirosław <[email protected]>
Acked-by: Vladimir Zapolskiy <[email protected]>
---
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 71749f48caee..448a267641df 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

2020-08-12 06:31:36

by Vladimir Zapolskiy

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

Hi Michał,

On 8/12/20 4:31 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: [email protected]
> Fixes: 9177514ce349 ("regulator: fix memory leak on error path of regulator_register()")
> Signed-off-by: Michał Mirosław <[email protected]>
> Acked-by: Vladimir Zapolskiy <[email protected]>
> ---
> 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 71749f48caee..448a267641df 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;

Here config == NULL

> }
>
> 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);

And above 'config' NULL pointer could be dereferenced now, right?

> - kfree(rdev);
> kfree(config);
> + put_device(&rdev->dev);
> rinse:
> if (dangling_cfg_gpiod)
> gpiod_put(cfg->ena_gpiod);
>

--
Best wishes,
Vladimir

2020-08-12 14:11:13

by Michał Mirosław

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

On Wed, Aug 12, 2020 at 09:29:12AM +0300, Vladimir Zapolskiy wrote:
> On 8/12/20 4:31 AM, Micha? Miros?aw wrote:
[...]
> > config = kmemdup(cfg, sizeof(*cfg), GFP_KERNEL);
> > if (config == NULL) {
> > - kfree(rdev);
> > ret = -ENOMEM;
> > - goto rinse;
> > + goto clean;
[...]
> > clean:
> > if (dangling_of_gpiod)
> > gpiod_put(config->ena_gpiod);
>
> And above 'config' NULL pointer could be dereferenced now, right?

If config is NULL, dangling_of_gpiod cannot be true.

Best Regards,
Micha? Miros?aw

2020-08-13 08:32:48

by Vladimir Zapolskiy

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

On 8/12/20 5:09 PM, Michał Mirosław wrote:
> On Wed, Aug 12, 2020 at 09:29:12AM +0300, Vladimir Zapolskiy wrote:
>> On 8/12/20 4:31 AM, Michał Mirosław wrote:
> [...]
>>> config = kmemdup(cfg, sizeof(*cfg), GFP_KERNEL);
>>> if (config == NULL) {
>>> - kfree(rdev);
>>> ret = -ENOMEM;
>>> - goto rinse;
>>> + goto clean;
> [...]
>>> clean:
>>> if (dangling_of_gpiod)
>>> gpiod_put(config->ena_gpiod);
>>
>> And above 'config' NULL pointer could be dereferenced now, right?
>
> If config is NULL, dangling_of_gpiod cannot be true.
>

It's true, thanks. Probably to avoid the known if(false) it might be better
to add a new goto label.

Also it seems to me that it's safe to enter regulator_dev_release() before
doing an assignment to rdev->dev.of_node and incrementing an of_node counter.

Reviewed-by: Vladimir Zapolskiy <[email protected]>

--
Best wishes,
Vladimir