diff mbox series

cxl/region: Fix cxlr_pmem leaks

Message ID 20240428030748.318985-1-lizhijian@fujitsu.com
State Accepted
Commit 1c987cf22d6b65ade46145c03eef13f0e3e81d83
Headers show
Series cxl/region: Fix cxlr_pmem leaks | expand

Commit Message

Zhijian Li (Fujitsu) April 28, 2024, 3:07 a.m. UTC
Before this error path, cxlr_pmem pointed to a kzalloc() memory, free
it to avoid this memory leaking.

Fixes: f17b558d6663 ("cxl/pmem: Refactor nvdimm device registration, delete the workqueue")
Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
---
 drivers/cxl/core/region.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Jonathan Cameron April 30, 2024, 4:45 p.m. UTC | #1
On Sun, 28 Apr 2024 11:07:48 +0800
Li Zhijian <lizhijian@fujitsu.com> wrote:

> Before this error path, cxlr_pmem pointed to a kzalloc() memory, free
> it to avoid this memory leaking.
> 
> Fixes: f17b558d6663 ("cxl/pmem: Refactor nvdimm device registration, delete the workqueue")
> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>

Fix is good, but this looks like nice case for conversion to cleanup.h stuff
perhaps better to just do that?  Would need a small amount of dancing
on the final return to return cxlr->cxlr_pmem + pointer steal
when setting cxlr->cxlr_pmem a few lines up.

Also guard for the rwsem.

Dave, Dan, worth doing or take this minimal fix and spin around later?

If you think this is the way to go.
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/cxl/core/region.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 5c186e0a39b9..812b2948b6c6 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -2719,6 +2719,7 @@ static struct cxl_pmem_region *cxl_pmem_region_alloc(struct cxl_region *cxlr)
>  		if (i == 0) {
>  			cxl_nvb = cxl_find_nvdimm_bridge(cxlmd);
>  			if (!cxl_nvb) {
> +				kfree(cxlr_pmem);
>  				cxlr_pmem = ERR_PTR(-ENODEV);
>  				goto out;
>  			}
Dan Williams April 30, 2024, 7:58 p.m. UTC | #2
Jonathan Cameron wrote:
> On Sun, 28 Apr 2024 11:07:48 +0800
> Li Zhijian <lizhijian@fujitsu.com> wrote:
> 
> > Before this error path, cxlr_pmem pointed to a kzalloc() memory, free
> > it to avoid this memory leaking.
> > 
> > Fixes: f17b558d6663 ("cxl/pmem: Refactor nvdimm device registration, delete the workqueue")
> > Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
> 
> Fix is good, but this looks like nice case for conversion to cleanup.h stuff
> perhaps better to just do that?  Would need a small amount of dancing
> on the final return to return cxlr->cxlr_pmem + pointer steal
> when setting cxlr->cxlr_pmem a few lines up.
> 
> Also guard for the rwsem.
> 
> Dave, Dan, worth doing or take this minimal fix and spin around later?
> 
> If you think this is the way to go.
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

I had spun up the conversion patch as a replacement in response to this.
However, after looking at that I think merging this fix with a follow-on
cleanup is they way to go because the cleanup touches locking, memory
allocation, and changes the calling convention.

Reviewed-by: Dan Williams <dan.j.williams@intel.com>

I'll send the follow-on cleanup shortly.
Fan Ni April 30, 2024, 10:55 p.m. UTC | #3
On Sun, Apr 28, 2024 at 11:07:48AM +0800, Li Zhijian wrote:
> Before this error path, cxlr_pmem pointed to a kzalloc() memory, free
> it to avoid this memory leaking.
> 
> Fixes: f17b558d6663 ("cxl/pmem: Refactor nvdimm device registration, delete the workqueue")
> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
> ---

Reviewed-by: Fan Ni <fan.ni@samsung.com>

>  drivers/cxl/core/region.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 5c186e0a39b9..812b2948b6c6 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -2719,6 +2719,7 @@ static struct cxl_pmem_region *cxl_pmem_region_alloc(struct cxl_region *cxlr)
>  		if (i == 0) {
>  			cxl_nvb = cxl_find_nvdimm_bridge(cxlmd);
>  			if (!cxl_nvb) {
> +				kfree(cxlr_pmem);
>  				cxlr_pmem = ERR_PTR(-ENODEV);
>  				goto out;
>  			}
> -- 
> 2.29.2
>
Markus Elfring May 1, 2024, 7:23 p.m. UTC | #4
> Before this error path, cxlr_pmem pointed to a kzalloc() memory, free
> it to avoid this memory leaking.

Can the following wording be a bit nicer?

   The local variable “cxlr_pmem” referred to dynamically allocated memory.
   Free it in one error case before it is reset to an error pointer.


Would the summary phrase “Fix a memory leak in cxl_pmem_region_alloc()”
be more appropriate here?

Regards,
Markus
diff mbox series

Patch

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 5c186e0a39b9..812b2948b6c6 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -2719,6 +2719,7 @@  static struct cxl_pmem_region *cxl_pmem_region_alloc(struct cxl_region *cxlr)
 		if (i == 0) {
 			cxl_nvb = cxl_find_nvdimm_bridge(cxlmd);
 			if (!cxl_nvb) {
+				kfree(cxlr_pmem);
 				cxlr_pmem = ERR_PTR(-ENODEV);
 				goto out;
 			}