linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/7] nvmem/gpio: fix resource management
@ 2020-02-18  9:42 Bartosz Golaszewski
  2020-02-18  9:42 ` [PATCH v2 1/7] nvmem: fix memory leak in error path Bartosz Golaszewski
                   ` (6 more replies)
  0 siblings, 7 replies; 16+ messages in thread
From: Bartosz Golaszewski @ 2020-02-18  9:42 UTC (permalink / raw)
  To: Linus Walleij, Srinivas Kandagatla, Khouloud Touil, Geert Uytterhoeven
  Cc: linux-gpio, linux-kernel, Bartosz Golaszewski

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

This series addresses a couple problems with memory management in nvmem
core.

First we fix two earlier memory leaks - this is obvious stable material.
Next we extend the GPIO framework to use reference counting for GPIO
descriptors. We then use it to fix the resource management problem with
the write-protect pin.

Finally we add some readability tweaks.

While the memory leak with wp-gpios is now in mainline - I'm not sure how
to go about applying the kref patch. This is theoretically a new feature
but it's also the cleanest way of fixing the problem.

v1 -> v2:
- make gpiod_ref() helper return 
- reorganize the series for easier merging
- fix another memory leak

Bartosz Golaszewski (6):
  nvmem: fix memory leak in error path
  nvmem: fix another memory leak in error path
  gpiolib: use kref in gpio_desc
  nvmem: increase the reference count of a gpio passed over config
  nvmem: remove a stray newline in nvmem_register()
  nvmem: add a newline for readability

Khouloud Touil (1):
  nvmem: release the write-protect pin

 drivers/gpio/gpiolib.c        | 30 +++++++++++++++++++++++++++---
 drivers/gpio/gpiolib.h        |  1 +
 drivers/nvmem/core.c          | 18 +++++++++++-------
 include/linux/gpio/consumer.h |  1 +
 4 files changed, 40 insertions(+), 10 deletions(-)

-- 
2.25.0


^ permalink raw reply	[flat|nested] 16+ messages in thread
* [PATCH v2 1/7] nvmem: fix memory leak in error path
  2020-02-18  9:42 [PATCH v2 0/7] nvmem/gpio: fix resource management Bartosz Golaszewski
@ 2020-02-18  9:42 ` Bartosz Golaszewski
  2020-02-18  9:42 ` [PATCH v2 2/7] nvmem: fix another " Bartosz Golaszewski
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 16+ messages in thread
From: Bartosz Golaszewski @ 2020-02-18  9:42 UTC (permalink / raw)
  To: Linus Walleij, Srinivas Kandagatla, Khouloud Touil, Geert Uytterhoeven
  Cc: linux-gpio, linux-kernel, Bartosz Golaszewski, stable

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

We need to remove the ida mapping when returning from nvmem_register()
with an error.

Cc: <stable@vger.kernel.org>
Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/nvmem/core.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index ef326f243f36..b0be03d5f240 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -353,7 +353,7 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
 		nvmem->wp_gpio = gpiod_get_optional(config->dev, "wp",
 						    GPIOD_OUT_HIGH);
 	if (IS_ERR(nvmem->wp_gpio))
-		return ERR_CAST(nvmem->wp_gpio);
+		goto err_ida_remove;
 
 
 	kref_init(&nvmem->refcnt);
@@ -430,6 +430,8 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
 	device_del(&nvmem->dev);
 err_put_device:
 	put_device(&nvmem->dev);
+err_ida_remove:
+	ida_simple_remove(&nvmem_ida, nvmem->id);
 
 	return ERR_PTR(rval);
 }
-- 
2.25.0


^ permalink raw reply related	[flat|nested] 16+ messages in thread
* [PATCH v2 2/7] nvmem: fix another memory leak in error path
  2020-02-18  9:42 [PATCH v2 0/7] nvmem/gpio: fix resource management Bartosz Golaszewski
  2020-02-18  9:42 ` [PATCH v2 1/7] nvmem: fix memory leak in error path Bartosz Golaszewski
@ 2020-02-18  9:42 ` Bartosz Golaszewski
  2020-02-18  9:47   ` Srinivas Kandagatla
  2020-02-18  9:56   ` Srinivas Kandagatla
  2020-02-18  9:42 ` [PATCH v2 3/7] gpiolib: use kref in gpio_desc Bartosz Golaszewski
                   ` (4 subsequent siblings)
  6 siblings, 2 replies; 16+ messages in thread
From: Bartosz Golaszewski @ 2020-02-18  9:42 UTC (permalink / raw)
  To: Linus Walleij, Srinivas Kandagatla, Khouloud Touil, Geert Uytterhoeven
  Cc: linux-gpio, linux-kernel, Bartosz Golaszewski, stable

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

The nvmem struct is only freed on the first error check after its
allocation and leaked after that. Fix it with a new label.

Cc: <stable@vger.kernel.org>
Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/nvmem/core.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index b0be03d5f240..c9b3f4047154 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -343,10 +343,8 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
 		return ERR_PTR(-ENOMEM);
 
 	rval  = ida_simple_get(&nvmem_ida, 0, 0, GFP_KERNEL);
