[PATCH] powerpc/pseries/htmdump: Fix leak in htmdump_init_debugfs

[PATCH] powerpc/pseries/htmdump: Fix leak in htmdump_init_debugfs

[Michail Tatas]

If any allocation fails during init all previous allocations are
leaked and the debugfs directory is left.
Fix by freeing the allocations that have already happened and also
remove the directory that has been created.

Signed-off-by: Michail Tatas <michail.tatas@gmail.com>
---
 arch/powerpc/platforms/pseries/htmdump.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/htmdump.c b/arch/powerpc/platforms/pseries/htmdump.c
index 489a80e87082..f33941b80ada 100644
--- a/arch/powerpc/platforms/pseries/htmdump.c
+++ b/arch/powerpc/platforms/pseries/htmdump.c
@@ -527,28 +527,28 @@ static int htmdump_init_debugfs(void)
 	htm_status_buf = kmalloc(PAGE_SIZE, GFP_KERNEL);
 	if (!htm_status_buf) {
 		pr_err("Failed to allocate htmstatus buf\n");
-		return -ENOMEM;
+		goto htm_status_buf_err;
 	}
 
 	/* Debugfs interface file to present System Processor Configuration */
 	htm_info_buf = kmalloc(PAGE_SIZE, GFP_KERNEL);
 	if (!htm_info_buf) {
 		pr_err("Failed to allocate htm info buf\n");
-		return -ENOMEM;
+		goto htm_info_buf_err;
 	}
 
 	/* Debugfs interface file to present HTM capabilities */
 	htm_caps_buf = kmalloc(PAGE_SIZE, GFP_KERNEL);
 	if (!htm_caps_buf) {
 		pr_err("Failed to allocate htm caps buf\n");
-		return -ENOMEM;
+		goto htm_caps_buf_err;
 	}
 
 	/* Memory to present HTM system memory configuration */
 	htm_mem_buf = kmalloc(PAGE_SIZE, GFP_KERNEL);
 	if (!htm_mem_buf) {
 		pr_err("Failed to allocate htm mem buf\n");
-		return -ENOMEM;
+		goto htm_mem_buf_err;
 	}
 
 	debugfs_create_file("htmstatus", 0400, htmdump_debugfs_dir, htm_status_buf, &htmstatus_fops);
@@ -557,6 +557,17 @@ static int htmdump_init_debugfs(void)
 	debugfs_create_file("htmsystem_mem", 0400, htmdump_debugfs_dir, htm_mem_buf, &htmsystem_mem_fops);
 
 	return 0;
+
+htm_mem_buf_err:
+	kfree(htm_caps_buf);
+htm_caps_buf_err:
+	kfree(htm_info_buf);
+htm_info_buf_err:
+	kfree(htm_status_buf);
+htm_status_buf_err:
+	debugfs_remove_recursive(htmdump_debugfs_dir);
+	kfree(htm_buf);
+	return -ENOMEM;
 }
 
 static int __init htmdump_init(void)
-- 
2.43.0

Re: [PATCH] powerpc/pseries/htmdump: Fix leak in htmdump_init_debugfs

[Ritesh Harjani]

Michail Tatas <michail.tatas@gmail.com> writes:

> If any allocation fails during init all previous allocations are
> leaked and the debugfs directory is left.
> Fix by freeing the allocations that have already happened and also
> remove the directory that has been created.
>
> Signed-off-by: Michail Tatas <michail.tatas@gmail.com>
> ---
>  arch/powerpc/platforms/pseries/htmdump.c | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/arch/powerpc/platforms/pseries/htmdump.c b/arch/powerpc/platforms/pseries/htmdump.c
> index 489a80e87082..f33941b80ada 100644
> --- a/arch/powerpc/platforms/pseries/htmdump.c
> +++ b/arch/powerpc/platforms/pseries/htmdump.c
> @@ -527,28 +527,28 @@ static int htmdump_init_debugfs(void)
>  	htm_status_buf = kmalloc(PAGE_SIZE, GFP_KERNEL);
>  	if (!htm_status_buf) {
>  		pr_err("Failed to allocate htmstatus buf\n");
> -		return -ENOMEM;
> +		goto htm_status_buf_err;
>  	}
>  
>  	/* Debugfs interface file to present System Processor Configuration */
>  	htm_info_buf = kmalloc(PAGE_SIZE, GFP_KERNEL);
>  	if (!htm_info_buf) {
>  		pr_err("Failed to allocate htm info buf\n");
> -		return -ENOMEM;
> +		goto htm_info_buf_err;
>  	}
>  
>  	/* Debugfs interface file to present HTM capabilities */
>  	htm_caps_buf = kmalloc(PAGE_SIZE, GFP_KERNEL);
>  	if (!htm_caps_buf) {
>  		pr_err("Failed to allocate htm caps buf\n");
> -		return -ENOMEM;
> +		goto htm_caps_buf_err;
>  	}
>  
>  	/* Memory to present HTM system memory configuration */
>  	htm_mem_buf = kmalloc(PAGE_SIZE, GFP_KERNEL);
>  	if (!htm_mem_buf) {
>  		pr_err("Failed to allocate htm mem buf\n");
> -		return -ENOMEM;
> +		goto htm_mem_buf_err;
>  	}
>  
>  	debugfs_create_file("htmstatus", 0400, htmdump_debugfs_dir, htm_status_buf, &htmstatus_fops);
> @@ -557,6 +557,17 @@ static int htmdump_init_debugfs(void)
>  	debugfs_create_file("htmsystem_mem", 0400, htmdump_debugfs_dir, htm_mem_buf, &htmsystem_mem_fops);
>  
>  	return 0;
> +
> +htm_mem_buf_err:
> +	kfree(htm_caps_buf);
> +htm_caps_buf_err:
> +	kfree(htm_info_buf);
> +htm_info_buf_err:
> +	kfree(htm_status_buf);
> +htm_status_buf_err:
> +	debugfs_remove_recursive(htmdump_debugfs_dir);
> +	kfree(htm_buf);
> +	return -ENOMEM;
>  }

So far the only error this function returns is -ENOMEM, so this looks
ok. Later if someone adds a different error return value, then it should
introduce a new ret variable and we should return ret from here.

But as for this patch - this looks good to me. So please feel free to add:
Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>