| Message ID | 20230926184520.2239723-2-michal.wilczynski@intel.com (mailing list archive) |
|---|---|
| State | Rejected, archived |
| Headers | show |
| Series | Fix memory leak and move to modern scope based rollback | expand |
On 9/26/23 11:45, Michal Wilczynski wrote: > devm_*() family of functions purpose is managing memory attached to a > device. So in general it should only be used for allocations that should > last for the whole lifecycle of the device. This is not the case for > acpi_nfit_init_interleave_set(). There are two allocations that are only > used locally in this function. What's more - if the function exits on > error path memory is never freed. It's still attached to dev and would > be freed on device detach, so this leak could be called a 'local leak'. > > Fix this by switching from devm_kcalloc() to kcalloc(), and adding > proper rollback. > > Fixes: eaf961536e16 ("libnvdimm, nfit: add interleave-set state-tracking infrastructure") > Reported-by: Andy Shevchenko <andy.shevchenko@gmail.com> > Signed-off-by: Michal Wilczynski <michal.wilczynski@intel.com> Reviewed-by: Dave Jiang <dave.jiang@intel.com> > --- > drivers/acpi/nfit/core.c | 23 +++++++++++++++-------- > 1 file changed, 15 insertions(+), 8 deletions(-) > > diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c > index f0e6738ae3c9..78f0f855c4de 100644 > --- a/drivers/acpi/nfit/core.c > +++ b/drivers/acpi/nfit/core.c > @@ -2262,6 +2262,7 @@ static int acpi_nfit_init_interleave_set(struct acpi_nfit_desc *acpi_desc, > u16 nr = ndr_desc->num_mappings; > struct nfit_set_info2 *info2; > struct nfit_set_info *info; > + int err = 0; > int i; > > nd_set = devm_kzalloc(dev, sizeof(*nd_set), GFP_KERNEL); > @@ -2269,13 +2270,15 @@ static int acpi_nfit_init_interleave_set(struct acpi_nfit_desc *acpi_desc, > return -ENOMEM; > import_guid(&nd_set->type_guid, spa->range_guid); > > - info = devm_kcalloc(dev, nr, sizeof(*info), GFP_KERNEL); > + info = kcalloc(nr, sizeof(*info), GFP_KERNEL); > if (!info) > return -ENOMEM; > > - info2 = devm_kcalloc(dev, nr, sizeof(*info2), GFP_KERNEL); > - if (!info2) > - return -ENOMEM; > + info2 = kcalloc(nr, sizeof(*info2), GFP_KERNEL); > + if (!info2) { > + err = -ENOMEM; > + goto free_info; > + } > > for (i = 0; i < nr; i++) { > struct nd_mapping_desc *mapping = &ndr_desc->mapping[i]; > @@ -2289,7 +2292,8 @@ static int acpi_nfit_init_interleave_set(struct acpi_nfit_desc *acpi_desc, > > if (!memdev || !nfit_mem->dcr) { > dev_err(dev, "%s: failed to find DCR\n", __func__); > - return -ENODEV; > + err = -ENODEV; > + goto free_info2; > } > > map->region_offset = memdev->region_offset; > @@ -2337,10 +2341,13 @@ static int acpi_nfit_init_interleave_set(struct acpi_nfit_desc *acpi_desc, > } > > ndr_desc->nd_set = nd_set; > - devm_kfree(dev, info); > - devm_kfree(dev, info2); > > - return 0; > +free_info2: > + kfree(info2); > +free_info: > + kfree(info); > + > + return err; > } > > static int ars_get_cap(struct acpi_nfit_desc *acpi_desc,
On Tue, Sep 26, 2023 at 09:45:19PM +0300, Michal Wilczynski wrote: > devm_*() family of functions purpose is managing memory attached to a > device. So in general it should only be used for allocations that should > last for the whole lifecycle of the device. This is not the case for > acpi_nfit_init_interleave_set(). There are two allocations that are only > used locally in this function. What's more - if the function exits on > error path memory is never freed. It's still attached to dev and would > be freed on device detach, so this leak could be called a 'local leak'. > > Fix this by switching from devm_kcalloc() to kcalloc(), and adding > proper rollback. LGTM, Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Michal Wilczynski wrote: > devm_*() family of functions purpose is managing memory attached to a > device. So in general it should only be used for allocations that should > last for the whole lifecycle of the device. This is not the case for > acpi_nfit_init_interleave_set(). There are two allocations that are only > used locally in this function. What's more - if the function exits on > error path memory is never freed. It's still attached to dev and would > be freed on device detach, so this leak could be called a 'local leak'. This analysis is incorrect devm cleans up on driver ->probe() failure in addition to ->remove(), and these error returns result in ->probe() failures. No leak, i.e. this is not a fix. The conversion to modern probe is ok if you want to resubmit that one without this intervening change.
diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c index f0e6738ae3c9..78f0f855c4de 100644 --- a/drivers/acpi/nfit/core.c +++ b/drivers/acpi/nfit/core.c @@ -2262,6 +2262,7 @@ static int acpi_nfit_init_interleave_set(struct acpi_nfit_desc *acpi_desc, u16 nr = ndr_desc->num_mappings; struct nfit_set_info2 *info2; struct nfit_set_info *info; + int err = 0; int i; nd_set = devm_kzalloc(dev, sizeof(*nd_set), GFP_KERNEL); @@ -2269,13 +2270,15 @@ static int acpi_nfit_init_interleave_set(struct acpi_nfit_desc *acpi_desc, return -ENOMEM; import_guid(&nd_set->type_guid, spa->range_guid); - info = devm_kcalloc(dev, nr, sizeof(*info), GFP_KERNEL); + info = kcalloc(nr, sizeof(*info), GFP_KERNEL); if (!info) return -ENOMEM; - info2 = devm_kcalloc(dev, nr, sizeof(*info2), GFP_KERNEL); - if (!info2) - return -ENOMEM; + info2 = kcalloc(nr, sizeof(*info2), GFP_KERNEL); + if (!info2) { + err = -ENOMEM; + goto free_info; + } for (i = 0; i < nr; i++) { struct nd_mapping_desc *mapping = &ndr_desc->mapping[i]; @@ -2289,7 +2292,8 @@ static int acpi_nfit_init_interleave_set(struct acpi_nfit_desc *acpi_desc, if (!memdev || !nfit_mem->dcr) { dev_err(dev, "%s: failed to find DCR\n", __func__); - return -ENODEV; + err = -ENODEV; + goto free_info2; } map->region_offset = memdev->region_offset; @@ -2337,10 +2341,13 @@ static int acpi_nfit_init_interleave_set(struct acpi_nfit_desc *acpi_desc, } ndr_desc->nd_set = nd_set; - devm_kfree(dev, info); - devm_kfree(dev, info2); - return 0; +free_info2: + kfree(info2); +free_info: + kfree(info); + + return err; } static int ars_get_cap(struct acpi_nfit_desc *acpi_desc,
devm_*() family of functions purpose is managing memory attached to a device. So in general it should only be used for allocations that should last for the whole lifecycle of the device. This is not the case for acpi_nfit_init_interleave_set(). There are two allocations that are only used locally in this function. What's more - if the function exits on error path memory is never freed. It's still attached to dev and would be freed on device detach, so this leak could be called a 'local leak'. Fix this by switching from devm_kcalloc() to kcalloc(), and adding proper rollback. Fixes: eaf961536e16 ("libnvdimm, nfit: add interleave-set state-tracking infrastructure") Reported-by: Andy Shevchenko <andy.shevchenko@gmail.com> Signed-off-by: Michal Wilczynski <michal.wilczynski@intel.com> --- drivers/acpi/nfit/core.c | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-)