-	if (rval < 0) {
-		kfree(nvmem);
-		return ERR_PTR(rval);
-	}
+	if (rval < 0)
+		goto err_free_nvmem;
 	if (config->wp_gpio)
 		nvmem->wp_gpio = config->wp_gpio;
 	else
@@ -432,6 +430,8 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
 	put_device(&nvmem->dev);
 err_ida_remove:
 	ida_simple_remove(&nvmem_ida, nvmem->id);
+err_free_nvmem:
+	kfree(nvmem);
 
 	return ERR_PTR(rval);
 }
-- 
2.25.0


^ permalink raw reply related	[flat|nested] 16+ messages in thread
* [PATCH v2 3/7] gpiolib: use kref in gpio_desc
  2020-02-18  9:42 [PATCH v2 0/7] nvmem/gpio: fix resource management Bartosz Golaszewski
  2020-02-18  9:42 ` [PATCH v2 1/7] nvmem: fix memory leak in error path Bartosz Golaszewski
  2020-02-18  9:42 ` [PATCH v2 2/7] nvmem: fix another " Bartosz Golaszewski
@ 2020-02-18  9:42 ` Bartosz Golaszewski
  2020-02-18  9:42 ` [PATCH v2 4/7] nvmem: increase the reference count of a gpio passed over config Bartosz Golaszewski
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 16+ messages in thread
From: Bartosz Golaszewski @ 2020-02-18  9:42 UTC (permalink / raw)
  To: Linus Walleij, Srinivas Kandagatla, Khouloud Touil, Geert Uytterhoeven
  Cc: linux-gpio, linux-kernel, Bartosz Golaszewski

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

GPIO descriptors are freed by consumers using gpiod_put(). The name of
this function suggests some reference counting is going on but it's not
true.

Use kref to actually introduce reference counting for gpio_desc objects.
Add a corresponding gpiod_get() helper for increasing the reference count.

This doesn't change anything for already existing (correct) drivers but
allows us to keep track of GPIO descs used by multiple users.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/gpio/gpiolib.c        | 30 +++++++++++++++++++++++++++---
 drivers/gpio/gpiolib.h        |  1 +
 include/linux/gpio/consumer.h |  1 +
 3 files changed, 29 insertions(+), 3 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 4d0106ceeba7..582425e9b449 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2798,6 +2798,8 @@ static int gpiod_request_commit(struct gpio_desc *desc, const char *label)
 		goto done;
 	}
 
+	kref_init(&desc->ref);
+
 	if (chip->request) {
 		/* chip->request may sleep */
 		spin_unlock_irqrestore(&gpio_lock, flags);
@@ -2933,6 +2935,13 @@ void gpiod_free(struct gpio_desc *desc)
 	}
 }
 
+static void gpiod_free_kref(struct kref *ref)
+{
+	struct gpio_desc *desc = container_of(ref, struct gpio_desc, ref);
+
+	gpiod_free(desc);
+}
+
 /**
  * gpiochip_is_requested - return string iff signal was requested
  * @chip: controller managing the signal
@@ -5067,18 +5076,33 @@ struct gpio_descs *__must_check gpiod_get_array_optional(struct device *dev,
 EXPORT_SYMBOL_GPL(gpiod_get_array_optional);
 
 /**
- * gpiod_put - dispose of a GPIO descriptor
- * @desc:	GPIO descriptor to dispose of
+ * gpiod_put - decrease the reference count of a GPIO descriptor
+ * @desc:	GPIO descriptor to unref
  *
  * No descriptor can be used after gpiod_put() has been called on it.
  */
 void gpiod_put(struct gpio_desc *desc)
 {
 	if (desc)
-		gpiod_free(desc);
+		kref_put(&desc->ref, gpiod_free_kref);
 }
 EXPORT_SYMBOL_GPL(gpiod_put);
 
+/**
+ * gpiod_ref - increase the reference count of a GPIO descriptor
+ * @desc:	GPIO descriptor to reference
+ *
+ * Returns the same gpio_desc after increasing the reference count.
+ */
+struct gpio_desc *gpiod_ref(struct gpio_desc *desc)
+{
+	if (desc)
+		kref_get(&desc->ref);
+
+	return desc;
+}
+EXPORT_SYMBOL_GPL(gpiod_ref);
+
 /**
  * gpiod_put_array - dispose of multiple GPIO descriptors
  * @descs:	struct gpio_descs containing an array of descriptors
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index 3e0aab2945d8..51a92c43dd55 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -119,6 +119,7 @@ struct gpio_desc {
 	const char		*label;
 	/* Name of the GPIO */
 	const char		*name;
+	struct kref		ref;
 };
 
 int gpiod_request(struct gpio_desc *desc, const char *label);
diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
index bf2d017dd7b7..c7b5fb3d9d64 100644
--- a/include/linux/gpio/consumer.h
+++ b/include/linux/gpio/consumer.h
@@ -81,6 +81,7 @@ struct gpio_descs *__must_check gpiod_get_array(struct device *dev,
 struct gpio_descs *__must_check gpiod_get_array_optional(struct device *dev,
 							const char *con_id,
 							enum gpiod_flags flags);
+struct gpio_desc *gpiod_ref(struct gpio_desc *desc);
 void gpiod_put(struct gpio_desc *desc);
 void gpiod_put_array(struct gpio_descs *descs);
 
-- 
2.25.0


^ permalink raw reply related	[flat|nested] 16+ messages in thread
* [PATCH v2 4/7] nvmem: increase the reference count of a gpio passed over config
  2020-02-18  9:42 [PATCH v2 0/7] nvmem/gpio: fix resource management Bartosz Golaszewski
                   ` (2 preceding siblings ...)
  2020-02-18  9:42 ` [PATCH v2 3/7] gpiolib: use kref in gpio_desc Bartosz Golaszewski
@ 2020-02-18  9:42 ` Bartosz Golaszewski
  2020-02-20  3:03   ` kbuild test robot
  2020-02-18  9:42 ` [PATCH v2 5/7] nvmem: release the write-protect pin Bartosz Golaszewski
                   ` (2 subsequent siblings)
  6 siblings, 1 reply; 16+ messages in thread
From: Bartosz Golaszewski @ 2020-02-18  9:42 UTC (permalink / raw)
  To: Linus Walleij, Srinivas Kandagatla, Khouloud Touil, Geert Uytterhoeven
  Cc: linux-gpio, linux-kernel, Bartosz Golaszewski

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

We can obtain the write-protect GPIO in nvmem_register() by requesting
it ourselves or by storing the gpio_desc passed in nvmem_config. In the
latter case we need to increase the reference count so that it gets
freed correctly.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/nvmem/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index c9b3f4047154..c4a6c044b020 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -346,7 +346,7 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
 	if (rval < 0)
 		goto err_free_nvmem;
 	if (config->wp_gpio)
-		nvmem->wp_gpio = config->wp_gpio;
+		nvmem->wp_gpio = gpiod_ref(config->wp_gpio);
 	else
 		nvmem->wp_gpio = gpiod_get_optional(config->dev, "wp",
 						    GPIOD_OUT_HIGH);
-- 
2.25.0


^ permalink raw reply related	[flat|nested] 16+ messages in thread
* [PATCH v2 5/7] nvmem: release the write-protect pin
  2020-02-18  9:42 [PATCH v2 0/7] nvmem/gpio: fix resource management Bartosz Golaszewski
                   ` (3 preceding siblings ...)
  2020-02-18  9:42 ` [PATCH v2 4/7] nvmem: increase the reference count of a gpio passed over config Bartosz Golaszewski
@ 2020-02-18  9:42 ` Bartosz Golaszewski
  2020-02-18 12:24   ` Srinivas Kandagatla
  2020-02-18  9:42 ` [PATCH v2 6/7] nvmem: remove a stray newline in nvmem_register() Bartosz Golaszewski
  2020-02-18  9:42 ` [PATCH v2 7/7] nvmem: add a newline for readability Bartosz Golaszewski
  6 siblings, 1 reply; 16+ messages in thread
From: Bartosz Golaszewski @ 2020-02-18  9:42 UTC (permalink / raw)
  To: Linus Walleij, Srinivas Kandagatla, Khouloud Touil, Geert Uytterhoeven
  Cc: linux-gpio, linux-kernel, Bartosz Golaszewski

From: Khouloud Touil <ktouil@baylibre.com>

Put the write-protect GPIO descriptor in nvmem_release() and in the
error path in nvmem_register().

Fixes: 2a127da461a9 ("nvmem: add support for the write-protect pin")
Reported-by: geert@linux-m68k.org
Signed-off-by: Khouloud Touil <ktouil@baylibre.com>
[Bartosz:
    - also put the gpio in error path in nvmem_register(),
    - tweak the commit message: the GPIO is not auto-released]
Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/nvmem/core.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index c4a6c044b020..e47152e9db34 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -72,6 +72,7 @@ static void nvmem_release(struct device *dev)
 	struct nvmem_device *nvmem = to_nvmem_device(dev);
 
 	ida_simple_remove(&nvmem_ida, nvmem->id);
+	gpiod_put(nvmem->wp_gpio);
 	kfree(nvmem);
 }
 
@@ -428,6 +429,7 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
 	device_del(&nvmem->dev);
 err_put_device:
 	put_device(&nvmem->dev);
+	gpiod_put(nvmem->wp_gpio);
 err_ida_remove:
 	ida_simple_remove(&nvmem_ida, nvmem->id);
 err_free_nvmem:
-- 
2.25.0


^ permalink raw reply related	[flat|nested] 16+ messages in thread
* [PATCH v2 6/7] nvmem: remove a stray newline in nvmem_register()
  2020-02-18  9:42 [PATCH v2 0/7] nvmem/gpio: fix resource management Bartosz Golaszewski
                   ` (4 preceding siblings ...)
  2020-02-18  9:42 ` [PATCH v2 5/7] nvmem: release the write-protect pin Bartosz Golaszewski
@ 2020-02-18  9:42 ` Bartosz Golaszewski
  2020-02-18  9:42 ` [PATCH v2 7/7] nvmem: add a newline for readability Bartosz Golaszewski
  6 siblings, 0 replies; 16+ messages in thread
