Messages in this thread | | | From | "Zhijian Li (Fujitsu)" <> | Subject | Re: [PATCH] cxl/region: Fix memregion leaks in devm_cxl_add_region() | Date | Tue, 7 May 2024 02:42:37 +0000 |
| |
On 30/04/2024 00:23, Ira Weiny wrote: > Li Zhijian wrote: >> Move memregion_free(id) out of cxl_region_alloc() and >> explicately free memregion in devm_cxl_add_region() error path. > ^^^^ > explicitly > > BTW you should run checkpatch.pl which will check spelling.
I remember I've done this check, but it seems the it doesn't work for me. Did I miss something?
$ ./scripts/checkpatch.pl 0001-cxl-region-Fix-memregion-leaks-in-devm_cxl_add_regio.patch total: 0 errors, 0 warnings, 23 lines checked
0001-cxl-region-Fix-memregion-leaks-in-devm_cxl_add_regio.patch has no obvious style problems and is ready for submission.
> > I'm not following what the problem is you are trying to fix. This seems > like it just moves the memregion_free() around a bit but the logic is the > same. >
Sorry, I think my commit log may be misleading. In fact, this patch mainly fixes the memregion leak in devm_cxl_add_region() when it gets an invalid mode.
>> default: >> dev_err(&cxlrd->cxlsd.cxld.dev, "unsupported mode %d\n", mode); >> + memregion_free(id); >> return ERR_PTR(-EINVAL); >> }
Additionally, to maintain consistent error handling, I also moved memregion_free(id) from cxl_region_alloc() to devm_cxl_add_region() so that devm_cxl_add_region() can free memregion explicitly in error path.
Thanks Zhijian
> Ira > >> >> After cxl_region_alloc() succeed, memregion will be freed by >> cxl_region_type.release() >> >> Fixes: 6e099264185d ("cxl/region: Add volatile region creation support") >> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com> >> --- >> drivers/cxl/core/region.c | 9 +++++---- >> 1 file changed, 5 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c >> index 812b2948b6c6..8535718a20f0 100644 >> --- a/drivers/cxl/core/region.c >> +++ b/drivers/cxl/core/region.c >> @@ -2250,10 +2250,8 @@ static struct cxl_region *cxl_region_alloc(struct cxl_root_decoder *cxlrd, int i >> struct device *dev; >> >> cxlr = kzalloc(sizeof(*cxlr), GFP_KERNEL); >> - if (!cxlr) { >> - memregion_free(id); >> + if (!cxlr) >> return ERR_PTR(-ENOMEM); >> - } >> >> dev = &cxlr->dev; >> device_initialize(dev); >> @@ -2358,12 +2356,15 @@ static struct cxl_region *devm_cxl_add_region(struct cxl_root_decoder *cxlrd, >> break; >> default: >> dev_err(&cxlrd->cxlsd.cxld.dev, "unsupported mode %d\n", mode); >> + memregion_free(id); >> return ERR_PTR(-EINVAL); >> } >> >> cxlr = cxl_region_alloc(cxlrd, id); >> - if (IS_ERR(cxlr)) >> + if (IS_ERR(cxlr)) { >> + memregion_free(id); >> return cxlr; >> + } >> cxlr->mode = mode; >> cxlr->type = type; >> >> -- >> 2.29.2 >> > > >
| |