[PATCH v2] ACPI: NFIT: Fix local use of devm_*()

[PATCH v2] ACPI: NFIT: Fix local use of devm_*()

[Michal Wilczynski]

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.

Fix this by switching from devm_kcalloc() to kcalloc(), and adding
modern scope based rollback. This is similar to C++ RAII and is
preferred way for handling local memory allocations.

Reported-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Suggested-by: Dave Jiang <dave.jiang@intel.com>
Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Reviewed-by: Dave Jiang <dave.jiang@intel.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Michal Wilczynski <michal.wilczynski@intel.com>
---
v2:
 - removed first commit from the patchset, as the commit couldn't
   be marked as a fix
 - squashed those commits together, since the second one were
   mostly overwriting the previous one

 drivers/acpi/nfit/core.c | 21 ++++++++-------------
 1 file changed, 8 insertions(+), 13 deletions(-)

diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
index 3826f49d481b..67a844a705c4 100644
--- a/drivers/acpi/nfit/core.c
+++ b/drivers/acpi/nfit/core.c
@@ -2257,26 +2257,23 @@ static int acpi_nfit_init_interleave_set(struct acpi_nfit_desc *acpi_desc,
 		struct nd_region_desc *ndr_desc,
 		struct acpi_nfit_system_address *spa)
 {
+	u16 nr = ndr_desc->num_mappings;
+	struct nfit_set_info2 *info2 __free(kfree) =
+		kcalloc(nr, sizeof(*info2), GFP_KERNEL);
+	struct nfit_set_info *info __free(kfree) =
+		kcalloc(nr, sizeof(*info), GFP_KERNEL);
 	struct device *dev = acpi_desc->dev;
 	struct nd_interleave_set *nd_set;
-	u16 nr = ndr_desc->num_mappings;
-	struct nfit_set_info2 *info2;
-	struct nfit_set_info *info;
 	int i;
 
+	if (!info || !info2)
+		return -ENOMEM;
+
 	nd_set = devm_kzalloc(dev, sizeof(*nd_set), GFP_KERNEL);
 	if (!nd_set)
 		return -ENOMEM;
 	import_guid(&nd_set->type_guid, spa->range_guid);
 
-	info = devm_kcalloc(dev, nr, sizeof(*info), GFP_KERNEL);
-	if (!info)
-		return -ENOMEM;
-
-	info2 = devm_kcalloc(dev, nr, sizeof(*info2), GFP_KERNEL);
-	if (!info2)
-		return -ENOMEM;
-
 	for (i = 0; i < nr; i++) {
 		struct nd_mapping_desc *mapping = &ndr_desc->mapping[i];
 		struct nvdimm *nvdimm = mapping->nvdimm;
@@ -2337,8 +2334,6 @@ 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;
 }
-- 
2.41.0

Re: [PATCH v2] ACPI: NFIT: Fix local use of devm_*()

[Dan Williams]

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. 

No, this assertion is not accurate, if it were strictly true then
devm_kfree() should be deleted. This patch is only a cleanup to switch
the automatic cleanup pattern from devm to the new cleanup.h helpers.

I am all for modernizing code over time, but patches that make
assertions of "memory leaks" and "incorrect API usage" in code that has
been untouched for almost a decade demand more scrutiny than what
transpired here.

Re: [PATCH v2] ACPI: NFIT: Fix local use of devm_*()

[Wilczynski, Michal]

On 10/13/2023 6:38 PM, Dan Williams wrote:
> 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. 
> No, this assertion is not accurate, if it were strictly true then
> devm_kfree() should be deleted. This patch is only a cleanup to switch
> the automatic cleanup pattern from devm to the new cleanup.h helpers.

The memory in question is only used locally in a function, so there is no reason
to use devm_*() family of functions. I think devm_kfree() is more for special
cases where the memory is meant to be used for the whole lifecycle of device,
but some special case occurs and it's not and it needs to be freed.

This is an incorrect API usage. Would you propose to change all memory
allocations currently being done to devm_*() family simply because devm_kfree()
exists ? Why introduce extra overhead if you don't have to ?

>
> I am all for modernizing code over time, but patches that make
> assertions of "memory leaks" and "incorrect API usage" in code that has
> been untouched for almost a decade demand more scrutiny than what
> transpired here.

I understand that it's not necessarily a big problem, and the code works
perfectly, I can change the phrasing if you don't like it, but still the
cleanup.h helpers don't really care that much what functions they call
to allocate/free. They are meant to care about the scope - like constructor
destructor in C++ - you can call anything there.

So this commit changes 2 things:

- change family of function to allocate from
   devm_kcalloc() to kcalloc()
- use scope based mechanism to call those functions


Thanks a lot for your review !
Michał

Re: [PATCH v2] ACPI: NFIT: Fix local use of devm_*()

[Dan Williams]

Wilczynski, Michal wrote:
> On 10/13/2023 6:38 PM, Dan Williams wrote:
> > 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. 
> > No, this assertion is not accurate, if it were strictly true then
> > devm_kfree() should be deleted. This patch is only a cleanup to switch
> > the automatic cleanup pattern from devm to the new cleanup.h helpers.
> 
> The memory in question is only used locally in a function, so there is no reason
> to use devm_*() family of functions. I think devm_kfree() is more for special
> cases where the memory is meant to be used for the whole lifecycle of device,
> but some special case occurs and it's not and it needs to be freed.
> 
> This is an incorrect API usage. Would you propose to change all memory
> allocations currently being done to devm_*() family simply because devm_kfree()
> exists ?

Michal, please work with someone else to get these cleanups upstream, I
am done with this thread.

Re: [PATCH v2] ACPI: NFIT: Fix local use of devm_*()

[Wilczynski, Michal]

On 10/13/2023 7:05 PM, Dan Williams wrote:
> Wilczynski, Michal wrote:
>> On 10/13/2023 6:38 PM, Dan Williams wrote:
>>> 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. 
>>> No, this assertion is not accurate, if it were strictly true then
>>> devm_kfree() should be deleted. This patch is only a cleanup to switch
>>> the automatic cleanup pattern from devm to the new cleanup.h helpers.
>> The memory in question is only used locally in a function, so there is no reason
>> to use devm_*() family of functions. I think devm_kfree() is more for special
>> cases where the memory is meant to be used for the whole lifecycle of device,
>> but some special case occurs and it's not and it needs to be freed.
>>
>> This is an incorrect API usage. Would you propose to change all memory
>> allocations currently being done to devm_*() family simply because devm_kfree()
>> exists ?
> Michal, please work with someone else to get these cleanups upstream, I
> am done with this thread.

I'm really sorry if I offended you, I didn't mean to.

Michał

Re: [PATCH v2] ACPI: NFIT: Fix local use of devm_*()

[Dan Williams]

Wilczynski, Michal wrote:
> 
> 
> On 10/13/2023 7:05 PM, Dan Williams wrote:
> > Wilczynski, Michal wrote:
> >> On 10/13/2023 6:38 PM, Dan Williams wrote:
> >>> 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. 
> >>> No, this assertion is not accurate, if it were strictly true then
> >>> devm_kfree() should be deleted. This patch is only a cleanup to switch
> >>> the automatic cleanup pattern from devm to the new cleanup.h helpers.
> >> The memory in question is only used locally in a function, so there is no reason
> >> to use devm_*() family of functions. I think devm_kfree() is more for special
> >> cases where the memory is meant to be used for the whole lifecycle of device,
> >> but some special case occurs and it's not and it needs to be freed.
> >>
> >> This is an incorrect API usage. Would you propose to change all memory
> >> allocations currently being done to devm_*() family simply because devm_kfree()
> >> exists ?
> > Michal, please work with someone else to get these cleanups upstream, I
> > am done with this thread.
> 
> I'm really sorry if I offended you, I didn't mean to.

Hey, it happens.

I am not offended, just frustrated.

Going forward, my advice for anyone doing advocacy for a patch set is to
be clear about "what happens if upstream does not take this patch?".
When you view it from that angle the patch changelog that would have
received an immediate Reviewed-by with no additional comment from me is
something along the lines of:

"The new cleanup.h facilities that arrived in v6.5-rc1 can replace the
the usage of devm semantics in acpi_nfit_init_interleave_set(). That
routine appears to only be using devm to avoid goto statements. The new
__free() annotation at variable declaration time can achieve the same
effect more efficiently.

There is no end user visible side effects of this patch, I was motivated
to send this cleanup to practice using the new helpers."

As Linus mentions, subtlety is difficult to convey in mailing list
interactions, and you may not have picked up on it, but the frustration
for me began with the claim of a memory leak that turned out to be
false. That was the prompt to consider "what other claims should I be
careful about now that the fundamental justification for touching this
old code has gone away."

So if you want to try again with the justification of the patch limited
to a cleanup, we can move forward.

Re: [PATCH v2] ACPI: NFIT: Fix local use of devm_*()

[Andy Shevchenko]

On Sat, Oct 14, 2023 at 12:20 AM Dan Williams <dan.j.williams@intel.com> wrote:
> Wilczynski, Michal wrote:

...

> "The new cleanup.h facilities that arrived in v6.5-rc1 can replace the
> the usage of devm semantics in acpi_nfit_init_interleave_set(). That
> routine appears to only be using devm to avoid goto statements. The new
> __free() annotation at variable declaration time can achieve the same
> effect more efficiently.
>
> There is no end user visible side effects of this patch, I was motivated
> to send this cleanup to practice using the new helpers."

The end-user side effect (educational and not run-time) is that: "One
should really be careful about the scope of the devm_*() APIs and use
of them just for the sake of the RAII replacement is not the best
idea, while code is still working. Hence it gives a better example for
whoever tries to use this code for educational purposes."

-- 
With Best Regards,
Andy Shevchenko