From: Bartosz Golaszewski @ 2020-02-18  9:42 UTC (permalink / raw)
  To: Linus Walleij, Srinivas Kandagatla, Khouloud Touil, Geert Uytterhoeven
  Cc: linux-gpio, linux-kernel, Bartosz Golaszewski

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Two newlines are unnecessary - remove one.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/nvmem/core.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index e47152e9db34..5e7d46eccaf6 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -354,7 +354,6 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
 	if (IS_ERR(nvmem->wp_gpio))
 		goto err_ida_remove;
 
-
 	kref_init(&nvmem->refcnt);
 	INIT_LIST_HEAD(&nvmem->cells);
 
-- 
2.25.0


^ permalink raw reply related	[flat|nested] 16+ messages in thread
* [PATCH v2 7/7] nvmem: add a newline for readability
  2020-02-18  9:42 [PATCH v2 0/7] nvmem/gpio: fix resource management Bartosz Golaszewski
                   ` (5 preceding siblings ...)
  2020-02-18  9:42 ` [PATCH v2 6/7] nvmem: remove a stray newline in nvmem_register() Bartosz Golaszewski
@ 2020-02-18  9:42 ` Bartosz Golaszewski
  6 siblings, 0 replies; 16+ messages in thread
From: Bartosz Golaszewski @ 2020-02-18  9:42 UTC (permalink / raw)
  To: Linus Walleij, Srinivas Kandagatla, Khouloud Touil, Geert Uytterhoeven
  Cc: linux-gpio, linux-kernel, Bartosz Golaszewski

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Visibly separate the GPIO request from the previous operation in the
code with a newline.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/nvmem/core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 5e7d46eccaf6..ec955db4673c 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -346,6 +346,7 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
 	rval  = ida_simple_get(&nvmem_ida, 0, 0, GFP_KERNEL);
 	if (rval < 0)
 		goto err_free_nvmem;
+
 	if (config->wp_gpio)
 		nvmem->wp_gpio = gpiod_ref(config->wp_gpio);
 	else
-- 
2.25.0


^ permalink raw reply related	[flat|nested] 16+ messages in thread
* Re: [PATCH v2 2/7] nvmem: fix another memory leak in error path
  2020-02-18  9:42 ` [PATCH v2 2/7] nvmem: fix another " Bartosz Golaszewski
@ 2020-02-18  9:47   ` Srinivas Kandagatla
  2020-02-18  9:50     ` Bartosz Golaszewski
  2020-02-18  9:56   ` Srinivas Kandagatla
  1 sibling, 1 reply; 16+ messages in thread
From: Srinivas Kandagatla @ 2020-02-18  9:47 UTC (permalink / raw)
  To: Bartosz Golaszewski, Linus Walleij, Khouloud Touil, Geert Uytterhoeven
  Cc: linux-gpio, linux-kernel, Bartosz Golaszewski, stable



On 18/02/2020 09:42, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> The nvmem struct is only freed on the first error check after its
> allocation and leaked after that. Fix it with a new label.
> 
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>

looks like 1/7 introduced the bug and 2/7 fixes it.
IMO, you should 1/7 and 2/7 should be single patch.

--srini

> ---
>   drivers/nvmem/core.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> index b0be03d5f240..c9b3f4047154 100644
> --- a/drivers/nvmem/core.c
> +++ b/drivers/nvmem/core.c
> @@ -343,10 +343,8 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
>   		return ERR_PTR(-ENOMEM);
>   
>   	rval  = ida_simple_get(&nvmem_ida, 0, 0, GFP_KERNEL);
> -	if (rval < 0) {
> -		kfree(nvmem);
> -		return ERR_PTR(rval);
> -	}
> +	if (rval < 0)
> +		goto err_free_nvmem;
>   	if (config->wp_gpio)
>   		nvmem->wp_gpio = config->wp_gpio;
>   	else
> @@ -432,6 +430,8 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
>   	put_device(&nvmem->dev);
>   err_ida_remove:
>   	ida_simple_remove(&nvmem_ida, nvmem->id);
> +err_free_nvmem:
> +	kfree(nvmem);
>   
>   	return ERR_PTR(rval);
>   }
> 

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH v2 2/7] nvmem: fix another memory leak in error path
  2020-02-18  9:47   ` Srinivas Kandagatla
