cxl/region: Fix cxlr_pmem leaks
Subsystems (2)
PATCH 1/1 cxl/region: Fix cxlr_pmem leaks
Before this error path, cxlr_pmem pointed to a kzalloc() memory, free it to avoid this memory leaking.
--- drivers/cxl/core/region.c | 1 + 1 file changed, 1 insertion(+)
drivers/cxl/core/region.c +1 -0
@@ -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; }
Signature · 1 line
2.29.2
Quoted 5 lines · On Sun, 28 Apr 2024 11:07:48 +0800 Li Zhijian <[email protected]> wrote:
Before this error path,
cxlr_pmempointed to akzalloc()memory, free it to avoid this memory leaking.Fixes f17b558d6663 ("cxl/pmem: Refactor nvdimm device registration, delete the workqueue")Signed-off-by Li Zhijian <[email protected]>
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.
Quoted 16 lines
--- drivers/cxl/core/region.c | 1 + 1 file changed, 1 insertion(+)drivers/cxl/core/region.c +1
-0@@ -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
Reviewed-byQuoted 14 lines · Jonathan Cameron
Quoted 5 lines · On Sun, 28 Apr 2024 11:07:48 +0800 Li Zhijian <[email protected]> wrote:
Before this error path,
cxlr_pmempointed to akzalloc()memory, free it to avoid this memory leaking.Fixes f17b558d6663 ("cxl/pmem: Refactor nvdimm device registration, delete the workqueue")Signed-off-by Li Zhijian <[email protected]>Fix is good, but this looks like nice case for conversion to
cleanup.hstuff perhaps better to just do that? Would need a small amount of dancing on the final return to returncxlr->cxlr_pmem+ pointer steal when settingcxlr->cxlr_pmema few lines up.
Quoted 6 lines
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 <[email protected]>
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.
I'll send the follow-on cleanup shortly.
Quoted 6 lines · Li Zhijian
Before this error path,
cxlr_pmempointed to akzalloc()memory, free it to avoid this memory leaking.Fixes f17b558d6663 ("cxl/pmem: Refactor nvdimm device registration, delete the workqueue")Signed-off-by Li Zhijian <[email protected]>---
Quoted 18 lines
drivers/cxl/core/region.c | 1 + 1 file changed, 1 insertion(+)drivers/cxl/core/region.c +1
-0@@ -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; }Signature · 1 line
2.29.2
Quoted 2 lines
Before this error path,
cxlr_pmempointed to akzalloc()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