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

[IBM]

Michail Tatas <michail.tatas@xxxxxxxxx> 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@xxxxxxxxx>
> ---
>  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@xxxxxxxxx>