@ 2020-02-18  9:50     ` Bartosz Golaszewski
  0 siblings, 0 replies; 16+ messages in thread
From: Bartosz Golaszewski @ 2020-02-18  9:50 UTC (permalink / raw)
  To: Srinivas Kandagatla
  Cc: Bartosz Golaszewski, Linus Walleij, Khouloud Touil,
	Geert Uytterhoeven, linux-gpio, LKML, Stable # 4 . 20+

wt., 18 lut 2020 o 10:47 Srinivas Kandagatla
<srinivas.kandagatla@linaro.org> napisał(a):
>
>
>
> On 18/02/2020 09:42, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >
> > The nvmem struct is only freed on the first error check after its
> > allocation and leaked after that. Fix it with a new label.
> >
> > Cc: <stable@vger.kernel.org>
> > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> looks like 1/7 introduced the bug and 2/7 fixes it.
> IMO, you should 1/7 and 2/7 should be single patch.
>

The bug already exists in mainline - the nvmem object is only freed if
ida_simple_get() fails, but any subsequent error leads to a memory
leak. In other words: it doesn't matter if it's a single patch or two.

Bart

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH v2 2/7] nvmem: fix another memory leak in error path
  2020-02-18  9:42 ` [PATCH v2 2/7] nvmem: fix another " Bartosz Golaszewski
  2020-02-18  9:47   ` Srinivas Kandagatla
@ 2020-02-18  9:56   ` Srinivas Kandagatla
  2020-02-18 10:05     ` Bartosz Golaszewski
  1 sibling, 1 reply; 16+ messages in thread
From: Srinivas Kandagatla @ 2020-02-18  9:56 UTC (permalink / raw)
  To: Bartosz Golaszewski, Linus Walleij, Khouloud Touil, Geert Uytterhoeven
  Cc: linux-gpio, linux-kernel, Bartosz Golaszewski, stable



On 18/02/2020 09:42, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> The nvmem struct is only freed on the first error check after its
> allocation and leaked after that. Fix it with a new label.
> 
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
>   drivers/nvmem/core.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> index b0be03d5f240..c9b3f4047154 100644
> --- a/drivers/nvmem/core.c
> +++ b/drivers/nvmem/core.c
> @@ -343,10 +343,8 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
>   		return ERR_PTR(-ENOMEM);
>   
>   	rval  = ida_simple_get(&nvmem_ida, 0, 0, GFP_KERNEL);
> -	if (rval < 0) {
> -		kfree(nvmem);
> -		return ERR_PTR(rval);
> -	}
> +	if (rval < 0)
> +		goto err_free_nvmem;
>   	if (config->wp_gpio)
>   		nvmem->wp_gpio = config->wp_gpio;
>   	else
> @@ -432,6 +430,8 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
>   	put_device(&nvmem->dev);
>   err_ida_remove:
>   	ida_simple_remove(&nvmem_ida, nvmem->id);
> +err_free_nvmem:
> +	kfree(nvmem);

This is not correct fix to start with, if the device has already been 
intialized before jumping here then nvmem would be freed as part of 
nvmem_release().

So the bug was actually introduced by adding err_ida_remove label.

You can free nvmem at that point but not at any point after that as 
device core would be holding a reference to it.

--srini



>   
>   	return ERR_PTR(rval);
>   }
> 

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH v2 2/7] nvmem: fix another memory leak in error path
  2020-02-18  9:56   ` Srinivas Kandagatla
@ 2020-02-18 10:05     ` Bartosz Golaszewski
  2020-02-18 10:11       ` Srinivas Kandagatla
  0 siblings, 1 reply; 16+ messages in thread
From: Bartosz Golaszewski @ 2020-02-18 10:05 UTC (permalink / raw)
  To: Srinivas Kandagatla
  Cc: Linus Walleij, Khouloud Touil, Geert Uytterhoeven,
	open list:GPIO SUBSYSTEM, Linux Kernel Mailing List,
	Bartosz Golaszewski, stable

wt., 18 lut 2020 o 10:56 Srinivas Kandagatla
<srinivas.kandagatla@linaro.org> napisał(a):
>
>
>
> On 18/02/2020 09:42, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >
> > The nvmem struct is only freed on the first error check after its
> > allocation and leaked after that. Fix it with a new label.
> >
> > Cc: <stable@vger.kernel.org>
> > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > ---
> >   drivers/nvmem/core.c | 8 ++++----
> >   1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> > index b0be03d5f240..c9b3f4047154 100644
> > --- a/drivers/nvmem/core.c
> > +++ b/drivers/nvmem/core.c
> > @@ -343,10 +343,8 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
> >               return ERR_PTR(-ENOMEM);
> >
> >       rval  = ida_simple_get(&nvmem_ida, 0, 0, GFP_KERNEL);
> > -     if (rval < 0) {
> > -             kfree(nvmem);
> > -             return ERR_PTR(rval);
> > -     }
> > +     if (rval < 0)
> > +             goto err_free_nvmem;
> >       if (config->wp_gpio)
> >               nvmem->wp_gpio = config->wp_gpio;
> >       else
> > @@ -432,6 +430,8 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
> >       put_device(&nvmem->dev);
> >   err_ida_remove:
> >       ida_simple_remove(&nvmem_ida, nvmem->id);
> > +err_free_nvmem:
> > +     kfree(nvmem);
>
> This is not correct fix to start with, if the device has already been
> intialized before jumping here then nvmem would be freed as part of
> nvmem_release().
>
> So the bug was actually introduced by adding err_ida_remove label.
>
> You can free nvmem at that point but not at any point after that as
> device core would be holding a reference to it.
>

OK I see - I missed the release() callback assignment. Frankly: I find
this split of resource management responsibility confusing. Since the
users are expected to call nvmem_unregister() anyway - wouldn't it be
more clear to just free all resources there? What is the advantage of
defining the release() callback for device type here?

