Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1208680 > unrolled thread

[PATCH 3/5] staging: wilc1000: replace MALLOC_WILC_BUFFER() macro to avoid possible memory leak

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 3/5] staging: wilc1000: replace MALLOC_WILC_BUFFER() macro to avoid possible memory leak Raphaël Beamonte <raphael.beamonte@gmail.com> - 2015-08-17 18:10 +0200
    Re: [PATCH 3/5] staging: wilc1000: replace MALLOC_WILC_BUFFER()  macro to avoid possible memory leak Dan Carpenter <dan.carpenter@oracle.com> - 2015-08-17 19:40 +0200

#1208680 — [PATCH 3/5] staging: wilc1000: replace MALLOC_WILC_BUFFER() macro to avoid possible memory leak

The MACRO_WILC_BUFFER() macro was using a return statement, and didn't
take care of possible memory leaks and subsequent bugs when it was failing
after succeeding some allocations. This patch corrects this behavior.

Signed-off-by: Raphaël Beamonte <raphael.beamonte@gmail.com>
---
 drivers/staging/wilc1000/wilc_exported_buf.c | 39 +++++++++++++++++++++-------
 1 file changed, 29 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/wilc1000/wilc_exported_buf.c b/drivers/staging/wilc1000/wilc_exported_buf.c
index bf392fb..c3aff9a 100644
--- a/drivers/staging/wilc1000/wilc_exported_buf.c
+++ b/drivers/staging/wilc1000/wilc_exported_buf.c
@@ -8,13 +8,6 @@
 #define LINUX_TX_SIZE	(64 * 1024)
 #define WILC1000_FW_SIZE (4 * 1024)
 
-#define MALLOC_WILC_BUFFER(name, size)	\
-	exported_ ## name = kmalloc(size, GFP_KERNEL);	  \
-	if (!exported_ ## name) {   \
-		printk("fail to alloc: %s memory\n", exported_ ## name);  \
-		return -ENOBUFS;	\
-	}
-
 /*
  * Add necessary buffer pointers
  */
@@ -40,17 +33,43 @@ void *get_fw_buffer(void)
 }
 EXPORT_SYMBOL(get_fw_buffer);
 
+static inline int kmalloc_wilc_buffer(void *buf, int size)
+{
+	buf = kmalloc(size, GFP_KERNEL);
+	if (!buf) {
+		printk("fail to alloc memory\n");
+		return -ENOBUFS;
+	}
+	return 0;
+}
+
 static int __init wilc_module_init(void)
 {
 	printk("wilc_module_init\n");
 	/*
 	 * alloc necessary memory
 	 */
-	MALLOC_WILC_BUFFER(g_tx_buf, LINUX_TX_SIZE)
-	MALLOC_WILC_BUFFER(g_rx_buf, LINUX_RX_SIZE)
-	MALLOC_WILC_BUFFER(g_fw_buf, WILC1000_FW_SIZE)
+	if (kmalloc_wilc_buffer(exported_g_tx_buf, LINUX_TX_SIZE))
+		goto error_g_tx_buf;
+
+	if (kmalloc_wilc_buffer(exported_g_rx_buf, LINUX_RX_SIZE))
+		goto error_g_rx_buf;
+
+	if (kmalloc_wilc_buffer(exported_g_fw_buf, WILC1000_FW_SIZE))
+		goto error_g_fw_buf;
 
 	return 0;
+
+error_g_fw_buf:
+	kfree(exported_g_rx_buf);
+	exported_g_rx_buf = NULL;
+
+error_g_rx_buf:
+	kfree(exported_g_tx_buf);
+	exported_g_tx_buf = NULL;
+
+error_g_tx_buf:
+	return -ENOBUFS;
 }
 
 static void __exit wilc_module_deinit(void)
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1208748 — Re: [PATCH 3/5] staging: wilc1000: replace MALLOC_WILC_BUFFER() macro to avoid possible memory leak

On Mon, Aug 17, 2015 at 12:08:35PM -0400, Raphaël Beamonte wrote:
> The MACRO_WILC_BUFFER() macro was using a return statement, and didn't
> take care of possible memory leaks and subsequent bugs when it was failing
> after succeeding some allocations. This patch corrects this behavior.
> 
> Signed-off-by: Raphaël Beamonte <raphael.beamonte@gmail.com>
> ---
>  drivers/staging/wilc1000/wilc_exported_buf.c | 39 +++++++++++++++++++++-------
>  1 file changed, 29 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/staging/wilc1000/wilc_exported_buf.c b/drivers/staging/wilc1000/wilc_exported_buf.c
> index bf392fb..c3aff9a 100644
> --- a/drivers/staging/wilc1000/wilc_exported_buf.c
> +++ b/drivers/staging/wilc1000/wilc_exported_buf.c
> @@ -8,13 +8,6 @@
>  #define LINUX_TX_SIZE	(64 * 1024)
>  #define WILC1000_FW_SIZE (4 * 1024)
>  
> -#define MALLOC_WILC_BUFFER(name, size)	\
> -	exported_ ## name = kmalloc(size, GFP_KERNEL);	  \
> -	if (!exported_ ## name) {   \
> -		printk("fail to alloc: %s memory\n", exported_ ## name);  \
> -		return -ENOBUFS;	\
> -	}
> -
>  /*
>   * Add necessary buffer pointers
>   */
> @@ -40,17 +33,43 @@ void *get_fw_buffer(void)
>  }
>  EXPORT_SYMBOL(get_fw_buffer);
>  
> +static inline int kmalloc_wilc_buffer(void *buf, int size)
> +{
> +	buf = kmalloc(size, GFP_KERNEL);
> +	if (!buf) {
> +		printk("fail to alloc memory\n");
> +		return -ENOBUFS;
> +	}
> +	return 0;
> +}
> +
>  static int __init wilc_module_init(void)
>  {
>  	printk("wilc_module_init\n");
>  	/*
>  	 * alloc necessary memory
>  	 */
> -	MALLOC_WILC_BUFFER(g_tx_buf, LINUX_TX_SIZE)
> -	MALLOC_WILC_BUFFER(g_rx_buf, LINUX_RX_SIZE)
> -	MALLOC_WILC_BUFFER(g_fw_buf, WILC1000_FW_SIZE)
> +	if (kmalloc_wilc_buffer(exported_g_tx_buf, LINUX_TX_SIZE))
> +		goto error_g_tx_buf;

Don't do it this way.  Just do:

	exported_g_tx_buf = kmalloc(LINUX_TX_SIZE, GFP_KERNEL);
	if (!exported_g_tx_buf)
		return -ENOMEM;

	exported_g_rx_buf = kmalloc(LINUX_TX_SIZE, GFP_KERNEL);
	if (!exported_g_rx_buf)
		goto free_tx_buf;

1) Avoid abstraction where possible.
2) The error code should be -ENOMEM
3) This is not a universal rule, but I feel it's better to return
   directly instead of using a do-nothing-goto.  Some people think
   using gotos everywhere forces you to think about error handling so
   it is worth making the code slightly less readable.  Based on my
   static analysis results, I do not believe that it makes people think
   about error handling.
4) Name the label after the where the label is (what it does on the next
   line) instead of basing it on the goto location.
5) Preserve the error codes.  Use ret = kmalloc_wilc_buffer(); if (ret).

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web