Move memregion_free(id) out of cxl_region_alloc() and
explicately free memregion in devm_cxl_add_region() error path.
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 <[email protected]>
---
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
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'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.
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 <[email protected]>
> ---
> 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
>
Zhijian Li (Fujitsu) wrote:
> > 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.
>
> This patch not only memregion_free(id) out of cxl_region_alloc() but
> also add an extra memregion_free(id) in EINVAL case which led to a
> memory leak previously
>
I think I would prefer to just eliminate that failure case, something
like:
-- >8 --
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 5c186e0a39b9..95ba32f8649c 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -2352,15 +2352,6 @@ static struct cxl_region *devm_cxl_add_region(struct cxl_root_decoder *cxlrd,
struct device *dev;
int rc;
- switch (mode) {
- case CXL_DECODER_RAM:
- case CXL_DECODER_PMEM:
- break;
- default:
- dev_err(&cxlrd->cxlsd.cxld.dev, "unsupported mode %d\n", mode);
- return ERR_PTR(-EINVAL);
- }
-
cxlr = cxl_region_alloc(cxlrd, id);
if (IS_ERR(cxlr))
return cxlr;
@@ -2415,6 +2406,15 @@ static struct cxl_region *__create_region(struct cxl_root_decoder *cxlrd,
{
int rc;
+ switch (mode) {
+ case CXL_DECODER_RAM:
+ case CXL_DECODER_PMEM:
+ break;
+ default:
+ dev_err(&cxlrd->cxlsd.cxld.dev, "unsupported mode %d\n", mode);
+ return ERR_PTR(-EINVAL);
+ }
+
rc = memregion_alloc(GFP_KERNEL);
if (rc < 0)
return ERR_PTR(rc);
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 <[email protected]>
>> ---
>> 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
>>
>
>
>
On 04/05/2024 00:07, Dan Williams wrote:
> Zhijian Li (Fujitsu) wrote:
>>> 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.
>>
>> This patch not only memregion_free(id) out of cxl_region_alloc() but
>> also add an extra memregion_free(id) in EINVAL case which led to a
>> memory leak previously
>>
>
> I think I would prefer to just eliminate that failure case, something
> like:
It sounds good to me. I will update it in V2
Thanks
Zhijian
>
> -- >8 --
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 5c186e0a39b9..95ba32f8649c 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -2352,15 +2352,6 @@ static struct cxl_region *devm_cxl_add_region(struct cxl_root_decoder *cxlrd,
> struct device *dev;
> int rc;
>
> - switch (mode) {
> - case CXL_DECODER_RAM:
> - case CXL_DECODER_PMEM:
> - break;
> - default:
> - dev_err(&cxlrd->cxlsd.cxld.dev, "unsupported mode %d\n", mode);
> - return ERR_PTR(-EINVAL);
> - }
> -
> cxlr = cxl_region_alloc(cxlrd, id);
> if (IS_ERR(cxlr))
> return cxlr;
> @@ -2415,6 +2406,15 @@ static struct cxl_region *__create_region(struct cxl_root_decoder *cxlrd,
> {
> int rc;
>
> + switch (mode) {
> + case CXL_DECODER_RAM:
> + case CXL_DECODER_PMEM:
> + break;
> + default:
> + dev_err(&cxlrd->cxlsd.cxld.dev, "unsupported mode %d\n", mode);
> + return ERR_PTR(-EINVAL);
> + }
> +
> rc = memregion_alloc(GFP_KERNEL);
> if (rc < 0)
> return ERR_PTR(rc);
>
On 5/6/24 7:42 PM, Zhijian Li (Fujitsu) wrote:
>
>
> 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.
Pass in --codespell parameter. And make sure you have the codespell dictionary installed (i.e. /usr/share/codespell/dictionary.txt).
DJ
>
>
>
>>
>> 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 <[email protected]>
>>> ---
>>> 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
>>>
>>
>>
On 07/05/2024 23:52, Dave Jiang wrote:
>
>
> On 5/6/24 7:42 PM, Zhijian Li (Fujitsu) wrote:
>>
>>
>> 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.
>
> Pass in --codespell parameter. And make sure you have the codespell dictionary installed (i.e. /usr/share/codespell/dictionary.txt).
Good to know this, many thanks!
Thanks
>
> DJ
>
>>
>>
>>
>>>
>>> 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 <[email protected]>
>>>> ---
>>>> 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
>>>>
>>>
>>>