Bartosz

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH v2 2/7] nvmem: fix another memory leak in error path
  2020-02-18 10:05     ` Bartosz Golaszewski
@ 2020-02-18 10:11       ` Srinivas Kandagatla
  2020-02-18 10:22         ` Bartosz Golaszewski
  0 siblings, 1 reply; 16+ messages in thread
From: Srinivas Kandagatla @ 2020-02-18 10:11 UTC (permalink / raw)
  To: Bartosz Golaszewski
  Cc: Linus Walleij, Khouloud Touil, Geert Uytterhoeven,
	open list:GPIO SUBSYSTEM, Linux Kernel Mailing List,
	Bartosz Golaszewski, stable



On 18/02/2020 10:05, Bartosz Golaszewski wrote:
> wt., 18 lut 2020 o 10:56 Srinivas Kandagatla
> <srinivas.kandagatla@linaro.org> napisał(a):
>>
>>
>>
>> On 18/02/2020 09:42, Bartosz Golaszewski wrote:
>>> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>>>
>>> The nvmem struct is only freed on the first error check after its
>>> allocation and leaked after that. Fix it with a new label.
>>>
>>> Cc: <stable@vger.kernel.org>
>>> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>>> ---
>>>    drivers/nvmem/core.c | 8 ++++----
>>>    1 file changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
>>> index b0be03d5f240..c9b3f4047154 100644
>>> --- a/drivers/nvmem/core.c
>>> +++ b/drivers/nvmem/core.c
>>> @@ -343,10 +343,8 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
>>>                return ERR_PTR(-ENOMEM);
>>>
>>>        rval  = ida_simple_get(&nvmem_ida, 0, 0, GFP_KERNEL);
>>> -     if (rval < 0) {
>>> -             kfree(nvmem);
>>> -             return ERR_PTR(rval);
>>> -     }
>>> +     if (rval < 0)
>>> +             goto err_free_nvmem;
>>>        if (config->wp_gpio)
>>>                nvmem->wp_gpio = config->wp_gpio;
>>>        else
>>> @@ -432,6 +430,8 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
>>>        put_device(&nvmem->dev);
>>>    err_ida_remove:
>>>        ida_simple_remove(&nvmem_ida, nvmem->id);
>>> +err_free_nvmem:
>>> +     kfree(nvmem);
>>
>> This is not correct fix to start with, if the device has already been
>> intialized before jumping here then nvmem would be freed as part of
>> nvmem_release().
>>
>> So the bug was actually introduced by adding err_ida_remove label.
>>
>> You can free nvmem at that point but not at any point after that as
>> device core would be holding a reference to it.
>>
> 
> OK I see - I missed the release() callback assignment. Frankly: I find
> this split of resource management responsibility confusing. Since the
> users are expected to call nvmem_unregister() anyway - wouldn't it be
> more clear to just free all resources there? What is the advantage of
> defining the release() callback for device type here?

Because we are using dev pointer from nvmem structure, and this dev 
pointer should be valid until release callback is invoked.

Freeing nvmem at any early stage would make dev pointer invalid and 
device core would dereference it!

--srini
> 
> Bartosz
> 

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH v2 2/7] nvmem: fix another memory leak in error path
  2020-02-18 10:11       ` Srinivas Kandagatla
@ 2020-02-18 10:22         ` Bartosz Golaszewski
  0 siblings, 0 replies; 16+ messages in thread
From: Bartosz Golaszewski @ 2020-02-18 10:22 UTC (permalink / raw)
  To: Srinivas Kandagatla
  Cc: Bartosz Golaszewski, Linus Walleij, Khouloud Touil,
	Geert Uytterhoeven, open list:GPIO SUBSYSTEM,
	Linux Kernel Mailing List, stable

wt., 18 lut 2020 o 11:11 Srinivas Kandagatla
<srinivas.kandagatla@linaro.org> napisał(a):
>
>
>
> On 18/02/2020 10:05, Bartosz Golaszewski wrote:
> > wt., 18 lut 2020 o 10:56 Srinivas Kandagatla
> > <srinivas.kandagatla@linaro.org> napisał(a):
> >>
> >>
> >>
> >> On 18/02/2020 09:42, Bartosz Golaszewski wrote:
> >>> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >>>
> >>> The nvmem struct is only freed on the first error check after its
> >>> allocation and leaked after that. Fix it with a new label.
> >>>
> >>> Cc: <stable@vger.kernel.org>
> >>> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >>> ---
> >>>    drivers/nvmem/core.c | 8 ++++----
> >>>    1 file changed, 4 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> >>> index b0be03d5f240..c9b3f4047154 100644
> >>> --- a/drivers/nvmem/core.c
> >>> +++ b/drivers/nvmem/core.c
> >>> @@ -343,10 +343,8 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
> >>>                return ERR_PTR(-ENOMEM);
> >>>
> >>>        rval  = ida_simple_get(&nvmem_ida, 0, 0, GFP_KERNEL);
> >>> -     if (rval < 0) {
> >>> -             kfree(nvmem);
> >>> -             return ERR_PTR(rval);
> >>> -     }
> >>> +     if (rval < 0)
> >>> +             goto err_free_nvmem;
> >>>        if (config->wp_gpio)
> >>>                nvmem->wp_gpio = config->wp_gpio;
> >>>        else
> >>> @@ -432,6 +430,8 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
> >>>        put_device(&nvmem->dev);
> >>>    err_ida_remove:
> >>>        ida_simple_remove(&nvmem_ida, nvmem->id);
> >>> +err_free_nvmem:
> >>> +     kfree(nvmem);
> >>
> >> This is not correct fix to start with, if the device has already been
> >> intialized before jumping here then nvmem would be freed as part of
> >> nvmem_release().
> >>
> >> So the bug was actually introduced by adding err_ida_remove label.
> >>
> >> You can free nvmem at that point but not at any point after that as
> >> device core would be holding a reference to it.
> >>
> >
> > OK I see - I missed the release() callback assignment. Frankly: I find
> > this split of resource management responsibility confusing. Since the
> > users are expected to call nvmem_unregister() anyway - wouldn't it be
> > more clear to just free all resources there? What is the advantage of
> > defining the release() callback for device type here?
>
> Because we are using dev pointer from nvmem structure, and this dev
> pointer should be valid until release callback is invoked.
>
> Freeing nvmem at any early stage would make dev pointer invalid and
> device core would dereference it!
>

Ok, let me brew up a v3 with that in mind.

Bart

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH v2 5/7] nvmem: release the write-protect pin
  2020-02-18  9:42 ` [PATCH v2 5/7] nvmem: release the write-protect pin Bartosz Golaszewski
