[PATCH] cxl/region: Fix memregion leaks in devm_cxl_add_region()

[PATCH] cxl/region: Fix memregion leaks in devm_cxl_add_region()

[Li Zhijian]

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 <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

Re: [PATCH] cxl/region: Fix memregion leaks in devm_cxl_add_region()

[Ira Weiny]

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 <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
> 

Re: [PATCH] cxl/region: Fix memregion leaks in devm_cxl_add_region()

[Dan Williams]

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);

Re: [PATCH] cxl/region: Fix memregion leaks in devm_cxl_add_region()

[Zhijian Li (Fujitsu)]

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);
> 

Re: [PATCH] cxl/region: Fix memregion leaks in devm_cxl_add_region()

[Zhijian Li (Fujitsu)]

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
>>
> 
> 
> 

Re: [PATCH] cxl/region: Fix memregion leaks in devm_cxl_add_region()

[Dave Jiang]

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 <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
>>>
>>
>>

Re: [PATCH] cxl/region: Fix memregion leaks in devm_cxl_add_region()

[Zhijian Li (Fujitsu)]

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 <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
>>>>
>>>
>>>