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

Groups > linux.kernel > #1208825 > unrolled thread

[PATCHv2 5/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

  [PATCHv2 5/5] staging: wilc1000: replace MALLOC_WILC_BUFFER() macro to avoid possible memory leak Raphaël Beamonte <raphael.beamonte@gmail.com> - 2015-08-17 21:30 +0200
    Re: [PATCHv2 5/5] staging: wilc1000: replace MALLOC_WILC_BUFFER()  macro to avoid possible memory leak Arend van Spriel <arend@broadcom.com> - 2015-08-17 21:50 +0200
      Re: [PATCHv2 5/5] staging: wilc1000: replace MALLOC_WILC_BUFFER()  macro to avoid possible memory leak Raphaël Beamonte <raphael.beamonte@gmail.com> - 2015-08-18 01:20 +0200
      [PATCHv3] staging: wilc1000: replace MALLOC_WILC_BUFFER() macro to avoid possible memory leak Raphaël Beamonte <raphael.beamonte@gmail.com> - 2015-08-18 01:20 +0200
        Re: [PATCHv3] staging: wilc1000: replace MALLOC_WILC_BUFFER() macro  to avoid possible memory leak Dan Carpenter <dan.carpenter@oracle.com> - 2015-08-18 01:50 +0200
          Re: [PATCHv3] staging: wilc1000: replace MALLOC_WILC_BUFFER() macro  to avoid possible memory leak Dan Carpenter <dan.carpenter@oracle.com> - 2015-08-18 11:20 +0200
            Re: [PATCHv3] staging: wilc1000: replace MALLOC_WILC_BUFFER() macro  to avoid possible memory leak Raphaël Beamonte <raphael.beamonte@gmail.com> - 2015-08-18 19:10 +0200
              Re: [PATCHv3] staging: wilc1000: replace MALLOC_WILC_BUFFER() macro  to avoid possible memory leak Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2015-08-19 05:00 +0200
                [PATCHv4 0/2] staging: wilc1000: code improvements Raphaël Beamonte <raphael.beamonte@gmail.com> - 2015-08-19 05:20 +0200

#1208825 — [PATCHv2 5/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 | 37 ++++++++++++++++++++--------
 1 file changed, 27 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/wilc1000/wilc_exported_buf.c b/drivers/staging/wilc1000/wilc_exported_buf.c
index c9a5943..0f3bdad 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
  */
@@ -45,11 +38,35 @@ static int __init wilc_module_init(void)
 	/*
 	 * 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)
+	exported_g_tx_buf = kmalloc(LINUX_TX_SIZE, GFP_KERNEL);
+	if (!exported_g_tx_buf) {
+		pr_err("fail to alloc tx buf");
+		return -ENOMEM;
+	}
+
+	exported_g_rx_buf = kmalloc(LINUX_RX_SIZE, GFP_KERNEL);
+	if (!exported_g_rx_buf) {
+		pr_err("fail to alloc rx buf");
+		goto free_g_tx_buf;
+	}
+
+	exported_g_fw_buf = kmalloc(WILC1000_FW_SIZE, GFP_KERNEL);
+	if (!exported_g_fw_buf) {
+		pr_err("fail to alloc fw buf");
+		goto free_g_rx_buf;
+	}
 
 	return 0;
+
+free_g_rx_buf:
+	kfree(exported_g_rx_buf);
+	exported_g_rx_buf = NULL;
+
+free_g_tx_buf:
+	kfree(exported_g_tx_buf);
+	exported_g_tx_buf = NULL;
+
+	return -ENOMEM;
 }
 
 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]

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

On 08/17/2015 09:28 PM, Raphaël Beamonte wrote:
> The MACRO_WILC_BUFFER() macro was using a return statement, and didn't

Probable MACRO_WILC_BUFFER should be MALLOC_WILC_BUFFER here.

> 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 | 37 ++++++++++++++++++++--------
>   1 file changed, 27 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/staging/wilc1000/wilc_exported_buf.c b/drivers/staging/wilc1000/wilc_exported_buf.c
> index c9a5943..0f3bdad 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
>    */
> @@ -45,11 +38,35 @@ static int __init wilc_module_init(void)
>   	/*
>   	 * 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)
> +	exported_g_tx_buf = kmalloc(LINUX_TX_SIZE, GFP_KERNEL);
> +	if (!exported_g_tx_buf) {
> +		pr_err("fail to alloc tx buf");

There is really no need to print an error message here. kmalloc will 
blurb enough info when it fails.

So these buffers are globals? So does this driver support multiple 
devices, ie. how are these used when two wilc1000 supported devices are 
present.

Regards,
Arend

> +		return -ENOMEM;
> +	}
> +
> +	exported_g_rx_buf = kmalloc(LINUX_RX_SIZE, GFP_KERNEL);
> +	if (!exported_g_rx_buf) {
> +		pr_err("fail to alloc rx buf");
> +		goto free_g_tx_buf;
> +	}
> +
> +	exported_g_fw_buf = kmalloc(WILC1000_FW_SIZE, GFP_KERNEL);
> +	if (!exported_g_fw_buf) {
> +		pr_err("fail to alloc fw buf");
> +		goto free_g_rx_buf;
> +	}
>
>   	return 0;
> +
> +free_g_rx_buf:
> +	kfree(exported_g_rx_buf);
> +	exported_g_rx_buf = NULL;
> +
> +free_g_tx_buf:
> +	kfree(exported_g_tx_buf);
> +	exported_g_tx_buf = NULL;
> +
> +	return -ENOMEM;
>   }
>
>   static void __exit wilc_module_deinit(void)
>

--
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] | [next] | [standalone]

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

2015-08-17 15:41 GMT-04:00 Arend van Spriel <arend@broadcom.com>:
> Probable MACRO_WILC_BUFFER should be MALLOC_WILC_BUFFER here.

Good catch!

> There is really no need to print an error message here. kmalloc will blurb
> enough info when it fails.

Ok!

> So these buffers are globals? So does this driver support multiple devices,
> ie. how are these used when two wilc1000 supported devices are present.

Not sure. I mostly did code refactoring to have a clearer source code
and try to respect the kernel code style. I don't have a compatible
device to try and test it unfortunately.

Thanks for the feedback. I just sent a revised version of the patch
taking your comments into account.
--
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] | [next] | [standalone]

#1208942 — [PATCHv3] staging: wilc1000: replace MALLOC_WILC_BUFFER() macro to avoid possible memory leak

The MALLOC_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 | 31 +++++++++++++++++++---------
 1 file changed, 21 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/wilc1000/wilc_exported_buf.c b/drivers/staging/wilc1000/wilc_exported_buf.c
index c3f6a0a..ec8d8e5 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;	\
-	}
-
 #define FREE_WILC_BUFFER(name)	\
 	kfree(exported_ ## name);
 
@@ -49,11 +42,29 @@ static int __init wilc_module_init(void)
 	/*
 	 * 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)
+	exported_g_tx_buf = kmalloc(LINUX_TX_SIZE, GFP_KERNEL);
+	if (!exported_g_tx_buf)
+		return -ENOMEM;
+
+	exported_g_rx_buf = kmalloc(LINUX_RX_SIZE, GFP_KERNEL);
+	if (!exported_g_rx_buf)
+		goto free_g_tx_buf;
+
+	exported_g_fw_buf = kmalloc(WILC1000_FW_SIZE, GFP_KERNEL);
+	if (!exported_g_fw_buf)
+		goto free_g_rx_buf;
 
 	return 0;
+
+free_g_rx_buf:
+	kfree(exported_g_rx_buf);
+	exported_g_rx_buf = NULL;
+
+free_g_tx_buf:
+	kfree(exported_g_tx_buf);
+	exported_g_tx_buf = NULL;
+
+	return -ENOMEM;
 }
 
 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] | [prev] | [next] | [standalone]

#1208963 — Re: [PATCHv3] staging: wilc1000: replace MALLOC_WILC_BUFFER() macro to avoid possible memory leak

On Mon, Aug 17, 2015 at 07:12:47PM -0400, Raphaël Beamonte wrote:
> The MALLOC_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>
> ---

Yes.  This looks good now.

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] | [next] | [standalone]

#1209148 — Re: [PATCHv3] staging: wilc1000: replace MALLOC_WILC_BUFFER() macro to avoid possible memory leak

To be honest, I have lost track of this patchset.  If you are planning
to redo the other patches can you send it in a new thread?

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] | [next] | [standalone]

#1209381 — Re: [PATCHv3] staging: wilc1000: replace MALLOC_WILC_BUFFER() macro to avoid possible memory leak

2015-08-18 5:15 GMT-04:00 Dan Carpenter <dan.carpenter@oracle.com>:
> To be honest, I have lost track of this patchset.  If you are planning
> to redo the other patches can you send it in a new thread?

Actually, Greg already included the "return statement" and
"DECLARE_WILC_BUFFER" ones.
The replacement of printk by netdev_* needs more work on my side to
get the net_device to be able to use the netdev_* functions.
And apparently Greg already received another patch with the
"FREE_WILC_BUFFER" replacement, though I don't see it in the
staging-testing tree yet.

So, I think this patch is the last one of this patchset that has to be
treated! That's why I rebased it on top of the current staging-testing
tree on my last send.

Thanks,
Raphaël
--
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] | [next] | [standalone]

#1209577 — Re: [PATCHv3] staging: wilc1000: replace MALLOC_WILC_BUFFER() macro to avoid possible memory leak

On Tue, Aug 18, 2015 at 01:06:39PM -0400, Raphaël Beamonte wrote:
> 2015-08-18 5:15 GMT-04:00 Dan Carpenter <dan.carpenter@oracle.com>:
> > To be honest, I have lost track of this patchset.  If you are planning
> > to redo the other patches can you send it in a new thread?
> 
> Actually, Greg already included the "return statement" and
> "DECLARE_WILC_BUFFER" ones.
> The replacement of printk by netdev_* needs more work on my side to
> get the net_device to be able to use the netdev_* functions.
> And apparently Greg already received another patch with the
> "FREE_WILC_BUFFER" replacement, though I don't see it in the
> staging-testing tree yet.

Maybe it was something else, but it would not apply.  Please use git
rebase to figure it out and resend all of your outstanding patches, I
too am confused at this point.

thanks,

greg k-h
--
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] | [next] | [standalone]

#1209587 — [PATCHv4 0/2] staging: wilc1000: code improvements

Hi,

As requested, here are the two remaining ready patches of this
patchset. I pulled and rebased against staging-testing just now.
They should thus be usable without problem!

Thanks,
Raphaël


Raphaël Beamonte (2):
  staging: wilc1000: remove FREE_WILC_BUFFER()
  staging: wilc1000: replace MALLOC_WILC_BUFFER() macro to avoid
    possible memory leak

 drivers/staging/wilc1000/wilc_exported_buf.c | 40 +++++++++++++++++-----------
 1 file changed, 24 insertions(+), 16 deletions(-)

-- 
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] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web