@ 2020-02-18 12:24   ` Srinivas Kandagatla
  0 siblings, 0 replies; 16+ messages in thread
From: Srinivas Kandagatla @ 2020-02-18 12:24 UTC (permalink / raw)
  To: Bartosz Golaszewski, Linus Walleij, Khouloud Touil, Geert Uytterhoeven
  Cc: linux-gpio, linux-kernel, Bartosz Golaszewski



On 18/02/2020 09:42, Bartosz Golaszewski wrote:
> @@ -428,6 +429,7 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
>   	device_del(&nvmem->dev);
>   err_put_device:
>   	put_device(&nvmem->dev);
> +	gpiod_put(nvmem->wp_gpio);
>   err_ida_remove:
>   	ida_simple_remove(&nvmem_ida, nvmem->id);
>   err_free_nvmem:

This is also redundant as explained in my previous comments about release()

--srini


> -- 

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH v2 4/7] nvmem: increase the reference count of a gpio passed over config
  2020-02-18  9:42 ` [PATCH v2 4/7] nvmem: increase the reference count of a gpio passed over config Bartosz Golaszewski
@ 2020-02-20  3:03   ` kbuild test robot
  0 siblings, 0 replies; 16+ messages in thread
From: kbuild test robot @ 2020-02-20  3:03 UTC (permalink / raw)
  To: Bartosz Golaszewski
  Cc: kbuild-all, Linus Walleij, Srinivas Kandagatla, Khouloud Touil,
	Geert Uytterhoeven, linux-gpio, linux-kernel,
	Bartosz Golaszewski

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

Hi Bartosz,

I love your patch! Perhaps something to improve:

[auto build test WARNING on gpio/for-next]
[also build test WARNING on linus/master v5.6-rc2]
[cannot apply to next-20200219]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Bartosz-Golaszewski/nvmem-gpio-fix-resource-management/20200220-045651
base:   https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.git for-next
config: nios2-3c120_defconfig (attached as .config)
compiler: nios2-linux-gcc (GCC) 7.5.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.5.0 make.cross ARCH=nios2 

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

All warnings (new ones prefixed by >>):

   drivers/nvmem/core.c: In function 'nvmem_register':
   drivers/nvmem/core.c:349:20: error: implicit declaration of function 'gpiod_ref'; did you mean 'gpiod_get'? [-Werror=implicit-function-declaration]
      nvmem->wp_gpio = gpiod_ref(config->wp_gpio);
                       ^~~~~~~~~
                       gpiod_get
>> drivers/nvmem/core.c:349:18: warning: assignment makes pointer from integer without a cast [-Wint-conversion]
      nvmem->wp_gpio = gpiod_ref(config->wp_gpio);
                     ^
   cc1: some warnings being treated as errors

vim +349 drivers/nvmem/core.c

   322	
   323	/**
   324	 * nvmem_register() - Register a nvmem device for given nvmem_config.
   325	 * Also creates an binary entry in /sys/bus/nvmem/devices/dev-name/nvmem
   326	 *
   327	 * @config: nvmem device configuration with which nvmem device is created.
   328	 *
   329	 * Return: Will be an ERR_PTR() on error or a valid pointer to nvmem_device
   330	 * on success.
   331	 */
   332	
   333	struct nvmem_device *nvmem_register(const struct nvmem_config *config)
   334	{
   335		struct nvmem_device *nvmem;
   336		int rval;
   337	
   338		if (!config->dev)
   339			return ERR_PTR(-EINVAL);
   340	
   341		nvmem = kzalloc(sizeof(*nvmem), GFP_KERNEL);
   342		if (!nvmem)
   343			return ERR_PTR(-ENOMEM);
   344	
   345		rval  = ida_simple_get(&nvmem_ida, 0, 0, GFP_KERNEL);
   346		if (rval < 0)
   347			goto err_free_nvmem;
   348		if (config->wp_gpio)
 > 349			nvmem->wp_gpio = gpiod_ref(config->wp_gpio);
   350		else
   351			nvmem->wp_gpio = gpiod_get_optional(config->dev, "wp",
   352							    GPIOD_OUT_HIGH);
   353		if (IS_ERR(nvmem->wp_gpio))
   354			goto err_ida_remove;
   355	
   356	
   357		kref_init(&nvmem->refcnt);
   358		INIT_LIST_HEAD(&nvmem->cells);
   359	
   360		nvmem->id = rval;
   361		nvmem->owner = config->owner;
   362		if (!nvmem->owner && config->dev->driver)
   363			nvmem->owner = config->dev->driver->owner;
   364		nvmem->stride = config->stride ?: 1;
   365		nvmem->word_size = config->word_size ?: 1;
   366		nvmem->size = config->size;
   367		nvmem->dev.type = &nvmem_provider_type;
   368		nvmem->dev.bus = &nvmem_bus_type;
   369		nvmem->dev.parent = config->dev;
   370		nvmem->priv = config->priv;
   371		nvmem->type = config->type;
   372		nvmem->reg_read = config->reg_read;
   373		nvmem->reg_write = config->reg_write;
   374		if (!config->no_of_node)
   375			nvmem->dev.of_node = config->dev->of_node;
   376	
   377		if (config->id == -1 && config->name) {
   378			dev_set_name(&nvmem->dev, "%s", config->name);
   379		} else {
   380			dev_set_name(&nvmem->dev, "%s%d",
   381				     config->name ? : "nvmem",
   382				     config->name ? config->id : nvmem->id);
   383		}
   384	
   385		nvmem->read_only = device_property_present(config->dev, "read-only") ||
   386				   config->read_only || !nvmem->reg_write;
   387	
   388		nvmem->dev.groups = nvmem_sysfs_get_groups(nvmem, config);
   389	
   390		device_initialize(&nvmem->dev);
   391	
   392		dev_dbg(&nvmem->dev, "Registering nvmem device %s\n", config->name);
   393	
   394		rval = device_add(&nvmem->dev);
   395		if (rval)
   396			goto err_put_device;
   397	
   398		if (config->compat) {
   399			rval = nvmem_sysfs_setup_compat(nvmem, config);
   400			if (rval)
   401				goto err_device_del;
   402		}
   403	
   404		if (config->cells) {
   405			rval = nvmem_add_cells(nvmem, config->cells, config->ncells);
   406			if (rval)
   407				goto err_teardown_compat;
   408		}
   409	
   410		rval = nvmem_add_cells_from_table(nvmem);
   411		if (rval)
   412			goto err_remove_cells;
   413	
   414		rval = nvmem_add_cells_from_of(nvmem);
   415		if (rval)
   416			goto err_remove_cells;
   417	
   418		blocking_notifier_call_chain(&nvmem_notifier, NVMEM_ADD, nvmem);
   419	
   420		return nvmem;
   421	
   422	err_remove_cells:
   423		nvmem_device_remove_all_cells(nvmem);
   424	err_teardown_compat:
   425		if (config->compat)
   426			nvmem_sysfs_remove_compat(nvmem, config);
   427	err_device_del:
   428		device_del(&nvmem->dev);
   429	err_put_device:
   430		put_device(&nvmem->dev);
   431	err_ida_remove:
   432		ida_simple_remove(&nvmem_ida, nvmem->id);
   433	err_free_nvmem:
   434		kfree(nvmem);
   435	
   436		return ERR_PTR(rval);
   437	}
   438	EXPORT_SYMBOL_GPL(nvmem_register);
   439	

---
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: 9676 bytes --]

^ permalink raw reply	[flat|nested] 16+ messages in thread
end of thread, other threads:[~2020-02-20  3:04 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-18  9:42 [PATCH v2 0/7] nvmem/gpio: fix resource management Bartosz Golaszewski
2020-02-18  9:42 ` [PATCH v2 1/7] nvmem: fix memory leak in error path Bartosz Golaszewski
2020-02-18  9:42 ` [PATCH v2 2/7] nvmem: fix another " Bartosz Golaszewski
2020-02-18  9:47   ` Srinivas Kandagatla
2020-02-18  9:50     ` Bartosz Golaszewski
2020-02-18  9:56   ` Srinivas Kandagatla
2020-02-18 10:05     ` Bartosz Golaszewski
2020-02-18 10:11       ` Srinivas Kandagatla
2020-02-18 10:22         ` Bartosz Golaszewski
2020-02-18  9:42 ` [PATCH v2 3/7] gpiolib: use kref in gpio_desc Bartosz Golaszewski
2020-02-18  9:42 ` [PATCH v2 4/7] nvmem: increase the reference count of a gpio passed over config Bartosz Golaszewski
2020-02-20  3:03   ` kbuild test robot
2020-02-18  9:42 ` [PATCH v2 5/7] nvmem: release the write-protect pin Bartosz Golaszewski
2020-02-18 12:24   ` Srinivas Kandagatla
2020-02-18  9:42 ` [PATCH v2 6/7] nvmem: remove a stray newline in nvmem_register() Bartosz Golaszewski
2020-02-18  9:42 ` [PATCH v2 7/7] nvmem: add a newline for readability Bartosz Golaszewski

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).