diff mbox series

[v2] ACPI / APEI: release resources if gen_pool_add fails

Message ID 1560734870-27742-1-git-send-email-zhangliguang@linux.alibaba.com
State Changes Requested, archived
Headers show
Series [v2] ACPI / APEI: release resources if gen_pool_add fails | expand

Commit Message

Liguang Zhang June 17, 2019, 1:27 a.m. UTC
To avoid memory leaks, destroy ghes_estatus_pool and release memory
allocated via vmalloc() on errors in ghes_estatus_pool_init().

Signed-off-by: Liguang Zhang <zhangliguang@linux.alibaba.com>
Reviewed-by: James Morse <james.morse@arm.com>
Tested-by: James Morse <james.morse@arm.com>
---
 drivers/acpi/apei/ghes.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

Comments

Borislav Petkov June 21, 2019, 3:29 p.m. UTC | #1
On Mon, Jun 17, 2019 at 09:27:50AM +0800, luanshi wrote:
> To avoid memory leaks, destroy ghes_estatus_pool and release memory
> allocated via vmalloc() on errors in ghes_estatus_pool_init().
> 
> Signed-off-by: Liguang Zhang <zhangliguang@linux.alibaba.com>
> Reviewed-by: James Morse <james.morse@arm.com>
> Tested-by: James Morse <james.morse@arm.com>
> ---
>  drivers/acpi/apei/ghes.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index 993940d..4e5de30 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
> @@ -153,6 +153,7 @@ static void ghes_unmap(void __iomem *vaddr, enum fixed_addresses fixmap_idx)
>  int ghes_estatus_pool_init(int num_ghes)
>  {
>  	unsigned long addr, len;
> +	int rc = 0;
>  
>  	ghes_estatus_pool = gen_pool_create(GHES_ESTATUS_POOL_MIN_ALLOC_ORDER, -1);
>  	if (!ghes_estatus_pool)
> @@ -163,8 +164,10 @@ int ghes_estatus_pool_init(int num_ghes)
>  
>  	ghes_estatus_pool_size_request = PAGE_ALIGN(len);
>  	addr = (unsigned long)vmalloc(PAGE_ALIGN(len));
> -	if (!addr)
> +	if (!addr) {
> +		gen_pool_destroy(ghes_estatus_pool);
>  		return -ENOMEM;
> +	}
>  
>  	/*
>  	 * New allocation must be visible in all pgd before it can be found by
> @@ -172,7 +175,12 @@ int ghes_estatus_pool_init(int num_ghes)
>  	 */
>  	vmalloc_sync_all();
>  
> -	return gen_pool_add(ghes_estatus_pool, addr, PAGE_ALIGN(len), -1);
> +	rc = gen_pool_add(ghes_estatus_pool, addr, PAGE_ALIGN(len), -1);
> +	if (rc) {
> +		gen_pool_destroy(ghes_estatus_pool);
> +		vfree((void *)addr);
> +	}
> +	return rc;

Please put the error path in labels at the end of the function to which
you goto from each error case, like it is usually done in kernel code,
instead of repeating the free calls in each error handling path.

Grep the tree for examples, if you need some.

Thx.
Liguang Zhang June 23, 2019, 2:48 a.m. UTC | #2
Hi Borislav,

在 2019/6/21 23:29, Borislav Petkov 写道:
> On Mon, Jun 17, 2019 at 09:27:50AM +0800, luanshi wrote:
>> To avoid memory leaks, destroy ghes_estatus_pool and release memory
>> allocated via vmalloc() on errors in ghes_estatus_pool_init().
>>
>> Signed-off-by: Liguang Zhang <zhangliguang@linux.alibaba.com>
>> Reviewed-by: James Morse <james.morse@arm.com>
>> Tested-by: James Morse <james.morse@arm.com>
>> ---
>>   drivers/acpi/apei/ghes.c | 12 ++++++++++--
>>   1 file changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
>> index 993940d..4e5de30 100644
>> --- a/drivers/acpi/apei/ghes.c
>> +++ b/drivers/acpi/apei/ghes.c
>> @@ -153,6 +153,7 @@ static void ghes_unmap(void __iomem *vaddr, enum fixed_addresses fixmap_idx)
>>   int ghes_estatus_pool_init(int num_ghes)
>>   {
>>   	unsigned long addr, len;
>> +	int rc = 0;
>>   
>>   	ghes_estatus_pool = gen_pool_create(GHES_ESTATUS_POOL_MIN_ALLOC_ORDER, -1);
>>   	if (!ghes_estatus_pool)
>> @@ -163,8 +164,10 @@ int ghes_estatus_pool_init(int num_ghes)
>>   
>>   	ghes_estatus_pool_size_request = PAGE_ALIGN(len);
>>   	addr = (unsigned long)vmalloc(PAGE_ALIGN(len));
>> -	if (!addr)
>> +	if (!addr) {
>> +		gen_pool_destroy(ghes_estatus_pool);
>>   		return -ENOMEM;
>> +	}
>>   
>>   	/*
>>   	 * New allocation must be visible in all pgd before it can be found by
>> @@ -172,7 +175,12 @@ int ghes_estatus_pool_init(int num_ghes)
>>   	 */
>>   	vmalloc_sync_all();
>>   
>> -	return gen_pool_add(ghes_estatus_pool, addr, PAGE_ALIGN(len), -1);
>> +	rc = gen_pool_add(ghes_estatus_pool, addr, PAGE_ALIGN(len), -1);
>> +	if (rc) {
>> +		gen_pool_destroy(ghes_estatus_pool);
>> +		vfree((void *)addr);
>> +	}
>> +	return rc;
> Please put the error path in labels at the end of the function to which
> you goto from each error case, like it is usually done in kernel code,
> instead of repeating the free calls in each error handling path.

Thanks, I will modify this patch by your suggestion and send the patch 
v3 for review.

Thanks,

Liguang

>
> Grep the tree for examples, if you need some.
>
> Thx.
>
diff mbox series

Patch

diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index 993940d..4e5de30 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -153,6 +153,7 @@  static void ghes_unmap(void __iomem *vaddr, enum fixed_addresses fixmap_idx)
 int ghes_estatus_pool_init(int num_ghes)
 {
 	unsigned long addr, len;
+	int rc = 0;
 
 	ghes_estatus_pool = gen_pool_create(GHES_ESTATUS_POOL_MIN_ALLOC_ORDER, -1);
 	if (!ghes_estatus_pool)
@@ -163,8 +164,10 @@  int ghes_estatus_pool_init(int num_ghes)
 
 	ghes_estatus_pool_size_request = PAGE_ALIGN(len);
 	addr = (unsigned long)vmalloc(PAGE_ALIGN(len));
-	if (!addr)
+	if (!addr) {
+		gen_pool_destroy(ghes_estatus_pool);
 		return -ENOMEM;
+	}
 
 	/*
 	 * New allocation must be visible in all pgd before it can be found by
@@ -172,7 +175,12 @@  int ghes_estatus_pool_init(int num_ghes)
 	 */
 	vmalloc_sync_all();
 
-	return gen_pool_add(ghes_estatus_pool, addr, PAGE_ALIGN(len), -1);
+	rc = gen_pool_add(ghes_estatus_pool, addr, PAGE_ALIGN(len), -1);
+	if (rc) {
+		gen_pool_destroy(ghes_estatus_pool);
+		vfree((void *)addr);
+	}
+	return rc;
 }
 
 static int map_gen_v2(struct ghes *ghes)