From: Bartosz Golaszewski <[email protected]>
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
From: Bartosz Golaszewski <[email protected]>
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: <[email protected]>
Signed-off-by: Bartosz Golaszewski <[email protected]>
---
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
On 18/02/2020 09:42, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <[email protected]>
>
> 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: <[email protected]>
> Signed-off-by: Bartosz Golaszewski <[email protected]>
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);
> }
>
wt., 18 lut 2020 o 10:47 Srinivas Kandagatla
<[email protected]> napisał(a):
>
>
>
> On 18/02/2020 09:42, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <[email protected]>
> >
> > 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: <[email protected]>
> > Signed-off-by: Bartosz Golaszewski <[email protected]>
>
> 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
On 18/02/2020 09:42, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <[email protected]>
>
> 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: <[email protected]>
> Signed-off-by: Bartosz Golaszewski <[email protected]>
> ---
> 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);
> }
>
wt., 18 lut 2020 o 10:56 Srinivas Kandagatla
<[email protected]> napisał(a):
>
>
>
> On 18/02/2020 09:42, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <[email protected]>
> >
> > 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: <[email protected]>
> > Signed-off-by: Bartosz Golaszewski <[email protected]>
> > ---
> > 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
On 18/02/2020 10:05, Bartosz Golaszewski wrote:
> wt., 18 lut 2020 o 10:56 Srinivas Kandagatla
> <[email protected]> napisał(a):
>>
>>
>>
>> On 18/02/2020 09:42, Bartosz Golaszewski wrote:
>>> From: Bartosz Golaszewski <[email protected]>
>>>
>>> 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: <[email protected]>
>>> Signed-off-by: Bartosz Golaszewski <[email protected]>
>>> ---
>>> 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
>
wt., 18 lut 2020 o 11:11 Srinivas Kandagatla
<[email protected]> napisał(a):
>
>
>
> On 18/02/2020 10:05, Bartosz Golaszewski wrote:
> > wt., 18 lut 2020 o 10:56 Srinivas Kandagatla
> > <[email protected]> napisał(a):
> >>
> >>
> >>
> >> On 18/02/2020 09:42, Bartosz Golaszewski wrote:
> >>> From: Bartosz Golaszewski <[email protected]>
> >>>
> >>> 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: <[email protected]>
> >>> Signed-off-by: Bartosz Golaszewski <[email protected]>
> >>> ---
> >>> 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